Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vector destructuring #4091

Closed
wants to merge 13 commits into from
Closed

Vector destructuring #4091

wants to merge 13 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 1, 2012

This is an initial implementation of vector destructuring as in #1844. It's not quite there yet but I'd like to gather feedback, assuming there is still interest in this being part of the language. I've added some tests, two of which are failing at the moment as there's no pattern reachability and exhaustiveness detection just yet.

As mentioned in the bug, there already is a list structure in libstd, which can already be easily destructured and matched against. It's not a first-class citizen in Rust, so it may be worth making it easy for vectors as well.

@erickt
Copy link
Contributor

erickt commented Dec 3, 2012

Cool! Does this copy the subset of the vector, or is it a slice subset? If a copy, how much harder would it be to change this use slices?

@ghost
Copy link
Author

ghost commented Dec 3, 2012

@erickt They're slices.

@graydon
Copy link
Contributor

graydon commented Dec 3, 2012

Definitely still interested in this being part of the language. This is great.

The only thing that stands out is that I think we'll want slightly different syntax than the bit written in the RFC. Specifically: a foo... pattern should be written ..foo (that is, use the DOTDOT token rather than ELLIPSIS, and parse it before the ident / wildcard subpattern, not after). This is to better integrate with / match existing syntax for FRU, range patterns and repeating vector-element expressions. ELLIPSIS is being removed from the language (it's only used for old-style macros presently) and the repeated element usually trails, to simplify parsing.

@graydon
Copy link
Contributor

graydon commented Dec 3, 2012

Oh, maybe a couple other substantial nits in the implementation. Lemme see..

@graydon
Copy link
Contributor

graydon commented Dec 3, 2012

That's all I can see. Probably someone else familiar with the compilation of patterns should also take a look, but over all this looks like excellent work. Thanks so much!

@ghost
Copy link
Author

ghost commented Dec 3, 2012

@graydon Thanks for the review! I'm fine with a different syntax.

I'm finishing off the pattern usefulness logic, will add a few tests for parsing errors (just noticed there aren't any) and resubmit.

@erickt
Copy link
Contributor

erickt commented Dec 3, 2012

What about destructuring from the beginning, such as [..foo, bar], or in the middle, like [foo, ..bar, baz]? The tests don't cover that functionality. How hard would it be to add that?

@ghost
Copy link
Author

ghost commented Dec 3, 2012

@erickt It would be fairly easy to add. I could imagine use cases for [foo, ..bar, baz] such as is_palindrome().

What I like about only having the [head, ..tail] variant as opposed to both [head, ..tail] and [..init, last] is that it encourages you to always process your data from left to right which makes code easier to read.

@ghost
Copy link
Author

ghost commented Dec 3, 2012

However, since this works with vectors where the (head, tail) destructuring isn't really any more natural or cheaper as in lists, I guess we could have both. :)

@ghost
Copy link
Author

ghost commented Dec 4, 2012

This should now be finding unreachable arms and determining whether or not the whole arm set is exhaustive.

Will look at the lifetime issues tomorrow.

@bstrie
Copy link
Contributor

bstrie commented Dec 4, 2012

What an awesome surprise!

Does the exhaustiveness example that you posted in the issue work?

fn every_other_element<T: Copy>(
    values: &[T]
) -> ~[T] {
    match values {
        [head, _, ..tail] => ~[head] + every_other_element(tail),
        [head] => ~[head],
        [] => ~[]
    }
}

Didn't see a test case for it, and in fact I'm not even 100% sure that it's possible to prove that this sort of thing is exhaustive in general. :)

@bstrie
Copy link
Contributor

bstrie commented Dec 4, 2012

Also, does vector matching define any irrefutable patterns that would now be acceptable in assignment position and function arguments?

@ghost
Copy link
Author

ghost commented Dec 4, 2012

@bstrie 1) Yes, that example compiles now.

  1. Technically, [..tail] is irrefutable but since it doesn't do anything useful, currently all vec patterns are rejected in assignments and function arguments.

@ghost
Copy link
Author

ghost commented Dec 7, 2012

I changed the alt checking code to preserve the region on the slice type but in the test I added the compiler doesn't actually complain due to what's probably the reason behind #3243.

The transformation pass doesn't rely on the region and mutability information so I left it as-is. re_static is already used as a placeholder in ty::normalize_ty() for that same reason.

@ghost
Copy link
Author

ghost commented Dec 7, 2012

I realised this should be rebased against incoming. I'll do that after the next review pass.

@catamorphism
Copy link
Contributor

Closing this since #4143 supersedes it.

@ghost ghost deleted the vector-destructuring branch March 9, 2014 14:04
arcnmx pushed a commit to arcnmx/rust that referenced this pull request Dec 17, 2022
Compute data layout of types

cc rust-lang#4091

Things that aren't working:
* Closures
* Generators (so no support for `Future` I think)
* Opaque types
* Type alias and associated types which may need normalization

Things that show wrong result:
* ~Enums with explicit discriminant~
* SIMD types
* ~`NonZero*` and similar standard library items which control layout with special attributes~

At the user level, I didn't put much work, since I wasn't confident about what is the best way to present this information. Currently it shows size and align for ADTs, and size, align, offset for struct fields, in the hover, similar to clangd. I used it some days and I feel I liked it, but we may consider it too noisy and move it to an assist or command.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants