Skip to content

[IMP] Implement Go To References Part 1 - python code #531

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

Open
fda-odoo wants to merge 9 commits into
base: alpha
Choose a base branch
from
Open

[IMP] Implement Go To References Part 1 - python code #531

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3951d0d
[IMP] Implement Phase 1 of Go To References (#466)
MihaiStreames Jan 15, 2026
7052fde
[IMP] refactor definition to get symbol and build location by after
fda-odoo Feb 3, 2026
3a33f4e
[IMP] GotoDeclaration
fda-odoo Feb 17, 2026
0475a75
[IMP] no_hash hashmap + cache for typeshed types
fda-odoo Feb 19, 2026
a6fc73b
[IMP] improve searches performances by avoiding useless searches in m…
fda-odoo Feb 19, 2026
aaf65d4
[IMP] iai_profiler to gungraun
fda-odoo Feb 19, 2026
7013b1b
[IMP] various compiler warning fixes
fda-odoo Feb 19, 2026
8ab3068
[ FIX] tests + remove duplicated references with models
fda-odoo Feb 19, 2026
b937cb6
[FIX] fix duplicate references and order in tests
fda-odoo Feb 24, 2026
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
4 changes: 2 additions & 2 deletions server/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,11 @@ csv = "1.3.1"
nix = { version = "0.29.0", features = ["process"] }

[[bench]]
name = "iai_profiler"
name = "gungraun_bench"
harness = false

[dev-dependencies]
iai-callgrind = "0.14.2"
gungraun = "0.17.2"
assert_fs = "1.1"

[features]
Expand Down
18 changes: 10 additions & 8 deletions server/benches/iai_profiler.rs → server/benches/gungraun_bench.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,20 @@ use tracing_subscriber::{fmt, FmtSubscriber, layer::SubscriberExt};

use std::env;

use iai_callgrind::{
library_benchmark, library_benchmark_group, main, LibraryBenchmarkConfig,
FlamegraphConfig
use gungraun::{
Callgrind, FlamegraphConfig, LibraryBenchmarkConfig, library_benchmark, library_benchmark_group, main
};
use std::hint::black_box;

/*
To run iai-callgrind:
To run gungraun-callgrind:
install valgrind
run cargo install --version 0.14.0 iai-callgrind-runner
run cargo install --version 0.14.0 gungraun-runner
run cargo bench -- --nocapture
*/

#[library_benchmark]
fn iai_main() {
fn gungraun_main() {
// TODO: Audit that the environment access only happens in single-threaded code.
unsafe { env::set_var("RUST_BACKTRACE", "full") };

Expand Down Expand Up @@ -87,9 +87,11 @@ fn iai_main() {
info!(">>>>>>>>>>>>>>>>>> End Session <<<<<<<<<<<<<<<<<<");
}

library_benchmark_group!(name = my_group; benchmarks = iai_main);
library_benchmark_group!(name = my_group; benchmarks = gungraun_main);

main!(
config = LibraryBenchmarkConfig::default().flamegraph(FlamegraphConfig::default());
config = LibraryBenchmarkConfig::default().tool(Callgrind::default()
.flamegraph(FlamegraphConfig::default())
),
library_benchmark_groups = my_group
);
137 changes: 107 additions & 30 deletions server/src/core/evaluation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,16 @@ use itertools::Itertools;
use itertools::FoldWhile::{Continue, Done};
use ruff_python_ast::{Arguments, Expr, ExprCall, Identifier, Number, Operator, Parameter, UnaryOp};
use ruff_text_size::{Ranged, TextRange, TextSize};
use lsp_types::{Diagnostic, Position, Range};
use lsp_types::{Diagnostic, Location, Position, Range};
use weak_table::traits::WeakElement;
use std::cmp::{max, min};
use std::collections::{HashMap, HashSet};
use std::i32;
use std::rc::{Rc, Weak};
use std::cell::RefCell;
use crate::core::diagnostics::{create_diagnostic, DiagnosticCode};
use crate::{constants::*, Sy};
use crate::utils::NoHashBuilder;
use crate::{Sy, constants::*, oyarn};
use crate::core::odoo::SyncOdoo;
use crate::threads::SessionInfo;
use crate::S;
Expand Down Expand Up @@ -282,7 +283,7 @@ impl Evaluation {
Evaluation {
symbol: EvaluationSymbol {
sym: EvaluationSymbolPtr::WEAK(EvaluationSymbolWeak{
weak: Rc::downgrade(&odoo.get_symbol("", &(vec![Sy!("builtins")], vec![Sy!("list")]), u32::MAX).last().expect("builtins list not found")),
weak: odoo.get_ts_list(),
Copy link
Collaborator

@cammarosano cammarosano Feb 24, 2026

Choose a reason for hiding this comment

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

Very minor:
You replaced this expect with a unwrap (inside get_ts_list). I would suggest keeping the expect with an informative message.
(same for the other get_ts_*s below)
Also, see other comment about using a macro. That could be a way to bring the expect back.

context: HashMap::new(),
instance: Some(true),
is_super: false,
Expand All @@ -298,7 +299,7 @@ impl Evaluation {
Evaluation {
symbol: EvaluationSymbol {
sym: EvaluationSymbolPtr::WEAK(EvaluationSymbolWeak{
weak: Rc::downgrade(&odoo.get_symbol("", &(vec![Sy!("builtins")], vec![Sy!("tuple")]), u32::MAX).last().expect("builtins list not found")),
weak: odoo.get_ts_tuple(),
context: HashMap::new(),
instance: Some(true),
is_super: false,
Expand All @@ -314,7 +315,7 @@ impl Evaluation {
Evaluation {
symbol: EvaluationSymbol {
sym: EvaluationSymbolPtr::WEAK(EvaluationSymbolWeak{
weak: Rc::downgrade(&odoo.get_symbol("", &(vec![Sy!("builtins")], vec![Sy!("dict")]), u32::MAX).last().expect("builtins list not found")),
weak: odoo.get_ts_dict(),
context: HashMap::new(),
instance: Some(true),
is_super: false,
Expand All @@ -330,7 +331,7 @@ impl Evaluation {
Evaluation {
symbol: EvaluationSymbol {
sym: EvaluationSymbolPtr::WEAK(EvaluationSymbolWeak{
weak: Rc::downgrade(&odoo.get_symbol("", &(vec![Sy!("builtins")], vec![Sy!("set")]), u32::MAX).last().expect("builtins set not found")),
weak: odoo.get_ts_set(),
context: HashMap::new(),
instance: Some(true),
is_super: false,
Expand All @@ -354,40 +355,36 @@ impl Evaluation {
}

pub fn new_constant(odoo: &mut SyncOdoo, values: Expr, range: TextRange) -> Evaluation {
let tree_value = match &values {
let symbol = match &values {
Expr::StringLiteral(_s) => {
(vec![Sy!("builtins")], vec![Sy!("str")])
odoo.get_ts_string()
},
Expr::BooleanLiteral(_b) => {
(vec![Sy!("builtins")], vec![Sy!("bool")])
odoo.get_ts_boolean()
},
Expr::NumberLiteral(_n) => {
match _n.value {
Number::Float(_) => (vec![Sy!("builtins")], vec![Sy!("float")]),
Number::Int(_) => (vec![Sy!("builtins")], vec![Sy!("int")]),
Number::Complex { .. } => (vec![Sy!("builtins")], vec![Sy!("complex")]),
Number::Float(_) => odoo.get_ts_float(),
Number::Int(_) => odoo.get_ts_int(),
Number::Complex { .. } => odoo.get_ts_complex(),
}
},
Expr::BytesLiteral(_b) => {
(vec![Sy!("builtins")], vec![Sy!("bytes")])
odoo.get_ts_bytes()
},
Expr::EllipsisLiteral(_e) => {
(vec![Sy!("builtins")], vec![Sy!("Ellipsis")])
odoo.get_ts_ellipsis()
},
Expr::NoneLiteral(_n) => {
let mut eval = Evaluation::new_none();
eval.range = Some(range);
eval.value = Some(EvaluationValue::CONSTANT(values));
return eval
}
_ => {(vec![Sy!("builtins")], vec![Sy!("object")])}
_ => {
odoo.get_ts_object()
}
};
let symbol;
if !values.is_none_literal_expr() {
symbol = Rc::downgrade(&odoo.get_symbol("", &tree_value, u32::MAX).last().expect("builtins class not found"));
} else {
symbol = Weak::new();
}
Evaluation {
symbol: EvaluationSymbol {
sym: EvaluationSymbolPtr::WEAK(EvaluationSymbolWeak{
Expand Down Expand Up @@ -511,7 +508,7 @@ impl Evaluation {
/// else:
/// i="test"
/// It will return two evaluation for i, one with 5 and one for "test"
pub fn from_sections(parent: &Symbol, sections: &HashMap<u32, Vec<Rc<RefCell<Symbol>>>>) -> Vec<Evaluation> {
pub fn from_sections(parent: &Symbol, sections: &HashMap<u32, Vec<Rc<RefCell<Symbol>>>, NoHashBuilder>) -> Vec<Evaluation> {
let mut res = vec![];
let section = parent.as_symbol_mgr().get_section_for(u32::MAX);
let content_symbols = parent.as_symbol_mgr()._get_loc_symbol(sections, u32::MAX, &SectionIndex::INDEX(section.index), &mut HashSet::new());
Expand Down Expand Up @@ -688,6 +685,7 @@ impl Evaluation {
let mut evals = vec![];
let mut diagnostics = vec![];
let module = parent.borrow().find_module();
let mut found_one_reference = false;

match ast {
ExprOrIdent::Expr(Expr::StringLiteral(expr)) => {
Expand Down Expand Up @@ -1007,6 +1005,9 @@ impl Evaluation {
Some(ContextValue::SYMBOL(s)) => s.clone(),
_ => Weak::new()
};
if session.sync_odoo.evaluation_search.is_some() {
Evaluation::search_reference_in_arg(session, expr, &parent);
}
if is_in_validation {
let on_instance = base_sym_weak_eval.context.get(&S!("is_attr_of_instance")).map(|v| v.as_bool());
call_argument_diagnostics.last_mut().unwrap().extend(Evaluation::validate_call_arguments(session,
Expand Down Expand Up @@ -1175,7 +1176,7 @@ impl Evaluation {
let bases = Symbol::follow_ref(&base, session, &mut None, false, false, None, None);
let value = Evaluation::expr_to_str(session, &sub.slice, parent.clone(), max_infer, false, &mut diagnostics);
diagnostics.extend(value.1);
for base in bases.iter() {
for base in bases.iter() {
match base {
EvaluationSymbolPtr::WEAK(base_sym_weak_eval) if base_sym_weak_eval.instance == Some(false) => {
if let Some(SymType::CLASS) = base.upgrade_weak().map(|s| s.borrow().typ()) {
Expand Down Expand Up @@ -1266,11 +1267,13 @@ impl Evaluation {
}
},
ExprOrIdent::Expr(Expr::BinOp(operator)) => {
match operator.op {
Operator::Add => {

},
_ => {}
if odoo.evaluation_search.is_some() {
match operator.op {
Operator::Add | Operator::Mult | Operator::Sub | Operator::Div | Operator::BitAnd | Operator::BitOr | Operator::BitXor |Operator::FloorDiv | Operator::LShift | Operator::MatMult | Operator::Mod | Operator::Pow | Operator::RShift => {
Evaluation::eval_from_ast(session, &operator.left, parent.clone(), max_infer, false, required_dependencies);
Evaluation::eval_from_ast(session, &operator.right, parent.clone(), max_infer, false, required_dependencies);
}
}
}
Comment on lines +1270 to 1277
Copy link
Collaborator

@mmahrouss mmahrouss Feb 24, 2026

Choose a reason for hiding this comment

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

The match is not needed here, we can drop it

Suggested change
if odoo.evaluation_search.is_some() {
match operator.op {
Operator::Add | Operator::Mult | Operator::Sub | Operator::Div | Operator::BitAnd | Operator::BitOr | Operator::BitXor |Operator::FloorDiv | Operator::LShift | Operator::MatMult | Operator::Mod | Operator::Pow | Operator::RShift => {
Evaluation::eval_from_ast(session, &operator.left, parent.clone(), max_infer, false, required_dependencies);
Evaluation::eval_from_ast(session, &operator.right, parent.clone(), max_infer, false, required_dependencies);
}
}
}
if odoo.evaluation_search.is_some() {
Evaluation::eval_from_ast(session, &operator.left, parent.clone(), max_infer, false, required_dependencies);
Evaluation::eval_from_ast(session, &operator.right, parent.clone(), max_infer, false, required_dependencies);
}

},
ExprOrIdent::Expr(Expr::If(if_expr)) => {
Expand All @@ -1292,7 +1295,7 @@ impl Evaluation {
evals.push(Evaluation {
symbol: EvaluationSymbol {
sym: EvaluationSymbolPtr::WEAK(EvaluationSymbolWeak{
weak: Rc::downgrade(&odoo.get_symbol("", &(vec![Sy!("builtins")], vec![Sy!("bool")]), u32::MAX).last().expect("builtins class not found")),
weak: odoo.get_ts_boolean(),
context: HashMap::new(),
instance: Some(true),
is_super: false,
Expand All @@ -1302,6 +1305,9 @@ impl Evaluation {
value: None,
range: Some(unary_operator.range()),
});
if odoo.evaluation_search.is_some() { //Still evaluate if we are searching for something
Evaluation::eval_from_ast(session, &unary_operator.operand, parent.clone(), max_infer, for_annotation, required_dependencies);
}
break 'u_op_block
},
};
Expand Down Expand Up @@ -1337,7 +1343,7 @@ impl Evaluation {
Evaluation {
symbol: EvaluationSymbol {
sym: EvaluationSymbolPtr::WEAK(EvaluationSymbolWeak{
weak: Rc::downgrade(&odoo.get_symbol("", &(vec![Sy!("builtins")], vec![Sy!("str")]), u32::MAX).last().expect("builtins class not found")),
weak: odoo.get_ts_string(),
context: HashMap::new(),
instance: Some(true),
is_super: false,
Expand All @@ -1351,6 +1357,65 @@ impl Evaluation {
}
_ => {}
}
if let Some(evaluation_search) = session.sync_odoo.evaluation_search.as_ref() {
Copy link
Collaborator

@cammarosano cammarosano Feb 25, 2026

Choose a reason for hiding this comment

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

Beware of early returns in the code above this part!
A possible way to ensure this part always runs would be to make this a sibling function of current analyze_ast:

fn analyze_ast() -> result {
   result = old_analyze_ast;
   new_function_for_references(result.evals);
   return result;
}

And then found_one_reference would be declared inside thenew_function_for_references

Edit: maybe make new_function_for_references get called by eval_from_ast, right after the call to analyze_ast?

for eval in evals.iter() {
if eval.symbol.sym.is_weak() && let Some(weak) = eval.symbol.sym.as_weak().weak.upgrade() {
if Rc::ptr_eq(&weak, evaluation_search) {
if found_one_reference {
//if we have multiple matches, it means that that ast can reference it multiple times, but we only want to know if that ast matches or not
continue;
}
Comment on lines +1361 to +1367
Copy link
Collaborator

@mmahrouss mmahrouss Feb 24, 2026

Choose a reason for hiding this comment

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

We could move the check of found_one_reference and break early to avoid extra checks

Suggested change
for eval in evals.iter() {
if eval.symbol.sym.is_weak() && let Some(weak) = eval.symbol.sym.as_weak().weak.upgrade() {
if Rc::ptr_eq(&weak, evaluation_search) {
if found_one_reference {
//if we have multiple matches, it means that that ast can reference it multiple times, but we only want to know if that ast matches or not
continue;
}
for eval in evals.iter() {
if found_one_reference {
//if we have multiple matches, it means that that ast can reference it multiple times, but we only want to know if that ast matches or not
break;
}
if eval.symbol.sym.is_weak() && let Some(weak) = eval.symbol.sym.as_weak().weak.upgrade() {
if Rc::ptr_eq(&weak, evaluation_search) {

let file = parent.borrow().get_file().unwrap().upgrade().unwrap();
let file_info = session.sync_odoo.get_file_mgr().borrow().get_file_info(&file.borrow().paths()[0]);
if let Some(file_info) = file_info {
found_one_reference = true;
let range= ast.range();
let transformed_range = file_info.borrow().text_range_to_range(&range, session.sync_odoo.encoding);
//check that a previous call to analyze_ast (recursively for ex) didn't already add this expression
let uri = FileMgr::pathname2uri(&file.borrow().paths().first().unwrap());
if session.sync_odoo.evaluation_locations.is_empty() ||
session.sync_odoo.evaluation_locations.last().unwrap().uri != uri ||
session.sync_odoo.evaluation_locations.last().unwrap().range.start.line != transformed_range.start.line{
Comment on lines +1375 to +1378
Copy link
Collaborator

@cammarosano cammarosano Feb 25, 2026

Choose a reason for hiding this comment

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

Maybe add a comment explaining what this deduplication of consecutive matches solves? (env["some_model"].sudo().browse() etc)

session.sync_odoo.evaluation_locations.push(Location {
uri: uri,
range: transformed_range,
});
}
}
}
}
if let Some(value) = eval.value.as_ref() {
match value {
EvaluationValue::CONSTANT(constant) => {
match constant {
Expr::StringLiteral(s) => {
if evaluation_search.borrow().typ() == SymType::CLASS {
let class_bw = evaluation_search.borrow();
let class = class_bw.as_class_sym();
if let Some(model_data) = class._model.as_ref() {
if oyarn!("{}", s.value.to_str()) == model_data.name {
let file = parent.borrow().get_file().unwrap().upgrade().unwrap();
let file_info = session.sync_odoo.get_file_mgr().borrow().get_file_info(&file.borrow().paths()[0]);
if let Some(file_info) = file_info {
let transformed_range = file_info.borrow().text_range_to_range(&s.range, session.sync_odoo.encoding);
let uri = FileMgr::pathname2uri(&file.borrow().paths().first().unwrap());
session.sync_odoo.evaluation_locations.push(Location {
uri: uri,
range: transformed_range,
});
}
}
}
}
},
_ => {}
}
Comment on lines +1387 to +1412
Copy link
Collaborator

@cammarosano cammarosano Feb 25, 2026

Choose a reason for hiding this comment

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

Every time
    you write code like this
        a puppy
            dies
                 !!!!
Image

😆

I suggest replacing this pattern

match value {
   the_one_pattern(x) => {...}
   _ => {}
}

By this:

let the_one_pattern(x) = value else {
     continue;
}
...

The same for if let without any code after it -> let ... else { continue }

},
_ => {}
}
}
}
}
AnalyzeAstResult { evaluations: evals, diagnostics }
}

Expand Down Expand Up @@ -1735,6 +1800,18 @@ impl Evaluation {
}
diagnostics
}

fn search_reference_in_arg(session: &mut SessionInfo, expr_call: &ExprCall, parent: &Rc<RefCell<Symbol>>) {
for arg in expr_call.arguments.args.iter() {
if arg.is_starred_expr() {
continue;
}
Evaluation::eval_from_ast(session, arg, parent.clone(), &expr_call.range.start(), false, &mut vec![]);
}
for arg in expr_call.arguments.keywords.iter() {
Evaluation::eval_from_ast(session, &arg.value, parent.clone(), &expr_call.range.start(), false, &mut vec![]);
}
}
}

impl EvaluationSymbol {
Expand Down
Loading