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

[WIP; Crater] Use parse_cond_expr instead parse_expr in if guards #74315

Closed
wants to merge 2 commits into from

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Jul 14, 2020

This PR improves diagnostic a bit for #15980.

@rust-highfive
Copy link
Collaborator

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 14, 2020
@tesuji tesuji changed the title Use parse_cond_expr instead parse_expr in if guards Use parse_cond_expr instead parse_expr in if guards Jul 14, 2020
@tesuji
Copy link
Contributor Author

tesuji commented Jul 14, 2020

I added a test for unimplemented #51114 , also cc #74232 .

@tesuji tesuji force-pushed the ifletguard branch 2 times, most recently from a99f28e to 2ddb949 Compare July 14, 2020 08:39
@estebank
Copy link
Contributor

I think that the change is correct but I want a crater run in case this affects code out in the wild.

@bors try @craterbot check

@bors
Copy link
Contributor

bors commented Jul 14, 2020

⌛ Trying commit a1a3d97 with merge 0ab79c4b48ad19260fa46623f452c1b403a360e8...

@bors
Copy link
Contributor

bors commented Jul 14, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 0ab79c4b48ad19260fa46623f452c1b403a360e8 (0ab79c4b48ad19260fa46623f452c1b403a360e8)

@estebank
Copy link
Contributor

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-74315 created and queued.
🤖 Automatically detected try build 0ab79c4b48ad19260fa46623f452c1b403a360e8
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 14, 2020
Comment on lines +1 to +5
// This function is to check whether `if-let` guard error points to
// `let_chains` feature gate, which is wrong and confuse people.
pub fn foo(a: &[u8], b: bool) -> bool {
match b {
true if let [1, 2, 3, ..] = a => true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it is a subset of "let chains" (or rather, let expressions in more positions), at most you should change what feature-gate it points to.

Looking around at the let chains tests, you want all of these cases to emit feature-gate errors:

fn _macros() {
macro_rules! noop_expr { ($e:expr) => {}; }
noop_expr!((let 0 = 1));
//~^ ERROR `let` expressions in this position are experimental [E0658]
macro_rules! use_expr {
($e:expr) => {
if $e {}
while $e {}
}
}
use_expr!((let 0 = 1 && 0 == 0));
//~^ ERROR `let` expressions in this position are experimental [E0658]
//~| ERROR `let` expressions are not supported here
//~| ERROR `let` expressions are not supported here
use_expr!((let 0 = 1));
//~^ ERROR `let` expressions in this position are experimental [E0658]
//~| ERROR `let` expressions are not supported here
//~| ERROR `let` expressions are not supported here
#[cfg(FALSE)] (let 0 = 1);
//~^ ERROR `let` expressions in this position are experimental [E0658]
use_expr!(let 0 = 1);
//~^ ERROR no rules expected the token `let`
// ^--- FIXME(53667): Consider whether `Let` can be added to `ident_can_begin_expr`.
}

Note that noop_expr! and #[cfg(FALSE)] must emit feature-gate errors despite those AST nodes never reaching HIR lowering.

I think you should copy that entire test, remove _while, and then put every if let in a guard, e.g.:

match () { () if let ... = ... => {} }

@tesuji tesuji changed the title Use parse_cond_expr instead parse_expr in if guards [WIP; Crater] Use parse_cond_expr instead parse_expr in if guards Jul 20, 2020
@craterbot
Copy link
Collaborator

🚧 Experiment pr-74315 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-74315 is completed!
📊 12 regressed and 4 fixed (113052 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jul 24, 2020
@tesuji
Copy link
Contributor Author

tesuji commented Jul 24, 2020

Closing in favor of #74566.
Will triage crater there.

@tesuji tesuji closed this Jul 24, 2020
@tesuji tesuji deleted the ifletguard branch July 24, 2020 16:07
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 22, 2020
Gate if-let guard feature

Enhanced on rust-lang#74315. That PR is in crater queue so I don't want to push to it.

Close  rust-lang#74232
cc rust-lang#51114
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants