From 693d5390a887e033a67ecc7ef17ead9ece5c2c16 Mon Sep 17 00:00:00 2001 From: Rodolpho Lima Date: Tue, 2 Dec 2025 19:34:49 +0100 Subject: [PATCH 1/2] [FIX] follow_ref dropping evaluations incorrectly The issue: In follow_ref's main loop, popping from the front of the results Deque when meaning to replace an evaluation would not necessarily remove the correct one. In order to fix this and reduce complexity of the main loop with a moving index over a Deque, we switch to a work queue approach: - evaluations that need to be processed are pushed to and popped from the work queue, - final evaluations are pushed to the results vector. Other changes: - `acc` was renamed to `visited` for clarity. - some indentation could be reduced with `let else` constructs, improving readability. --- server/src/core/symbols/symbol.rs | 202 +++++++++--------- .../data/python/expressions/properties.py | 20 ++ server/tests/test_follow_ref.rs | 53 +++++ 3 files changed, 170 insertions(+), 105 deletions(-) create mode 100644 server/tests/data/python/expressions/properties.py create mode 100644 server/tests/test_follow_ref.rs diff --git a/server/src/core/symbols/symbol.rs b/server/src/core/symbols/symbol.rs index 7f9dffd3..749e4bea 100644 --- a/server/src/core/symbols/symbol.rs +++ b/server/src/core/symbols/symbol.rs @@ -2198,121 +2198,113 @@ impl Symbol { If a symbol in the chain is a descriptor, return the __get__ return evaluation. */ pub fn follow_ref(evaluation: &EvaluationSymbolPtr, session: &mut SessionInfo, context: &mut Option, stop_on_type: bool, stop_on_value: bool, max_scope: Option>>) -> Vec { - match evaluation { - EvaluationSymbolPtr::WEAK(w) => { - let Some(symbol) = w.weak.upgrade() else { - return vec![evaluation.clone()]; - }; - if stop_on_value { - if let Some(evals) = symbol.borrow().evaluations() { - for eval in evals.iter() { - if eval.value.is_some() { - return vec![evaluation.clone()]; - } - } + let EvaluationSymbolPtr::WEAK(w) = evaluation else { + // Non-weak evaluations are final + return vec![evaluation.clone()]; + }; + let Some(symbol) = w.weak.upgrade() else { + return vec![evaluation.clone()]; + }; + if stop_on_value { + if let Some(evals) = symbol.borrow().evaluations() { + for eval in evals.iter() { + if eval.value.is_some() { + return vec![evaluation.clone()]; } } - //return a list of all possible evaluation: a weak ptr to the final symbol, and a bool indicating if this is an instance or not - let mut results = Symbol::next_refs(session, symbol.clone(), context, &w.context, stop_on_type, &mut vec![]); - if results.is_empty() { - return vec![evaluation.clone()]; + } + } + //return a list of all possible evaluation: a weak ptr to the final symbol, and a bool indicating if this is an instance or not + let mut work_queue = Symbol::next_refs(session, symbol.clone(), context, &w.context, stop_on_type, &mut vec![]); + if work_queue.is_empty() { + return vec![evaluation.clone()]; + } + if w.instance.is_some_and(|v| v) { + //if the previous evaluation was set to True, we want to keep it + work_queue = work_queue.into_iter().map(|mut r| { + if let EvaluationSymbolPtr::WEAK(ref mut weak) = r { + weak.instance = Some(true); } - if w.instance.is_some_and(|v| v) { - //if the previous evaluation was set to True, we want to keep it - results = results.into_iter().map(|mut r| { - if let EvaluationSymbolPtr::WEAK(ref mut weak) = r { - weak.instance = Some(true); + r + }).collect(); + } + let mut results = Vec::new(); + let mut visited: PtrWeakHashSet>> = PtrWeakHashSet::new(); + let can_eval_external = !symbol.borrow().is_external(); + while let Some(current_eval) = work_queue.pop_front() { + let EvaluationSymbolPtr::WEAK(next_ref_weak) = ¤t_eval else { + // Non-weak references are final + results.push(current_eval); + continue; + }; + let Some(sym_rc) = next_ref_weak.weak.upgrade() else { + // Discard evaluation to expired reference + continue; + }; + // Avoid cycles + if visited.contains(&sym_rc) { + continue; + } + visited.insert(sym_rc.clone()); + let next_ref_weak_instance = next_ref_weak.instance.clone(); + let sym_type = sym_rc.borrow().typ(); + match sym_type { + SymType::VARIABLE => { + { + let sym = sym_rc.borrow(); + let var = sym.as_variable(); + if (stop_on_type && matches!(next_ref_weak.is_instance(), Some(false)) && !var.is_import_variable) || + (stop_on_value && var.evaluations.len() == 1 && var.evaluations[0].value.is_some()) || + (max_scope.is_some() && !sym.has_rc_in_parents(max_scope.as_ref().unwrap().clone(), true)) { + // current evaluation is final + results.push(current_eval); + continue; } - r - }).collect(); - } - let mut acc: PtrWeakHashSet>> = PtrWeakHashSet::new(); - let can_eval_external = !symbol.borrow().is_external(); - let mut index = 0; - while index < results.len() { - let next_ref = &results[index]; - index += 1; - match next_ref { - EvaluationSymbolPtr::WEAK(next_ref_weak) => { - let sym = next_ref_weak.weak.upgrade(); - let next_ref_weak_instance = next_ref_weak.instance.clone(); - if sym.is_none() { - index += 1; - continue; - } - let sym_rc = sym.unwrap(); - if acc.contains(&sym_rc) { - index -= 1; - results.remove(index); - continue; + } + if sym_rc.borrow().as_variable().evaluations.is_empty() && sym_rc.borrow().name() != "__all__" && can_eval_external { + //no evaluation? let's check that the file has been evaluated + let file_symbol = sym_rc.borrow().get_file(); + if let Some(file_symbol) = file_symbol { + if let Some(file) = file_symbol.upgrade() { + SyncOdoo::build_now(session, &file, BuildSteps::ARCH_EVAL); } - acc.insert(sym_rc.clone()); - let sym_type = sym_rc.borrow().typ(); - match sym_type { - SymType::VARIABLE => { - { - let sym = sym_rc.borrow(); - let var = sym.as_variable(); - if stop_on_type && matches!(next_ref_weak.is_instance(), Some(false)) && !var.is_import_variable { - continue; - } - if stop_on_value && var.evaluations.len() == 1 && var.evaluations[0].value.is_some() { - continue; - } - if max_scope.is_some() && !sym.has_rc_in_parents(max_scope.as_ref().unwrap().clone(), true) { - continue; - } - } - if sym_rc.borrow().as_variable().evaluations.is_empty() && sym_rc.borrow().name() != "__all__" && can_eval_external { - //no evaluation? let's check that the file has been evaluated - let file_symbol = sym_rc.borrow().get_file(); - if let Some(file_symbol) = file_symbol { - if let Some(file) = file_symbol.upgrade() { - SyncOdoo::build_now(session, &file, BuildSteps::ARCH_EVAL); - } - } - } - let mut next_sym_refs = Symbol::next_refs(session, sym_rc.clone(), context, &next_ref_weak.context, stop_on_type, &mut vec![]); - if !next_sym_refs.is_empty() { - results.pop_front(); - index -= 1; - // /!\ we want to keep instance = True if previous evaluation was set to True! - if next_ref_weak_instance.is_some_and(|v| v) { - next_sym_refs = next_sym_refs.into_iter().map(|mut next_results| { - if let EvaluationSymbolPtr::WEAK(weak) = &mut next_results { - weak.instance = Some(true); - } - next_results - }).collect(); - } - for next_results in next_sym_refs { - results.push_back(next_results); - } - } - }, - SymType::CLASS => { - //On class, follow descriptor declarations - let next_sym_refs = Symbol::next_refs(session, sym_rc.clone(), context, &next_ref_weak.context, stop_on_type, &mut vec![]); - if !next_sym_refs.is_empty() { - results.pop_front(); - index -= 1; - for next_results in next_sym_refs { - results.push_back(next_results); - } - } - }, - _ => {} + } + } + let mut next_sym_refs = Symbol::next_refs(session, sym_rc.clone(), context, &next_ref_weak.context, stop_on_type, &mut vec![]); + if next_sym_refs.is_empty() { + // keep current evaluation + results.push(current_eval); + continue; + } + // /!\ we want to keep instance = True if previous evaluation was set to True! + if next_ref_weak_instance.is_some_and(|v| v) { + next_sym_refs = next_sym_refs.into_iter().map(|mut next_results| { + if let EvaluationSymbolPtr::WEAK(weak) = &mut next_results { + weak.instance = Some(true); } - }, - _ => {} + next_results + }).collect(); } + // enqueue evaluations to follow, replacing current evaluation + work_queue.extend(next_sym_refs); + }, + SymType::CLASS => { + //On class, follow descriptor declarations + let next_sym_refs = Symbol::next_refs(session, sym_rc.clone(), context, &next_ref_weak.context, stop_on_type, &mut vec![]); + if next_sym_refs.is_empty() { + // keep current evaluation + results.push(current_eval); + } else { + // enqueue evaluations to follow, replacing current evaluation + work_queue.extend(next_sym_refs); + } + }, + _ => { + results.push(current_eval); } - Vec::from(results) // :'( a whole copy? - }, - _ => { - return vec![evaluation.clone()]; } } + results } pub fn all_symbols(&self) -> impl Iterator>> + use<> { diff --git a/server/tests/data/python/expressions/properties.py b/server/tests/data/python/expressions/properties.py new file mode 100644 index 00000000..c34c7cde --- /dev/null +++ b/server/tests/data/python/expressions/properties.py @@ -0,0 +1,20 @@ +class TestClass: + @property + def answer(self): + return 42 + + @property + def ambiguous_answer(self): + return 42 if True else "forty-two" + +a = TestClass() # a: TestClass +b = a # b: TestClass +if True: + b = 1 # b: int +b # b: (int | TestClass) +c = b # c: (int | TestClass) +if True: + c = "hi" # c: str +c # c: (str | int | TestClass) +d = c # d: (str | int | TestClass) + diff --git a/server/tests/test_follow_ref.rs b/server/tests/test_follow_ref.rs new file mode 100644 index 00000000..f2383612 --- /dev/null +++ b/server/tests/test_follow_ref.rs @@ -0,0 +1,53 @@ +mod setup; +mod test_utils; +use odoo_ls_server::core::file_mgr::FileInfo; +use odoo_ls_server::core::odoo::SyncOdoo; +use odoo_ls_server::core::symbols::symbol::Symbol; +use odoo_ls_server::threads::SessionInfo; +use odoo_ls_server::utils::PathSanitizer; +use std::cell::RefCell; +use std::env; +use std::path::PathBuf; +use std::rc::Rc; + +#[test] +fn test_follow_ref() { + let (mut odoo, config) = setup::setup::setup_server(false); + let mut session = setup::setup::create_init_session(&mut odoo, config); + let path = env::current_dir() + .unwrap() + .join("tests/data/python/expressions/properties.py") + .sanitize(); + setup::setup::prepare_custom_entry_point(&mut session, path.as_str()); + assert!(session.sync_odoo.entry_point_mgr.borrow().custom_entry_points .len() == 1); + let file_mgr = session.sync_odoo.get_file_mgr(); + let file_info = file_mgr.borrow().get_file_info(&path).unwrap(); + let file_symbol = SyncOdoo::get_symbol_of_opened_file(&mut session, &PathBuf::from(&path)) + .expect("Failed to get file symbol"); + + test_variable_type_resolution(session, file_info, file_symbol); +} + +fn test_variable_type_resolution( + mut session: SessionInfo<'_>, + file_info: Rc>, + file_symbol: Rc>, +) { + for (var, (line, character), expected_type) in vec![ + ("a", (9, 0), "TestClass"), + ("b", (10, 0), "TestClass"), + ("b", (12, 4), "int"), + ("b", (13, 0), "(int | TestClass)"), + ("c", (14, 0), "(int | TestClass)"), + ("c", (16, 4), "str"), + ("c", (17, 0), "(str | int | TestClass)"), + ("d", (18, 0), "(str | int | TestClass)"), + ] { + let hover = + test_utils::get_hover_markdown(&mut session, &file_symbol, &file_info, line, character) + .expect(&format!("Should get hover text for {}", var)); + assert!( + hover.contains(format!("{var}: {expected_type}").as_str()), + "Hover over '{}' should show type '{}'. Got: {}", var, expected_type, hover); + } +} From 0dbe979cb5d79c3700cfda6ab726ccc179beb838 Mon Sep 17 00:00:00 2001 From: Rodolpho Lima Date: Fri, 12 Dec 2025 01:00:06 +0100 Subject: [PATCH 2/2] [IMP] Follow property functions when following references When following symbol references, also follow functions that are properties, in addition to variables and class instances. This ensures that properties are correctly resolved to the return type of the underlying function. --- server/src/core/symbols/symbol.rs | 48 +++++++++++++------ .../data/python/expressions/properties.py | 6 +++ server/tests/test_follow_ref.rs | 34 +++++++++++-- 3 files changed, 69 insertions(+), 19 deletions(-) diff --git a/server/src/core/symbols/symbol.rs b/server/src/core/symbols/symbol.rs index 749e4bea..09577951 100644 --- a/server/src/core/symbols/symbol.rs +++ b/server/src/core/symbols/symbol.rs @@ -2166,24 +2166,23 @@ impl Symbol { } } } - if let Symbol::Variable(v) = symbol { - for eval in v.evaluations.iter() { + + let sym_type = symbol.typ(); + if sym_type == SymType::VARIABLE || (sym_type == SymType::FUNCTION && symbol.as_func().is_property) { + for eval in symbol.evaluations().unwrap_or(&vec![]) { let ctx = &mut Some(symbol_context.clone().into_iter().chain(context.clone().unwrap_or(HashMap::new()).into_iter()).collect::>()); let mut sym = eval.symbol.get_symbol(session, ctx, diagnostics, None); - match sym { - EvaluationSymbolPtr::WEAK(ref mut w) => { - if let Some(base_attr) = symbol_context.get(&S!("base_attr")) { - if !w.context.get(&S!("is_attr_of_instance")).map(|x| x.as_bool()).unwrap_or(false) { - w.context.insert(S!("base_attr"), base_attr.clone()); - } + if let EvaluationSymbolPtr::WEAK(ref mut w) = sym { + if let Some(base_attr) = symbol_context.get(&S!("base_attr")) { + if !w.context.get(&S!("is_attr_of_instance")).map(|x| x.as_bool()).unwrap_or(false) { + w.context.insert(S!("base_attr"), base_attr.clone()); } - if let Some(base_attr) = symbol_context.get(&S!("is_attr_of_instance")) { - if !w.context.get(&S!("is_attr_of_instance")).map(|x| x.as_bool()).unwrap_or(false) { - w.context.insert(S!("is_attr_of_instance"), base_attr.clone()); - } + } + if let Some(base_attr) = symbol_context.get(&S!("is_attr_of_instance")) { + if !w.context.get(&S!("is_attr_of_instance")).map(|x| x.as_bool()).unwrap_or(false) { + w.context.insert(S!("is_attr_of_instance"), base_attr.clone()); } - }, - _ => {} + } } if !sym.is_expired_if_weak() { res.push_back(sym); @@ -2299,7 +2298,28 @@ impl Symbol { work_queue.extend(next_sym_refs); } }, + SymType::FUNCTION => { + let is_property = sym_rc.borrow().as_func().is_property; + if !is_property { + // Functions are final unless they are properties + results.push(current_eval); + continue; + } + let next_sym_refs = Symbol::next_refs(session, sym_rc.clone(), context, &next_ref_weak.context, stop_on_type, &mut vec![]); + if next_sym_refs.is_empty() { + // keep current evaluation + results.push(current_eval); + } else { + // enqueue evaluations to follow, replacing current evaluation + work_queue.extend(next_sym_refs); + } + // TODO: this arm can be easily merged with the one for SymType::CLASS, but + // - do we need to propagate instance = True here too (like we do for variables, but not class...)? + // - do we need to build_now for functions like we do for variables? In case yes, BuildSteps::ARCH_EVAL or VALIDATION? + // - stop_on_type, stop_on_value and max_scope apply also for this case? + } _ => { + // Other symbol types are final results.push(current_eval); } } diff --git a/server/tests/data/python/expressions/properties.py b/server/tests/data/python/expressions/properties.py index c34c7cde..2e5d9cb6 100644 --- a/server/tests/data/python/expressions/properties.py +++ b/server/tests/data/python/expressions/properties.py @@ -18,3 +18,9 @@ def ambiguous_answer(self): c # c: (str | int | TestClass) d = c # d: (str | int | TestClass) +the_answer = a.answer # the_answer: int +the_answer2 = d.answer # the_answer2: int + +ambiguous_answer = a.ambiguous_answer # ambiguous_answer: (int | str) +ambiguous_answer2 = d.ambiguous_answer # ambiguous_answer2: (int | str) + diff --git a/server/tests/test_follow_ref.rs b/server/tests/test_follow_ref.rs index f2383612..91ad58b6 100644 --- a/server/tests/test_follow_ref.rs +++ b/server/tests/test_follow_ref.rs @@ -12,6 +12,7 @@ use std::rc::Rc; #[test] fn test_follow_ref() { + // setup let (mut odoo, config) = setup::setup::setup_server(false); let mut session = setup::setup::create_init_session(&mut odoo, config); let path = env::current_dir() @@ -25,13 +26,15 @@ fn test_follow_ref() { let file_symbol = SyncOdoo::get_symbol_of_opened_file(&mut session, &PathBuf::from(&path)) .expect("Failed to get file symbol"); - test_variable_type_resolution(session, file_info, file_symbol); + // actual tests + test_variable_type_resolution(&mut session, &file_info, &file_symbol); + test_property_type_resolution(&mut session, &file_info, &file_symbol); } fn test_variable_type_resolution( - mut session: SessionInfo<'_>, - file_info: Rc>, - file_symbol: Rc>, + session: &mut SessionInfo<'_>, + file_info: &Rc>, + file_symbol: &Rc>, ) { for (var, (line, character), expected_type) in vec![ ("a", (9, 0), "TestClass"), @@ -44,10 +47,31 @@ fn test_variable_type_resolution( ("d", (18, 0), "(str | int | TestClass)"), ] { let hover = - test_utils::get_hover_markdown(&mut session, &file_symbol, &file_info, line, character) + test_utils::get_hover_markdown(session, file_symbol, file_info, line, character) .expect(&format!("Should get hover text for {}", var)); assert!( hover.contains(format!("{var}: {expected_type}").as_str()), "Hover over '{}' should show type '{}'. Got: {}", var, expected_type, hover); } } + +fn test_property_type_resolution( + session: &mut SessionInfo<'_>, + file_info: &Rc>, + file_symbol: &Rc>, +) { + for (var, (line, character), expected_type) in vec![ + ("the_answer", (20, 0), "int"), + ("the_answer2", (21, 0), "int"), + ("ambiguous_answer", (23, 0), "(int | str)"), + ("ambiguous_answer2", (24, 0), "(int | str)"), + ] { + let hover = + test_utils::get_hover_markdown(session, file_symbol, file_info, line, character) + .expect(&format!("Should get hover text for {}", var)); + assert!( + hover.contains(format!("{var}: {expected_type}").as_str()), + "Hover over '{}' should show type '{}'. Got: {}", var, expected_type, hover); + } +} +