diff --git a/src/librustc_errors/diagnostic.rs b/src/librustc_errors/diagnostic.rs index 3f1b91256c468..fd74d8673da4d 100644 --- a/src/librustc_errors/diagnostic.rs +++ b/src/librustc_errors/diagnostic.rs @@ -298,9 +298,31 @@ 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.span_suggestion_with_style( + sp, + msg, + suggestion, + applicability, + SuggestionStyle::ShowCode, + ); + self + } + + pub fn span_suggestion_with_style( + &mut self, + sp: Span, + msg: &str, + suggestion: String, + applicability: Applicability, + style: SuggestionStyle, + ) -> &mut Self { self.suggestions.push(CodeSuggestion { substitutions: vec![Substitution { parts: vec![SubstitutionPart { @@ -309,16 +331,37 @@ impl Diagnostic { }], }], msg: msg.to_owned(), - style: SuggestionStyle::ShowCode, + style, applicability, }); self } + pub fn span_suggestion_verbose( + &mut self, + sp: Span, + msg: &str, + suggestion: String, + applicability: Applicability, + ) -> &mut Self { + self.span_suggestion_with_style( + sp, + msg, + suggestion, + applicability, + SuggestionStyle::ShowAlways, + ); + 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 { @@ -340,17 +383,13 @@ impl Diagnostic { pub fn span_suggestion_short( &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::HideCodeInline, + self.span_suggestion_with_style( + sp, + msg, + suggestion, applicability, - }); + SuggestionStyle::HideCodeInline, + ); self } @@ -363,17 +402,13 @@ impl Diagnostic { pub fn span_suggestion_hidden( &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::HideCodeAlways, + self.span_suggestion_with_style( + sp, + msg, + suggestion, applicability, - }); + SuggestionStyle::HideCodeAlways, + ); self } @@ -384,17 +419,13 @@ impl Diagnostic { pub fn tool_only_span_suggestion( &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::CompletelyHidden, + self.span_suggestion_with_style( + sp, + msg, + suggestion, applicability, - }); + SuggestionStyle::CompletelyHidden, + ); self } diff --git a/src/librustc_errors/emitter.rs b/src/librustc_errors/emitter.rs index 9aea46da68b1a..68f933363daa1 100644 --- a/src/librustc_errors/emitter.rs +++ b/src/librustc_errors/emitter.rs @@ -218,10 +218,14 @@ pub trait Emitter { sugg.msg.split_whitespace().count() < 10 && // don't display multiline suggestions as labels !sugg.substitutions[0].parts[0].snippet.contains('\n') && - // 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 + ![ + // when this style is set we want the suggestion to be a message, not inline + SuggestionStyle::HideCodeAlways, + // trivial suggestion for tooling's sake, never shown + SuggestionStyle::CompletelyHidden, + // subtle suggestion, never shown inline + SuggestionStyle::ShowAlways, + ].contains(&sugg.style) { 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 f9dc13ce97eea..2fae584c15362 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 4ad0bd06d99ae..f376c19a66ccd 100644 --- a/src/libsyntax/parse/diagnostics.rs +++ b/src/libsyntax/parse/diagnostics.rs @@ -17,6 +17,7 @@ use syntax_pos::{Span, DUMMY_SP, MultiSpan, SpanSnippetError}; use log::{debug, trace}; use std::mem; +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 { @@ -543,35 +544,154 @@ 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 (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 (`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, + outer_op: &AssocOp, + ) -> PResult<'a, Option>> { + debug_assert!( + outer_op.is_comparison(), + "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. - 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", ); + + let suggest = |err: &mut DiagnosticBuilder<'_>| { + err.span_suggestion_verbose( + 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>> - 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). - return Err(err); + if *outer_op == AssocOp::Less { + let snapshot = self.clone(); + self.bump(); + // So far we have parsed `foo(` or `foo< bar >::`, so we rewind the + // parser and bail out. + mem::replace(self, snapshot.clone()); + } + } + 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); + + 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. + 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); + 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 >(` + suggest(&mut err); + // Consume the fn call arguments. + 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 + // 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) + }; } err.emit(); } _ => {} } - Ok(()) + 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( @@ -1364,6 +1484,23 @@ impl<'a> Parser<'a> { err } + fn consume_tts( + &mut self, + mut acc: i64, // `i64` because malformed code can have more closing delims than opening. + // 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 { + 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 diff --git a/src/libsyntax/parse/parser/expr.rs b/src/libsyntax/parse/parser/expr.rs index 23674ad589dc5..b459782d237c7 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 { diff --git a/src/test/ui/did_you_mean/issue-40396.rs b/src/test/ui/did_you_mean/issue-40396.rs index 6902779f33d23..1893355205433 100644 --- a/src/test/ui/did_you_mean/issue-40396.rs +++ b/src/test/ui/did_you_mean/issue-40396.rs @@ -1,16 +1,8 @@ -fn foo() { +fn main() { (0..13).collect>(); //~^ ERROR chained comparison -} - -fn bar() { Vec::new(); //~^ ERROR chained comparison -} - -fn qux() { (0..13).collect(); //~^ ERROR chained comparison } - -fn main() {} diff --git a/src/test/ui/did_you_mean/issue-40396.stderr b/src/test/ui/did_you_mean/issue-40396.stderr index 7a08fda27e355..7fc7c2628c472 100644 --- a/src/test/ui/did_you_mean/issue-40396.stderr +++ b/src/test/ui/did_you_mean/issue-40396.stderr @@ -2,28 +2,31 @@ error: chained comparison operators require parentheses --> $DIR/issue-40396.rs:2:20 | LL | (0..13).collect>(); - | ^^^^^^^^ + | ^^^^^ +help: use `::<...>` instead of `<...>` 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 | (0..13).collect::>(); + | ^^ error: chained comparison operators require parentheses - --> $DIR/issue-40396.rs:7:8 + --> $DIR/issue-40396.rs:4:8 | LL | Vec::new(); - | ^^^^^^^ + | ^^^^^ +help: use `::<...>` instead of `<...>` 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 + --> $DIR/issue-40396.rs:6:20 | LL | (0..13).collect(); - | ^^^^^^^^ + | ^^^^^ +help: use `::<...>` instead of `<...>` 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 | (0..13).collect::(); + | ^^ error: aborting due to 3 previous errors 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 3dcc0c8f3d496..9c7a25d589a1f 100644 --- a/src/test/ui/parser/require-parens-for-chained-comparison.rs +++ b/src/test/ui/parser/require-parens-for-chained-comparison.rs @@ -3,15 +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: or use `(...)` + //~| HELP use `::<...>` instead of `<...>` to specify type arguments + + f, Option>>(1, 2); + //~^ ERROR chained comparison operators require parentheses + //~| HELP use `::<...>` instead of `<...>` to specify type arguments + + use std::convert::identity; + let _ = identity; + //~^ ERROR chained comparison operators require parentheses + //~| 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 e927f4c32484e..5aa37a40cbd3d 100644 --- a/src/test/ui/parser/require-parens-for-chained-comparison.stderr +++ b/src/test/ui/parser/require-parens-for-chained-comparison.stderr @@ -2,21 +2,41 @@ error: chained comparison operators require parentheses --> $DIR/require-parens-for-chained-comparison.rs:5:11 | LL | false == false == false; - | ^^^^^^^^^^^^^^^^^ + | ^^^^^^^^^^^ error: chained comparison operators require parentheses --> $DIR/require-parens-for-chained-comparison.rs:8:11 | LL | false == 0 < 2; - | ^^^^^^^^ + | ^^^^^^ error: chained comparison operators require parentheses --> $DIR/require-parens-for-chained-comparison.rs:13:6 | LL | f(); - | ^^^^ + | ^^^ +help: use `::<...>` instead of `<...>` to specify type arguments | - = help: use `::<...>` instead of `<...>` if you meant to specify type arguments +LL | f::(); + | ^^ + +error: chained comparison operators require parentheses + --> $DIR/require-parens-for-chained-comparison.rs:17:6 + | +LL | f, 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 + | +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 @@ -37,6 +57,6 @@ LL | false == 0 < 2; = note: expected type `bool` found type `{integer}` -error: aborting due to 5 previous errors +error: aborting due to 7 previous errors For more information about this error, try `rustc --explain E0308`.