Rust: Tracking issue for RFC 2086: Allow Irrefutable Patterns in if-let and while-let statements

[aturon]

This is a tracking issue for the RFC "Allow Irrefutable Patterns in if-let and while-let statements" (rust-lang/rfcs#2086).

Steps:

• [x] Implement the RFC (mentoring instructions here)
• [x] Adjust documentation (https://github.com/rust-lang-nursery/reference/issues/507)
• [x] Stabilization PR (https://github.com/rust-lang/rust/pull/57535)

[arielb1]

Mentoring Instructions

This is actually an easy one:

The code that emits the error message is the check_arms function here:
https://github.com/rust-lang/rust/blob/325ba23d5525ecdd555f19c7f527bce913dfd756/src/librustc_const_eval/check_match.rs#L317

It nicely contains branches for if let and while let, so we just need to adapt them to emit lints instead of errors. We emit lints using the lint_node function - you can see an example calls of it below in the function. Remember to add a feature gate.

You'll also want to declare the new lints you are using, which is done in:
https://github.com/rust-lang/rust/blob/325ba23d5525ecdd555f19c7f527bce913dfd756/src/librustc/lint/builtin.rs#L19
Just add another declare_lint! for each new lint.

Feature Gates

I should add some version of this to the README.md, but until then:

Also, you'll want to feature gate the new feature, so that it can't be used accidentally until its stabilized. For that, add a feature gate to the list of feature gates here:
https://github.com/rust-lang/rust/blob/325ba23d5525ecdd555f19c7f527bce913dfd756/src/libsyntax/feature_gate.rs#L388
And link this tracking issue (#44495). You can test the feature in tcx.sess (e.g. tcx.sess.features.borrow().specialization). If it's not present, you should maintain the old behavior.

Don't forget to add tests, both for the new behavior and the old one (the test that the old behavior remains without a feature gate should be in src/test/compile-fail/feature-gate-YOUR-FEATURE.rs).

[Nokel81]

I will now start on the implementation of this

[nikomatsakis]

@Nokel81 just checking in -- how's it going? Any questions?

[Nokel81]

No, school work came up but I should have a PR ready by Wednesday

[davidtwco]

After discussion with @nikomatsakis in Gitter, I'm going to take a stab at this one.

[Nokel81]

Okay, I just haven't had time to finish my implementation what with school

[davidtwco]

If you've already started on this then I'm happy to leave it @Nokel81.

[Nokel81]

It looks like I should have some time this weekend to finish it so from a time perspective I would prefer it if I could finish

[davidtwco]

Sure thing.

[nikomatsakis]

Anybody working on this actively?

[Nokel81]

I am and I have what I believe to be a full solution but I have had trouble compiling rustc because I am on windows with VS2017. I even tried following the instructions for not VS2015 but that didn't seem to work

[nikomatsakis]

@Nokel81 Sorry, missed that message -- perhaps give some more details of the problem you are encountering? (Alternatively, can you point us at your branch..?)

[nikomatsakis]

(Though honestly the best thing is probably to hop on gitter or maybe #rust on IRC and ask about, since I at least don't know much about getting Rust to build on Windows.)

[Nokel81]

Okay thanks. I will try that tomorrow

[nikomatsakis]

@Nokel81 any luck?

[austinbes]

@nokel81 If you're still having trouble building, I'm interested in taking a crack at this

[Nokel81]

I believe that I have solved the building problem and should be able to get a PR tonight. Thanks for the offer though.

[austinbes]

Good to hear!

[Nokel81]

45858 I have gotten some work done on this.

[Nokel81]

So somethings that @nikomatsakis brought up is that the RFC currently states for the lint to be error (actually Deny) by default. He brought up that this is generally not the case with lints. What are people's thoughts on this matter. I would be fine with moving it to a warn lint by default

[Ixrec]

My interpretation of the comments on https://github.com/rust-lang/rfcs/pull/2086 is that the lint was proposed as deny/error-by-default rather than warn-by-default because macros are _the only_ reason we don't want to keep this as a hard error. Although nobody ever explicitly made that argument, I think it's a good one. The other case of a deny-by-default lint that I'm aware of is our epoch/edition policy for "removing" stable syntax/features, which you could rephrase as old editions are _the only_ reason we don't want a hard error. Perhaps we could generalize those cases to:

deny-by-default makes sense when 'typical code' should never have a reason to do this (with some justification better than our inability to imagine a good reason), and it would be a hard error if not for [use case in atypical code]

afaik the main argument against deny-by-default lints in principle is that "lint" usually implies best effort, fuzzy-at-best guarantees, therefore a deny-by-default lint implies non-deterministic compilation failures. Since this one would clearly be deterministic, it seems fine to me.

tl;dr weak preference for deny-by-default, though really either is fine by me.

[scottmcm]

The other reason for warn-by-default is that sometimes work-in-progress code happens to do things (like have dead code) that don't make sense when it's done, but on which we don't want to block running tests because it doesn't mean that the code that's there is necessarily wrong.

Personally I also have a weak preference for deny-by-default, since it seems like a fundamental mis­under­standing or typo that means it's not doing what you expected (especially if one accidentally has if let x = my_option instead of if let Some(x) = my_option, say), but I could also see an argument for warn.

I think my suggestion for now is to make this just a "turn a hard error into a lint error" change, following the RFC, and let another issue debate whether it should actually be a warning.

[Centril]

My preference is for making this warn-by-default since this is more a matter of style than about correctness and also because of the WIP reasoning due to @scottmcm.

This being solely about macros is no longer the case with https://github.com/rust-lang/rfcs/pull/2497:

if let P0 = E0
    && let P1 = E1
    && let P2 = E2
{
    ...
}

Here P1 could be irrefutable, but not P0 and P1.
I think it makes sense stylistically for P1 to do some irrefutable binding that will then be used by E2.

I would suggest that the rule be that warning(s) be emitted if the first or the last let P_i = E_i in the chain are irrefutable. Here, this would mean that if P1 is irrefutable, no warning is emitted.

[Centril]

@rfcbot merge

This was implemented in https://github.com/rust-lang/rust/pull/49469 which was merged on 2018-06-26 which was 11 weeks ago (more than enough...). I thus propose that we stabilize it with an amendment to make the default lint level Warn instead of Deny (reasons outlined in https://github.com/rust-lang/rust/issues/44495#issuecomment-421588956).

Unstable book:

• https://github.com/rust-lang/rust/blob/master/src/doc/unstable-book/src/language-features/irrefutable-let-patterns.md

Tests:

• https://github.com/rust-lang/rust/blob/master/src/test/ui/should-fail-with_gate_irrefutable_pattern_deny.rs
• https://github.com/rust-lang/rust/blob/master/src/test/ui/run-pass/binding/allow_irrefutable_let_patterns.rs

[rfcbot]

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

• [x] @Centril
• [x] @Zoxc
• [x] @aturon
• [x] @cramertj
• [x] @eddyb
• [x] @estebank
• [x] @joshtriplett
• [x] @michaelwoerister
• [x] @nagisa
• [ ] @nikomatsakis
• [x] @nrc
• [x] @oli-obk
• [x] @petrochenkov
• [x] @pnkfelix
• [x] @scottmcm
• [x] @varkor
• [ ] @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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.

[scottmcm]

@rfcbot reviewed

That said, I still lean towards deny here. I don't think my "WIP reasoning" is a strong argument for warn here, because there obviously aren't any more alternatives that you could be intending to add later.

[rfcbot]

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

[Nokel81]

I would lean towards deny for a single if let but warn for multiple if lets because of the set up process that multiple if lets can allow for

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

[Centril]

@Nokel81 would you be willing to write up a stabilization PR?

[Nokel81]

Sure, however, there was a little discussion on the level. Shall I go with what was proposed in the merge by you (that is Warn instead of Deny)

[Centril]

@Nokel81 Thank you.
What was proposed in the FCP merge proposal; that is: Warn.

[Nokel81]

I will see if I can do this tonight

[Centril]

@Nokel81 any progress? :)

[Nokel81]

@Centril I have made the code change but I wanted to make sure that I could build it before making the PR. My build fails (even without the code change so I think it is something to do with my environment) with the following error

error: failed to run custom build command for `alloc_jemalloc v0.0.0 (/home/sebastian/rust/src/liballoc_jemalloc)`                                      

[Nokel81]

55639

Would any of you be able to review the stabilization PR?

[Centril]

Stabilized in https://github.com/rust-lang/rust/pull/57535.
Reference issue: https://github.com/rust-lang-nursery/reference/issues/507.

Nothing left to do, so closing out.