Detect incorrect chaining of if and if let conditions and recover #103405

chenyukang · 2022-10-22T16:27:42Z

Fixes #103381

rust-highfive · 2022-10-22T16:27:45Z

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

rustbot · 2022-10-22T16:27:45Z

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

chenyukang · 2022-10-22T16:28:03Z

r? @estebank

compiler-errors

This breaks existing code -- if expressions are allowed to be their own conditions within other if statements.

fn main() {
    if true && if let 1 = 1 { true } else { true } {}
}

Before this PR: compiles

After this PR:

error: unexpected `if` in the condition expression
 --> /home/gh-compiler-errors/test.rs:2:16
  |
2 |     if true && if let 1 = 1 { true } else { true } {}
  |                ^^^
  |
help: remove the `if`
  |
2 -     if true && if let 1 = 1 { true } else { true } {}
2 +     if true && let 1 = 1 { true } else { true } {}
  |

error[E0658]: `let` expressions in this position are unstable
 --> /home/gh-compiler-errors/test.rs:2:19
  |
2 |     if true && if let 1 = 1 { true } else { true } {}
  |                   ^^^^^^^^^
  |
  = note: see issue #53667 <https://github.com/rust-lang/rust/issues/53667> for more information
  = help: add `#![feature(let_chains)]` to the crate attributes to enable

error[E0308]: mismatched types
 --> /home/gh-compiler-errors/test.rs:2:31
  |
2 |     if true && if let 1 = 1 { true } else { true } {}
  |     --------------------------^^^^----------------
  |     |                         |
  |     |                         expected `()`, found `bool`
  |     expected this to be `()`

error[E0308]: mismatched types
 --> /home/gh-compiler-errors/test.rs:2:45
  |
2 |     if true && if let 1 = 1 { true } else { true } {}
  |     ----------------------------------------^^^^--
  |     |                                       |
  |     |                                       expected `()`, found `bool`
  |     expected this to be `()`

error: aborting due to 4 previous errors

Some errors have detailed explanations: E0308, E0658.
For more information about an error, try `rustc --explain E0308`.

chenyukang · 2022-10-23T04:26:57Z

This breaks existing code -- if expressions are allowed to be their own conditions within other if statements.

This code will also compile in current compiler:

fn should_ok() {
    if true && if let x = 1 { true } else { true } {}
}

With this warning:

warning: irrefutable `if let` pattern
  --> /home/cat/code/rust/src/test/ui/parser/issue-103381.rs:13:19
   |
13 |     if true && if let x = 1 { true } else { true } {}
   |                   ^^^^^^^^^
   |
   = note: `#[warn(irrefutable_let_patterns)]` on by default
   = note: this pattern will always match, so the `if let` is useless
   = help: consider replacing the `if let` with a `let`

warning: 1 warning emitted

compiler-errors · 2022-10-23T05:30:28Z

Just noting that "irrefutable if let pattern" warning is unrelated to this issue, and is just because let x = 1 is a "irrefutable" binding, i.e. not actually conditional.

chenyukang · 2022-10-23T05:45:47Z

The job x86_64-gnu-llvm-13 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

From the UI testing result, we have this syntax：

fn shoule_match_ok() {
    #[cfg(feature = "full")]
    {
        let a = 1;
        let b = 2;
        if match a {
            1 if b == 1 => true,
            _ => false,
        } && if a > 1 { true } else { false }
        {
            true
        }
    }
}

I think it's not a readable code, but it compiled and runnable 😂
Anyway, I plan to restrict this suggestion to limited scenarios:

            // recovery from `if b && if let Some(x)`
            if op == AssocOp::LAnd
                && self.token.is_keyword(kw::If)
                && self.look_ahead(1, |t| t.is_bool_lit())
                || (self.look_ahead(1, |t| t.is_keyword(kw::Let))
                    && self.look_ahead(2, |t| {
                        t.is_ident_named(sym::Some) || t.is_ident_named(sym::None)
                    }))
            {
                // eat the `if`
                self.bump();
                self.sess.emit_err(UnexpectedIfWithIf(
                    self.prev_token.span.to(self.token.span.shrink_to_lo()),
                ));
            }

Not sure whether there is a better way.

compiler-errors · 2022-10-23T06:05:53Z

@chenyukang, this approach still has issues:

fn main() {
    if true && if true { true } else { false } {}
}

