-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Implementation of RFC 2086 - Allow Irrefutable Let patterns #49469
Implementation of RFC 2086 - Allow Irrefutable Let patterns #49469
Conversation
// option. This file may not be copied, modified, or distributed | ||
// except according to those terms. | ||
|
||
// check that #[allow_irrefutable_lets] is feature-gated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change this comment to
// gate-test-irrefutable_let_pattern
so that the computer can recognize it.
Please fix the filename of the new unstable book entry. It should be called |
|
||
// gate-test-irrefutable_let_pattern | ||
fn main() { | ||
#[allow_irrefutable_lets] //~ ERROR allow_irrefutable_lets attribute is currently unstable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be #[allow(irrefutable_let_pattern)]
I believe.
This comment has been minimized.
This comment has been minimized.
I don't quite understand what that message means, how would I make it correspond? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot to rename allow_irrefutable_lets.md
|
||
// gate-test-irrefutable_let_pattern | ||
fn main() { | ||
#[allow(irrefutable_let_pattern)] //~ ERROR allow_irrefutable_lets attribute is currently unstable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is too long. Split it into two lines:
#[allow(…)]
//~^ ERROR …
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@TimNN looks like the bot got some false result |
This comment has been minimized.
This comment has been minimized.
Okay, now I am confused. What is the conversion for naming the unstable book file? |
(Author fixed the code; Travis is still unhappy)
@Nokel81 🤔 Maybe try to use hyphens instead of underscores? Tidy should be fixed to suggest the naming convention if this is the case 😑 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
So here it says to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look great! Presuming we can pacify travis, the only thing I think we need is a test. I imagine something that (a) enables the feature gate and then (b) adds #![deny(irrefutable_let_pattern)]
and shows that indeed we get a lint error.
|
||
// gate-test-irrefutable_let_pattern | ||
fn main() { | ||
#[allow(irrefutable_let_pattern)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you have the allow
here? Without the feature gate, there shouldn't be any lints generated, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is correct, however, from the RFC the default is a deny lint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what I think is we should have, make sure that it fails with no gates or allow. Add the feature gate and check that it denies and then one with the feature gate and the allow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test was here to make sure that the feature gate was also required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the default is a deny lint
Hmm, that's odd. We don't usually do deny-by-default lints...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case I agree testing all the combinations is good. The matter of default setting is separate and can be discussed on the tracking issue.
☔ The latest upstream changes (presumably #49696) made this pull request unmergeable. Please resolve the merge conflicts. |
So while looking in |
I would have expected the default to be "warn", not allow |
Makes more sense now |
(Please remember to squash the commits with an informative message before merging this.) |
Failure seems unrelated, manually restarted the build. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Ok, this failure is legit. Apparently Travis times out (30m) while doctesting the new page in the unstable book. |
Is "doctest" a specific thing? Is there a page I can read so I can manually check the unstable book page?Or do I have to write some explicit tests for the page?
|
Doctesting means trying to compile code snippets in a documentation page. It's done automatically to ensure all code snippets in the Rust documentation are valid. The only code block in that page is this, so that's probably the cause. |
I see, will fix this tonight then.
|
@pietroalbini I have looked into the code that is in the unstable book page but it is the exact same (except for comments) of the code in |
@Nokel81 can you try not having an infinite loop in that snippet? |
r=me if travis is happy |
@bors r=nikomatsakis |
📌 Commit 9168034 has been approved by |
⌛ Testing commit 9168034 with merge c191d63009a37669b90dd060ed2000e20b22a0e6... |
💔 Test failed - status-travis |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
1 similar comment
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@bors retry |
…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)
☀️ Test successful - status-appveyor, status-travis |
This is the set of changes for RFC2086. Tracking issue #44495. Rendered here