From 89571a1bbc25a03ac0195460c7ec463d3f089def Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Mon, 9 Mar 2020 20:21:37 -0700 Subject: [PATCH] Tweak chained operators diagnostic Use more selective spans Improve suggestion output Be more selective when displaying suggestions Silence some knock-down type errors --- src/librustc_parse/parser/diagnostics.rs | 144 ++++++++++++------ src/test/ui/did_you_mean/issue-40396.stderr | 22 +-- .../parser/chained-comparison-suggestion.rs | 13 ++ .../chained-comparison-suggestion.stderr | 125 +++++++-------- .../require-parens-for-chained-comparison.rs | 6 +- ...quire-parens-for-chained-comparison.stderr | 47 +++--- 6 files changed, 197 insertions(+), 160 deletions(-) diff --git a/src/librustc_parse/parser/diagnostics.rs b/src/librustc_parse/parser/diagnostics.rs index 87255386b9e66..4771031984d1e 100644 --- a/src/librustc_parse/parser/diagnostics.rs +++ b/src/librustc_parse/parser/diagnostics.rs @@ -459,9 +459,18 @@ impl<'a> Parser<'a> { err: &mut DiagnosticBuilder<'_>, inner_op: &Expr, outer_op: &Spanned, - ) { + ) -> bool /* recover */ { 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::().is_err() && !matches!(r1.kind, ExprKind::Lit(_)) { + // The parser has encountered `foo.bar 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(), - ), + err.span_suggestion_verbose( + inner_op.span.shrink_to_hi(), + "split the comparison into two", + format!(" && {}", expr_to_str(&r1)), 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(), - ), + 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) => { + err.multipart_suggestion( + "parenthesize the comparison", + vec![ + (r1.span.shrink_to_lo(), "(".to_string()), + (r2.span.shrink_to_hi(), ")".to_string()), + ], + Applicability::MaybeIncorrect, + ); + true + } + Err(mut expr_err) => { + expr_err.cancel(); + mem::replace(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(); + err.multipart_suggestion( + "parenthesize the comparison", + vec![ + (l1.span.shrink_to_lo(), "(".to_string()), + (r1.span.shrink_to_hi(), ")".to_string()), + ], Applicability::MaybeIncorrect, ); + match self.parse_expr() { + Ok(_) => { + true + } + Err(mut expr_err) => { + expr_err.cancel(); + mem::replace(self, snapshot); + false + } + } } - _ => {} - } + _ => false, + }; } + false } /// Produces an error if comparison operators are chained (RFC #558). @@ -534,31 +573,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: + if op.node == BinOpKind::Lt && outer_op.node == AssocOp::Less + || outer_op.node == AssocOp::Greater { - // Foo>> + // Include `<` to provide this recommendation + // even in a case like `Foo>>` if outer_op.node == AssocOp::Less { let snapshot = self.clone(); self.bump(); @@ -617,15 +651,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)); + } } _ => {} } diff --git a/src/test/ui/did_you_mean/issue-40396.stderr b/src/test/ui/did_you_mean/issue-40396.stderr index f952136a7bfe3..10972697f9fcd 100644 --- a/src/test/ui/did_you_mean/issue-40396.stderr +++ b/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>(); - | ^^^^^ + | ^ ^ | -help: split the comparison into two... - | -LL | (0..13).collect < Vec && Vec >(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -help: ...or parenthesize one of the comparisons - | -LL | ((0..13).collect < Vec) >(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `::<...>` instead of `<...>` to specify type arguments | LL | (0..13).collect::>(); @@ -21,7 +13,7 @@ error: comparison operators cannot be chained --> $DIR/issue-40396.rs:4:8 | LL | Vec::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(); - | ^^^^^ - | -help: split the comparison into two... - | -LL | (0..13).collect < Vec && Vec (); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -help: ...or parenthesize one of the comparisons + | ^ ^ | -LL | ((0..13).collect < Vec) (); - | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `::<...>` instead of `<...>` to specify type arguments | LL | (0..13).collect::(); diff --git a/src/test/ui/parser/chained-comparison-suggestion.rs b/src/test/ui/parser/chained-comparison-suggestion.rs index 0431196f1744e..bbd46082c9f90 100644 --- a/src/test/ui/parser/chained-comparison-suggestion.rs +++ b/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() {} diff --git a/src/test/ui/parser/chained-comparison-suggestion.stderr b/src/test/ui/parser/chained-comparison-suggestion.stderr index 5c10a4599dd03..067920d12f486 100644 --- a/src/test/ui/parser/chained-comparison-suggestion.stderr +++ b/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`. diff --git a/src/test/ui/parser/require-parens-for-chained-comparison.rs b/src/test/ui/parser/require-parens-for-chained-comparison.rs index e27b03dddc5be..4e97904ed6d5f 100644 --- a/src/test/ui/parser/require-parens-for-chained-comparison.rs +++ b/src/test/ui/parser/require-parens-for-chained-comparison.rs @@ -4,11 +4,11 @@ struct X; 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(); //~^ ERROR comparison operators cannot be chained @@ -16,8 +16,6 @@ fn main() { f, Option>>(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; diff --git a/src/test/ui/parser/require-parens-for-chained-comparison.stderr b/src/test/ui/parser/require-parens-for-chained-comparison.stderr index 44edf2de7f8de..7001aa8e8a1d8 100644 --- a/src/test/ui/parser/require-parens-for-chained-comparison.stderr +++ b/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(); - | ^^^ + | ^ ^ | 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, Option>>(1, 2); - | ^^^^^^^^ - | -help: split the comparison into two... - | -LL | f < Result && Result , Option>>(1, 2); - | ^^^^^^^^^^^^^^^^^^^^^^ -help: ...or parenthesize one of the comparisons + | ^ ^ | -LL | (f < Result) , Option>>(1, 2); - | ^^^^^^^^^^^^^^ help: use `::<...>` instead of `<...>` to specify type arguments | LL | f::, Option>>(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; - | ^^^^ + | ^ ^ | = 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`.