From 088a1808d22e571dbf83f477d94ea9c78ccc3219 Mon Sep 17 00:00:00 2001 From: varkor Date: Sat, 11 Jan 2020 00:19:09 +0000 Subject: [PATCH] Add suggestions when encountering chained comparisons --- src/librustc_parse/parser/diagnostics.rs | 83 +++++++-- src/librustc_parse/parser/expr.rs | 42 ++--- src/test/ui/did_you_mean/issue-40396.rs | 6 +- src/test/ui/did_you_mean/issue-40396.stderr | 22 ++- .../parser/chained-comparison-suggestion.rs | 40 +++++ .../chained-comparison-suggestion.stderr | 159 ++++++++++++++++++ .../require-parens-for-chained-comparison.rs | 12 +- ...quire-parens-for-chained-comparison.stderr | 20 ++- 8 files changed, 335 insertions(+), 49 deletions(-) create mode 100644 src/test/ui/parser/chained-comparison-suggestion.rs create mode 100644 src/test/ui/parser/chained-comparison-suggestion.stderr diff --git a/src/librustc_parse/parser/diagnostics.rs b/src/librustc_parse/parser/diagnostics.rs index 9abfbc698c5..ecbd183b6e9 100644 --- a/src/librustc_parse/parser/diagnostics.rs +++ b/src/librustc_parse/parser/diagnostics.rs @@ -4,6 +4,7 @@ use rustc_error_codes::*; use rustc_errors::{pluralize, struct_span_err}; use rustc_errors::{Applicability, DiagnosticBuilder, Handler, PResult}; +use rustc_span::source_map::Spanned; use rustc_span::symbol::kw; use rustc_span::{MultiSpan, Span, SpanSnippetError, DUMMY_SP}; use syntax::ast::{ @@ -500,6 +501,58 @@ pub(super) fn check_trailing_angle_brackets(&mut self, segment: &PathSegment, en } } + /// Check to see if a pair of chained operators looks like an attempt at chained comparison, + /// e.g. `1 < x <= 3`. If so, suggest either splitting the comparison into two, or + /// parenthesising the leftmost comparison. + fn attempt_chained_comparison_suggestion( + &mut self, + err: &mut DiagnosticBuilder<'_>, + inner_op: &Expr, + outer_op: &Spanned, + ) { + if let ExprKind::Binary(op, ref l1, ref r1) = inner_op.kind { + match (op.node, &outer_op.node) { + // `x < y < z` and friends. + (BinOpKind::Lt, AssocOp::Less) | (BinOpKind::Lt, AssocOp::LessEqual) | + (BinOpKind::Le, AssocOp::LessEqual) | (BinOpKind::Le, AssocOp::Less) | + // `x > y > z` and friends. + (BinOpKind::Gt, AssocOp::Greater) | (BinOpKind::Gt, AssocOp::GreaterEqual) | + (BinOpKind::Ge, AssocOp::GreaterEqual) | (BinOpKind::Ge, AssocOp::Greater) => { + let expr_to_str = |e: &Expr| { + 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(), + ), + Applicability::MaybeIncorrect, + ); + } + _ => {} + } + } + } + /// Produces an error if comparison operators are chained (RFC #558). /// We only need to check the LHS, not the RHS, because all comparison ops have same /// precedence (see `fn precedence`) and are left-associative (see `fn fixity`). @@ -515,27 +568,31 @@ pub(super) fn check_trailing_angle_brackets(&mut self, segment: &PathSegment, en /// / \ /// inner_op r2 /// / \ - /// l1 r1 + /// l1 r1 pub(super) fn check_no_chained_comparison( &mut self, - lhs: &Expr, - outer_op: &AssocOp, + inner_op: &Expr, + outer_op: &Spanned, ) -> PResult<'a, Option>> { debug_assert!( - outer_op.is_comparison(), + outer_op.node.is_comparison(), "check_no_chained_comparison: {:?} is not comparison", - outer_op, + outer_op.node, ); let mk_err_expr = |this: &Self, span| Ok(Some(this.mk_expr(span, ExprKind::Err, AttrVec::new()))); - match lhs.kind { + 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_span); - let mut err = self - .struct_span_err(op_span, "chained comparison operators require parentheses"); + 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); let suggest = |err: &mut DiagnosticBuilder<'_>| { err.span_suggestion_verbose( @@ -547,12 +604,12 @@ pub(super) fn check_no_chained_comparison( }; if op.node == BinOpKind::Lt && - *outer_op == AssocOp::Less || // Include `<` to provide this recommendation - *outer_op == AssocOp::Greater + outer_op.node == AssocOp::Less || // Include `<` to provide this recommendation + outer_op.node == AssocOp::Greater // even in a case like the following: { // Foo>> - if *outer_op == AssocOp::Less { + if outer_op.node == AssocOp::Less { let snapshot = self.clone(); self.bump(); // So far we have parsed `foo { expr_err.cancel(); @@ -606,7 +663,7 @@ pub(super) fn check_no_chained_comparison( // FIXME: actually check that the two expressions in the binop are // paths and resynthesize new fn call expression instead of using // `ExprKind::Err` placeholder. - mk_err_expr(self, lhs.span.to(self.prev_span)) + mk_err_expr(self, inner_op.span.to(self.prev_span)) } } } else { diff --git a/src/librustc_parse/parser/expr.rs b/src/librustc_parse/parser/expr.rs index 90f15375aec..3c286d363c9 100644 --- a/src/librustc_parse/parser/expr.rs +++ b/src/librustc_parse/parser/expr.rs @@ -5,7 +5,7 @@ use crate::maybe_recover_from_interpolated_ty_qpath; use rustc_errors::{Applicability, PResult}; -use rustc_span::source_map::{self, Span}; +use rustc_span::source_map::{self, Span, Spanned}; use rustc_span::symbol::{kw, sym, Symbol}; use std::mem; use syntax::ast::{self, AttrStyle, AttrVec, CaptureBy, Field, Ident, Lit, DUMMY_NODE_ID}; @@ -181,17 +181,17 @@ pub(super) fn parse_assoc_expr_with( }; let cur_op_span = self.token.span; - let restrictions = if op.is_assign_like() { + let restrictions = if op.node.is_assign_like() { self.restrictions & Restrictions::NO_STRUCT_LITERAL } else { self.restrictions }; - let prec = op.precedence(); + let prec = op.node.precedence(); if prec < min_prec { break; } // Check for deprecated `...` syntax - if self.token == token::DotDotDot && op == AssocOp::DotDotEq { + if self.token == token::DotDotDot && op.node == AssocOp::DotDotEq { self.err_dotdotdot_syntax(self.token.span); } @@ -200,11 +200,12 @@ pub(super) fn parse_assoc_expr_with( } self.bump(); - if op.is_comparison() { + if op.node.is_comparison() { if let Some(expr) = self.check_no_chained_comparison(&lhs, &op)? { return Ok(expr); } } + let op = op.node; // Special cases: if op == AssocOp::As { lhs = self.parse_assoc_op_cast(lhs, lhs_span, ExprKind::Cast)?; @@ -298,7 +299,7 @@ pub(super) fn parse_assoc_expr_with( } fn should_continue_as_assoc_expr(&mut self, lhs: &Expr) -> bool { - match (self.expr_is_complete(lhs), self.check_assoc_op()) { + match (self.expr_is_complete(lhs), self.check_assoc_op().map(|op| op.node)) { // Semi-statement forms are odd: // See https://github.com/rust-lang/rust/issues/29071 (true, None) => false, @@ -343,19 +344,22 @@ fn error_found_expr_would_be_stmt(&self, lhs: &Expr) { /// The method does not advance the current token. /// /// Also performs recovery for `and` / `or` which are mistaken for `&&` and `||` respectively. - fn check_assoc_op(&self) -> Option { - match (AssocOp::from_token(&self.token), &self.token.kind) { - (op @ Some(_), _) => op, - (None, token::Ident(sym::and, false)) => { - self.error_bad_logical_op("and", "&&", "conjunction"); - Some(AssocOp::LAnd) - } - (None, token::Ident(sym::or, false)) => { - self.error_bad_logical_op("or", "||", "disjunction"); - Some(AssocOp::LOr) - } - _ => None, - } + fn check_assoc_op(&self) -> Option> { + Some(Spanned { + node: match (AssocOp::from_token(&self.token), &self.token.kind) { + (Some(op), _) => op, + (None, token::Ident(sym::and, false)) => { + self.error_bad_logical_op("and", "&&", "conjunction"); + AssocOp::LAnd + } + (None, token::Ident(sym::or, false)) => { + self.error_bad_logical_op("or", "||", "disjunction"); + AssocOp::LOr + } + _ => return None, + }, + span: self.token.span, + }) } /// Error on `and` and `or` suggesting `&&` and `||` respectively. diff --git a/src/test/ui/did_you_mean/issue-40396.rs b/src/test/ui/did_you_mean/issue-40396.rs index 18933552054..e4e94bb9492 100644 --- a/src/test/ui/did_you_mean/issue-40396.rs +++ b/src/test/ui/did_you_mean/issue-40396.rs @@ -1,8 +1,8 @@ fn main() { (0..13).collect>(); - //~^ ERROR chained comparison + //~^ ERROR comparison operators cannot be chained Vec::new(); - //~^ ERROR chained comparison + //~^ ERROR comparison operators cannot be chained (0..13).collect(); - //~^ ERROR chained comparison + //~^ ERROR comparison operators cannot be chained } diff --git a/src/test/ui/did_you_mean/issue-40396.stderr b/src/test/ui/did_you_mean/issue-40396.stderr index 749d1093cca..f952136a7bf 100644 --- a/src/test/ui/did_you_mean/issue-40396.stderr +++ b/src/test/ui/did_you_mean/issue-40396.stderr @@ -1,15 +1,23 @@ -error: chained comparison operators require parentheses +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::>(); | ^^ -error: chained comparison operators require parentheses +error: comparison operators cannot be chained --> $DIR/issue-40396.rs:4:8 | LL | Vec::new(); @@ -20,12 +28,20 @@ help: use `::<...>` instead of `<...>` to specify type arguments LL | Vec::::new(); | ^^ -error: chained comparison operators require parentheses +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 new file mode 100644 index 00000000000..0431196f174 --- /dev/null +++ b/src/test/ui/parser/chained-comparison-suggestion.rs @@ -0,0 +1,40 @@ +// Check that we get nice suggestions when attempting a chained comparison. + +fn comp1() { + 1 < 2 <= 3; //~ ERROR comparison operators cannot be chained + //~^ ERROR mismatched types +} + +fn comp2() { + 1 < 2 < 3; //~ ERROR comparison operators cannot be chained +} + +fn comp3() { + 1 <= 2 < 3; //~ ERROR comparison operators cannot be chained + //~^ ERROR mismatched types +} + +fn comp4() { + 1 <= 2 <= 3; //~ ERROR comparison operators cannot be chained + //~^ ERROR mismatched types +} + +fn comp5() { + 1 > 2 >= 3; //~ ERROR comparison operators cannot be chained + //~^ ERROR mismatched types +} + +fn comp6() { + 1 > 2 > 3; //~ ERROR comparison operators cannot be chained +} + +fn comp7() { + 1 >= 2 > 3; //~ ERROR comparison operators cannot be chained +} + +fn comp8() { + 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 new file mode 100644 index 00000000000..5c10a4599dd --- /dev/null +++ b/src/test/ui/parser/chained-comparison-suggestion.stderr @@ -0,0 +1,159 @@ +error: comparison operators cannot be chained + --> $DIR/chained-comparison-suggestion.rs:4:7 + | +LL | 1 < 2 <= 3; + | ^^^^^^ + | +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... + | +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... + | +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... + | +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... + | +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... + | +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... + | +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... + | +LL | 1 >= 2 && 2 >= 3; + | ^^^^^^^^^^^^^^ +help: ...or parenthesize one of the comparisons + | +LL | (1 >= 2) >= 3; + | ^^^^^^^^^^^ + +error[E0308]: mismatched types + --> $DIR/chained-comparison-suggestion.rs:4:14 + | +LL | 1 < 2 <= 3; + | ^ expected `bool`, found integer + +error[E0308]: mismatched types + --> $DIR/chained-comparison-suggestion.rs:13:14 + | +LL | 1 <= 2 < 3; + | ^ expected `bool`, found integer + +error[E0308]: mismatched types + --> $DIR/chained-comparison-suggestion.rs:18:15 + | +LL | 1 <= 2 <= 3; + | ^ expected `bool`, found integer + +error[E0308]: mismatched types + --> $DIR/chained-comparison-suggestion.rs:23:14 + | +LL | 1 > 2 >= 3; + | ^ expected `bool`, found integer + +error[E0308]: mismatched types + --> $DIR/chained-comparison-suggestion.rs:36:15 + | +LL | 1 >= 2 >= 3; + | ^ expected `bool`, found integer + +error: aborting due to 13 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 9c7a25d589a..e27b03dddc5 100644 --- a/src/test/ui/parser/require-parens-for-chained-comparison.rs +++ b/src/test/ui/parser/require-parens-for-chained-comparison.rs @@ -3,24 +3,26 @@ fn f() {} fn main() { false == false == false; - //~^ ERROR chained comparison operators require parentheses + //~^ ERROR comparison operators cannot be chained false == 0 < 2; - //~^ ERROR chained comparison operators require parentheses + //~^ ERROR comparison operators cannot be chained //~| ERROR mismatched types //~| ERROR mismatched types f(); - //~^ ERROR chained comparison operators require parentheses + //~^ ERROR comparison operators cannot be chained //~| HELP use `::<...>` instead of `<...>` to specify type arguments f, Option>>(1, 2); - //~^ ERROR chained comparison operators require parentheses + //~^ 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; let _ = identity; - //~^ ERROR chained comparison operators require parentheses + //~^ ERROR comparison operators cannot be chained //~| HELP use `::<...>` instead of `<...>` to specify type arguments //~| HELP or use `(...)` if you meant to specify fn arguments } 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 bece9a38800..44edf2de7f8 100644 --- a/src/test/ui/parser/require-parens-for-chained-comparison.stderr +++ b/src/test/ui/parser/require-parens-for-chained-comparison.stderr @@ -1,16 +1,16 @@ -error: chained comparison operators require parentheses +error: comparison operators cannot be chained --> $DIR/require-parens-for-chained-comparison.rs:5:11 | LL | false == false == false; | ^^^^^^^^^^^ -error: chained comparison operators require parentheses +error: comparison operators cannot be chained --> $DIR/require-parens-for-chained-comparison.rs:8:11 | LL | false == 0 < 2; | ^^^^^^ -error: chained comparison operators require parentheses +error: comparison operators cannot be chained --> $DIR/require-parens-for-chained-comparison.rs:13:6 | LL | f(); @@ -21,19 +21,27 @@ help: use `::<...>` instead of `<...>` to specify type arguments LL | f::(); | ^^ -error: chained comparison operators require parentheses +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: chained comparison operators require parentheses - --> $DIR/require-parens-for-chained-comparison.rs:22:21 +error: comparison operators cannot be chained + --> $DIR/require-parens-for-chained-comparison.rs:24:21 | LL | let _ = identity; | ^^^^