resolve: Use same rules for disambiguating fresh bindings in `match` and `let` #45050

petrochenkov · 2017-10-05T20:05:57Z

Resolve Unit as a unit struct pattern in

struct Unit;

let Unit = x;

consistently with

match x {
    Unit => {}
}

It was previously an error.
(The change also applies to unit variants and constants.)

Fixes https://users.rust-lang.org/t/e0530-cannot-shadow-unit-structs-what-in-the-earthly-what/13054
(This particular change doesn't depend on a fix for the issue mentioned in https://users.rust-lang.org/t/e0530-cannot-shadow-unit-structs-what-in-the-earthly-what/13054/4)

cc @rust-lang/lang
r? @nikomatsakis

cramertj · 2017-10-05T20:13:06Z

src/test/compile-fail/const-pattern-irrefutable.rs

-               //~^ NOTE cannot be named the same as a constant
-    let d = 4; //~ ERROR let bindings cannot shadow constants
-               //~^ NOTE cannot be named the same as a constant
+    let a = 4; //~ ERROR refutable pattern in local binding: `_` not covered


Can we provide some better hint as to why this error is occurring? I could see this being rather perplexing.

cramertj · 2017-10-05T22:34:40Z

src/test/compile-fail/pattern-binding-disambiguation.rs

+        BracedStruct => {} // OK, `BracedStruct` is a fresh binding
+    }
+    match UnitVariant {
+        UnitVariant => {} // OK, `UnitVariant` is a unit variant pattern


Why isn't this a non-exhaustive error? Is UnitVariant a new match binding shadowing E::UnitVariant?

Exhaustiveness is checked in a separate late pass that isn't reached here because type checking completes with errors.

nikomatsakis · 2017-10-16T18:29:46Z

I think this is a good PR but -- like @cramertj -- I too am mildly concerned about diagnostics. That said, the naming conventions here probably make this a largely moot point (at the time when we adopted this hard error, we had not yet adopted the upper-case naming pattern for variants, if memory serves).

Errors like let bar = 22 are definitely on the confusing side, and it'd be nice if we could give a useful hint there. Seems like this shouldn't be too hard -- i.e., we can do it in check_decl_initializer, right? (if there is a type error, and the pattern is a "simple identifier" that resolves to a constant, etc...)

petrochenkov · 2017-10-16T18:38:35Z

Yeah, I'll add a note/help for #45050 (comment) once the change is decided to be desirable in general.

nikomatsakis · 2017-10-16T19:13:07Z

@petrochenkov ok, I'm in favor other than the diagnostics issue. Do you think we ought to do an FCP?

nikomatsakis · 2017-10-16T19:13:36Z

I suppose we should.

nikomatsakis · 2017-10-16T19:18:11Z

@rfcbot fcp merge

This is a change to our handling of constants that appear in irrefutable patterns. In particular, at present, it is an error to use a variant name in an irrefutable pattern, which means that people who try to do newtype'd variants on () run into some annoyances:

struct TypeError;
fn foo() -> Result<(), TypeError> { .. }

fn bar() {
    let TypeError = foo().unwrap_err(); // artificial but wev
}

This is obviously inconsistent with refutable arms. The origin of this rule goes way back to the point where we first decided to use the name resolution context to decide how to interpret a bare identifier like TypeError in a pattern. At the time, the case conventions were that variant names were lower case (e.g., we wrote none, not None), and hence we decided that seeing let type_error = foo().unwrap_err() would be too confusing to the reader, so it was better to make it be an error if a variant or constant were used in that position (but permit those in match arms). "Refuse to guess", was the basic idea.

Now that variants are traditionally named with an upper-case, this rule is less important, and the confusion tends to arise in the opposite way: people try to use unit structs in an irrefutable pattern, and get an error. Furthermore, as our diagnostics have improved, we've now started to tilt towards handling these sorts of confusing cases with improved diagnostics instead of hard errors. @petrochenkov has promised to put such a message in (the current PR does not include any special error message hangling).

I think we should remove this special-case rule and make all patterns behave the same.

rfcbot · 2017-10-16T20:17:44Z

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

cramertj · 2017-10-16T20:29:31Z

@rfcbot reviewed

kennytm · 2017-10-25T14:05:53Z

Ping @aturon @eddyb @nrc @pnkfelix @withoutboats for the ticky boxes!

kennytm · 2017-11-02T13:52:39Z

Ping @withoutboats again for the ticky boxes!

rfcbot · 2017-11-02T16:51:40Z

🔔 This is now entering its final comment period, as per the review above. 🔔

…and `let`

petrochenkov · 2017-11-04T01:00:21Z

Updated with better diagnostics for #45050 (comment)

nikomatsakis · 2017-11-06T19:12:00Z

src/test/ui/const-pattern-irrefutable.stderr

+  --> $DIR/const-pattern-irrefutable.rs:24:9
+   |
+24 |     let d = 4; //~ ERROR refutable pattern in local binding: `_` not covered
+   |         ^ interpreted as a constant pattern, not new variable


nikomatsakis · 2017-11-10T17:24:50Z

@bors r+

Going to r+. I don't anticipate any commentary from FCP, but we can always back out -- on the off chance bors gets around to landing this anytime in the next few days. ;)

bors · 2017-11-10T17:24:51Z

📌 Commit 765076f has been approved by nikomatsakis

bors · 2017-11-10T17:25:02Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: [intrinsics] add missing div and rem vector intrinsics #45804

bors · 2017-11-10T17:25:02Z

📌 Commit 765076f has been approved by nikomatsakis

bors · 2017-11-10T19:10:18Z

⌛ Testing commit 765076f with merge a35a3ab...

@nikomatsakis

resolve: Use same rules for disambiguating fresh bindings in `match` and `let` Resolve `Unit` as a unit struct pattern in ```rust struct Unit; let Unit = x; ``` consistently with ```rust match x { Unit => {} } ``` It was previously an error. (The change also applies to unit variants and constants.) Fixes https://users.rust-lang.org/t/e0530-cannot-shadow-unit-structs-what-in-the-earthly-what/13054 (This particular change doesn't depend on a fix for the issue mentioned in https://users.rust-lang.org/t/e0530-cannot-shadow-unit-structs-what-in-the-earthly-what/13054/4) cc @rust-lang/lang r? @nikomatsakis

bors · 2017-11-10T21:42:04Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing a35a3ab to master...

rust-highfive assigned nikomatsakis Oct 5, 2017

cramertj reviewed Oct 5, 2017

View reviewed changes

shepmaster added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 6, 2017

carols10cents removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Oct 9, 2017

petrochenkov added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Oct 15, 2017

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 16, 2017

rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Oct 16, 2017

alexcrichton added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 19, 2017

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Nov 2, 2017

petrochenkov added 3 commits November 4, 2017 03:09

resolve: Use same rules for disambiguating fresh bindings in match …

db0be32

…and `let`

Move some tests to UI

3386757

Give better help for identifier patterns failing exhaustiveness check

765076f

petrochenkov force-pushed the ambind branch from 5a12c24 to 765076f Compare November 4, 2017 00:59

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Nov 4, 2017

nikomatsakis reviewed Nov 6, 2017

View reviewed changes

kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 10, 2017

bors merged commit 765076f into rust-lang:master Nov 10, 2017

petrochenkov mentioned this pull request Apr 5, 2018

We shouldn't even try to resolve irrefutable patterns as constants #49680

Closed

petrochenkov deleted the ambind branch June 5, 2019 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resolve: Use same rules for disambiguating fresh bindings in `match` and `let` #45050

resolve: Use same rules for disambiguating fresh bindings in `match` and `let` #45050

petrochenkov commented Oct 5, 2017

cramertj Oct 5, 2017

cramertj Oct 5, 2017 •

edited

Loading

petrochenkov Oct 6, 2017

nikomatsakis commented Oct 16, 2017

petrochenkov commented Oct 16, 2017

nikomatsakis commented Oct 16, 2017

nikomatsakis commented Oct 16, 2017

nikomatsakis commented Oct 16, 2017

rfcbot commented Oct 16, 2017 •

edited by withoutboats

Loading

cramertj commented Oct 16, 2017

kennytm commented Oct 25, 2017

kennytm commented Nov 2, 2017

rfcbot commented Nov 2, 2017

petrochenkov commented Nov 4, 2017

nikomatsakis Nov 6, 2017

nikomatsakis commented Nov 10, 2017 •

edited

Loading

bors commented Nov 10, 2017

bors commented Nov 10, 2017

bors commented Nov 10, 2017

bors commented Nov 10, 2017

bors commented Nov 10, 2017

resolve: Use same rules for disambiguating fresh bindings in match and let #45050

resolve: Use same rules for disambiguating fresh bindings in match and let #45050

Conversation

petrochenkov commented Oct 5, 2017

cramertj Oct 5, 2017

Choose a reason for hiding this comment

cramertj Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

petrochenkov Oct 6, 2017

Choose a reason for hiding this comment

nikomatsakis commented Oct 16, 2017

petrochenkov commented Oct 16, 2017

nikomatsakis commented Oct 16, 2017

nikomatsakis commented Oct 16, 2017

nikomatsakis commented Oct 16, 2017

rfcbot commented Oct 16, 2017 • edited by withoutboats Loading

cramertj commented Oct 16, 2017

kennytm commented Oct 25, 2017

kennytm commented Nov 2, 2017

rfcbot commented Nov 2, 2017

petrochenkov commented Nov 4, 2017

nikomatsakis Nov 6, 2017

Choose a reason for hiding this comment

nikomatsakis commented Nov 10, 2017 • edited Loading

bors commented Nov 10, 2017

bors commented Nov 10, 2017

bors commented Nov 10, 2017

bors commented Nov 10, 2017

bors commented Nov 10, 2017

resolve: Use same rules for disambiguating fresh bindings in `match` and `let` #45050

resolve: Use same rules for disambiguating fresh bindings in `match` and `let` #45050

cramertj Oct 5, 2017 •

edited

Loading

rfcbot commented Oct 16, 2017 •

edited by withoutboats

Loading

nikomatsakis commented Nov 10, 2017 •

edited

Loading