Rollup merge of #69878 - estebank:chained-ops, r=Centril

Tweak chained operators diagnostic

Use more selective spans
Improve suggestion output
Be more selective when displaying suggestions
Silence some knock-down type errors

r? @Centril

src/librustc_parse/parser/diagnostics.rs

@@ -17,7 +17,6 @@
 use rustc_span::{MultiSpan, Span, SpanSnippetError, DUMMY_SP};
 
 use log::{debug, trace};
-use std::mem;
 
 const TURBOFISH: &str = "use `::<...>` instead of `<...>` to specify type arguments";
 
@@ -459,9 +458,28 @@ fn attempt_chained_comparison_suggestion(
         err: &mut DiagnosticBuilder<'_>,
         inner_op: &Expr,
         outer_op: &Spanned<AssocOp>,
-    ) {
+    ) -> bool /* advanced the cursor */ {
         if let ExprKind::Binary(op, ref l1, ref r1) = inner_op.kind {
-            match (op.node, &outer_op.node) {
+            if let ExprKind::Field(_, ident) = l1.kind {
+                if ident.as_str().parse::<i32>().is_err() && !matches!(r1.kind, ExprKind::Lit(_)) {
+                    // The parser has encountered `foo.bar<baz`, the likelihood of the turbofish
+                    // suggestion being the only one to apply is high.
+                    return false;
+                }
+            }
+            let mut enclose = |left: Span, right: Span| {
+                err.multipart_suggestion(
+                    "parenthesize the comparison",
+                    vec![
+                        (left.shrink_to_lo(), "(".to_string()),
+                        (right.shrink_to_hi(), ")".to_string()),
+                    ],
+                    Applicability::MaybeIncorrect,
+                );
+            };
+            return match (op.node, &outer_op.node) {
+                // `x == y == z`
+                (BinOpKind::Eq, AssocOp::Equal) |
                 // `x < y < z` and friends.
                 (BinOpKind::Lt, AssocOp::Less) | (BinOpKind::Lt, AssocOp::LessEqual) |
                 (BinOpKind::Le, AssocOp::LessEqual) | (BinOpKind::Le, AssocOp::Less) |
@@ -472,35 +490,55 @@ fn attempt_chained_comparison_suggestion(
                         self.span_to_snippet(e.span)
                             .unwrap_or_else(|_| pprust::expr_to_string(&e))
                     };
-                    err.span_suggestion(
-                        inner_op.span.to(outer_op.span),
-                        "split the comparison into two...",
-                        format!(
-                            "{} {} {} && {} {}",
-                            expr_to_str(&l1),
-                            op.node.to_string(),
-                            expr_to_str(&r1),
-                            expr_to_str(&r1),
-                            outer_op.node.to_ast_binop().unwrap().to_string(),
-                        ),
-                        Applicability::MaybeIncorrect,
-                    );
-                    err.span_suggestion(
-                        inner_op.span.to(outer_op.span),
-                        "...or parenthesize one of the comparisons",
-                        format!(
-                            "({} {} {}) {}",
-                            expr_to_str(&l1),
-                            op.node.to_string(),
-                            expr_to_str(&r1),
-                            outer_op.node.to_ast_binop().unwrap().to_string(),
-                        ),
+                    err.span_suggestion_verbose(
+                            inner_op.span.shrink_to_hi(),
+                        "split the comparison into two",
+                        format!(" && {}", expr_to_str(&r1)),
                         Applicability::MaybeIncorrect,
                     );
+                    false // Keep the current parse behavior, where the AST is `(x < y) < z`.
                 }
-                _ => {}
-            }
+                // `x == y < z`
+                (BinOpKind::Eq, AssocOp::Less) | (BinOpKind::Eq, AssocOp::LessEqual) |
+                (BinOpKind::Eq, AssocOp::Greater) | (BinOpKind::Eq, AssocOp::GreaterEqual) => {
+                    // Consume `z`/outer-op-rhs.
+                    let snapshot = self.clone();
+                    match self.parse_expr() {
+                        Ok(r2) => {
+                            // We are sure that outer-op-rhs could be consumed, the suggestion is
+                            // likely correct.
+                            enclose(r1.span, r2.span);
+                            true
+                        }
+                        Err(mut expr_err) => {
+                            expr_err.cancel();
+                            *self = snapshot;
+                            false
+                        }
+                    }
+                }
+                // `x > y == z`
+                (BinOpKind::Lt, AssocOp::Equal) | (BinOpKind::Le, AssocOp::Equal) |
+                (BinOpKind::Gt, AssocOp::Equal) | (BinOpKind::Ge, AssocOp::Equal) => {
+                    let snapshot = self.clone();
+                    // At this point it is always valid to enclose the lhs in parentheses, no
+                    // further checks are necessary.
+                    match self.parse_expr() {
+                        Ok(_) => {
+                            enclose(l1.span, r1.span);
+                            true
+                        }
+                        Err(mut expr_err) => {
+                            expr_err.cancel();
+                            *self = snapshot;
+                            false
+                        }
+                    }
+                }
+                _ => false,
+            };
         }
+        false
     }
 
     /// Produces an error if comparison operators are chained (RFC #558).
