Skip to content

[FIX] [IMP] Resolve property type #511

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
cammarosano wants to merge 2 commits into
base: alpha
Choose a base branch
from
+234 −119
Draft

[FIX] [IMP] Resolve property type #511

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
693d539
[FIX] follow_ref dropping evaluations incorrectly
cammarosano Dec 2, 2025
0dbe979
[IMP] Follow property functions when following references
cammarosano Dec 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
250 changes: 131 additions & 119 deletions server/src/core/symbols/symbol.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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::<HashMap<_, _>>());
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);
Expand All @@ -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<Context>, stop_on_type: bool, stop_on_value: bool, max_scope: Option<Rc<RefCell<Symbol>>>) -> Vec<EvaluationSymbolPtr> {
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<Weak<RefCell<Symbol>>> = 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) = &current_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<Weak<RefCell<Symbol>>> = 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?
Comment on lines +2316 to +2319
Copy link
Collaborator Author

@cammarosano cammarosano Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be discussed with @fda-odoo

}
_ => {
// 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<Item= Rc<RefCell<Symbol>>> + use<> {
Expand Down
26 changes: 26 additions & 0 deletions server/tests/data/python/expressions/properties.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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)

77 changes: 77 additions & 0 deletions server/tests/test_follow_ref.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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<RefCell<FileInfo>>,
file_symbol: &Rc<RefCell<Symbol>>,
) {
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<RefCell<FileInfo>>,
file_symbol: &Rc<RefCell<Symbol>>,
) {
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);
}
}