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

RFC: Allow Irrefutable Patterns in if-let and while-let statements #2086

Merged
merged 11 commits into from Sep 11, 2017

Conversation

@Nokel81
Copy link
Contributor

commented Jul 27, 2017

This is an RFC for allowing irrefutable patterns in if-let and while-let statements

Edit: Rendered

[updated to link to final rendered version]

@eddyb

This comment has been minimized.

Copy link
Member

commented Jul 27, 2017

@Nokel81 The title of the PR should be the RFC name (you can edit it).

@Nokel81 Nokel81 changed the title Inital RFC commit RFC: Allow Irrefutable Patterns in if-let and while-let statements Jul 27, 2017

@main--

This comment has been minimized.

Copy link

commented Jul 27, 2017

It allows programmers to manually write the line if let _ = expr { } else { } which is generally obfuscating and not desirable. However, a different RFC should solve this.

I thoroughly disagree. This is already solved right now - the code is wrong and thus an error. You're proposing a different solution which has to deal with this.

If anything, this is an unresolved question: If we can't solve it by making it an error, how do we solve it?

This RFC is also missing its Alternatives section even though I can think of two options off the top of my head:

  • The trivial alternative: Do nothing. As your motivation explains, this only matters for macros anyways plus there already is an acceptable workaround (match). Code that needs this frequently can just package this workaround in its own macro and be done.
  • Turn the error into a lint that errors by default. Unreachable match arms are usually wrong except in some macros and that's why you can disable the warning there with #[allow(unreachable_patterns)]. The same goes for irrefutable if/while-let patterns, so it only seems natural to apply a similar solution.
@Nokel81

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2017

I will add those alternatives. From what I understand #[allow] is not allowed infront of if statements

@main--

This comment has been minimized.

Copy link

commented Jul 27, 2017

Indeed it is not but perhaps that could be changed?

In any case, you could always just add the attribute to a surrounding block.

@Nokel81

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2017

I would say that it should be changed since from what I understand it is unique to if since while allows it. Also the warning should be equivalent to the one for #2087 since it is essentially also a tautology.

@scottmcm scottmcm added the T-lang label Jul 27, 2017

@burdges

This comment has been minimized.

Copy link

commented Jul 27, 2017

I think if cfg! should work sensibly, and similarly for the patterns given to macros that you worry about here, but an obviously irrefutable pattern like if let (a,b) = foo() { .. } should be written { let (a,b) = foo(); .. } instead.

Would it work if there were a way to declare an irrefutable pattern refutable? #![ensure_refutable] perhaps?

@Nokel81

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2017

So instead of a warning, keep it a error but still have an [#allow] for it?

@nrc

This comment has been minimized.

Copy link
Member

commented Aug 1, 2017

I'd be in favour of the lint option here - seems reasonable for macros to #[allow] a statement to opt-in to this behaviour. Technically this doesn't work for expressions, but I think that just means the macro needs to use a temporary variable.

@joshtriplett

This comment has been minimized.

Copy link
Member

commented Aug 2, 2017

I likewise prefer the solution of making this an error-by-default lint that macros can turn off. For non-macro code, errors like this help Rust users (especially new users) learn and avoid using suboptimal forms that just happen to work. And macros can easily turn the lint off.

@tikue

This comment has been minimized.

Copy link

commented Aug 2, 2017

I am definitely in favor of some way to relax this error. I've stumbled on this in macros, and existing workarounds are not great. It's worth pointing out that match is not a drop-in solution to this RFC because in macros you can't always determine if there should be a _ => ... arm or not; this comes up when dealing with enums with an unknown number of variants. For such cases, the only solution right now (that I've found) is to add a NotIrrefutable variant. Pretty hacky. I didn't know about #[allow(unreachable_patterns)]. That's not so bad actually, but not very discoverable right now.

@joshtriplett

This comment has been minimized.

Copy link
Member

commented Aug 3, 2017

@Nokel81 In the @rust-lang/lang meeting, we were all in favor of the solution of making this an error-by-default lint. Could you please adjust the RFC to that effect, and then we'd be inclined to accept it?

@Nokel81

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2017

Yup, done. I hope the wording is to your liking

@joshtriplett

This comment has been minimized.

Copy link
Member

commented Aug 3, 2017

@Nokel81 Thanks for the fast response!