This fails to compile with your new changes:

error: unexpected `if` in the condition expression
 --> /home/gh-compiler-errors/test.rs:2:16
  |
2 |     if true && if true { true } else { false } {}
  |                ^^^
  |
help: remove the `if`
  |
2 -     if true && if true { true } else { false } {}
2 +     if true && true { true } else { false } {}
  |

error[E0308]: mismatched types
 --> /home/gh-compiler-errors/test.rs:2:26
  |
2 |     if true && if true { true } else { false } {}
  |     ---------------------^^^^-----------------
  |     |                    |
  |     |                    expected `()`, found `bool`
  |     expected this to be `()`

error[E0308]: mismatched types
 --> /home/gh-compiler-errors/test.rs:2:40
  |
2 |     if true && if true { true } else { false } {}
  |     -----------------------------------^^^^^--
  |     |                                  |
  |     |                                  expected `()`, found `bool`
  |     expected this to be `()`

error: aborting due to 3 previous errors

I'm pretty sure it's impossible to detect #103381 inside of parse_assoc_expr_with -- you need to add a suggestion to remove if only after the parser has already detected an error, probably by passing the correct state down to parse_if_after_cond or some similar function that is involved in parsing if statements.

This may be hard to do, or not worth the changes to the parser to allow this state to be tracked.

chenyukang · 2022-10-23T06:12:31Z

Ok, I will have look later Michael Goulet ***@***.***> 于 2022年10月23日周日 14:06写道：

…

@chenyukang <https://github.com/chenyukang>, this approach still has issues: fn main() { if true && if true { true } else { false } {}} This fails to compile with your new changes: error: unexpected `if` in the condition expression --> /home/gh-compiler-errors/test.rs:2:16 | 2 | if true && if true { true } else { false } {} | ^^^ | help: remove the `if` | 2 - if true && if true { true } else { false } {} 2 + if true && true { true } else { false } {} | error[E0308]: mismatched types --> /home/gh-compiler-errors/test.rs:2:26 | 2 | if true && if true { true } else { false } {} | ---------------------^^^^----------------- | | | | | expected `()`, found `bool` | expected this to be `()` error[E0308]: mismatched types --> /home/gh-compiler-errors/test.rs:2:40 | 2 | if true && if true { true } else { false } {} | -----------------------------------^^^^^-- | | | | | expected `()`, found `bool` | expected this to be `()` error: aborting due to 3 previous errors ------------------------------ I'm pretty sure it's impossible to detect #103381 <#103381> inside of parse_assoc_expr_with -- you need to add a suggestion to remove if only *after* the parser has already detected an error, probably by passing the correct state down to parse_if_after_cond or some similar function that is involved in parsing if statements. — Reply to this email directly, view it on GitHub <#103405 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABYJ5VZ7VWUZIXRMEQRUX3WETIUXANCNFSM6AAAAAARL5BSCU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Noratrieb · 2022-10-23T11:45:34Z

Hi, I've seen you changed some diagnostic structs in your PR. After #103345, the way we refer to fluent messages changed. They are now in a flat namespace with the same identifier as in the fluent file. For example, parser::cool_thing is now parser_cool_thing and parser::suggestion just suggestion.
You should rebase to the latest master and change your fluent message references as described above. Thanks!

chenyukang · 2022-10-23T12:03:59Z

This may be hard to do, or not worth the changes to the parser to allow this state to be tracked.

Take some time to investigate it.
Yes, seems it is not easy since we need to rollback too much.
I think we'd better don't try to suggest and fix this case right now, it's easy for developer fix without diagnostics:

 let _ = if true && if true { true } else { false };
 let _ = if true && if false { true } else { false };

Only add check for && if let Option...

chenyukang · 2022-11-09T13:47:48Z

@rustbot ready

camsteffen · 2022-11-09T14:40:57Z

compiler/rustc_parse/src/parser/expr.rs

+                && self.token.is_keyword(kw::If)
+                && self.look_ahead(1, |t| t.is_keyword(kw::Let))
+                && self
+                    .look_ahead(2, |t| t.is_ident_named(sym::Some) || t.is_ident_named(sym::None))


The following code is perfectly valid:

if true && if let Some(1) = Some(1) { true } else { true } { todo!() }

In general, it is always possible to write valid code containing if <some condition> && if let <some pattern>.

