Properly find definitions for variable declarations

src/lang/defs.rs

@@ -2,8 +2,8 @@ use std::iter;
 
 use cairo_lang_defs::db::DefsGroup;
 use cairo_lang_defs::ids::{
-    LanguageElementId, LookupItemId, MemberId, ModuleId, ModuleItemId, NamedLanguageElementId,
-    SubmoduleLongId, TopLevelLanguageElementId, TraitItemId, VarId,
+    LanguageElementId, LocalVarLongId, LookupItemId, MemberId, ModuleId, ModuleItemId,
+    NamedLanguageElementId, SubmoduleLongId, TopLevelLanguageElementId, TraitItemId, VarId,
 };
 use cairo_lang_diagnostics::ToOption;
 use cairo_lang_doc::db::DocGroup;
@@ -417,6 +417,10 @@ fn find_definition(
     identifier: &ast::TerminalIdentifier,
     lookup_items: &[LookupItemId],
 ) -> Option<(ResolvedItem, SyntaxStablePtrId)> {
+    // The lookup_resolved_*_item_by_ptr queries tend to sometimes return an encompassing item
+    // instead of actually resolving the identifier.
+    // The following series of heuristics resolve the identifier in alternative ways for such cases.
+
     if let Some(parent) = identifier.as_syntax_node().parent() {
         if parent.kind(db) == SyntaxKind::ItemModule {
             let Some(containing_module_file_id) = db.find_module_file_containing_node(&parent)
@@ -444,7 +448,12 @@ fn find_definition(
         return Some((ResolvedItem::Member(member_id), member_id.untyped_stable_ptr(db)));
     }
 
-    for lookup_item_id in lookup_items.iter().copied() {
+    if let Some(var_id) = try_extract_variable_declaration(db, identifier, lookup_items) {
+        let item = ResolvedGenericItem::Variable(var_id);
+        return Some((ResolvedItem::Generic(item.clone()), resolved_generic_item_def(db, item)?));
+    }
+
+    for &lookup_item_id in lookup_items {
         if let Some(item) =
             db.lookup_resolved_generic_item_by_ptr(lookup_item_id, identifier.stable_ptr())
         {
@@ -462,41 +471,36 @@ fn find_definition(
         }
     }
 
-    // Skip variable definition, otherwise we would get parent ModuleItem for variable.
-    if identifier.as_syntax_node().parent_of_kind(db, SyntaxKind::StatementLet).is_none() {
-        let item = match lookup_items.first().copied()? {
-            LookupItemId::ModuleItem(item) => {
-                ResolvedGenericItem::from_module_item(db, item).to_option()?
-            }
-            LookupItemId::TraitItem(trait_item) => {
-                if let TraitItemId::Function(trait_function_id) = trait_item {
-                    let parent_trait = trait_item.trait_id(db);
-                    let generic_parameters = db.trait_generic_params(parent_trait).to_option()?;
-                    let concrete_trait = ConcreteTraitLongId {
-                        trait_id: parent_trait,
-                        generic_args: generic_params_to_args(&generic_parameters, db),
-                    };
-                    let concrete_trait = db.intern_concrete_trait(concrete_trait);
-
-                    ResolvedGenericItem::GenericFunction(GenericFunctionId::Impl(
-                        ImplGenericFunctionId {
-                            impl_id: ImplLongId::SelfImpl(concrete_trait).intern(db),
-                            function: trait_function_id,
-                        },
-                    ))
-                } else {
-                    ResolvedGenericItem::Trait(trait_item.trait_id(db))
-                }
-            }
-            LookupItemId::ImplItem(impl_item) => {
-                ResolvedGenericItem::Impl(impl_item.impl_def_id(db))
+    // FIXME(mkaput): This logic always kicks in if we're finding definition of undefined symbol
+    //   which is very wrong in such cases.
+    let item = match lookup_items.first().copied()? {
+        LookupItemId::ModuleItem(item) => {
+            ResolvedGenericItem::from_module_item(db, item).to_option()?
+        }
+        LookupItemId::TraitItem(trait_item) => {
+            if let TraitItemId::Function(trait_function_id) = trait_item {
+                let parent_trait = trait_item.trait_id(db);
+                let generic_parameters = db.trait_generic_params(parent_trait).to_option()?;
+                let concrete_trait = ConcreteTraitLongId {
+                    trait_id: parent_trait,
+                    generic_args: generic_params_to_args(&generic_parameters, db),
+                };
+                let concrete_trait = db.intern_concrete_trait(concrete_trait);
+
+                ResolvedGenericItem::GenericFunction(GenericFunctionId::Impl(
+                    ImplGenericFunctionId {
+                        impl_id: ImplLongId::SelfImpl(concrete_trait).intern(db),
+                        function: trait_function_id,
+                    },
+                ))
+            } else {
+                ResolvedGenericItem::Trait(trait_item.trait_id(db))
             }
-        };
+        }
+        LookupItemId::ImplItem(impl_item) => ResolvedGenericItem::Impl(impl_item.impl_def_id(db)),
+    };
 
-        Some((ResolvedItem::Generic(item.clone()), resolved_generic_item_def(db, item)?))
-    } else {
-        None
-    }
+    Some((ResolvedItem::Generic(item.clone()), resolved_generic_item_def(db, item)?))
 }
 
 /// Extracts [`MemberId`] if the [`TerminalIdentifier`] is used as a struct member
@@ -567,6 +571,56 @@ fn try_extract_member(
     }
 }
 
+/// Lookups if the identifier is a declaration of a variable/param in one of the lookup items.
+///
+/// Declaration identifiers aren't kept in `ResolvedData`, which is searched for by
+/// `lookup_resolved_generic_item_by_ptr` and `lookup_resolved_concrete_item_by_ptr`.
+/// Therefore, we have to look for these ourselves.
+fn try_extract_variable_declaration(
+    db: &AnalysisDatabase,
+    identifier: &ast::TerminalIdentifier,
+    lookup_items: &[LookupItemId],
+) -> Option<VarId> {
+    let function_id = lookup_items.first()?.function_with_body()?;
+
+    // Look at function parameters.
+    if let Some(param) = identifier.as_syntax_node().parent_of_kind(db, SyntaxKind::Param) {
+        // Closures have different semantic model structures than regular functions.
+        let params = if let Some(expr_closure_ast) = param.parent_of_type::<ast::ExprClosure>(db) {
+            let expr_id =
+                db.lookup_expr_by_ptr(function_id, expr_closure_ast.stable_ptr().into()).ok()?;
+            let Expr::ExprClosure(expr_closure_semantic) = db.expr_semantic(function_id, expr_id)
+            else {
+                unreachable!("expected semantic for ast::ExprClosure to be Expr::ExprClosure");
+            };
+            expr_closure_semantic.params
+        } else {
+            let signature = db.function_with_body_signature(function_id).ok()?;
+            signature.params
+        };
+
+        if let Some(param) =
+            params.into_iter().find(|param| param.stable_ptr == identifier.stable_ptr())
+        {
+            return Some(VarId::Param(param.id));
+        }
+    }
+
+    // Look at patterns in the function body.
+    if let Some(pattern) = identifier.as_syntax_node().parent_of_type::<ast::Pattern>(db) {
+        // Bail out if the pattern happens to not exist in the semantic model.
+        // We don't need semantics for returning, though, due to the way how local variables are
+        // identified there, so we're happily ignoring the result value.
+        db.lookup_pattern_by_ptr(function_id, pattern.stable_ptr()).ok()?;
+
+        return Some(VarId::Local(
+            LocalVarLongId(function_id.module_file_id(db), identifier.stable_ptr()).intern(db),
+        ));
+    }
+
+    None
+}
+
 fn resolved_concrete_item_def(
     db: &AnalysisDatabase,
     item: ResolvedConcreteItem,

tests/e2e/find_references/enums.rs

@@ -46,7 +46,7 @@ fn enum_name() {
     ")
 }
 
-// FIXME(#164): Pattern should also be selected.
+// FIXME(#129): Pattern should also be selected.
 #[test]
 fn enum_variants() {
     test_transform!(find_references, r#"

tests/e2e/find_references/fns.rs

@@ -33,7 +33,6 @@ fn fn_via_call() {
     ")
 }
 
-// FIXME(#164): Pattern should not match here.
 #[test]
 fn unused_function() {
     test_transform!(find_references, r#"
@@ -45,7 +44,7 @@ fn unused_function() {
     "#, @r"
     fn <sel=declaration>pow2</sel>(x: felt252) -> felt252 { x * x }
     fn main() {
-        let <sel>pow2</sel> = 2;
+        let pow2 = 2;
         let x = pow2 + pow2;
     }
     ")

tests/e2e/find_references/vars.rs

@@ -11,7 +11,15 @@ fn var_via_binding() {
     fn bar() {
         let foobar = 42; // bad
     }
-    "#, @"none response")
+    "#, @r"
+    fn main() {
+        let <sel=declaration>foobar</sel> = 1233; // good
+        let x = <sel>foobar</sel> + 1; // good
+    }
+    fn bar() {
+        let foobar = 42; // bad
+    }
+    ")
 }
 
 #[test]
@@ -133,8 +141,8 @@ fn param_via_binding() {
         num * num
     }
     "#, @r"
-    fn <sel=declaration>pow</sel>(num: felt252) -> felt252 {
-        num * num
+    fn pow(<sel=declaration>num</sel>: felt252) -> felt252 {
+        <sel>num</sel> * <sel>num</sel>
     }
     ")
 }
@@ -204,3 +212,16 @@ fn closure_param_via_use() {
     }
     ")
 }
+
+#[test]
+fn closure_param_via_binding() {
+    test_transform!(find_references, r#"
+    fn main() {
+        let f = |a<caret>bc: felt252| abc + 1;
+    }
+    "#, @r"
+    fn main() {
+        let f = |<sel=declaration>abc</sel>: felt252| <sel>abc</sel> + 1;
+    }
+    ")
+}

tests/e2e/goto_definition/vars.rs

@@ -2,7 +2,7 @@ use crate::goto_definition::goto_definition;
 use crate::support::insta::test_transform;
 
 #[test]
-fn var_in_expr() {
+fn var_via_expr() {
     test_transform!(goto_definition, r"
     fn main() {
         let abc: felt252 = 0; // good
@@ -22,6 +22,27 @@ fn var_in_expr() {
     ")
 }
 
+#[test]
+fn var_via_binding() {
+    test_transform!(goto_definition, r"
+    fn main() {
+        let a<caret>bc: felt252 = 0; // good
+        let _ = abc * 2;
+    }
+    fn foo() {
+        let abc: felt252 = 1;  // bad
+    }
+    ", @r"
+    fn main() {
+        let <sel>abc</sel>: felt252 = 0; // good
+        let _ = abc * 2;
+    }
+    fn foo() {
+        let abc: felt252 = 1;  // bad
+    }
+    ")
+}
+
 #[test]
 fn fn_param_via_expr() {
     test_transform!(goto_definition, r"
@@ -37,6 +58,21 @@ fn fn_param_via_expr() {
     ")
 }
 
+#[test]
+fn fn_param_via_binding() {
+    test_transform!(goto_definition, r"
+    fn main(a<caret>bc: felt252, def: felt252) { // good
+        let _ = abc * 2;
+    }
+    fn foo(abc: felt252) {} // bad
+    ", @r"
+    fn main(<sel>abc</sel>: felt252, def: felt252) { // good
+        let _ = abc * 2;
+    }
+    fn foo(abc: felt252) {} // bad
+    ")
+}
+
 #[test]
 fn closure_param_via_expr() {
     test_transform!(goto_definition, r"
@@ -59,3 +95,26 @@ fn closure_param_via_expr() {
     fn foo(abc: felt252) {} // bad
     ")
 }
+
+#[test]
+fn closure_param_via_binding() {
+    test_transform!(goto_definition, r"
+    fn foo(a: felt252) -> felt252 {
+        let abc: felt252 = 0; // bad
+        let c = |a<caret>bc| { // good
+            abc + 3
+        };
+    }
+
+    fn foo(abc: felt252) {} // bad
+    ", @r"
+    fn foo(a: felt252) -> felt252 {
+        let abc: felt252 = 0; // bad
+        let c = |<sel>abc</sel>| { // good
+            abc + 3
+        };
+    }
+
+    fn foo(abc: felt252) {} // bad
+    ")
+}

tests/e2e/hover/basic.rs

@@ -11,7 +11,14 @@ fn let_mut() {
     source_context = """
         let mut x<caret> = 5;
     """
-    popover = "Type: `core::felt252`"
+    highlight = """
+        let mut <sel>x</sel> = 5;
+    """
+    popover = """
+    ```cairo
+    let mut x: core::felt252
+    ```
+    """
     "#)
 }
 
@@ -278,10 +285,7 @@ fn param_name() {
     """
     popover = """
     ```cairo
-    hello
-    ```
-    ```cairo
-    fn value_in_cents(coin: Coin) -> felt252
+    coin: hello::Coin
     ```
     """
     "#)

tests/e2e/hover/partial.rs

@@ -11,7 +11,14 @@ fn uninfered_mut_ident() {
     source_context = """
         let mut xy<caret>z = unknown_function();
     """
-    popover = "Type: `<missing>`"
+    highlight = """
+        let mut <sel>xyz</sel> = unknown_function();
+    """
+    popover = """
+    ```cairo
+    let mut xyz: <missing>
+    ```
+    """
     "#)
 }
 
@@ -25,9 +32,15 @@ fn uninfered_value() {
     source_context = """
         let mut xyz = unkn<caret>own_function();
     """
+    highlight = """
+        let mut xyz = <sel>unknown_function</sel>();
+    """
     popover = """
     ```cairo
-    <missing>
+    hello
+    ```
+    ```cairo
+    fn main()
     ```
     """
     "#)
@@ -70,10 +83,7 @@ fn missing_type_param() {
     """
     popover = """
     ```cairo
-    hello
-    ```
-    ```cairo
-    fn f(abc) -> felt252
+    abc: <missing>
     ```
     """
     "#)