What's there looks reasonable, but it would help if the summary section had an explanation of the proposed change and reason (e.g. "This can break macros that want to accept patterns generically; this RFC proposes changing this to an error-by-default lint that can be disabled by such macros."). Also, in the detailed design section, please come up with a proposed name for the lint (e.g. irrefutable_conditional_pattern), and write an example of some code using it that the compiler should accept.

WATGAMER\snmalton
Added the macro reason for why this should be added to the language. …
…A proposed lint name and some code examples
@Nokel81

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2017

How is this?

@joshtriplett

This comment has been minimized.

Copy link
Member

commented Aug 4, 2017

@Nokel81 The verbiage looks great. One minor nit: lint names use underscores, not dashes, so can you s/irrefutable-let-pattern/irrefutable_let_pattern/g? With that, I think this will be ready for review.

@Nokel81

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2017

Done

@aturon

This comment has been minimized.

Copy link
Member

commented Aug 4, 2017

Thanks @Nokel81 and @joshtriplett!

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

commented Aug 4, 2017

Team member @aturon 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.

@Nokel81

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2017

@aturon Shouldn't S-waiting-on-author be removed?

@durka

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2017

As a minor note, I don't like the lint name, because a "let pattern" (i.e. Foo { x, y } in let Foo { x, y }: Foo = make_foo();) already is, and must be, irrefutable. But I don't really have a better suggestion and it can be bikeshedded later.

@Nokel81

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2017

The reason that I went with this name is because the current error is "irrefutable if-let pattern". We could change the lint but then what about for while loops (though that may be out of the scope of this RFC)

@yodaldevoid

This comment has been minimized.

Copy link

commented Aug 31, 2017

I have a minor nit with the RFC as written. Under the "Motivation" section the following phrase makes no sense to me:

... to be both much larger and include a compiler allow.

I'm not sure what this is actually trying to say, really, but perhaps it could be reworded?

if let $p = $val {
$b
}
```

This comment has been minimized.

Copy link
@ExpHP

ExpHP Sep 8, 2017

These two adjacent code blocks are confusing, especially since the second one starts a new thought.

I picture something more along the lines of:

Currently, one must write

(first example)

rather than the more natural syntax

(second example)

because reasons.

[summary]: #summary

Currently when using an if let statement and an irrefutable pattern (read always match) is used the compiler complains with an `E0162: irrefutable if-let pattern`.
The current state breaks macros who want to accept patterns generically and this RFC proposes changing this error to an error-by-default which is allowed to be disabled by such macros.

This comment has been minimized.

Copy link
@ExpHP

ExpHP Sep 8, 2017

error-by-default lint?

Some small wording updates
Changed the flow of the motivation section so as to better distinguish the code examples.
Added the word 'lint' as it was missing from the summary section
@rfcbot

This comment has been minimized.

Copy link

commented Sep 9, 2017

The final comment period is now complete.

@aturon aturon merged commit f192404 into rust-lang:master Sep 11, 2017

@aturon

This comment has been minimized.

Copy link
Member

commented Sep 11, 2017

This RFC has been merged! Tracking issue.

Thanks @Nokel81!

@Nokel81

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2017

Thank you

@Nokel81 Nokel81 deleted the Nokel81:allow_irrefutable-ifwhile-let branch Sep 11, 2017

@vitiral

This comment has been minimized.

Copy link

commented Sep 13, 2017

@Nokel81 the rendered link is now broken

@Nokel81 Nokel81 restored the Nokel81:allow_irrefutable-ifwhile-let branch Sep 13, 2017

@Nokel81

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2017

@vitiral fixed

@SoniEx2 SoniEx2 referenced this pull request Jan 17, 2018

Closed

Generic let #2297

bors added a commit to rust-lang/rust that referenced this pull request Jun 26, 2018

Auto merge of #49469 - Nokel81:allow-irrefutable-let-patterns, r=niko…
…matsakis

Implementation of RFC 2086 - Allow Irrefutable Let patterns

This is the set of changes for RFC2086. Tracking issue #44495. Rendered [here](rust-lang/rfcs#2086)

bors added a commit to rust-lang/rust that referenced this pull request Jun 26, 2018

Auto merge of #49469 - Nokel81:allow-irrefutable-let-patterns, r=niko…
…matsakis

Implementation of RFC 2086 - Allow Irrefutable Let patterns

This is the set of changes for RFC2086. Tracking issue #44495. Rendered [here](rust-lang/rfcs#2086)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.