I don't think it's possible to detect the scenario you're looking for until after you parse the inner if let .. expression.

compiler-errors · 2022-11-10T06:26:46Z

@chenyukang, camsteffen is correct, and as I noted in my previous comment, I'm pretty sure it's impossible to detect #103381 inside of parse_assoc_expr_with. Marking as S-waiting-on-author until this PR is reformulated.

This check needs to live in another function using a different strategy, after parsing has detected a failure. Be very careful about adding parser recoveries before we've detected an error, since it's very¹ easy² to accidentally introduce changes in parser behavior while improving error messages during parsing...

chenyukang · 2022-11-10T12:50:24Z

@chenyukang, camsteffen is correct, and as I noted in my previous comment, I'm pretty sure it's impossible to detect #103381 inside of parse_assoc_expr_with. Marking as S-waiting-on-author until this PR is reformulated.

This check needs to live in another function using a different strategy, after parsing has detected a failure. Be very careful about adding parser recoveries before we've detected an error, since it's very1 easy2 to accidentally introduce changes in parser behavior while improving error messages during parsing...

Got it, thanks!
I didn't understand the trivial differences between those code pattern.

chenyukang · 2022-11-10T16:24:39Z

I think we'd better don't recover from these scenarios, but give proper diagnostics 😁
Code updated.

estebank · 2022-11-10T22:56:03Z

r? @compiler-errors as you're already familiar with the changes :)

estebank · 2022-11-10T22:57:35Z

src/test/ui/parser/issue-103381.stderr

+error: unexpected `if` in the condition expression
+  --> $DIR/issue-103381.rs:19:15
+   |
+LL |     if true && if true { true } else { false };
+   |               ^^^^
+   |
+help: remove the `if`
+   |
+LL -     if true && if true { true } else { false };
+LL +     if true && true { true } else { false };


What happens for if true && if true { true } else { false } { true } else { false };?

Given where you call the recovery it should succeed, right? In that case I believe the code changes look fine because the error is only ever precluding another, so there are no changes of regressions.

Given where you call the recovery it should succeed, right?

yeah, this is my thinking. I'll give this a better review later, but I much prefer this approach since it's already deep in the "bad path".

Yes, this code will compile successfully, I have added it as a UI.

…itions

compiler-errors · 2022-11-15T03:24:38Z

@rustbot ready

I will get around to reviewing this soon.

bors · 2022-11-15T06:58:10Z

