Merge #8008

8008: Completion context expected type r=matklad a=JoshMcguigan

Currently there are two ways completions use to determine the expected type. There is the `expected_type` field on the `CompletionContext`, as well as the `expected_name_and_type` method on the `RenderContext`. These two things returned slightly different results, and their results were only valid if you had pre-checked some (undocumented) invariants. A simple combination of the two approaches doesn't work because they are both too willing to go far up the syntax tree to find something that fits what they are looking for.

This PR makes the following changes:

1. Updates the algorithm that sets `expected_type` on `CompletionContext`
2. Adds `expected_name` field to `CompletionContext`
3. Re-writes the `expected_name_and_type` method to simply return the underlying fields from `CompletionContext` (I'd like to save actually removing this method for a follow up PR just to keep the scope of the changes down)
4. Adds unit tests for the `expected_type`/`expected_name` fields

All the existing unit tests still pass (unmodified), but this new algorithm certainly has some gaps (although I believe all the `FIXME` introduced in this PR are also flaws in the current code). I wanted to stop here and get some feedback though - is this approach fundamentally sound? 

Co-authored-by: Josh Mcguigan <[email protected]>

2 files changed, 286 insertions, 26 deletions

diff --git a/crates/ide_completion/src/context.rs b/crates/ide_completion/src/context.rs
index 17d9a3adf..89e9bda78 100644
--- a/crates/ide_completion/src/context.rs
+++ b/crates/ide_completion/src/context.rs
@@ -35,6 +35,7 @@ pub(crate) struct CompletionContext<'a> {
     /// The token before the cursor, in the macro-expanded file.
     pub(super) token: SyntaxToken,
     pub(super) krate: Option<hir::Crate>,
+    pub(super) expected_name: Option<String>,
     pub(super) expected_type: Option<Type>,
     pub(super) name_ref_syntax: Option<ast::NameRef>,
     pub(super) function_syntax: Option<ast::Fn>,
@@ -135,6 +136,7 @@ impl<'a> CompletionContext<'a> {
             original_token,
             token,
             krate,
+            expected_name: None,
             expected_type: None,
             name_ref_syntax: None,
             function_syntax: None,
@@ -290,23 +292,96 @@ impl<'a> CompletionContext<'a> {
         file_with_fake_ident: SyntaxNode,
         offset: TextSize,
     ) {
-        // FIXME: this is wrong in at least two cases:
-        //  * when there's no token `foo($0)`
-        //  * when there is a token, but it happens to have type of it's own
-        self.expected_type = self
-            .token
-            .ancestors()
-            .find_map(|node| {
-                let ty = match_ast! {
+        let expected = {
+            let mut node = self.token.parent();
+            loop {
+                let ret = match_ast! {
                     match node {
-                        ast::Pat(it) => self.sema.type_of_pat(&it),
-                        ast::Expr(it) => self.sema.type_of_expr(&it),
-                        _ => return None,
+                        ast::LetStmt(it) => {
+                            cov_mark::hit!(expected_type_let_with_leading_char);
+                            cov_mark::hit!(expected_type_let_without_leading_char);
+                            let ty = it.pat()
+                                .and_then(|pat| self.sema.type_of_pat(&pat));
+                            let name = if let Some(ast::Pat::IdentPat(ident)) = it.pat() {
+                                Some(ident.syntax().text().to_string())
+                            } else {
+                                None
+                            };
+
+                            (ty, name)
+                        },
+                        ast::ArgList(it) => {
+                            cov_mark::hit!(expected_type_fn_param_with_leading_char);
+                            cov_mark::hit!(expected_type_fn_param_without_leading_char);
+                            ActiveParameter::at_token(
+                                &self.sema,
+                                self.token.clone(),
+                            ).map(|ap| (Some(ap.ty), Some(ap.name)))
+                            .unwrap_or((None, None))
+                        },
+                        ast::RecordExprFieldList(it) => {
+                            cov_mark::hit!(expected_type_struct_field_without_leading_char);
+                            self.token.prev_sibling_or_token()
+                                .and_then(|se| se.into_node())
+                                .and_then(|node| ast::RecordExprField::cast(node))
+                                .and_then(|rf| self.sema.resolve_record_field(&rf))
+                                .map(|f|(
+                                    Some(f.0.signature_ty(self.db)),
+                                    Some(f.0.name(self.db).to_string()),
+                                ))
+                                .unwrap_or((None, None))
+                        },
+                        ast::RecordExprField(it) => {
+                            cov_mark::hit!(expected_type_struct_field_with_leading_char);
+                            self.sema
+                                .resolve_record_field(&it)
+                                .map(|f|(
+                                    Some(f.0.signature_ty(self.db)),
+                                    Some(f.0.name(self.db).to_string()),
+                                ))
+                                .unwrap_or((None, None))
+                        },
+                        ast::MatchExpr(it) => {
+                            cov_mark::hit!(expected_type_match_arm_without_leading_char);
+                            let ty = it.expr()
+                                .and_then(|e| self.sema.type_of_expr(&e));
+
+                            (ty, None)
+                        },
+                        ast::IdentPat(it) => {
+                            cov_mark::hit!(expected_type_if_let_with_leading_char);
+                            cov_mark::hit!(expected_type_if_let_without_leading_char);
+                            cov_mark::hit!(expected_type_match_arm_with_leading_char);
+                            let ty = self.sema.type_of_pat(&ast::Pat::from(it));
+
+                            (ty, None)
+                        },
+                        ast::Fn(it) => {
+                            cov_mark::hit!(expected_type_fn_ret_with_leading_char);
+                            cov_mark::hit!(expected_type_fn_ret_without_leading_char);
+                            let ty = self.token.ancestors()
+                                .find_map(|ancestor| ast::Expr::cast(ancestor))
+                                .and_then(|expr| self.sema.type_of_expr(&expr));
+
+                            (ty, None)
+                        },
+                        _ => {
+                            match node.parent() {
+                                Some(n) => {
+                                    node = n;
+                                    continue;
+                                },
+                                None => (None, None),
+                            }
+                        },
                     }
                 };
-                Some(ty)
-            })
-            .flatten();
+
+                break ret;
+            }
+        };
+        self.expected_type = expected.0;
+        self.expected_name = expected.1;
         self.attribute_under_caret = find_node_at_offset(&file_with_fake_ident, offset);
 
         // First, let's try to complete a reference to some declaration.
@@ -535,3 +610,197 @@ fn path_or_use_tree_qualifier(path: &ast::Path) -> Option<ast::Path> {
     let use_tree = use_tree_list.syntax().parent().and_then(ast::UseTree::cast)?;
     use_tree.path()
 }
+
+#[cfg(test)]
+mod tests {
+    use expect_test::{expect, Expect};
+    use hir::HirDisplay;
+
+    use crate::test_utils::{position, TEST_CONFIG};
+
+    use super::CompletionContext;
+
+    fn check_expected_type_and_name(ra_fixture: &str, expect: Expect) {
+        let (db, pos) = position(ra_fixture);
+        let completion_context = CompletionContext::new(&db, pos, &TEST_CONFIG).unwrap();
+
+        let ty = completion_context
+            .expected_type
+            .map(|t| t.display_test(&db).to_string())
+            .unwrap_or("?".to_owned());
+
+        let name = completion_context.expected_name.unwrap_or("?".to_owned());
+
+        expect.assert_eq(&format!("ty: {}, name: {}", ty, name));
+    }
+
+    #[test]
+    fn expected_type_let_without_leading_char() {
+        cov_mark::check!(expected_type_let_without_leading_char);
+        check_expected_type_and_name(
+            r#"
+fn foo() {
+    let x: u32 = $0;
+}
+"#,
+            expect![[r#"ty: u32, name: x"#]],
+        );
+    }
+
+    #[test]
+    fn expected_type_let_with_leading_char() {
+        cov_mark::check!(expected_type_let_with_leading_char);
+        check_expected_type_and_name(
+            r#"
+fn foo() {
+    let x: u32 = c$0;
+}
+"#,
+            expect![[r#"ty: u32, name: x"#]],
+        );
+    }
+
+    #[test]
+    fn expected_type_fn_param_without_leading_char() {
+        cov_mark::check!(expected_type_fn_param_without_leading_char);
+        check_expected_type_and_name(
+            r#"
+fn foo() {
+    bar($0);
+}
+
+fn bar(x: u32) {}
+"#,
+            expect![[r#"ty: u32, name: x"#]],
+        );
+    }
+
+    #[test]
+    fn expected_type_fn_param_with_leading_char() {
+        cov_mark::check!(expected_type_fn_param_with_leading_char);
+        check_expected_type_and_name(
+            r#"
+fn foo() {
+    bar(c$0);
+}
+
+fn bar(x: u32) {}
+"#,
+            expect![[r#"ty: u32, name: x"#]],
+        );
+    }
+
+    #[test]
+    fn expected_type_struct_field_without_leading_char() {
+        cov_mark::check!(expected_type_struct_field_without_leading_char);
+        check_expected_type_and_name(
+            r#"
+struct Foo { a: u32 }
+fn foo() {
+    Foo { a: $0 };
+}
+"#,
+            expect![[r#"ty: u32, name: a"#]],
+        )
+    }
+
+    #[test]
+    fn expected_type_struct_field_with_leading_char() {
+        cov_mark::check!(expected_type_struct_field_with_leading_char);
+        check_expected_type_and_name(
+            r#"
+struct Foo { a: u32 }
+fn foo() {
+    Foo { a: c$0 };
+}
+"#,
+            expect![[r#"ty: u32, name: a"#]],
+        );
+    }
+
+    #[test]
+    fn expected_type_match_arm_without_leading_char() {
+        cov_mark::check!(expected_type_match_arm_without_leading_char);
+        check_expected_type_and_name(
+            r#"
+enum E { X }
+fn foo() {
+   match E::X { $0 }
+}
+"#,
+            expect![[r#"ty: E, name: ?"#]],
+        );
+    }
+
+    #[test]
+    fn expected_type_match_arm_with_leading_char() {
+        cov_mark::check!(expected_type_match_arm_with_leading_char);
+        check_expected_type_and_name(
+            r#"
+enum E { X }
+fn foo() {
+   match E::X { c$0 }
+}
+"#,
+            expect![[r#"ty: E, name: ?"#]],
+        );
+    }
+
+    #[test]
+    fn expected_type_if_let_without_leading_char() {
+        cov_mark::check!(expected_type_if_let_without_leading_char);
+        check_expected_type_and_name(
+            r#"
+enum Foo { Bar, Baz, Quux }
+
+fn foo() {
+    let f = Foo::Quux;
+    if let $0 = f { }
+}
+"#,
+            expect![[r#"ty: (), name: ?"#]],
+        ) // FIXME should be `ty: u32, name: ?`
+    }
+
+    #[test]
+    fn expected_type_if_let_with_leading_char() {
+        cov_mark::check!(expected_type_if_let_with_leading_char);
+        check_expected_type_and_name(
+            r#"
+enum Foo { Bar, Baz, Quux }
+
+fn foo() {
+    let f = Foo::Quux;
+    if let c$0 = f { }
+}
+"#,
+            expect![[r#"ty: Foo, name: ?"#]],
+        )
+    }
+
+    #[test]
+    fn expected_type_fn_ret_without_leading_char() {
+        cov_mark::check!(expected_type_fn_ret_without_leading_char);
+        check_expected_type_and_name(
+            r#"
+fn foo() -> u32 {
+    $0
+}
+"#,
+            expect![[r#"ty: (), name: ?"#]],
+        ) // FIXME this should be `ty: u32, name: ?`
+    }
+
+    #[test]
+    fn expected_type_fn_ret_with_leading_char() {
+        cov_mark::check!(expected_type_fn_ret_with_leading_char);
+        check_expected_type_and_name(
+            r#"
+fn foo() -> u32 {
+    c$0
+}
+"#,
+            expect![[r#"ty: u32, name: ?"#]],
+        )
+    }
+}
diff --git a/crates/ide_completion/src/render.rs b/crates/ide_completion/src/render.rs
index 905f0b197..3e1bff4d6 100644
--- a/crates/ide_completion/src/render.rs
+++ b/crates/ide_completion/src/render.rs
@@ -119,17 +119,10 @@ impl<'a> RenderContext<'a> {
         node.docs(self.db())
     }
 
+    // FIXME delete this method in favor of directly using the fields
+    // on CompletionContext
     fn expected_name_and_type(&self) -> Option<(String, Type)> {
-        if let Some(record_field) = &self.completion.record_field_syntax {
-            cov_mark::hit!(record_field_type_match);
-            let (struct_field, _local) = self.completion.sema.resolve_record_field(record_field)?;
-            Some((struct_field.name(self.db()).to_string(), struct_field.signature_ty(self.db())))
-        } else if let Some(active_parameter) = &self.completion.active_parameter {
-            cov_mark::hit!(active_param_type_match);
-            Some((active_parameter.name.clone(), active_parameter.ty.clone()))
-        } else {
-            None
-        }
+        Some((self.completion.expected_name.clone()?, self.completion.expected_type.clone()?))
     }
 }
 
@@ -852,7 +845,6 @@ fn foo(xs: Vec<i128>)
 
     #[test]
     fn active_param_relevance() {
-        cov_mark::check!(active_param_type_match);
         check_relevance(
             r#"
 struct S { foo: i64, bar: u32, baz: u32 }
@@ -869,7 +861,6 @@ fn foo(s: S) { test(s.$0) }
 
     #[test]
     fn record_field_relevances() {
-        cov_mark::check!(record_field_type_match);
         check_relevance(
             r#"
 struct A { foo: i64, bar: u32, baz: u32 }