From 248865709401968ce2b1191410b1d6ea6509647f Mon Sep 17 00:00:00 2001 From: Marek Kaput Date: Fri, 31 Jan 2025 14:29:29 +0100 Subject: [PATCH] Properly find definitions for variable declarations fix #164 cc #129 commit-id:919fc6f0 --- src/lang/defs.rs | 126 ++++++++++++++++++++--------- tests/e2e/find_references/enums.rs | 2 +- tests/e2e/find_references/fns.rs | 3 +- tests/e2e/find_references/vars.rs | 27 ++++++- tests/e2e/goto_definition/vars.rs | 61 +++++++++++++- tests/e2e/hover/basic.rs | 14 ++-- tests/e2e/hover/partial.rs | 22 +++-- tests/e2e/hover/paths.rs | 8 +- tests/e2e/hover/starknet.rs | 5 +- tests/e2e/hover/structs.rs | 8 +- tests/e2e/hover/variables.rs | 27 ++++++- 11 files changed, 240 insertions(+), 63 deletions(-) diff --git a/src/lang/defs.rs b/src/lang/defs.rs index 7a504c4f..a4065488 100644 --- a/src/lang/defs.rs +++ b/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 { + 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::(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::(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, diff --git a/tests/e2e/find_references/enums.rs b/tests/e2e/find_references/enums.rs index a791a132..1b635a60 100644 --- a/tests/e2e/find_references/enums.rs +++ b/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#" diff --git a/tests/e2e/find_references/fns.rs b/tests/e2e/find_references/fns.rs index 228b0ca2..6663df42 100644 --- a/tests/e2e/find_references/fns.rs +++ b/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 pow2(x: felt252) -> felt252 { x * x } fn main() { - let pow2 = 2; + let pow2 = 2; let x = pow2 + pow2; } ") diff --git a/tests/e2e/find_references/vars.rs b/tests/e2e/find_references/vars.rs index c14041a2..23a7d100 100644 --- a/tests/e2e/find_references/vars.rs +++ b/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 foobar = 1233; // good + let x = foobar + 1; // good + } + fn bar() { + let foobar = 42; // bad + } + ") } #[test] @@ -133,8 +141,8 @@ fn param_via_binding() { num * num } "#, @r" - fn pow(num: felt252) -> felt252 { - num * num + fn pow(num: felt252) -> felt252 { + num * num } ") } @@ -204,3 +212,16 @@ fn closure_param_via_use() { } ") } + +#[test] +fn closure_param_via_binding() { + test_transform!(find_references, r#" + fn main() { + let f = |abc: felt252| abc + 1; + } + "#, @r" + fn main() { + let f = |abc: felt252| abc + 1; + } + ") +} diff --git a/tests/e2e/goto_definition/vars.rs b/tests/e2e/goto_definition/vars.rs index d98e8394..ff5795a9 100644 --- a/tests/e2e/goto_definition/vars.rs +++ b/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 abc: felt252 = 0; // good + let _ = abc * 2; + } + fn foo() { + let abc: felt252 = 1; // bad + } + ", @r" + fn main() { + let abc: 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(abc: felt252, def: felt252) { // good + let _ = abc * 2; + } + fn foo(abc: felt252) {} // bad + ", @r" + fn main(abc: 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 = |abc| { // good + abc + 3 + }; + } + + fn foo(abc: felt252) {} // bad + ", @r" + fn foo(a: felt252) -> felt252 { + let abc: felt252 = 0; // bad + let c = |abc| { // good + abc + 3 + }; + } + + fn foo(abc: felt252) {} // bad + ") +} diff --git a/tests/e2e/hover/basic.rs b/tests/e2e/hover/basic.rs index cde3d792..7d08fdeb 100644 --- a/tests/e2e/hover/basic.rs +++ b/tests/e2e/hover/basic.rs @@ -11,7 +11,14 @@ fn let_mut() { source_context = """ let mut x = 5; """ - popover = "Type: `core::felt252`" + highlight = """ + let mut x = 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 ``` """ "#) diff --git a/tests/e2e/hover/partial.rs b/tests/e2e/hover/partial.rs index 998a0943..5791c887 100644 --- a/tests/e2e/hover/partial.rs +++ b/tests/e2e/hover/partial.rs @@ -11,7 +11,14 @@ fn uninfered_mut_ident() { source_context = """ let mut xyz = unknown_function(); """ - popover = "Type: ``" + highlight = """ + let mut xyz = unknown_function(); + """ + popover = """ + ```cairo + let mut xyz: + ``` + """ "#) } @@ -25,9 +32,15 @@ fn uninfered_value() { source_context = """ let mut xyz = unknown_function(); """ + highlight = """ + let mut xyz = unknown_function(); + """ popover = """ ```cairo - + hello + ``` + ```cairo + fn main() ``` """ "#) @@ -70,10 +83,7 @@ fn missing_type_param() { """ popover = """ ```cairo - hello - ``` - ```cairo - fn f(abc) -> felt252 + abc: ``` """ "#) diff --git a/tests/e2e/hover/paths.rs b/tests/e2e/hover/paths.rs index 1cf65082..74d15914 100644 --- a/tests/e2e/hover/paths.rs +++ b/tests/e2e/hover/paths.rs @@ -502,9 +502,15 @@ fn struct_constructor_with_path_first() { source_context = """ let _ = super::some_module::internal_module::nested_internal_module::PublicStruct {}; """ + highlight = """ + let _ = super::some_module::internal_module::nested_internal_module::PublicStruct {}; + """ popover = """ ```cairo - hello::some_module::internal_module::nested_internal_module::PublicStruct + hello::happy_cases + ``` + ```cairo + fn constructor_with_path() ``` """ "#) diff --git a/tests/e2e/hover/starknet.rs b/tests/e2e/hover/starknet.rs index d437712a..eb607a1e 100644 --- a/tests/e2e/hover/starknet.rs +++ b/tests/e2e/hover/starknet.rs @@ -70,10 +70,7 @@ fn constructor_ref_self() { """ popover = """ ```cairo - hello::Balance - ``` - ```cairo - fn constructor(ref self: ContractState, value_: u128) + ref self: hello::Balance::ContractState ``` """ "#) diff --git a/tests/e2e/hover/structs.rs b/tests/e2e/hover/structs.rs index e4da2217..19961272 100644 --- a/tests/e2e/hover/structs.rs +++ b/tests/e2e/hover/structs.rs @@ -248,9 +248,15 @@ fn non_existent_struct_member_init() { source_context = """ let _ = NonExistentStruct { member: 0 }; """ + highlight = """ + let _ = NonExistentStruct { member: 0 }; + """ popover = """ ```cairo - + hello::unhappy_cases + ``` + ```cairo + fn non_existent_struct ``` """ "#) diff --git a/tests/e2e/hover/variables.rs b/tests/e2e/hover/variables.rs index 26c15ed1..89b4511b 100644 --- a/tests/e2e/hover/variables.rs +++ b/tests/e2e/hover/variables.rs @@ -11,7 +11,14 @@ fn ident_typed() { source_context = """ let abc: felt252 = 0; """ - popover = "Type: `core::felt252`" + highlight = """ + let abc: felt252 = 0; + """ + popover = """ + ```cairo + let abc: core::felt252 + ``` + """ "#) } @@ -25,7 +32,14 @@ fn ident() { source_context = """ let xyz = 3; """ - popover = "Type: `core::felt252`" + highlight = """ + let xyz = 3; + """ + popover = """ + ```cairo + let xyz: core::felt252 + ``` + """ "#) } @@ -40,7 +54,14 @@ fn ident_mut() { source_context = """ let mut def = abc * 2; """ - popover = "Type: `core::felt252`" + highlight = """ + let mut def = abc * 2; + """ + popover = """ + ```cairo + let mut def: core::felt252 + ``` + """ "#) }