☔ The latest upstream changes (presumably #104418) made this pull request unmergeable. Please resolve the merge conflicts.

chenyukang · 2022-11-16T09:12:53Z

@rustbot ready

conflict resolved.

compiler-errors · 2022-11-16T23:10:32Z

@bors r+

bors · 2022-11-16T23:10:34Z

📌 Commit cd2bde1 has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2022-11-16T23:10:35Z

🌲 The tree is currently closed for pull requests below priority 1. This pull request will be tested once the tree is reopened.

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#101162 (Migrate rustc_resolve to use SessionDiagnostic, part # 1) - rust-lang#103386 (Don't allow `CoerceUnsized` into `dyn*` (except for trait upcasting)) - rust-lang#103405 (Detect incorrect chaining of if and if let conditions and recover) - rust-lang#103594 (Fix non-associativity of `Instant` math on `aarch64-apple-darwin` targets) - rust-lang#104006 (Add variant_name function to `LangItem`) - rust-lang#104494 (Migrate GUI test to use functions) - rust-lang#104516 (rustdoc: clean up sidebar width CSS) - rust-lang#104550 (fix a typo) Failed merges: - rust-lang#104554 (Use `ErrorGuaranteed::unchecked_claim_error_was_emitted` less) r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned petrochenkov Oct 22, 2022

rustbot added A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 22, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 22, 2022

rust-highfive assigned estebank and unassigned petrochenkov Oct 22, 2022

This comment has been minimized.

Sign in to view

compiler-errors requested changes Oct 22, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

chenyukang force-pushed the yukang/fix-103381-and-if branch from 31bad2e to 3fea71e Compare October 23, 2022 11:47

chenyukang force-pushed the yukang/fix-103381-and-if branch from 3fea71e to 43269b6 Compare October 23, 2022 12:05

compiler-errors 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 Nov 2, 2022

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

camsteffen reviewed Nov 9, 2022

View reviewed changes

compiler-errors 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 Nov 10, 2022

chenyukang force-pushed the yukang/fix-103381-and-if branch from 43269b6 to e7998ec Compare November 10, 2022 16:22

chenyukang force-pushed the yukang/fix-103381-and-if branch from a6ea4bc to 6cd4a2a Compare November 10, 2022 16:36

rustbot assigned compiler-errors and unassigned estebank Nov 10, 2022

estebank reviewed Nov 10, 2022

View reviewed changes

chenyukang added 2 commits November 15, 2022 08:53

fix rust-lang#103381, Detect incorrect chaining of if and if let cond…

74c9a6c

…itions

move testcase for tidy

cd2bde1

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

chenyukang force-pushed the yukang/fix-103381-and-if branch from dafa06f to cd2bde1 Compare November 15, 2022 08:46

bors 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 16, 2022

matthiaskrgr mentioned this pull request Nov 18, 2022

Rollup of 8 pull requests #104573

Merged

bors merged commit 3efbf30 into rust-lang:master Nov 18, 2022

rustbot added this to the 1.67.0 milestone Nov 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect incorrect chaining of if and if let conditions and recover #103405

Detect incorrect chaining of if and if let conditions and recover #103405

chenyukang commented Oct 22, 2022

rust-highfive commented Oct 22, 2022

rustbot commented Oct 22, 2022

chenyukang commented Oct 22, 2022

This comment has been minimized.

compiler-errors left a comment

chenyukang commented Oct 23, 2022 •

edited

Loading

compiler-errors commented Oct 23, 2022

chenyukang commented Oct 23, 2022 •

edited

Loading

compiler-errors commented Oct 23, 2022 •

edited

Loading

chenyukang commented Oct 23, 2022 via email

This comment has been minimized.

Noratrieb commented Oct 23, 2022

chenyukang commented Oct 23, 2022 •

edited

Loading

chenyukang commented Nov 9, 2022

camsteffen Nov 9, 2022

compiler-errors commented Nov 10, 2022 •

edited

Loading

chenyukang commented Nov 10, 2022

chenyukang commented Nov 10, 2022

estebank commented Nov 10, 2022

estebank Nov 10, 2022 •

edited

Loading

compiler-errors Nov 10, 2022

chenyukang Nov 11, 2022

compiler-errors commented Nov 15, 2022

bors commented Nov 15, 2022

chenyukang commented Nov 16, 2022

compiler-errors commented Nov 16, 2022

bors commented Nov 16, 2022

bors commented Nov 16, 2022

Detect incorrect chaining of if and if let conditions and recover #103405

Detect incorrect chaining of if and if let conditions and recover #103405

Conversation

chenyukang commented Oct 22, 2022

rust-highfive commented Oct 22, 2022

rustbot commented Oct 22, 2022

chenyukang commented Oct 22, 2022

This comment has been minimized.

compiler-errors left a comment

Choose a reason for hiding this comment

chenyukang commented Oct 23, 2022 • edited Loading

compiler-errors commented Oct 23, 2022

chenyukang commented Oct 23, 2022 • edited Loading

compiler-errors commented Oct 23, 2022 • edited Loading

chenyukang commented Oct 23, 2022 via email

This comment has been minimized.

Noratrieb commented Oct 23, 2022

chenyukang commented Oct 23, 2022 • edited Loading

chenyukang commented Nov 9, 2022

camsteffen Nov 9, 2022

Choose a reason for hiding this comment

compiler-errors commented Nov 10, 2022 • edited Loading

chenyukang commented Nov 10, 2022

chenyukang commented Nov 10, 2022

estebank commented Nov 10, 2022

estebank Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

compiler-errors Nov 10, 2022

Choose a reason for hiding this comment

chenyukang Nov 11, 2022

Choose a reason for hiding this comment

compiler-errors commented Nov 15, 2022

bors commented Nov 15, 2022

chenyukang commented Nov 16, 2022

compiler-errors commented Nov 16, 2022

bors commented Nov 16, 2022

bors commented Nov 16, 2022

chenyukang commented Oct 23, 2022 •

edited

Loading

chenyukang commented Oct 23, 2022 •

edited

Loading

compiler-errors commented Oct 23, 2022 •

edited

Loading

chenyukang commented Oct 23, 2022 •

edited

Loading

compiler-errors commented Nov 10, 2022 •

edited

Loading

estebank Nov 10, 2022 •

edited

Loading