Improve the diagnostic around impl Trait <-> generic param mismatch

6 files changed, 243 insertions, 83 deletions

diff --git a/src/librustc_errors/diagnostic.rs b/src/librustc_errors/diagnostic.rs
index 75401f21862..81b32f436c8 100644
--- a/src/librustc_errors/diagnostic.rs
+++ b/src/librustc_errors/diagnostic.rs
@@ -259,6 +259,25 @@ impl Diagnostic {
         self
     }
 
+    pub fn multipart_suggestion(
+        &mut self,
+        msg: &str,
+        suggestion: Vec<(Span, String)>,
+    ) -> &mut Self {
+        self.suggestions.push(CodeSuggestion {
+            substitutions: vec![Substitution {
+                parts: suggestion
+                    .into_iter()
+                    .map(|(span, snippet)| SubstitutionPart { snippet, span })
+                    .collect(),
+            }],
+            msg: msg.to_owned(),
+            show_code_when_inline: true,
+            applicability: Applicability::Unspecified,
+        });
+        self
+    }
+
     /// Prints out a message with multiple suggested edits of the code.
     pub fn span_suggestions(&mut self, sp: Span, msg: &str, suggestions: Vec<String>) -> &mut Self {
         self.suggestions.push(CodeSuggestion {
diff --git a/src/librustc_errors/diagnostic_builder.rs b/src/librustc_errors/diagnostic_builder.rs
index 7e9ca8633a5..b813edadc57 100644
--- a/src/librustc_errors/diagnostic_builder.rs
+++ b/src/librustc_errors/diagnostic_builder.rs
@@ -178,6 +178,11 @@ impl<'a> DiagnosticBuilder<'a> {
                                           msg: &str,
                                           suggestion: String)
                                           -> &mut Self);
+    forward!(pub fn multipart_suggestion(
+        &mut self,
+        msg: &str,
+        suggestion: Vec<(Span, String)>
+    ) -> &mut Self);
     forward!(pub fn span_suggestion(&mut self,
                                     sp: Span,
                                     msg: &str,
diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs
index cb59af54804..bdaee97e8f6 100644
--- a/src/librustc_resolve/lib.rs
+++ b/src/librustc_resolve/lib.rs
@@ -41,7 +41,7 @@ use rustc::ty;
 use rustc::hir::{Freevar, FreevarMap, TraitCandidate, TraitMap, GlobMap};
 use rustc::util::nodemap::{NodeMap, NodeSet, FxHashMap, FxHashSet, DefIdMap};
 
-use syntax::codemap::{BytePos, CodeMap};
+use syntax::codemap::CodeMap;
 use syntax::ext::hygiene::{Mark, MarkKind, SyntaxContext};
 use syntax::ast::{self, Name, NodeId, Ident, FloatTy, IntTy, UintTy};
 use syntax::ext::base::SyntaxExtension;
@@ -210,12 +210,12 @@ fn resolve_struct_error<'sess, 'a>(resolver: &'sess Resolver,
             // Try to retrieve the span of the function signature and generate a new message with
             // a local type parameter
             let sugg_msg = "try using a local type parameter instead";
-            if let Some((sugg_span, new_snippet)) = generate_local_type_param_snippet(cm, span) {
+            if let Some((sugg_span, new_snippet)) = cm.generate_local_type_param_snippet(span) {
                 // Suggest the modification to the user
                 err.span_suggestion(sugg_span,
                                     sugg_msg,
                                     new_snippet);
-            } else if let Some(sp) = generate_fn_name_span(cm, span) {
+            } else if let Some(sp) = cm.generate_fn_name_span(span) {
                 err.span_label(sp, "try adding a local type parameter in this method instead");
             } else {
                 err.help("try using a local type parameter instead");
@@ -412,77 +412,6 @@ fn reduce_impl_span_to_impl_keyword(cm: &CodeMap, impl_span: Span) -> Span {
     impl_span
 }
 
-fn generate_fn_name_span(cm: &CodeMap, span: Span) -> Option<Span> {
-    let prev_span = cm.span_extend_to_prev_str(span, "fn", true);
-    cm.span_to_snippet(prev_span).map(|snippet| {
-        let len = snippet.find(|c: char| !c.is_alphanumeric() && c != '_')
-            .expect("no label after fn");
-        prev_span.with_hi(BytePos(prev_span.lo().0 + len as u32))
-    }).ok()
-}
-
-/// Take the span of a type parameter in a function signature and try to generate a span for the
-/// function name (with generics) and a new snippet for this span with the pointed type parameter as
-/// a new local type parameter.
-///
-/// For instance:
-/// ```rust,ignore (pseudo-Rust)
-/// // Given span
-/// fn my_function(param: T)
-/// //                    ^ Original span
-///
-/// // Result
-/// fn my_function(param: T)
-/// // ^^^^^^^^^^^ Generated span with snippet `my_function<T>`
-/// ```
-///
-/// Attention: The method used is very fragile since it essentially duplicates the work of the
-/// parser. If you need to use this function or something similar, please consider updating the
-/// codemap functions and this function to something more robust.
-fn generate_local_type_param_snippet(cm: &CodeMap, span: Span) -> Option<(Span, String)> {
-    // Try to extend the span to the previous "fn" keyword to retrieve the function
-    // signature
-    let sugg_span = cm.span_extend_to_prev_str(span, "fn", false);
-    if sugg_span != span {
-        if let Ok(snippet) = cm.span_to_snippet(sugg_span) {
-            // Consume the function name
-            let mut offset = snippet.find(|c: char| !c.is_alphanumeric() && c != '_')
-                .expect("no label after fn");
-
-            // Consume the generics part of the function signature
-            let mut bracket_counter = 0;
-            let mut last_char = None;
-            for c in snippet[offset..].chars() {
-                match c {
-                    '<' => bracket_counter += 1,
-                    '>' => bracket_counter -= 1,
-                    '(' => if bracket_counter == 0 { break; }
-                    _ => {}
-                }
-                offset += c.len_utf8();
-                last_char = Some(c);
-            }
-
-            // Adjust the suggestion span to encompass the function name with its generics
-            let sugg_span = sugg_span.with_hi(BytePos(sugg_span.lo().0 + offset as u32));
-
-            // Prepare the new suggested snippet to append the type parameter that triggered
-            // the error in the generics of the function signature
-            let mut new_snippet = if last_char == Some('>') {
-                format!("{}, ", &snippet[..(offset - '>'.len_utf8())])
-            } else {
-                format!("{}<", &snippet[..offset])
-            };
-            new_snippet.push_str(&cm.span_to_snippet(span).unwrap_or("T".to_string()));
-            new_snippet.push('>');
-
-            return Some((sugg_span, new_snippet));
-        }
-    }
-
-    None
-}
-
 #[derive(Copy, Clone, Debug)]
 struct BindingInfo {
     span: Span,
diff --git a/src/librustc_typeck/check/compare_method.rs b/src/librustc_typeck/check/compare_method.rs
index ba950f90d0a..befe42f991f 100644
--- a/src/librustc_typeck/check/compare_method.rs
+++ b/src/librustc_typeck/check/compare_method.rs
@@ -720,7 +720,7 @@ fn compare_synthetic_generics<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
                                         _trait_item_span: Option<Span>) // FIXME necessary?
                                         -> Result<(), ErrorReported> {
     // FIXME(chrisvittal) Clean up this function, list of FIXME items:
-    //     1. Better messages for the span lables
+    //     1. Better messages for the span labels
     //     2. Explanation as to what is going on
     //     3. Correct the function signature for what we actually use
     // If we get here, we already have the same number of generics, so the zip will
@@ -751,8 +751,131 @@ fn compare_synthetic_generics<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
                                            E0643,
                                            "method `{}` has incompatible signature for trait",
                                            trait_m.name);
-            err.span_label(trait_span, "annotation in trait");
-            err.span_label(impl_span, "annotation in impl");
+            err.span_label(trait_span, "declaration in trait here");
+            match (impl_synthetic, trait_synthetic) {
+                // The case where the impl method uses `impl Trait` but the trait method uses
+                // explicit generics
+                (Some(hir::SyntheticTyParamKind::ImplTrait), None) => {
+                    err.span_label(impl_span, "expected generic parameter, found `impl Trait`");
+                    (|| {
+                        // try taking the name from the trait impl
+                        // FIXME: this is obviously suboptimal since the name can already be used
+                        // as another generic argument
+                        let new_name = tcx
+                            .sess
+                            .codemap()
+                            .span_to_snippet(trait_span)
+                            .ok()?;
+                        let trait_m = tcx.hir.as_local_node_id(trait_m.def_id)?;
+                        let trait_m = tcx.hir.trait_item(hir::TraitItemId { node_id: trait_m });
+
+                        let impl_m = tcx.hir.as_local_node_id(impl_m.def_id)?;
+                        let impl_m = tcx.hir.impl_item(hir::ImplItemId { node_id: impl_m });
+
+                        // in case there are no generics, take the spot between the function name
+                        // and the opening paren of the argument list
+                        let new_generics_span = tcx
+                            .sess
+                            .codemap()
+                            .generate_fn_name_span(impl_m.span)?
+                            .shrink_to_hi();
+                        // in case there are generics, just replace them
+                        let generics_span = impl_m
+                            .generics
+                            .span
+                            .substitute_dummy(new_generics_span);
+                        // replace with the generics from the trait
+                        let new_generics = tcx
+                            .sess
+                            .codemap()
+                            .span_to_snippet(trait_m.generics.span)
+                            .ok()?;
+
+                        err.multipart_suggestion(
+                            "try changing the `impl Trait` argument to a generic parameter",
+                            vec![
+                                // replace `impl Trait` with `T`
+                                (impl_span, new_name),
+                                // replace impl method generics with trait method generics
+                                // This isn't quite right, as users might have changed the names
+                                // of the generics, but it works for the common case
+                                (generics_span, new_generics),
+                            ],
+                        );
+                        Some(())
+                    })();
+                },
+                // The case where the trait method uses `impl Trait`, but the impl method uses
+                // explicit generics.
+                (None, Some(hir::SyntheticTyParamKind::ImplTrait)) => {
+                    err.span_label(impl_span, "expected `impl Trait`, found generic parameter");
+                    (|| {
+                        let impl_m = tcx.hir.as_local_node_id(impl_m.def_id)?;
+                        let impl_m = tcx.hir.impl_item(hir::ImplItemId { node_id: impl_m });
+                        let input_tys = match impl_m.node {
+                            hir::ImplItemKind::Method(ref sig, _) => &sig.decl.inputs,
+                            _ => unreachable!(),
+                        };
+                        struct Visitor(Option<Span>, hir::def_id::DefId);
+                        impl<'v> hir::intravisit::Visitor<'v> for Visitor {
+                            fn visit_ty(&mut self, ty: &'v hir::Ty) {
+                                hir::intravisit::walk_ty(self, ty);
+                                match ty.node {
+                                    hir::TyPath(hir::QPath::Resolved(None, ref path)) => {
+                                        if let hir::def::Def::TyParam(def_id) = path.def {
+                                            if def_id == self.1 {
+                                                self.0 = Some(ty.span);
+                                            }
+                                        }
+                                    },
+                                    _ => {}
+                                }
+                            }
+                            fn nested_visit_map<'this>(
+                                &'this mut self
+                            ) -> hir::intravisit::NestedVisitorMap<'this, 'v> {
+                                hir::intravisit::NestedVisitorMap::None
+                            }
+                        }
+                        let mut visitor = Visitor(None, impl_def_id);
+                        for ty in input_tys {
+                            hir::intravisit::Visitor::visit_ty(&mut visitor, ty);
+                        }
+                        let span = visitor.0?;
+
+                        let param = impl_m.generics.params.iter().filter_map(|param| {
+                            match param {
+                                hir::GenericParam::Type(param) => {
+                                    if param.id == impl_node_id {
+                                        Some(param)
+                                    } else {
+                                        None
+                                    }
+                                },
+                                hir::GenericParam::Lifetime(..) => None,
+                            }
+                        }).next()?;
+                        let bounds = param.bounds.first()?.span().to(param.bounds.last()?.span());
+                        let bounds = tcx
+                            .sess
+                            .codemap()
+                            .span_to_snippet(bounds)
+                            .ok()?;
+
+                        err.multipart_suggestion(
+                            "try removing the generic parameter and using `impl Trait` instead",
+                            vec![
+                                // delete generic parameters
+                                (impl_m.generics.span, String::new()),
+                                // replace param usage with `impl Trait`
+                                (span, format!("impl {}", bounds)),
+                            ],
+                        );
+                        Some(())
+                    })();
+                },
+                _ => unreachable!(),
+            }
             err.emit();
             error_found = true;
         }
diff --git a/src/libsyntax/codemap.rs b/src/libsyntax/codemap.rs
index 73924c4270e..0f25c56c457 100644
--- a/src/libsyntax/codemap.rs
+++ b/src/libsyntax/codemap.rs
@@ -874,6 +874,78 @@ impl CodeMap {
     pub fn count_lines(&self) -> usize {
         self.files().iter().fold(0, |a, f| a + f.count_lines())
     }
+
+
+    pub fn generate_fn_name_span(&self, span: Span) -> Option<Span> {
+        let prev_span = self.span_extend_to_prev_str(span, "fn", true);
+        self.span_to_snippet(prev_span).map(|snippet| {
+            let len = snippet.find(|c: char| !c.is_alphanumeric() && c != '_')
+                .expect("no label after fn");
+            prev_span.with_hi(BytePos(prev_span.lo().0 + len as u32))
+        }).ok()
+    }
+
+    /// Take the span of a type parameter in a function signature and try to generate a span for the
+    /// function name (with generics) and a new snippet for this span with the pointed type parameter as
+    /// a new local type parameter.
+    ///
+    /// For instance:
+    /// ```rust,ignore (pseudo-Rust)
+    /// // Given span
+    /// fn my_function(param: T)
+    /// //                    ^ Original span
+    ///
+    /// // Result
+    /// fn my_function(param: T)
+    /// // ^^^^^^^^^^^ Generated span with snippet `my_function<T>`
+    /// ```
+    ///
+    /// Attention: The method used is very fragile since it essentially duplicates the work of the
+    /// parser. If you need to use this function or something similar, please consider updating the
+    /// codemap functions and this function to something more robust.
+    pub fn generate_local_type_param_snippet(&self, span: Span) -> Option<(Span, String)> {
+        // Try to extend the span to the previous "fn" keyword to retrieve the function
+        // signature
+        let sugg_span = self.span_extend_to_prev_str(span, "fn", false);
+        if sugg_span != span {
+            if let Ok(snippet) = self.span_to_snippet(sugg_span) {
+                // Consume the function name
+                let mut offset = snippet.find(|c: char| !c.is_alphanumeric() && c != '_')
+                    .expect("no label after fn");
+
+                // Consume the generics part of the function signature
+                let mut bracket_counter = 0;
+                let mut last_char = None;
+                for c in snippet[offset..].chars() {
+                    match c {
+                        '<' => bracket_counter += 1,
+                        '>' => bracket_counter -= 1,
+                        '(' => if bracket_counter == 0 { break; }
+                        _ => {}
+                    }
+                    offset += c.len_utf8();
+                    last_char = Some(c);
+                }
+
+                // Adjust the suggestion span to encompass the function name with its generics
+                let sugg_span = sugg_span.with_hi(BytePos(sugg_span.lo().0 + offset as u32));
+
+                // Prepare the new suggested snippet to append the type parameter that triggered
+                // the error in the generics of the function signature
+                let mut new_snippet = if last_char == Some('>') {
+                    format!("{}, ", &snippet[..(offset - '>'.len_utf8())])
+                } else {
+                    format!("{}<", &snippet[..offset])
+                };
+                new_snippet.push_str(&self.span_to_snippet(span).unwrap_or("T".to_string()));
+                new_snippet.push('>');
+
+                return Some((sugg_span, new_snippet));
+            }
+        }
+
+        None
+    }
 }
 
 impl CodeMapper for CodeMap {
diff --git a/src/test/ui/impl-trait/impl-generic-mismatch.stderr b/src/test/ui/impl-trait/impl-generic-mismatch.stderr
index ef66e9803aa..4f25a4faefe 100644
--- a/src/test/ui/impl-trait/impl-generic-mismatch.stderr
+++ b/src/test/ui/impl-trait/impl-generic-mismatch.stderr
@@ -2,30 +2,42 @@ error[E0643]: method `foo` has incompatible signature for trait
   --> $DIR/impl-generic-mismatch.rs:18:12
    |
 LL |     fn foo(&self, _: &impl Debug);
-   |                       ---------- annotation in trait
+   |                       ---------- declaration in trait here
 ...
 LL |     fn foo<U: Debug>(&self, _: &U) { }
-   |            ^ annotation in impl
+   |            ^ expected `impl Trait`, found generic parameter
+help: try removing the generic parameter and using `impl Trait` instead
+   |
+LL |     fn foo(&self, _: &impl Debug) { }
+   |
 
 error[E0643]: method `bar` has incompatible signature for trait
   --> $DIR/impl-generic-mismatch.rs:27:23
    |
 LL |     fn bar<U: Debug>(&self, _: &U);
-   |            - annotation in trait
+   |            - declaration in trait here
 ...
 LL |     fn bar(&self, _: &impl Debug) { }
-   |                       ^^^^^^^^^^ annotation in impl
+   |                       ^^^^^^^^^^ expected generic parameter, found `impl Trait`
+help: try changing the `impl Trait` argument to a generic parameter
+   |
+LL |     fn bar<U: Debug><U: Debug>(&self, _: &U);
+LL | }
+LL | 
+LL | impl Bar for () {
+LL |     fn bar(&self, _: &U) { }
+   |
 
 error[E0643]: method `hash` has incompatible signature for trait
   --> $DIR/impl-generic-mismatch.rs:38:33
    |
 LL |     fn hash(&self, hasher: &mut impl Hasher) {}
-   |                                 ^^^^^^^^^^^ annotation in impl
+   |                                 ^^^^^^^^^^^ expected generic parameter, found `impl Trait`
    | 
   ::: /home/oliver/Projects/rust/rust3/src/libcore/hash/mod.rs:185:13
    |
 LL |     fn hash<H: Hasher>(&self, state: &mut H);
-   |             - annotation in trait
+   |             - declaration in trait here
 
 error: aborting due to 3 previous errors