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

Tuple struct patterns can match unit structs #28692

Closed
petrochenkov opened this issue Sep 27, 2015 · 10 comments
Closed

Tuple struct patterns can match unit structs #28692

petrochenkov opened this issue Sep 27, 2015 · 10 comments
Labels

Comments

@petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Sep 27, 2015

This compiles on stable and nightly, but it shouldn't:

struct S;

fn main() {
    let S(..) = S;
}
@steveklabnik
Copy link
Member

@steveklabnik steveklabnik commented Sep 28, 2015

/cc @rust-lang/lang

This kind of makes sense because .. is "doesn't matter", but it's not very useful. I'm not sure if we want to disallow it or not.

@steveklabnik steveklabnik added the A-lang label Sep 28, 2015
@petrochenkov
Copy link
Contributor Author

@petrochenkov petrochenkov commented Sep 28, 2015

The problem is that "kinds" of structs are different, ()-structs and {}-structs never mix, normally.
I'm working on a fix, it's a direct continuation of #28336

@tbu-
Copy link
Contributor

@tbu- tbu- commented Sep 28, 2015

I think there was a recent RFC touching this.

@arielb1
Copy link
Contributor

@arielb1 arielb1 commented Sep 28, 2015

@petrochenkov

You can't match { } structs/variants because they don't have a value namespace entry, only unit structs/variants.

@durka
Copy link
Contributor

@durka durka commented Oct 12, 2015

To be honest, I don't see why S(..) can't match a {}-struct as well.

@arielb1
Copy link
Contributor

@arielb1 arielb1 commented Oct 12, 2015

@durka

You will get an unresolved enum variant, struct or const error.

@durka
Copy link
Contributor

@durka durka commented Oct 12, 2015

@arielb1 I know it doesn't work, but I don't see why it couldn't be made to work.

@pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Oct 13, 2015

@durka see rust-lang/rfcs#218 ; in particular https://github.com/rust-lang/rfcs/blob/master/text/0218-empty-struct-with-braces.md#empty-tuple-structs ; we are allowing unit and braced structs to mix in ways that we do not want to allow tuple structs to mix as well.

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Oct 13, 2015

Regarding the ability of S(..) to matching against a "unit struct", I've always seen this as a feature -- basically a way to enumerate a variant without saying anything about how much data is in it -- but I guess it is a bit fishy, particularly given the distinction between brace and tuple structs. I'm not really sure what my opinion is on this matter yet.

bors added a commit that referenced this issue Oct 14, 2015
This patch uses the same data structures for structs and enum variants in AST and HIR. These changes in data structures lead to noticeable simplification in most of code dealing with them.
I didn't touch the top level, i.e. `ItemStruct` is still `ItemStruct` and not `ItemEnum` with one variant, like in the type checker.
As part of this patch, structures and variants get the `kind` field making distinction between "normal" structs, tuple structs and unit structs explicit instead of relying on the number of fields and presence of constructor `NodeId`. In particular, we can now distinguish empty tuple structs from unit structs, which was impossible before! Comprehensive tests for empty structs are added and some improvements to empty struct feature gates are made. Some tests don't pass due to issue #28692 , they are still there for completeness, but are commented out.
This patch fixes issue mentioned in #16819 (comment), now emptiness of tuple structs is checked after expansion.
It also touches #28750 by providing span for visit_struct_def
cc #28336

r? @nrc
bors added a commit that referenced this issue Oct 14, 2015
This patch uses the same data structures for structs and enum variants in AST and HIR. These changes in data structures lead to noticeable simplification in most of code dealing with them.
I didn't touch the top level, i.e. `ItemStruct` is still `ItemStruct` and not `ItemEnum` with one variant, like in the type checker.
As part of this patch, structures and variants get the `kind` field making distinction between "normal" structs, tuple structs and unit structs explicit instead of relying on the number of fields and presence of constructor `NodeId`. In particular, we can now distinguish empty tuple structs from unit structs, which was impossible before! Comprehensive tests for empty structs are added and some improvements to empty struct feature gates are made. Some tests don't pass due to issue #28692 , they are still there for completeness, but are commented out.
This patch fixes issue mentioned in #16819 (comment), now emptiness of tuple structs is checked after expansion.
It also touches #28750 by providing span for visit_struct_def
cc #28336

r? @nrc
@petrochenkov
Copy link
Contributor Author

@petrochenkov petrochenkov commented Oct 14, 2015

@nikomatsakis
If we follow the route mentioned in rust-lang/rfcs#218 (comment), then S{..} will naturally be a "match anything" pattern for structs/variants.

bors added a commit that referenced this issue Nov 28, 2015
Fixes #28692
Fixes #28992
Fixes some other similar issues (see the tests)

[breaking-change], needs crater run (cc @brson or @alexcrichton )

The pattern with parens `UnitVariant(..)` for unit variants seems to be popular in rustc (see the second commit), but mostly used by one person (@nikomatsakis), according to git blame. If it causes breakage on crates.io I'll add an exceptional case for it.
bors added a commit that referenced this issue Nov 28, 2015
Fixes #28692
Fixes #28992
Fixes some other similar issues (see the tests)

[breaking-change], needs crater run (cc @brson or @alexcrichton )

The pattern with parens `UnitVariant(..)` for unit variants seems to be popular in rustc (see the second commit), but mostly used by one person (@nikomatsakis), according to git blame. If it causes breakage on crates.io I'll add an exceptional case for it.
@bors bors closed this in #29383 Nov 28, 2015
critiqjo pushed a commit to critiqjo/rustdoc that referenced this issue Dec 16, 2016
Fixes rust-lang/rust#28692
Fixes rust-lang/rust#28992
Fixes some other similar issues (see the tests)

[breaking-change], needs crater run (cc @brson or @alexcrichton )

The pattern with parens `UnitVariant(..)` for unit variants seems to be popular in rustc (see the second commit), but mostly used by one person (@nikomatsakis), according to git blame. If it causes breakage on crates.io I'll add an exceptional case for it.
@steveklabnik steveklabnik added the T-lang label Mar 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants