Merge #1144

1144: Refactor method candidate generation a bit r=flodiebold a=flodiebold

This fixes the order in which candidates are chosen a bit (not completely though, as the ignored test demonstrates), and makes autoderef work with trait methods.
As a side effect, this also makes completion of trait methods work :)

Co-authored-by: Florian Diebold <flodiebold@gmail.com>

crates/ra_hir/src/source_binder.rs

@@ -18,7 +18,7 @@ use ra_syntax::{
 
 use crate::{
     HirDatabase, Function, Struct, Enum, Const, Static, Either, DefWithBody, PerNs, Name,
-    AsName, Module, HirFileId, Crate, Trait, Resolver,
+    AsName, Module, HirFileId, Crate, Trait, Resolver, Ty,
     expr::{BodySourceMap, scope::{ScopeId, ExprScopes}},
     ids::LocationCtx,
     expr, AstId
@@ -343,6 +343,16 @@ impl SourceAnalyzer {
             .collect()
     }
 
+    pub fn iterate_method_candidates<T>(
+        &self,
+        db: &impl HirDatabase,
+        ty: Ty,
+        name: Option<&Name>,
+        callback: impl FnMut(&Ty, Function) -> Option<T>,
+    ) -> Option<T> {
+        ty.iterate_method_candidates(db, &self.resolver, name, callback)
+    }
+
     #[cfg(test)]
     pub(crate) fn body_source_map(&self) -> Arc<BodySourceMap> {
         self.body_source_map.clone().unwrap()

crates/ra_hir/src/ty/method_resolution.rs

@@ -129,57 +129,16 @@ impl Ty {
         name: &Name,
         resolver: &Resolver,
     ) -> Option<(Ty, Function)> {
-        // FIXME: trait methods should be used before autoderefs
+        self.iterate_method_candidates(db, resolver, Some(name), |ty, f| Some((ty.clone(), f)))
-        // (and we need to do autoderefs for trait method calls as well)
-        let inherent_method = self.clone().iterate_methods(db, |ty, f| {
-            let sig = f.signature(db);
-            if sig.name() == name && sig.has_self_param() {
-                Some((ty.clone(), f))
-            } else {
-                None
-            }
-        });
-        inherent_method.or_else(|| self.lookup_trait_method(db, name, resolver))
-    }
-
-    fn lookup_trait_method(
-        self,
-        db: &impl HirDatabase,
-        name: &Name,
-        resolver: &Resolver,
-    ) -> Option<(Ty, Function)> {
-        let mut candidates = Vec::new();
-        for t in resolver.traits_in_scope() {
-            let data = t.trait_data(db);
-            for item in data.items() {
-                match item {
-                    &TraitItem::Function(m) => {
-                        let sig = m.signature(db);
-                        if sig.name() == name && sig.has_self_param() {
-                            candidates.push((t, m));
-                        }
-                    }
-                    _ => {}
-                }
-            }
-        }
-        candidates.retain(|(t, _m)| {
-            let trait_ref =
-                TraitRef { trait_: *t, substs: fresh_substs_for_trait(db, *t, self.clone()) };
-            let (trait_ref, _) = super::traits::canonicalize(trait_ref);
-            db.implements(trait_ref).is_some()
-        });
-        // FIXME if there's multiple candidates here, that's an ambiguity error
-        let (_chosen_trait, chosen_method) = candidates.first()?;
-        // FIXME return correct receiver type
-        Some((self.clone(), *chosen_method))
     }
 
     // This would be nicer if it just returned an iterator, but that runs into
     // lifetime problems, because we need to borrow temp `CrateImplBlocks`.
-    pub fn iterate_methods<T>(
+    pub(crate) fn iterate_method_candidates<T>(
         self,
         db: &impl HirDatabase,
+        resolver: &Resolver,
+        name: Option<&Name>,
         mut callback: impl FnMut(&Ty, Function) -> Option<T>,
     ) -> Option<T> {
         // For method calls, rust first does any number of autoderef, and then one
@@ -192,22 +151,83 @@ impl Ty {
         // rustc does an autoderef and then autoref again).
 
         for derefed_ty in self.autoderef(db) {
-            let krate = match def_crate(db, &derefed_ty) {
+            if let Some(result) = derefed_ty.iterate_inherent_methods(db, name, &mut callback) {
-                Some(krate) => krate,
+                return Some(result);
-                None => continue,
+            }
-            };
+            if let Some(result) =
-            let impls = db.impls_in_crate(krate);
+                derefed_ty.iterate_trait_method_candidates(db, resolver, name, &mut callback)
+            {
+                return Some(result);
+            }
+        }
+        None
+    }
 
-            for impl_block in impls.lookup_impl_blocks(&derefed_ty) {
+    fn iterate_trait_method_candidates<T>(
-                for item in impl_block.items(db) {
+        &self,
-                    match item {
+        db: &impl HirDatabase,
-                        ImplItem::Method(f) => {
+        resolver: &Resolver,
-                            if let Some(result) = callback(&derefed_ty, f) {
+        name: Option<&Name>,
+        mut callback: impl FnMut(&Ty, Function) -> Option<T>,
+    ) -> Option<T> {
+        'traits: for t in resolver.traits_in_scope() {
+            let data = t.trait_data(db);
+            // we'll be lazy about checking whether the type implements the
+            // trait, but if we find out it doesn't, we'll skip the rest of the
+            // iteration
+            let mut known_implemented = false;
+            for item in data.items() {
+                match item {
+                    &TraitItem::Function(m) => {
+                        let sig = m.signature(db);
+                        if name.map_or(true, |name| sig.name() == name) && sig.has_self_param() {
+                            if !known_implemented {
+                                let trait_ref = TraitRef {
+                                    trait_: t,
+                                    substs: fresh_substs_for_trait(db, t, self.clone()),
+                                };
+                                let (trait_ref, _) = super::traits::canonicalize(trait_ref);
+                                if db.implements(trait_ref).is_none() {
+                                    continue 'traits;
+                                }
+                            }
+                            known_implemented = true;
+                            if let Some(result) = callback(self, m) {
                                 return Some(result);
                             }
                         }
-                        _ => {}
                     }
+                    _ => {}
+                }
+            }
+        }
+        None
+    }
+
+    fn iterate_inherent_methods<T>(
+        &self,
+        db: &impl HirDatabase,
+        name: Option<&Name>,
+        mut callback: impl FnMut(&Ty, Function) -> Option<T>,
+    ) -> Option<T> {
+        let krate = match def_crate(db, self) {
+            Some(krate) => krate,
+            None => return None,
+        };
+        let impls = db.impls_in_crate(krate);
+
+        for impl_block in impls.lookup_impl_blocks(self) {
+            for item in impl_block.items(db) {
+                match item {
+                    ImplItem::Method(f) => {
+                        let sig = f.signature(db);
+                        if name.map_or(true, |name| sig.name() == name) && sig.has_self_param() {
+                            if let Some(result) = callback(self, f) {
+                                return Some(result);
+                            }
+                        }
+                    }
+                    _ => {}
                 }
             }
         }

crates/ra_hir/src/ty/tests.rs

@@ -2336,6 +2336,66 @@ fn test() -> u64 {
     );
 }
 
+#[ignore]
+#[test]
+fn method_resolution_trait_before_autoref() {
+    let t = type_at(
+        r#"
+//- /main.rs
+trait Trait { fn foo(self) -> u128; }
+struct S;
+impl S { fn foo(&self) -> i8 { 0 } }
+impl Trait for S { fn foo(self) -> u128 { 0 } }
+fn test() { S.foo()<|>; }
+"#,
+    );
+    assert_eq!(t, "u128");
+}
+
+#[test]
+fn method_resolution_trait_before_autoderef() {
+    let t = type_at(
+        r#"
+//- /main.rs
+trait Trait { fn foo(self) -> u128; }
+struct S;
+impl S { fn foo(self) -> i8 { 0 } }
+impl Trait for &S { fn foo(self) -> u128 { 0 } }
+fn test() { (&S).foo()<|>; }
+"#,
+    );
+    assert_eq!(t, "u128");
+}
+
+#[test]
+fn method_resolution_impl_before_trait() {
+    let t = type_at(
+        r#"
+//- /main.rs
+trait Trait { fn foo(self) -> u128; }
+struct S;
+impl S { fn foo(self) -> i8 { 0 } }
+impl Trait for S { fn foo(self) -> u128 { 0 } }
+fn test() { S.foo()<|>; }
+"#,
+    );
+    assert_eq!(t, "i8");
+}
+
+#[test]
+fn method_resolution_trait_autoderef() {
+    let t = type_at(
+        r#"
+//- /main.rs
+trait Trait { fn foo(self) -> u128; }
+struct S;
+impl Trait for S { fn foo(self) -> u128 { 0 } }
+fn test() { (&S).foo()<|>; }
+"#,
+    );
+    assert_eq!(t, "u128");
+}
+
 fn type_at_pos(db: &MockDatabase, pos: FilePosition) -> String {
     let file = db.parse(pos.file_id);
     let expr = algo::find_node_at_offset::<ast::Expr>(file.syntax(), pos.offset).unwrap();
@@ -2344,6 +2404,11 @@ fn type_at_pos(db: &MockDatabase, pos: FilePosition) -> String {
     ty.display(db).to_string()
 }
 
+fn type_at(content: &str) -> String {
+    let (db, file_pos) = MockDatabase::with_position(content);
+    type_at_pos(&db, file_pos)
+}
+
 fn infer(content: &str) -> String {
     let (db, _, file_id) = MockDatabase::with_single_file(content);
     let source_file = db.parse(file_id);

crates/ra_ide_api/src/completion/complete_dot.rs

@@ -37,7 +37,7 @@ fn complete_fields(acc: &mut Completions, ctx: &CompletionContext, receiver: Ty)
 }
 
 fn complete_methods(acc: &mut Completions, ctx: &CompletionContext, receiver: Ty) {
-    receiver.iterate_methods(ctx.db, |_ty, func| {
+    ctx.analyzer.iterate_method_candidates(ctx.db, receiver, None, |_ty, func| {
         let sig = func.signature(ctx.db);
         if sig.has_self_param() {
             acc.add_function(ctx, func);
@@ -195,6 +195,32 @@ mod tests {
         );
     }
 
+    #[test]
+    fn test_trait_method_completion() {
+        assert_debug_snapshot_matches!(
+            do_ref_completion(
+                r"
+            struct A {}
+            trait Trait { fn the_method(&self); }
+            impl Trait for A {}
+            fn foo(a: A) {
+               a.<|>
+            }
+            ",
+            ),
+            @r###"[
+    CompletionItem {
+        label: "the_method",
+        source_range: [151; 151),
+        delete: [151; 151),
+        insert: "the_method()$0",
+        kind: Method,
+        detail: "fn the_method(&self)"
+    }
+]"###
+        );
+    }
+
     #[test]
     fn test_no_non_self_method() {
         assert_debug_snapshot_matches!(