Properly find definitions for variable declarations

cc #164 - great progress towards closing this
cc #129

commit-id:919fc6f0

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;
@@ -444,7 +444,7 @@ fn find_definition(
         return Some((ResolvedItem::Member(member_id), member_id.untyped_stable_ptr(db)));
     }
 
-    for lookup_item_id in lookup_items.iter().copied() {
+    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 +462,41 @@ 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))
+    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)?));
+    }
+
+    // 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 +567,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/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/test_data/hover/basic.txt

@@ -75,9 +75,11 @@ pub mod front_of_house {
 // = source context
     let mut x<caret> = 5;
 // = highlight
-No highlight information.
+    let mut <sel>x</sel> = 5;
 // = popover
-Type: `core::integer::u32`
+```cairo
+let mut x: core::integer::u32
+```
 
 //! > hover #1
 // = source context
@@ -208,9 +210,11 @@ Add to waitlist function.
 // = source context
     let mut re<caret>ct = Rectangle { width: 30, height: 50 };
 // = highlight
-No highlight information.
+    let mut <sel>rect</sel> = Rectangle { width: 30, height: 50 };
 // = popover
-Type: `hello::Rectangle`
+```cairo
+let mut rect: hello::Rectangle
+```
 
 //! > hover #10
 // = source context
@@ -254,9 +258,11 @@ Width of the rectangle.
 // = source context
     let ar<caret>ea = rect.area();
 // = highlight
-No highlight information.
+    let <sel>area</sel> = rect.area();
 // = popover
-Type: `core::integer::u64`
+```cairo
+let area: core::integer::u64
+```
 
 //! > hover #13
 // = source context
@@ -296,14 +302,8 @@ fn add_two(x: u32) -> u32
     fn area(<sel>self</sel>: @Rectangle) -> u64;
 // = popover
 ```cairo
-hello::RectangleTrait
-```
-```cairo
-trait RectangleTrait
-fn area(self: @Rectangle) -> u64
+self: @hello::Rectangle
 ```
----
-Calculate the area of the rectangle.
 
 //! > hover #16
 // = source context
@@ -331,13 +331,8 @@ Rectangle struct.
     fn area(<sel>self</sel>: @Rectangle) -> u64 {
 // = popover
 ```cairo
-hello
-```
-```cairo
-impl RectangleImpl of RectangleTrait
+self: @hello::Rectangle
 ```
----
-Implementing the `RectangleTrait` for the `Rectangle` struct.
 
 //! > hover #18
 // = source context
@@ -523,10 +518,7 @@ fn value_in_cents(co<caret>in: Coin) -> felt252 {
 fn value_in_cents(<sel>coin</sel>: Coin) -> felt252 {
 // = popover
 ```cairo
-hello
-```
-```cairo
-fn value_in_cents(coin: Coin) -> felt252
+coin: hello::Coin
 ```
 
 //! > hover #30