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

beta regression matching unit-like structs with .. #30379

Closed
durka opened this issue Dec 14, 2015 · 25 comments
Closed

beta regression matching unit-like structs with .. #30379

durka opened this issue Dec 14, 2015 · 25 comments
Assignees
Labels
P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@durka
Copy link
Contributor

durka commented Dec 14, 2015

This code

struct Empty;

match Empty {
    Empty(..) => {}
}

compiles on stable 1.5 (no RFC 218 warning) but fails on beta 1.6 with E0164.

@petrochenkov
Copy link
Contributor

This is an expected result of #29383

@durka
Copy link
Contributor Author

durka commented Dec 14, 2015

@petrochenkov breaking change with no warning cycle? I expected a warning like there is with a unitary enum variant.

@petrochenkov
Copy link
Contributor

The warning was introduced only for the case UnitVariant(..), other cases were not encountered during crater run.

@durka
Copy link
Contributor Author

durka commented Dec 14, 2015

I guess my crate wasn't up during the crater run then :( But this is the point of beta, right, to catch stuff like this?

@petrochenkov
Copy link
Contributor

I don't know, it's possible to turn all errors produced by #29383 into warnings temporarily, but IMHO it's simpler to fix the bug on your side.
This is a policy question, so let's someone else decide.
cc @nikomatsakis as a reviewer of #29383

EDIT: Oh wait, @pnkfelix was the reviewer, but I remember nikomatsakis commented there about errors/warnings.

@alexcrichton
Copy link
Member

triage: I-nominated

@alexcrichton alexcrichton added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 14, 2015
@durka
Copy link
Contributor Author

durka commented Dec 14, 2015

In this case it's in a complex macro (my guard crate) so there really is no workaround until #![feature(braced_empty_structs)]. But you may overrule me by saying tt-based macros are never stable :)

@bluss bluss added the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label Dec 14, 2015
@petrochenkov
Copy link
Contributor

@durka

In this case it's in a complex macro (my guard crate)

https://github.com/durka/guard/blob/master/src/lib.rs#L71
Hmm, does something like self::Empty work as a workaround for this path/identifier problem?

@durka
Copy link
Contributor Author

durka commented Dec 14, 2015

Yes (though it's ugly), unless the struct/enum is local to the function/block. So it's a rather niche case I suppose.

@durka
Copy link
Contributor Author

durka commented Dec 14, 2015

@petrochenkov actually no, that doesn't seem to help for structs imported from other modules: http://is.gd/TlO9Ro

@petrochenkov
Copy link
Contributor

@durka
Then the module name can be used instead of self - foo::Empty.
I know this is all horrible, but Empty(..) itself is a workaround too.

@pnkfelix
Copy link
Member

Hmm, I must have misunderstood #29383 ; I had thought there was a warning cycle associated with this change... e.g. wasn't this comment evidence of such a warning? What did I miss here...

@durka
Copy link
Contributor Author

durka commented Dec 14, 2015

@pnkfelix in beta, enums warn while structs error.

@pnkfelix
Copy link
Member

@pnkfelix in beta, enums warn while structs error.

Ah! Okay, I see.

@pnkfelix
Copy link
Member

@petrochenkov

it's possible to turn all errors produced by #29383 into warnings temporarily, but IMHO it's simpler to fix the bug on your side. This is a policy question, so let's someone else decide.

(I think the appropriate action here is to make them all warnings. But hopefully we'll settle this at the compiler team meeting in a few days.)

@petrochenkov
Copy link
Contributor

@durka
By the way, inability to use identifier in that position is not a parsing/expansion problem, it's a name resolution quirk. Patterns in match and let behave differently - let unlike match never considers identifiers to be variants or consts with rationale "it's rarely what you want".

struct S;

fn main() {
    let S = S; // error: declaration of `S` shadows an enum variant or unit-like struct in scope
}

@durka
Copy link
Contributor Author

durka commented Dec 14, 2015

Oh interesting, I didn't realize that was a hard error in some places.

On Mon, Dec 14, 2015 at 6:52 PM, Vadim Petrochenkov <
notifications@github.com> wrote:

@durka https://github.com/durka
By the way, inability to use identifier in that position is not a
parsing/expansion problem, it's a name resolution quirk. Patterns in match
and let behave differently - let unlike match never considers identifiers
to be variants or consts with rationale "it's rarely what you want"
https://github.com/rust-lang/rust/blob/master/src/librustc_resolve/lib.rs#L2519

struct S;

fn main() {
let S = S; // error: declaration of S shadows an enum variant or unit-like struct in scope
}


Reply to this email directly or view it on GitHub
#30379 (comment).

@petrochenkov
Copy link
Contributor

I still think reverting the error doesn't worth the effort.
The broken code in https://github.com/durka/guard is a test code, testing some pretty contrived construction equivalent to if let S(..) = S { /*do nothing*/ } else { return } or if false { return }.

@nikomatsakis
Copy link
Contributor

On Mon, Dec 14, 2015 at 03:52:40PM -0800, Vadim Petrochenkov wrote:

By the way, inability to use identifier in that position is not a parsing/expansion problem, it's a name resolution quirk. Patterns in match and let behave differently - let unlike match never considers identifiers to be variants or consts with rationale "it's rarely what you want"

Huh, yet an other quirk of resolve. I can't say I agree with that
logic. Well, it IS rarely what you want, but I'd probably still treat
them the same! Sigh.

@pnkfelix pnkfelix self-assigned this Dec 17, 2015
@pnkfelix
Copy link
Member

it's possible to turn all errors produced by #29383 into warnings temporarily, but [...]

Compiler team discussed this and decided that we'd prefer to take this tack.

I'll take care of this (after all, I overlooked it in my review of the original PR).

@brson
Copy link
Contributor

brson commented Jan 6, 2016

@pnkfelix Two weeks until this is released. Needs a fix soon.

@pnkfelix
Copy link
Member

pnkfelix commented Jan 6, 2016

@brson thanks, lost this in shuffle of other work

@pnkfelix
Copy link
Member

pnkfelix commented Jan 7, 2016

triage: P-high

@rust-highfive rust-highfive added the P-high High priority label Jan 7, 2016
bors added a commit that referenced this issue Jan 11, 2016
…rors, r=nikomatsakis

Downgrade unit struct match via S(..) warnings to errors

The error signalling was introduced in #29383

It was noted as a warning-cycle-less regression in #30379

Fix #30379
@pnkfelix pnkfelix reopened this Jan 12, 2016
@pnkfelix
Copy link
Member

This is a beta issue; it was an accident for me to close it in PR #30753 itself, since that solely affected nightly.

@brson
Copy link
Contributor

brson commented Jan 12, 2016

Merged on beta.

@brson brson closed this as completed Jan 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants