From 89571a1bbc25a03ac0195460c7ec463d3f089def Mon Sep 17 00:00:00 2001 From: Esteban Küber Date: Mon, 9 Mar 2020 20:21:37 -0700 Subject: 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 +++++++++++++++++++++---------- 1 file changed, 98 insertions(+), 46 deletions(-) (limited to 'src/librustc_parse/parser') diff --git a/src/librustc_parse/parser/diagnostics.rs b/src/librustc_parse/parser/diagnostics.rs index 87255386b9e..4771031984d 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)); + } } _ => {} } -- cgit 1.4.1-3-g733a5 From 4832f3fd5d471ec7a4bfe4a599d8a4378b6d248d Mon Sep 17 00:00:00 2001 From: Esteban Küber Date: Wed, 25 Mar 2020 18:10:18 -0700 Subject: review comments --- src/librustc_parse/parser/diagnostics.rs | 51 ++++++++++++++++---------------- 1 file changed, 25 insertions(+), 26 deletions(-) (limited to 'src/librustc_parse/parser') diff --git a/src/librustc_parse/parser/diagnostics.rs b/src/librustc_parse/parser/diagnostics.rs index 4771031984d..c4546dedfcd 100644 --- a/src/librustc_parse/parser/diagnostics.rs +++ b/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,7 +458,7 @@ impl<'a> Parser<'a> { err: &mut DiagnosticBuilder<'_>, inner_op: &Expr, outer_op: &Spanned, - ) -> bool /* recover */ { + ) -> bool /* advanced the cursor */ { if let ExprKind::Binary(op, ref l1, ref r1) = inner_op.kind { if let ExprKind::Field(_, ident) = l1.kind { if ident.as_str().parse::().is_err() && !matches!(r1.kind, ExprKind::Lit(_)) { @@ -468,6 +467,16 @@ impl<'a> Parser<'a> { 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) | @@ -492,23 +501,18 @@ impl<'a> Parser<'a> { // `x == y < z` (BinOpKind::Eq, AssocOp::Less) | (BinOpKind::Eq, AssocOp::LessEqual) | (BinOpKind::Eq, AssocOp::Greater) | (BinOpKind::Eq, AssocOp::GreaterEqual) => { - // Consume `/`z`/outer-op-rhs. + // 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, - ); + // 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(); - mem::replace(self, snapshot); + *self = snapshot; false } } @@ -517,21 +521,16 @@ impl<'a> Parser<'a> { (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, - ); + // 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(); - mem::replace(self, snapshot); + *self = snapshot; false } } @@ -588,11 +587,11 @@ impl<'a> Parser<'a> { ); }; + // Include `<` to provide this recommendation even in a case like + // `Foo>>` if op.node == BinOpKind::Lt && outer_op.node == AssocOp::Less || outer_op.node == AssocOp::Greater { - // Include `<` to provide this recommendation - // even in a case like `Foo>>` if outer_op.node == AssocOp::Less { let snapshot = self.clone(); self.bump(); @@ -606,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 { @@ -631,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) } } @@ -695,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. -- cgit 1.4.1-3-g733a5