@@ -534,31 +572,26 @@ pub(super) fn check_no_chained_comparison(
             |this: &Self, span| Ok(Some(this.mk_expr(span, ExprKind::Err, AttrVec::new())));
 
         match inner_op.kind {
-            ExprKind::Binary(op, _, _) if op.node.is_comparison() => {
-                // Respan to include both operators.
-                let op_span = op.span.to(self.prev_token.span);
-                let mut err =
-                    self.struct_span_err(op_span, "comparison operators cannot be chained");
-
-                // If it looks like a genuine attempt to chain operators (as opposed to a
-                // misformatted turbofish, for instance), suggest a correct form.
-                self.attempt_chained_comparison_suggestion(&mut err, inner_op, outer_op);
+            ExprKind::Binary(op, ref l1, ref r1) if op.node.is_comparison() => {
+                let mut err = self.struct_span_err(
+                    vec![op.span, self.prev_token.span],
+                    "comparison operators cannot be chained",
+                );
 
                 let suggest = |err: &mut DiagnosticBuilder<'_>| {
                     err.span_suggestion_verbose(
-                        op_span.shrink_to_lo(),
+                        op.span.shrink_to_lo(),
                         TURBOFISH,
                         "::".to_string(),
                         Applicability::MaybeIncorrect,
                     );
                 };
 
-                if op.node == BinOpKind::Lt &&
-                    outer_op.node == AssocOp::Less ||  // Include `<` to provide this recommendation
-                    outer_op.node == AssocOp::Greater
-                // even in a case like the following:
+                // Include `<` to provide this recommendation even in a case like
+                // `Foo<Bar<Baz<Qux, ()>>>`
+                if op.node == BinOpKind::Lt && outer_op.node == AssocOp::Less
+                    || outer_op.node == AssocOp::Greater
                 {
-                    //     Foo<Bar<Baz<Qux, ()>>>
                     if outer_op.node == AssocOp::Less {
                         let snapshot = self.clone();
                         self.bump();
@@ -572,7 +605,7 @@ pub(super) fn check_no_chained_comparison(
                         {
                             // We don't have `foo< bar >(` or `foo< bar >::`, so we rewind the
                             // parser and bail out.
-                            mem::replace(self, snapshot.clone());
+                            *self = snapshot.clone();
                         }
                     }
                     return if token::ModSep == self.token.kind {
@@ -597,7 +630,7 @@ pub(super) fn check_no_chained_comparison(
                                 expr_err.cancel();
                                 // Not entirely sure now, but we bubble the error up with the
                                 // suggestion.
-                                mem::replace(self, snapshot);
+                                *self = snapshot;
                                 Err(err)
                             }
                         }
@@ -617,15 +650,33 @@ pub(super) fn check_no_chained_comparison(
                             }
                         }
                     } else {
-                        // All we know is that this is `foo < bar >` and *nothing* else. Try to
-                        // be helpful, but don't attempt to recover.
-                        err.help(TURBOFISH);
-                        err.help("or use `(...)` if you meant to specify fn arguments");
-                        // These cases cause too many knock-down errors, bail out (#61329).
-                        Err(err)
+                        if !matches!(l1.kind, ExprKind::Lit(_))
+                            && !matches!(r1.kind, ExprKind::Lit(_))
+                        {
+                            // All we know is that this is `foo < bar >` and *nothing* else. Try to
+                            // be helpful, but don't attempt to recover.
+                            err.help(TURBOFISH);
+                            err.help("or use `(...)` if you meant to specify fn arguments");
+                        }
+
+                        // If it looks like a genuine attempt to chain operators (as opposed to a
+                        // misformatted turbofish, for instance), suggest a correct form.
+                        if self.attempt_chained_comparison_suggestion(&mut err, inner_op, outer_op)
+                        {
+                            err.emit();
+                            mk_err_expr(self, inner_op.span.to(self.prev_token.span))
+                        } else {
+                            // These cases cause too many knock-down errors, bail out (#61329).
+                            Err(err)
+                        }
                     };
                 }
+                let recover =
+                    self.attempt_chained_comparison_suggestion(&mut err, inner_op, outer_op);
                 err.emit();
+                if recover {
+                    return mk_err_expr(self, inner_op.span.to(self.prev_token.span));
+                }
             }
             _ => {}
         }
