Rollup merge of #64909 - estebank:turbofish-reloaded, r=Centril

When encountering chained operators use heuristics to recover from bad turbofish

9 files changed, 286 insertions, 86 deletions

diff --git a/src/librustc_errors/diagnostic.rs b/src/librustc_errors/diagnostic.rs
index 3f1b91256c4..fd74d8673da 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<Item = String>, applicability: Applicability) -> &mut Self
-    {
+    pub fn span_suggestions(
+        &mut self,
+        sp: Span,
+        msg: &str,
+        suggestions: impl Iterator<Item = String>,
+        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 9aea46da68b..68f933363da 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 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 4ad0bd06d99..f376c19a66c 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<bar>()` when they should have used
+    /// the turbofish (`foo::<bar>()`) 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<P<Expr>>> {
+        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<Bar<Baz<Qux, ()>>>
-                    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<bar<`, consume the rest of the type args.
+                        let modifiers = [
+                            (token::Lt, 1),
+                            (token::Gt, -1),
+                            (token::BinOp(token::Shr), -2),
+                        ];
+                        self.consume_tts(1, &modifiers[..]);
+
+                        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());
+                        }
+                    }
+                    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<bar>::new()` or return at `foo<bar>`.
+                        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 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 {
diff --git a/src/test/ui/did_you_mean/issue-40396.rs b/src/test/ui/did_you_mean/issue-40396.rs
index 6902779f33d..18933552054 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<Vec<i32>>();
     //~^ ERROR chained comparison
-}
-
-fn bar() {
     Vec<i32>::new();
     //~^ ERROR chained comparison
-}
-
-fn qux() {
     (0..13).collect<Vec<i32>();
     //~^ 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 7a08fda27e3..7fc7c2628c4 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<Vec<i32>>();
-   |                    ^^^^^^^^
+   |                    ^^^^^
+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::<Vec<i32>>();
+   |                    ^^
 
 error: chained comparison operators require parentheses
-  --> $DIR/issue-40396.rs:7:8
+  --> $DIR/issue-40396.rs:4:8
    |
 LL |     Vec<i32>::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::<i32>::new();
+   |        ^^
 
 error: chained comparison operators require parentheses
-  --> $DIR/issue-40396.rs:12:20
+  --> $DIR/issue-40396.rs:6:20
    |
 LL |     (0..13).collect<Vec<i32>();
-   |                    ^^^^^^^^
+   |                    ^^^^^
+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::<Vec<i32>();
+   |                    ^^
 
 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 3dcc0c8f3d4..9c7a25d589a 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<X>();
     //~^ ERROR chained comparison operators require parentheses
-    //~| HELP: use `::<...>` instead of `<...>`
-    //~| HELP: or use `(...)`
+    //~| HELP use `::<...>` instead of `<...>` to specify type arguments
+
+    f<Result<Option<X>, Option<Option<X>>>(1, 2);
+    //~^ ERROR chained comparison operators require parentheses
+    //~| HELP use `::<...>` instead of `<...>` to specify type arguments
+
+    use std::convert::identity;
+    let _ = identity<u8>;
+    //~^ 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 e927f4c3248..5aa37a40cbd 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<X>();
-   |      ^^^^
+   |      ^^^
+help: use `::<...>` instead of `<...>` to specify type arguments
    |
-   = help: use `::<...>` instead of `<...>` if you meant to specify type arguments
+LL |     f::<X>();
+   |      ^^
+
+error: chained comparison operators require parentheses
+  --> $DIR/require-parens-for-chained-comparison.rs:17:6
+   |
+LL |     f<Result<Option<X>, Option<Option<X>>>(1, 2);
+   |      ^^^^^^^^
+help: use `::<...>` instead of `<...>` to specify type arguments
+   |
+LL |     f::<Result<Option<X>, Option<Option<X>>>(1, 2);
+   |      ^^
+
+error: chained comparison operators require parentheses
+  --> $DIR/require-parens-for-chained-comparison.rs:22:21
+   |
+LL |     let _ = identity<u8>;
+   |                     ^^^^
+   |
+   = 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`.