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

Author: Centril

src/librustc_parse/parser/diagnostics.rs

@@ -17,7 +17,6 @@ use rustc_span::symbol::kw;
 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 @@ impl<'a> Parser<'a> {
         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 @@ impl<'a> Parser<'a> {
                         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 @@ impl<'a> Parser<'a> {
             |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 @@ impl<'a> Parser<'a> {
                         {
                             // 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 @@ impl<'a> Parser<'a> {
                                 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 @@ impl<'a> Parser<'a> {
                             }
                         }
                     } 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 @@ impl<'a> Parser<'a> {
 
         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() {}