@@ -643,7 +694,7 @@ fn consume_fn_args(&mut self) -> Result<(), ()> {
 
         if self.token.kind == token::Eof {
             // Not entirely sure that what we consumed were fn arguments, rollback.
-            mem::replace(self, snapshot);
+            *self = snapshot;
             Err(())
         } else {
             // 99% certain that the suggestion is correct, continue parsing.

src/test/ui/did_you_mean/issue-40396.stderr

@@ -2,16 +2,8 @@ error: comparison operators cannot be chained
   --> $DIR/issue-40396.rs:2:20
    |
 LL |     (0..13).collect<Vec<i32>>();
-   |                    ^^^^^
+   |                    ^   ^
    |
-help: split the comparison into two...
-   |
-LL |     (0..13).collect < Vec && Vec <i32>>();
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-help: ...or parenthesize one of the comparisons
-   |
-LL |     ((0..13).collect < Vec) <i32>>();
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
 help: use `::<...>` instead of `<...>` to specify type arguments
    |
 LL |     (0..13).collect::<Vec<i32>>();
@@ -21,7 +13,7 @@ error: comparison operators cannot be chained
   --> $DIR/issue-40396.rs:4:8
    |
 LL |     Vec<i32>::new();
-   |        ^^^^^
+   |        ^   ^
    |
 help: use `::<...>` instead of `<...>` to specify type arguments
    |
@@ -32,16 +24,8 @@ error: comparison operators cannot be chained
   --> $DIR/issue-40396.rs:6:20
    |
 LL |     (0..13).collect<Vec<i32>();
-   |                    ^^^^^
-   |
-help: split the comparison into two...
-   |
-LL |     (0..13).collect < Vec && Vec <i32>();
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-help: ...or parenthesize one of the comparisons
+   |                    ^   ^
    |
-LL |     ((0..13).collect < Vec) <i32>();
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
 help: use `::<...>` instead of `<...>` to specify type arguments
    |
 LL |     (0..13).collect::<Vec<i32>();

src/test/ui/parser/chained-comparison-suggestion.rs

@@ -37,4 +37,17 @@ fn comp8() {
     //~^ ERROR mismatched types
 }
 
+fn comp9() {
+    1 == 2 < 3; //~ ERROR comparison operators cannot be chained
+}
+
+fn comp10() {
+    1 > 2 == false; //~ ERROR comparison operators cannot be chained
+}
+
+fn comp11() {
+    1 == 2 == 3; //~ ERROR comparison operators cannot be chained
+    //~^ ERROR mismatched types
+}
+
 fn main() {}

src/test/ui/parser/chained-comparison-suggestion.stderr

@@ -2,127 +2,122 @@ error: comparison operators cannot be chained
   --> $DIR/chained-comparison-suggestion.rs:4:7
    |
 LL |     1 < 2 <= 3;
-   |       ^^^^^^
+   |       ^   ^^
    |
-help: split the comparison into two...
+help: split the comparison into two
    |
 LL |     1 < 2 && 2 <= 3;
-   |     ^^^^^^^^^^^^^
-help: ...or parenthesize one of the comparisons
-   |
-LL |     (1 < 2) <= 3;
-   |     ^^^^^^^^^^
+   |           ^^^^
 
 error: comparison operators cannot be chained
   --> $DIR/chained-comparison-suggestion.rs:9:7
    |
 LL |     1 < 2 < 3;
-   |       ^^^^^
+   |       ^   ^
    |
-   = help: use `::<...>` instead of `<...>` to specify type arguments
-   = help: or use `(...)` if you meant to specify fn arguments
-help: split the comparison into two...
+help: split the comparison into two
    |
 LL |     1 < 2 && 2 < 3;
-   |     ^^^^^^^^^^^^
-help: ...or parenthesize one of the comparisons
-   |
-LL |     (1 < 2) < 3;
-   |     ^^^^^^^^^
+   |           ^^^^
 
 error: comparison operators cannot be chained
   --> $DIR/chained-comparison-suggestion.rs:13:7
    |
 LL |     1 <= 2 < 3;
-   |       ^^^^^^
+   |       ^^   ^
    |
-help: split the comparison into two...
+help: split the comparison into two
    |
 LL |     1 <= 2 && 2 < 3;
-   |     ^^^^^^^^^^^^^
-help: ...or parenthesize one of the comparisons
-   |
-LL |     (1 <= 2) < 3;
-   |     ^^^^^^^^^^
+   |            ^^^^
 
 error: comparison operators cannot be chained
   --> $DIR/chained-comparison-suggestion.rs:18:7
    |
 LL |     1 <= 2 <= 3;
-   |       ^^^^^^^
+   |       ^^   ^^
    |
-help: split the comparison into two...
+help: split the comparison into two
    |
 LL |     1 <= 2 && 2 <= 3;
-   |     ^^^^^^^^^^^^^^
-help: ...or parenthesize one of the comparisons
-   |
-LL |     (1 <= 2) <= 3;
-   |     ^^^^^^^^^^^
+   |            ^^^^
 
 error: comparison operators cannot be chained
   --> $DIR/chained-comparison-suggestion.rs:23:7
    |
 LL |     1 > 2 >= 3;
-   |       ^^^^^^
+   |       ^   ^^
    |
-help: split the comparison into two...
+help: split the comparison into two
    |
 LL |     1 > 2 && 2 >= 3;
-   |     ^^^^^^^^^^^^^
-help: ...or parenthesize one of the comparisons
-   |
-LL |     (1 > 2) >= 3;
-   |     ^^^^^^^^^^
+   |           ^^^^
 
 error: comparison operators cannot be chained
   --> $DIR/chained-comparison-suggestion.rs:28:7
    |
 LL |     1 > 2 > 3;
-   |       ^^^^^
+   |       ^   ^
    |
-   = help: use `::<...>` instead of `<...>` to specify type arguments
-   = help: or use `(...)` if you meant to specify fn arguments
-help: split the comparison into two...
+help: split the comparison into two
    |
 LL |     1 > 2 && 2 > 3;
-   |     ^^^^^^^^^^^^
-help: ...or parenthesize one of the comparisons
-   |
-LL |     (1 > 2) > 3;
-   |     ^^^^^^^^^
+   |           ^^^^
 
 error: comparison operators cannot be chained
   --> $DIR/chained-comparison-suggestion.rs:32:7
    |
 LL |     1 >= 2 > 3;
-   |       ^^^^^^
+   |       ^^   ^
    |
-   = help: use `::<...>` instead of `<...>` to specify type arguments
-   = help: or use `(...)` if you meant to specify fn arguments
-help: split the comparison into two...
+help: split the comparison into two
    |
 LL |     1 >= 2 && 2 > 3;
-   |     ^^^^^^^^^^^^^
-help: ...or parenthesize one of the comparisons
-   |
-LL |     (1 >= 2) > 3;
-   |     ^^^^^^^^^^
+   |            ^^^^
 
 error: comparison operators cannot be chained
   --> $DIR/chained-comparison-suggestion.rs:36:7
    |
 LL |     1 >= 2 >= 3;
-   |       ^^^^^^^
+   |       ^^   ^^
    |
-help: split the comparison into two...
+help: split the comparison into two
    |
 LL |     1 >= 2 && 2 >= 3;
-   |     ^^^^^^^^^^^^^^
-help: ...or parenthesize one of the comparisons
+   |            ^^^^
+
+error: comparison operators cannot be chained
+  --> $DIR/chained-comparison-suggestion.rs:41:7
+   |
+LL |     1 == 2 < 3;
+   |       ^^   ^
    |
-LL |     (1 >= 2) >= 3;
-   |     ^^^^^^^^^^^
+help: parenthesize the comparison
+   |
+LL |     1 == (2 < 3);
+   |          ^     ^
+
+error: comparison operators cannot be chained
+  --> $DIR/chained-comparison-suggestion.rs:45:7
+   |
+LL |     1 > 2 == false;
+   |       ^   ^^
+   |
+help: parenthesize the comparison
+   |
+LL |     (1 > 2) == false;
+   |     ^     ^
+
+error: comparison operators cannot be chained
+  --> $DIR/chained-comparison-suggestion.rs:49:7
+   |
+LL |     1 == 2 == 3;
+   |       ^^   ^^
+   |
+help: split the comparison into two
+   |
+LL |     1 == 2 && 2 == 3;
+   |            ^^^^
 
 error[E0308]: mismatched types
   --> $DIR/chained-comparison-suggestion.rs:4:14
@@ -154,6 +149,12 @@ error[E0308]: mismatched types
 LL |     1 >= 2 >= 3;
    |               ^ expected `bool`, found integer
 
-error: aborting due to 13 previous errors
+error[E0308]: mismatched types
+  --> $DIR/chained-comparison-suggestion.rs:49:15
+   |
+LL |     1 == 2 == 3;
+   |               ^ expected `bool`, found integer
+
+error: aborting due to 17 previous errors
 
 For more information about this error, try `rustc --explain E0308`.

src/test/ui/parser/require-parens-for-chained-comparison.rs

@@ -4,11 +4,11 @@ fn f<T>() {}
 fn main() {
     false == false == false;
     //~^ ERROR comparison operators cannot be chained
+    //~| HELP split the comparison into two
 
     false == 0 < 2;
     //~^ ERROR comparison operators cannot be chained
-    //~| ERROR mismatched types
-    //~| ERROR mismatched types
+    //~| HELP parenthesize the comparison
 
     f<X>();
     //~^ ERROR comparison operators cannot be chained
@@ -16,8 +16,6 @@ fn main() {
 
     f<Result<Option<X>, Option<Option<X>>>(1, 2);
     //~^ ERROR comparison operators cannot be chained
-    //~| HELP split the comparison into two...
-    //~| ...or parenthesize one of the comparisons
     //~| HELP use `::<...>` instead of `<...>` to specify type arguments
 
     use std::convert::identity;

src/test/ui/parser/require-parens-for-chained-comparison.stderr

@@ -2,19 +2,29 @@ error: comparison operators cannot be chained
   --> $DIR/require-parens-for-chained-comparison.rs:5:11
    |
 LL |     false == false == false;
-   |           ^^^^^^^^^^^
+   |           ^^       ^^
+   |
+help: split the comparison into two
+   |
+LL |     false == false && false == false;
+   |                    ^^^^^^^^
 
 error: comparison operators cannot be chained
-  --> $DIR/require-parens-for-chained-comparison.rs:8:11
+  --> $DIR/require-parens-for-chained-comparison.rs:9:11
    |
 LL |     false == 0 < 2;
-   |           ^^^^^^
+   |           ^^   ^
+   |
+help: parenthesize the comparison
+   |
+LL |     false == (0 < 2);
+   |              ^     ^
 
 error: comparison operators cannot be chained
   --> $DIR/require-parens-for-chained-comparison.rs:13:6
    |
 LL |     f<X>();
-   |      ^^^
+   |      ^ ^
    |
 help: use `::<...>` instead of `<...>` to specify type arguments
    |
@@ -25,42 +35,21 @@ error: comparison operators cannot be chained
   --> $DIR/require-parens-for-chained-comparison.rs:17:6
    |
 LL |     f<Result<Option<X>, Option<Option<X>>>(1, 2);
-   |      ^^^^^^^^
-   |
-help: split the comparison into two...
-   |
-LL |     f < Result && Result <Option<X>, Option<Option<X>>>(1, 2);
-   |     ^^^^^^^^^^^^^^^^^^^^^^
-help: ...or parenthesize one of the comparisons
+   |      ^      ^
    |
-LL |     (f < Result) <Option<X>, Option<Option<X>>>(1, 2);
-   |     ^^^^^^^^^^^^^^
 help: use `::<...>` instead of `<...>` to specify type arguments
    |
 LL |     f::<Result<Option<X>, Option<Option<X>>>(1, 2);
    |      ^^
 
 error: comparison operators cannot be chained
-  --> $DIR/require-parens-for-chained-comparison.rs:24:21
+  --> $DIR/require-parens-for-chained-comparison.rs:22:21
    |
 LL |     let _ = identity<u8>;
-   |                     ^^^^
+   |                     ^  ^
    |
    = help: use `::<...>` instead of `<...>` to specify type arguments
    = help: or use `(...)` if you meant to specify fn arguments
 
-error[E0308]: mismatched types
-  --> $DIR/require-parens-for-chained-comparison.rs:8:14
-   |
-LL |     false == 0 < 2;
-   |              ^ expected `bool`, found integer
-
-error[E0308]: mismatched types
-  --> $DIR/require-parens-for-chained-comparison.rs:8:18
-   |
-LL |     false == 0 < 2;
-   |                  ^ expected `bool`, found integer
-
-error: aborting due to 7 previous errors
+error: aborting due to 5 previous errors
 
-For more information about this error, try `rustc --explain E0308`.