From ed60cf2475cabd3d9ad1afdc03bd6952d99b744c Mon Sep 17 00:00:00 2001 From: Esteban Küber Date: Sun, 29 Sep 2019 19:07:26 -0700 Subject: When encountering chained operators use heuristics to recover from bad turbofish --- src/libsyntax/parse/diagnostics.rs | 100 ++++++++++++++++++++++++++++++++----- src/libsyntax/parse/parser/expr.rs | 4 +- 2 files changed, 91 insertions(+), 13 deletions(-) (limited to 'src/libsyntax/parse') diff --git a/src/libsyntax/parse/diagnostics.rs b/src/libsyntax/parse/diagnostics.rs index e8d7b7663ed..0e3d873b252 100644 --- a/src/libsyntax/parse/diagnostics.rs +++ b/src/libsyntax/parse/diagnostics.rs @@ -543,16 +543,25 @@ impl<'a> Parser<'a> { } /// 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 and are left-associative. - crate fn check_no_chained_comparison(&self, lhs: &Expr, outer_op: &AssocOp) -> PResult<'a, ()> { - debug_assert!(outer_op.is_comparison(), - "check_no_chained_comparison: {:?} is not comparison", - outer_op); + /// We only need to check the LHS, not the RHS, because all comparison ops have same + /// precedence and are left-associative. + /// + /// This can also be hit if someone incorrectly writes `foo()` when they should have used + /// the turbofish syntax. We attempt some heuristic recovery if that is the case. + crate fn check_no_chained_comparison( + &mut self, + lhs: &Expr, + outer_op: &AssocOp, + ) -> PResult<'a, Option>> { + debug_assert!( + outer_op.is_comparison(), + "check_no_chained_comparison: {:?} is not comparison", + outer_op, + ); match lhs.kind { ExprKind::Binary(op, _, _) if op.node.is_comparison() => { // Respan to include both operators. - let op_span = op.span.to(self.token.span); + let op_span = op.span.to(self.prev_span); let mut err = self.struct_span_err( op_span, "chained comparison operators require parentheses", @@ -561,17 +570,84 @@ impl<'a> Parser<'a> { *outer_op == AssocOp::Less || // Include `<` to provide this recommendation *outer_op == AssocOp::Greater // even in a case like the following: { // Foo>> - err.help( - "use `::<...>` instead of `<...>` if you meant to specify type arguments"); - err.help("or use `(...)` if you meant to specify fn arguments"); - // These cases cause too many knock-down errors, bail out (#61329). + let msg = "use `::<...>` instead of `<...>` if you meant to specify type \ + arguments"; + if *outer_op == AssocOp::Less { + // if self.look_ahead(1, |t| t.kind == token::Lt || t.kind == token::ModSep) { + let snapshot = self.clone(); + self.bump(); + // So far we have parsed `foo 0 { + match &self.token.kind { + token::Lt => { + acc += 1; + } + token::Gt => { + acc -= 1; + } + token::BinOp(token::Shr) => { + acc -= 2; + } + token::Eof => { + break; + } + _ => {} + } + self.bump(); + } + if self.token.kind != token::OpenDelim(token::Paren) { + mem::replace(self, snapshot.clone()); + } + } + if self.token.kind == token::OpenDelim(token::Paren) { + err.span_suggestion( + op_span.shrink_to_lo(), + msg, + "::".to_string(), + Applicability::MaybeIncorrect, + ); + let snapshot = self.clone(); + self.bump(); + let mut acc = 1; + while acc > 0 { + match &self.token.kind { + token::OpenDelim(token::Paren) => { + acc += 1; + } + token::CloseDelim(token::Paren) => { + acc -= 1; + } + token::Eof => { + break; + } + _ => {} + } + self.bump(); + } + if self.token.kind == token::Eof { + mem::replace(self, snapshot); + return Err(err); + } else { + err.emit(); + return Ok(Some(self.mk_expr( + lhs.span.to(self.prev_span), + ExprKind::Err, + ThinVec::new(), + ))); + } + } else { + err.help(msg); + err.help("or use `(...)` if you meant to specify fn arguments"); + // These cases cause too many knock-down errors, bail out (#61329). + } return Err(err); } err.emit(); } _ => {} } - Ok(()) + Ok(None) } crate fn maybe_report_ambiguous_plus( diff --git a/src/libsyntax/parse/parser/expr.rs b/src/libsyntax/parse/parser/expr.rs index 23674ad589d..b459782d237 100644 --- a/src/libsyntax/parse/parser/expr.rs +++ b/src/libsyntax/parse/parser/expr.rs @@ -238,7 +238,9 @@ impl<'a> Parser<'a> { self.bump(); if op.is_comparison() { - self.check_no_chained_comparison(&lhs, &op)?; + if let Some(expr) = self.check_no_chained_comparison(&lhs, &op)? { + return Ok(expr); + } } // Special cases: if op == AssocOp::As { -- cgit 1.4.1-3-g733a5 From 6c9f298a8bee9b1716b2e6fcdb8305c3f4874fc6 Mon Sep 17 00:00:00 2001 From: Esteban Küber Date: Mon, 30 Sep 2019 12:19:22 -0700 Subject: review comments --- src/libsyntax/parse/diagnostics.rs | 87 +++++++++++++++++++++----------------- 1 file changed, 49 insertions(+), 38 deletions(-) (limited to 'src/libsyntax/parse') diff --git a/src/libsyntax/parse/diagnostics.rs b/src/libsyntax/parse/diagnostics.rs index 0e3d873b252..584c7c2ded5 100644 --- a/src/libsyntax/parse/diagnostics.rs +++ b/src/libsyntax/parse/diagnostics.rs @@ -560,6 +560,7 @@ impl<'a> Parser<'a> { ); match lhs.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( @@ -573,63 +574,54 @@ impl<'a> Parser<'a> { let msg = "use `::<...>` instead of `<...>` if you meant to specify type \ arguments"; if *outer_op == AssocOp::Less { - // if self.look_ahead(1, |t| t.kind == token::Lt || t.kind == token::ModSep) { let snapshot = self.clone(); self.bump(); - // So far we have parsed `foo 0 { - match &self.token.kind { - token::Lt => { - acc += 1; - } - token::Gt => { - acc -= 1; - } - token::BinOp(token::Shr) => { - acc -= 2; - } - token::Eof => { - break; - } - _ => {} - } - self.bump(); - } + // So far we have parsed `foo(`, so we rewind the parser and bail out. mem::replace(self, snapshot.clone()); } } if self.token.kind == token::OpenDelim(token::Paren) { + // We have high certainty that this was a bad turbofish at this point. + // `foo< bar >(` err.span_suggestion( op_span.shrink_to_lo(), msg, "::".to_string(), Applicability::MaybeIncorrect, ); + let snapshot = self.clone(); - self.bump(); - let mut acc = 1; - while acc > 0 { - match &self.token.kind { - token::OpenDelim(token::Paren) => { - acc += 1; - } - token::CloseDelim(token::Paren) => { - acc -= 1; - } - token::Eof => { - break; - } - _ => {} - } - self.bump(); - } + + // Consume the fn call arguments. + let modifiers = vec![ + (token::OpenDelim(token::Paren), 1), + (token::CloseDelim(token::Paren), -1), + ]; + let early_return = vec![token::Eof]; + self.bump(); // `(` + self.consume_tts(1, &modifiers[..], &early_return[..]); + if self.token.kind == token::Eof { + // Not entirely sure now, but we bubble the error up with the + // suggestion. mem::replace(self, snapshot); return Err(err); } else { + // 99% certain that the suggestion is correct, continue parsing. err.emit(); + // FIXME: actually check that the two expressions in the binop are + // paths and resynthesize new fn call expression instead of using + // `ExprKind::Err` placeholder. return Ok(Some(self.mk_expr( lhs.span.to(self.prev_span), ExprKind::Err, @@ -637,6 +629,8 @@ 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(msg); err.help("or use `(...)` if you meant to specify fn arguments"); // These cases cause too many knock-down errors, bail out (#61329). @@ -1424,6 +1418,23 @@ impl<'a> Parser<'a> { err } + fn consume_tts( + &mut self, + mut acc: i64, + modifier: &[(token::TokenKind, i64)], // Not using FxHasMap and FxHashSet due to + early_return: &[token::TokenKind], // `token::TokenKind: !Eq + !Hash`. + ) { + while acc > 0 { + if let Some((_, val)) = modifier.iter().filter(|(t, _)| *t == self.token.kind).next() { + acc += *val; + } + if early_return.contains(&self.token.kind) { + break; + } + self.bump(); + } + } + /// Replace duplicated recovered parameters with `_` pattern to avoid unecessary errors. /// /// This is necessary because at this point we don't know whether we parsed a function with -- cgit 1.4.1-3-g733a5 From d7dceaa0c57759d37a48b5a0aa1064b7c89f957b Mon Sep 17 00:00:00 2001 From: Esteban Küber Date: Mon, 30 Sep 2019 12:36:44 -0700 Subject: Account for missing turbofish in paths too --- src/libsyntax/parse/diagnostics.rs | 47 ++++++++++++++++++++++++++--- src/test/ui/did_you_mean/issue-40396.stderr | 5 +-- 2 files changed, 46 insertions(+), 6 deletions(-) (limited to 'src/libsyntax/parse') diff --git a/src/libsyntax/parse/diagnostics.rs b/src/libsyntax/parse/diagnostics.rs index 584c7c2ded5..4fbd36cfefc 100644 --- a/src/libsyntax/parse/diagnostics.rs +++ b/src/libsyntax/parse/diagnostics.rs @@ -585,12 +585,51 @@ impl<'a> Parser<'a> { let early_return = vec![token::Eof]; self.consume_tts(1, &modifiers[..], &early_return[..]); - if self.token.kind != token::OpenDelim(token::Paren) { - // We don't have `foo< bar >(`, so we rewind the parser and bail out. + if !&[ + token::OpenDelim(token::Paren), + token::ModSep, + ].contains(&self.token.kind) { + // We don't have `foo< bar >(` or `foo< bar >::`, so we rewind the + // parser and bail out. mem::replace(self, snapshot.clone()); } } - if self.token.kind == token::OpenDelim(token::Paren) { + if token::ModSep == self.token.kind { + // We have some certainty that this was a bad turbofish at this point. + // `foo< bar >::` + err.span_suggestion( + op_span.shrink_to_lo(), + msg, + "::".to_string(), + Applicability::MaybeIncorrect, + ); + + let snapshot = self.clone(); + + self.bump(); // `::` + // Consume the rest of the likely `foo::new()` or return at `foo`. + match self.parse_expr() { + Ok(_) => { + // 99% certain that the suggestion is correct, continue parsing. + err.emit(); + // FIXME: actually check that the two expressions in the binop are + // paths and resynthesize new fn call expression instead of using + // `ExprKind::Err` placeholder. + return Ok(Some(self.mk_expr( + lhs.span.to(self.prev_span), + ExprKind::Err, + ThinVec::new(), + ))); + } + Err(mut err) => { + err.cancel(); + // Not entirely sure now, but we bubble the error up with the + // suggestion. + mem::replace(self, snapshot); + return Err(err); + } + } + } else if token::OpenDelim(token::Paren) == self.token.kind { // We have high certainty that this was a bad turbofish at this point. // `foo< bar >(` err.span_suggestion( @@ -601,6 +640,7 @@ impl<'a> Parser<'a> { ); let snapshot = self.clone(); + self.bump(); // `(` // Consume the fn call arguments. let modifiers = vec![ @@ -608,7 +648,6 @@ impl<'a> Parser<'a> { (token::CloseDelim(token::Paren), -1), ]; let early_return = vec![token::Eof]; - self.bump(); // `(` self.consume_tts(1, &modifiers[..], &early_return[..]); if self.token.kind == token::Eof { diff --git a/src/test/ui/did_you_mean/issue-40396.stderr b/src/test/ui/did_you_mean/issue-40396.stderr index d0448cc265c..5e3771002b6 100644 --- a/src/test/ui/did_you_mean/issue-40396.stderr +++ b/src/test/ui/did_you_mean/issue-40396.stderr @@ -13,9 +13,10 @@ error: chained comparison operators require parentheses | LL | Vec::new(); | ^^^^^ +help: use `::<...>` instead of `<...>` if you meant to specify type arguments | - = help: use `::<...>` instead of `<...>` if you meant to specify type arguments - = help: or use `(...)` if you meant to specify fn arguments +LL | Vec::::new(); + | ^^ error: chained comparison operators require parentheses --> $DIR/issue-40396.rs:12:20 -- cgit 1.4.1-3-g733a5 From dfdc369b40da72eb9ff466fab89584c7815d7a80 Mon Sep 17 00:00:00 2001 From: Esteban Küber Date: Tue, 1 Oct 2019 11:24:05 -0700 Subject: review comments --- src/libsyntax/parse/diagnostics.rs | 79 ++++++++++++---------- src/test/ui/did_you_mean/issue-40396.stderr | 6 +- .../require-parens-for-chained-comparison.rs | 18 +++-- .../require-parens-for-chained-comparison.stderr | 15 +++- 4 files changed, 70 insertions(+), 48 deletions(-) (limited to 'src/libsyntax/parse') diff --git a/src/libsyntax/parse/diagnostics.rs b/src/libsyntax/parse/diagnostics.rs index 4fbd36cfefc..14c5dc6132c 100644 --- a/src/libsyntax/parse/diagnostics.rs +++ b/src/libsyntax/parse/diagnostics.rs @@ -17,6 +17,8 @@ use syntax_pos::{Span, DUMMY_SP, MultiSpan, SpanSnippetError}; use log::{debug, trace}; use std::mem; +const TURBOFISH: &'static str = "use the \"turbofish\" `::<...>` instead of `<...>` to specify \ + type arguments"; /// Creates a placeholder argument. crate fn dummy_arg(ident: Ident) -> Param { let pat = P(Pat { @@ -544,10 +546,20 @@ impl<'a> Parser<'a> { /// 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 and are left-associative. + /// precedence (see `fn precedence`) and are left-associative (see `fn fixity`). /// /// This can also be hit if someone incorrectly writes `foo()` when they should have used - /// the turbofish syntax. We attempt some heuristic recovery if that is the case. + /// the turbofish (`foo::()`) syntax. We attempt some heuristic recovery if that is the + /// case. + /// + /// Keep in mind that given that `outer_op.is_comparison()` holds and comparison ops are left + /// associative we can infer that we have: + /// + /// outer_op + /// / \ + /// inner_op r2 + /// / \ + /// l1 r1 crate fn check_no_chained_comparison( &mut self, lhs: &Expr, @@ -560,30 +572,36 @@ impl<'a> Parser<'a> { ); match lhs.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 suggest = |err: &mut DiagnosticBuilder<'_>| { + err.span_suggestion( + op_span.shrink_to_lo(), + TURBOFISH, + "::".to_string(), + Applicability::MaybeIncorrect, + ); + }; + if op.node == BinOpKind::Lt && *outer_op == AssocOp::Less || // Include `<` to provide this recommendation *outer_op == AssocOp::Greater // even in a case like the following: { // Foo>> - let msg = "use `::<...>` instead of `<...>` if you meant to specify type \ - arguments"; if *outer_op == AssocOp::Less { let snapshot = self.clone(); self.bump(); - // So far we have parsed `foo Parser<'a> { if token::ModSep == self.token.kind { // We have some certainty that this was a bad turbofish at this point. // `foo< bar >::` - err.span_suggestion( - op_span.shrink_to_lo(), - msg, - "::".to_string(), - Applicability::MaybeIncorrect, - ); + suggest(&mut err); let snapshot = self.clone(); - self.bump(); // `::` + // Consume the rest of the likely `foo::new()` or return at `foo`. match self.parse_expr() { Ok(_) => { @@ -621,8 +634,8 @@ impl<'a> Parser<'a> { ThinVec::new(), ))); } - Err(mut err) => { - err.cancel(); + Err(mut expr_err) => { + expr_err.cancel(); // Not entirely sure now, but we bubble the error up with the // suggestion. mem::replace(self, snapshot); @@ -632,45 +645,39 @@ impl<'a> Parser<'a> { } else if token::OpenDelim(token::Paren) == self.token.kind { // We have high certainty that this was a bad turbofish at this point. // `foo< bar >(` - err.span_suggestion( - op_span.shrink_to_lo(), - msg, - "::".to_string(), - Applicability::MaybeIncorrect, - ); + suggest(&mut err); let snapshot = self.clone(); self.bump(); // `(` // Consume the fn call arguments. - let modifiers = vec![ + let modifiers = [ (token::OpenDelim(token::Paren), 1), (token::CloseDelim(token::Paren), -1), ]; - let early_return = vec![token::Eof]; - self.consume_tts(1, &modifiers[..], &early_return[..]); + self.consume_tts(1, &modifiers[..], &[]); - if self.token.kind == token::Eof { + return if self.token.kind == token::Eof { // Not entirely sure now, but we bubble the error up with the // suggestion. mem::replace(self, snapshot); - return Err(err); + Err(err) } else { // 99% certain that the suggestion is correct, continue parsing. err.emit(); // FIXME: actually check that the two expressions in the binop are // paths and resynthesize new fn call expression instead of using // `ExprKind::Err` placeholder. - return Ok(Some(self.mk_expr( + Ok(Some(self.mk_expr( lhs.span.to(self.prev_span), ExprKind::Err, ThinVec::new(), - ))); + ))) } } 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(msg); + 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). } @@ -1459,15 +1466,15 @@ impl<'a> Parser<'a> { fn consume_tts( &mut self, - mut acc: i64, - modifier: &[(token::TokenKind, i64)], // Not using FxHasMap and FxHashSet due to + mut acc: i64, // `i64` because malformed code can have more closing delims than opening. + modifier: &[(token::TokenKind, i64)], // Not using `FxHashMap` and `FxHashSet` due to early_return: &[token::TokenKind], // `token::TokenKind: !Eq + !Hash`. ) { while acc > 0 { - if let Some((_, val)) = modifier.iter().filter(|(t, _)| *t == self.token.kind).next() { + if let Some((_, val)) = modifier.iter().find(|(t, _)| *t == self.token.kind) { acc += *val; } - if early_return.contains(&self.token.kind) { + if self.token.kind == token::Eof || early_return.contains(&self.token.kind) { break; } self.bump(); diff --git a/src/test/ui/did_you_mean/issue-40396.stderr b/src/test/ui/did_you_mean/issue-40396.stderr index dd19bc137e3..9757f8258c1 100644 --- a/src/test/ui/did_you_mean/issue-40396.stderr +++ b/src/test/ui/did_you_mean/issue-40396.stderr @@ -3,7 +3,7 @@ error: chained comparison operators require parentheses | LL | (0..13).collect>(); | ^^^^^ -help: use `::<...>` instead of `<...>` if you meant to specify type arguments +help: use the "turbofish" `::<...>` instead of `<...>` to specify type arguments | LL | (0..13).collect::>(); | ^^ @@ -13,7 +13,7 @@ error: chained comparison operators require parentheses | LL | Vec::new(); | ^^^^^ -help: use `::<...>` instead of `<...>` if you meant to specify type arguments +help: use the "turbofish" `::<...>` instead of `<...>` to specify type arguments | LL | Vec::::new(); | ^^ @@ -23,7 +23,7 @@ error: chained comparison operators require parentheses | LL | (0..13).collect(); | ^^^^^ -help: use `::<...>` instead of `<...>` if you meant to specify type arguments +help: use the "turbofish" `::<...>` instead of `<...>` to specify type arguments | LL | (0..13).collect::(); | ^^ 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 11839938cb0..f3bfe2d482f 100644 --- a/src/test/ui/parser/require-parens-for-chained-comparison.rs +++ b/src/test/ui/parser/require-parens-for-chained-comparison.rs @@ -3,18 +3,24 @@ struct X; fn main() { false == false == false; - //~^ ERROR: chained comparison operators require parentheses + //~^ ERROR chained comparison operators require parentheses false == 0 < 2; - //~^ ERROR: chained comparison operators require parentheses - //~| ERROR: mismatched types - //~| ERROR: mismatched types + //~^ ERROR chained comparison operators require parentheses + //~| ERROR mismatched types + //~| ERROR mismatched types f(); //~^ ERROR chained comparison operators require parentheses - //~| HELP: use `::<...>` instead of `<...>` + //~| HELP use the "turbofish" `::<...>` instead of `<...>` to specify type arguments f, Option>>(1, 2); //~^ ERROR chained comparison operators require parentheses - //~| HELP: use `::<...>` instead of `<...>` + //~| HELP use the "turbofish" `::<...>` instead of `<...>` to specify type arguments + + use std::convert::identity; + let _ = identity; + //~^ ERROR chained comparison operators require parentheses + //~| HELP use the "turbofish" `::<...>` 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 02fb56a7f9b..4b108e1db87 100644 --- a/src/test/ui/parser/require-parens-for-chained-comparison.stderr +++ b/src/test/ui/parser/require-parens-for-chained-comparison.stderr @@ -15,7 +15,7 @@ error: chained comparison operators require parentheses | LL | f(); | ^^^ -help: use `::<...>` instead of `<...>` if you meant to specify type arguments +help: use the "turbofish" `::<...>` instead of `<...>` to specify type arguments | LL | f::(); | ^^ @@ -25,11 +25,20 @@ error: chained comparison operators require parentheses | LL | f, Option>>(1, 2); | ^^^^^^^^ -help: use `::<...>` instead of `<...>` if you meant to specify type arguments +help: use the "turbofish" `::<...>` 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 + | +LL | let _ = identity; + | ^^^^ + | + = help: use the "turbofish" `::<...>` 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 | @@ -48,6 +57,6 @@ LL | false == 0 < 2; = note: expected type `bool` found type `{integer}` -error: aborting due to 6 previous errors +error: aborting due to 7 previous errors For more information about this error, try `rustc --explain E0308`. -- cgit 1.4.1-3-g733a5 From f1499a864688a484c04c4e53962dc8ec44f79a03 Mon Sep 17 00:00:00 2001 From: Esteban Küber Date: Tue, 1 Oct 2019 15:51:50 -0700 Subject: review comments --- src/libsyntax/parse/diagnostics.rs | 37 +++++++++++++++++-------------------- 1 file changed, 17 insertions(+), 20 deletions(-) (limited to 'src/libsyntax/parse') diff --git a/src/libsyntax/parse/diagnostics.rs b/src/libsyntax/parse/diagnostics.rs index 14c5dc6132c..72206ffb28d 100644 --- a/src/libsyntax/parse/diagnostics.rs +++ b/src/libsyntax/parse/diagnostics.rs @@ -570,6 +570,11 @@ impl<'a> Parser<'a> { "check_no_chained_comparison: {:?} is not comparison", outer_op, ); + + let mk_err_expr = |this: &Self, span| { + Ok(Some(this.mk_expr(span, ExprKind::Err, ThinVec::new()))) + }; + match lhs.kind { ExprKind::Binary(op, _, _) if op.node.is_comparison() => { // Respan to include both operators. @@ -601,7 +606,7 @@ impl<'a> Parser<'a> { (token::Gt, -1), (token::BinOp(token::Shr), -2), ]; - self.consume_tts(1, &modifiers[..], &[]); + self.consume_tts(1, &modifiers[..]); if !&[ token::OpenDelim(token::Paren), @@ -612,7 +617,7 @@ impl<'a> Parser<'a> { mem::replace(self, snapshot.clone()); } } - if token::ModSep == self.token.kind { + return if token::ModSep == self.token.kind { // We have some certainty that this was a bad turbofish at this point. // `foo< bar >::` suggest(&mut err); @@ -628,18 +633,14 @@ impl<'a> Parser<'a> { // FIXME: actually check that the two expressions in the binop are // paths and resynthesize new fn call expression instead of using // `ExprKind::Err` placeholder. - return Ok(Some(self.mk_expr( - lhs.span.to(self.prev_span), - ExprKind::Err, - ThinVec::new(), - ))); + mk_err_expr(self, lhs.span.to(self.prev_span)) } Err(mut expr_err) => { expr_err.cancel(); // Not entirely sure now, but we bubble the error up with the // suggestion. mem::replace(self, snapshot); - return Err(err); + Err(err) } } } else if token::OpenDelim(token::Paren) == self.token.kind { @@ -655,9 +656,9 @@ impl<'a> Parser<'a> { (token::OpenDelim(token::Paren), 1), (token::CloseDelim(token::Paren), -1), ]; - self.consume_tts(1, &modifiers[..], &[]); + self.consume_tts(1, &modifiers[..]); - return if self.token.kind == token::Eof { + if self.token.kind == token::Eof { // Not entirely sure now, but we bubble the error up with the // suggestion. mem::replace(self, snapshot); @@ -668,11 +669,7 @@ impl<'a> Parser<'a> { // FIXME: actually check that the two expressions in the binop are // paths and resynthesize new fn call expression instead of using // `ExprKind::Err` placeholder. - Ok(Some(self.mk_expr( - lhs.span.to(self.prev_span), - ExprKind::Err, - ThinVec::new(), - ))) + mk_err_expr(self, lhs.span.to(self.prev_span)) } } else { // All we know is that this is `foo < bar >` and *nothing* else. Try to @@ -680,8 +677,8 @@ impl<'a> Parser<'a> { 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). - } - return Err(err); + Err(err) + }; } err.emit(); } @@ -1467,14 +1464,14 @@ impl<'a> Parser<'a> { fn consume_tts( &mut self, mut acc: i64, // `i64` because malformed code can have more closing delims than opening. - modifier: &[(token::TokenKind, i64)], // Not using `FxHashMap` and `FxHashSet` due to - early_return: &[token::TokenKind], // `token::TokenKind: !Eq + !Hash`. + // Not using `FxHashMap` due to `token::TokenKind: !Eq + !Hash`. + modifier: &[(token::TokenKind, i64)], ) { while acc > 0 { if let Some((_, val)) = modifier.iter().find(|(t, _)| *t == self.token.kind) { acc += *val; } - if self.token.kind == token::Eof || early_return.contains(&self.token.kind) { + if self.token.kind == token::Eof { break; } self.bump(); -- cgit 1.4.1-3-g733a5 From 02f57f83a9ea5903cb02bdc304800661c8f4296f Mon Sep 17 00:00:00 2001 From: Esteban Küber Date: Thu, 3 Oct 2019 13:22:18 -0700 Subject: review comments --- src/librustc_errors/diagnostic.rs | 41 +++++++++++++--- src/librustc_errors/emitter.rs | 4 +- src/librustc_errors/lib.rs | 2 + src/libsyntax/parse/diagnostics.rs | 57 ++++++++++++---------- src/test/ui/did_you_mean/issue-40396.stderr | 6 +-- .../require-parens-for-chained-comparison.rs | 6 +-- .../require-parens-for-chained-comparison.stderr | 6 +-- 7 files changed, 81 insertions(+), 41 deletions(-) (limited to 'src/libsyntax/parse') diff --git a/src/librustc_errors/diagnostic.rs b/src/librustc_errors/diagnostic.rs index 3f1b91256c4..811a48a39f0 100644 --- a/src/librustc_errors/diagnostic.rs +++ b/src/librustc_errors/diagnostic.rs @@ -298,9 +298,13 @@ impl Diagnostic { /// * may contain a name of a function, variable, or type, but not whole expressions /// /// See `CodeSuggestion` for more information. - pub fn span_suggestion(&mut self, sp: Span, msg: &str, - suggestion: String, - applicability: Applicability) -> &mut Self { + pub fn span_suggestion( + &mut self, + sp: Span, + msg: &str, + suggestion: String, + applicability: Applicability, + ) -> &mut Self { self.suggestions.push(CodeSuggestion { substitutions: vec![Substitution { parts: vec![SubstitutionPart { @@ -315,10 +319,35 @@ impl Diagnostic { self } + pub fn span_suggestion_verbose( + &mut self, + sp: Span, + msg: &str, + suggestion: String, + applicability: Applicability, + ) -> &mut Self { + self.suggestions.push(CodeSuggestion { + substitutions: vec![Substitution { + parts: vec![SubstitutionPart { + snippet: suggestion, + span: sp, + }], + }], + msg: msg.to_owned(), + style: SuggestionStyle::ShowAlways, + applicability, + }); + self + } + /// Prints out a message with multiple suggested edits of the code. - pub fn span_suggestions(&mut self, sp: Span, msg: &str, - suggestions: impl Iterator, applicability: Applicability) -> &mut Self - { + pub fn span_suggestions( + &mut self, + sp: Span, + msg: &str, + suggestions: impl Iterator, + applicability: Applicability, + ) -> &mut Self { self.suggestions.push(CodeSuggestion { substitutions: suggestions.map(|snippet| Substitution { parts: vec![SubstitutionPart { diff --git a/src/librustc_errors/emitter.rs b/src/librustc_errors/emitter.rs index 0c7aa3582ac..bd8191065ee 100644 --- a/src/librustc_errors/emitter.rs +++ b/src/librustc_errors/emitter.rs @@ -221,7 +221,9 @@ pub trait Emitter { // when this style is set we want the suggestion to be a message, not inline sugg.style != SuggestionStyle::HideCodeAlways && // trivial suggestion for tooling's sake, never shown - sugg.style != SuggestionStyle::CompletelyHidden + sugg.style != SuggestionStyle::CompletelyHidden && + // subtle suggestion, never shown inline + sugg.style != SuggestionStyle::ShowAlways { let substitution = &sugg.substitutions[0].parts[0].snippet.trim(); let msg = if substitution.len() == 0 || sugg.style.hide_inline() { diff --git a/src/librustc_errors/lib.rs b/src/librustc_errors/lib.rs index f9dc13ce97e..2fae584c153 100644 --- a/src/librustc_errors/lib.rs +++ b/src/librustc_errors/lib.rs @@ -81,6 +81,8 @@ pub enum SuggestionStyle { /// This will *not* show the code if the suggestion is inline *and* the suggested code is /// empty. ShowCode, + /// Always show the suggested code independently. + ShowAlways, } impl SuggestionStyle { diff --git a/src/libsyntax/parse/diagnostics.rs b/src/libsyntax/parse/diagnostics.rs index 72206ffb28d..e3abf8ffc6c 100644 --- a/src/libsyntax/parse/diagnostics.rs +++ b/src/libsyntax/parse/diagnostics.rs @@ -17,8 +17,7 @@ use syntax_pos::{Span, DUMMY_SP, MultiSpan, SpanSnippetError}; use log::{debug, trace}; use std::mem; -const TURBOFISH: &'static str = "use the \"turbofish\" `::<...>` instead of `<...>` to specify \ - type arguments"; +const TURBOFISH: &'static str = "use `::<...>` instead of `<...>` to specify type arguments"; /// Creates a placeholder argument. crate fn dummy_arg(ident: Ident) -> Param { let pat = P(Pat { @@ -585,7 +584,7 @@ impl<'a> Parser<'a> { ); let suggest = |err: &mut DiagnosticBuilder<'_>| { - err.span_suggestion( + err.span_suggestion_verbose( op_span.shrink_to_lo(), TURBOFISH, "::".to_string(), @@ -647,29 +646,16 @@ impl<'a> Parser<'a> { // We have high certainty that this was a bad turbofish at this point. // `foo< bar >(` suggest(&mut err); - - let snapshot = self.clone(); - self.bump(); // `(` - // Consume the fn call arguments. - let modifiers = [ - (token::OpenDelim(token::Paren), 1), - (token::CloseDelim(token::Paren), -1), - ]; - self.consume_tts(1, &modifiers[..]); - - if self.token.kind == token::Eof { - // Not entirely sure now, but we bubble the error up with the - // suggestion. - mem::replace(self, snapshot); - Err(err) - } else { - // 99% certain that the suggestion is correct, continue parsing. - err.emit(); - // 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)) + match self.consume_fn_args() { + Err(()) => Err(err), + Ok(()) => { + err.emit(); + // 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)) + } } } else { // All we know is that this is `foo < bar >` and *nothing* else. Try to @@ -687,6 +673,27 @@ impl<'a> Parser<'a> { Ok(None) } + fn consume_fn_args(&mut self) -> Result<(), ()> { + let snapshot = self.clone(); + self.bump(); // `(` + + // Consume the fn call arguments. + let modifiers = [ + (token::OpenDelim(token::Paren), 1), + (token::CloseDelim(token::Paren), -1), + ]; + self.consume_tts(1, &modifiers[..]); + + if self.token.kind == token::Eof { + // Not entirely sure that what we consumed were fn arguments, rollback. + mem::replace(self, snapshot); + Err(()) + } else { + // 99% certain that the suggestion is correct, continue parsing. + Ok(()) + } + } + crate fn maybe_report_ambiguous_plus( &mut self, allow_plus: bool, diff --git a/src/test/ui/did_you_mean/issue-40396.stderr b/src/test/ui/did_you_mean/issue-40396.stderr index 9757f8258c1..7fc7c2628c4 100644 --- a/src/test/ui/did_you_mean/issue-40396.stderr +++ b/src/test/ui/did_you_mean/issue-40396.stderr @@ -3,7 +3,7 @@ error: chained comparison operators require parentheses | LL | (0..13).collect>(); | ^^^^^ -help: use the "turbofish" `::<...>` instead of `<...>` to specify type arguments +help: use `::<...>` instead of `<...>` to specify type arguments | LL | (0..13).collect::>(); | ^^ @@ -13,7 +13,7 @@ error: chained comparison operators require parentheses | LL | Vec::new(); | ^^^^^ -help: use the "turbofish" `::<...>` instead of `<...>` to specify type arguments +help: use `::<...>` instead of `<...>` to specify type arguments | LL | Vec::::new(); | ^^ @@ -23,7 +23,7 @@ error: chained comparison operators require parentheses | LL | (0..13).collect(); | ^^^^^ -help: use the "turbofish" `::<...>` instead of `<...>` to specify type arguments +help: use `::<...>` instead of `<...>` to specify type arguments | LL | (0..13).collect::(); | ^^ 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 f3bfe2d482f..9c7a25d589a 100644 --- a/src/test/ui/parser/require-parens-for-chained-comparison.rs +++ b/src/test/ui/parser/require-parens-for-chained-comparison.rs @@ -12,15 +12,15 @@ fn main() { f(); //~^ ERROR chained comparison operators require parentheses - //~| HELP use the "turbofish" `::<...>` instead of `<...>` to specify type arguments + //~| HELP use `::<...>` instead of `<...>` to specify type arguments f, Option>>(1, 2); //~^ ERROR chained comparison operators require parentheses - //~| HELP use the "turbofish" `::<...>` instead of `<...>` to specify type arguments + //~| HELP use `::<...>` instead of `<...>` to specify type arguments use std::convert::identity; let _ = identity; //~^ ERROR chained comparison operators require parentheses - //~| HELP use the "turbofish" `::<...>` instead of `<...>` to specify type arguments + //~| 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 4b108e1db87..5aa37a40cbd 100644 --- a/src/test/ui/parser/require-parens-for-chained-comparison.stderr +++ b/src/test/ui/parser/require-parens-for-chained-comparison.stderr @@ -15,7 +15,7 @@ error: chained comparison operators require parentheses | LL | f(); | ^^^ -help: use the "turbofish" `::<...>` instead of `<...>` to specify type arguments +help: use `::<...>` instead of `<...>` to specify type arguments | LL | f::(); | ^^ @@ -25,7 +25,7 @@ error: chained comparison operators require parentheses | LL | f, Option>>(1, 2); | ^^^^^^^^ -help: use the "turbofish" `::<...>` instead of `<...>` to specify type arguments +help: use `::<...>` instead of `<...>` to specify type arguments | LL | f::, Option>>(1, 2); | ^^ @@ -36,7 +36,7 @@ error: chained comparison operators require parentheses LL | let _ = identity; | ^^^^ | - = help: use the "turbofish" `::<...>` instead of `<...>` to specify type arguments + = help: use `::<...>` instead of `<...>` to specify type arguments = help: or use `(...)` if you meant to specify fn arguments error[E0308]: mismatched types -- cgit 1.4.1-3-g733a5 From 5f94a53d1a4e03787886b8bca750566d80255f85 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Sat, 5 Oct 2019 04:34:26 +0200 Subject: Account for macro invocation in `let mut $pat` diagnostic. --- src/libsyntax/parse/parser/pat.rs | 6 ++- .../issue-65122-mac-invoc-in-mut-patterns.rs | 26 +++++++++++++ .../issue-65122-mac-invoc-in-mut-patterns.stderr | 45 ++++++++++++++++++++++ 3 files changed, 76 insertions(+), 1 deletion(-) create mode 100644 src/test/ui/parser/issue-65122-mac-invoc-in-mut-patterns.rs create mode 100644 src/test/ui/parser/issue-65122-mac-invoc-in-mut-patterns.stderr (limited to 'src/libsyntax/parse') diff --git a/src/libsyntax/parse/parser/pat.rs b/src/libsyntax/parse/parser/pat.rs index 7eb2a73a11a..48f9e301610 100644 --- a/src/libsyntax/parse/parser/pat.rs +++ b/src/libsyntax/parse/parser/pat.rs @@ -4,7 +4,7 @@ use crate::{maybe_recover_from_interpolated_ty_qpath, maybe_whole}; use crate::ptr::P; use crate::ast::{self, Attribute, Pat, PatKind, FieldPat, RangeEnd, RangeSyntax, Mac}; use crate::ast::{BindingMode, Ident, Mutability, Path, QSelf, Expr, ExprKind}; -use crate::mut_visit::{noop_visit_pat, MutVisitor}; +use crate::mut_visit::{noop_visit_pat, noop_visit_mac, MutVisitor}; use crate::parse::token::{self}; use crate::print::pprust; use crate::source_map::{respan, Span, Spanned}; @@ -481,6 +481,10 @@ impl<'a> Parser<'a> { fn make_all_value_bindings_mutable(pat: &mut P) -> bool { struct AddMut(bool); impl MutVisitor for AddMut { + fn visit_mac(&mut self, mac: &mut Mac) { + noop_visit_mac(mac, self); + } + fn visit_pat(&mut self, pat: &mut P) { if let PatKind::Ident(BindingMode::ByValue(ref mut m @ Mutability::Immutable), ..) = pat.kind diff --git a/src/test/ui/parser/issue-65122-mac-invoc-in-mut-patterns.rs b/src/test/ui/parser/issue-65122-mac-invoc-in-mut-patterns.rs new file mode 100644 index 00000000000..97a405b6999 --- /dev/null +++ b/src/test/ui/parser/issue-65122-mac-invoc-in-mut-patterns.rs @@ -0,0 +1,26 @@ +// Regression test; used to ICE with 'visit_mac disabled by default' due to a +// `MutVisitor` in `fn make_all_value_bindings_mutable` (`parse/parser/pat.rs`). + +macro_rules! mac1 { + ($eval:expr) => { + let mut $eval = (); + //~^ ERROR `mut` must be followed by a named binding + }; +} + +macro_rules! mac2 { + ($eval:pat) => { + let mut $eval = (); + //~^ ERROR `mut` must be followed by a named binding + //~| ERROR expected identifier, found `does_not_exist!()` + }; +} + +fn foo() { + mac1! { does_not_exist!() } + //~^ ERROR cannot find macro `does_not_exist` in this scope + mac2! { does_not_exist!() } + //~^ ERROR cannot find macro `does_not_exist` in this scope +} + +fn main() {} diff --git a/src/test/ui/parser/issue-65122-mac-invoc-in-mut-patterns.stderr b/src/test/ui/parser/issue-65122-mac-invoc-in-mut-patterns.stderr new file mode 100644 index 00000000000..dd193d6a86e --- /dev/null +++ b/src/test/ui/parser/issue-65122-mac-invoc-in-mut-patterns.stderr @@ -0,0 +1,45 @@ +error: `mut` must be followed by a named binding + --> $DIR/issue-65122-mac-invoc-in-mut-patterns.rs:6:13 + | +LL | let mut $eval = (); + | ^^^^^^^^^ help: remove the `mut` prefix: `does_not_exist!()` +... +LL | mac1! { does_not_exist!() } + | --------------------------- in this macro invocation + | + = note: `mut` may be followed by `variable` and `variable @ pattern` + +error: expected identifier, found `does_not_exist!()` + --> $DIR/issue-65122-mac-invoc-in-mut-patterns.rs:13:17 + | +LL | let mut $eval = (); + | ^^^^^ expected identifier +... +LL | mac2! { does_not_exist!() } + | --------------------------- in this macro invocation + +error: `mut` must be followed by a named binding + --> $DIR/issue-65122-mac-invoc-in-mut-patterns.rs:13:13 + | +LL | let mut $eval = (); + | ^^^ help: remove the `mut` prefix: `does_not_exist!()` +... +LL | mac2! { does_not_exist!() } + | --------------------------- in this macro invocation + | + = note: `mut` may be followed by `variable` and `variable @ pattern` + +error: cannot find macro `does_not_exist` in this scope + --> $DIR/issue-65122-mac-invoc-in-mut-patterns.rs:20:13 + | +LL | mac1! { does_not_exist!() } + | ^^^^^^^^^^^^^^ + +error: cannot find macro `does_not_exist` in this scope + --> $DIR/issue-65122-mac-invoc-in-mut-patterns.rs:22:13 + | +LL | mac2! { does_not_exist!() } + | ^^^^^^^^^^^^^^ + +error: aborting due to 5 previous errors + -- cgit 1.4.1-3-g733a5