diff --git a/server/src/core/symbols/symbol.rs b/server/src/core/symbols/symbol.rs index 7f9dffd3..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); @@ -2198,121 +2197,134 @@ 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); + } + }, + 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); } - 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..2e5d9cb6 --- /dev/null +++ b/server/tests/data/python/expressions/properties.py @@ -0,0 +1,26 @@ +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) + +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 new file mode 100644 index 00000000..91ad58b6 --- /dev/null +++ b/server/tests/test_follow_ref.rs @@ -0,0 +1,77 @@ +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() { + // 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() + .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"); + + // 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( + session: &mut 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(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); + } +} +