Skip to content

Commit a45b3f7

Browse files
authored
fix: resolve barrel resolution quality and cycle regression (#848)
* refactor(native): extract magic numbers to named constants Extract hardcoded magic numbers to named constants in constants.rs: - Louvain: MAX_LEVELS=50, MAX_PASSES=20, MIN_GAIN=1e-12, DEFAULT_SEED=42 - Dataflow: TRUNCATION_LIMIT=120 - Build pipeline: FAST_PATH_MAX_CHANGED_FILES=5, FAST_PATH_MIN_EXISTING_FILES=20 Also extract DEFAULT_RANDOM_SEED=42 in TS louvain.ts. * refactor(native): extract shared barrel resolution into common module * refactor(native): improve helper and barrel resolution quality * fix: resolve +1 function cycle regression in barrel resolution * fix: use i64 type for FAST_PATH_MIN_EXISTING_FILES constant (#848) The constant is compared against existing_file_count which returns i64 from SQLite. Using usize caused a Rust compile error. * fix: address Greptile review feedback on barrel resolution and helpers (#848) Remove redundant guard in barrel_resolution.rs that was always true at the wildcard/empty-names branch. Eliminate skip_children_on_short_string flag in helpers.rs by using early return directly from the string match arm.
1 parent 8c3a250 commit a45b3f7

File tree

11 files changed

+424
-217
lines changed

11 files changed

+424
-217
lines changed
Lines changed: 189 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,189 @@
1+
//! Shared barrel-file resolution logic.
2+
//!
3+
//! Both `edge_builder.rs` (napi-driven) and `import_edges.rs` (SQLite-driven)
4+
//! need to recursively resolve a symbol through barrel reexport chains.
5+
//! This module extracts the common algorithm so both callers share a single
6+
//! implementation.
7+
8+
use std::collections::HashSet;
9+
10+
/// Minimal view of a single reexport entry, borrowed from the caller's data.
11+
pub struct ReexportRef<'a> {
12+
pub source: &'a str,
13+
pub names: &'a [String],
14+
pub wildcard_reexport: bool,
15+
}
16+
17+
/// Trait that abstracts over the different context types in `edge_builder` and
18+
/// `import_edges`. Each implementor provides access to its own reexport map
19+
/// and definition index so the resolution algorithm stays generic.
20+
pub trait BarrelContext {
21+
/// Return the reexport entries for `barrel_path`, or `None` if the path
22+
/// has no reexports.
23+
fn reexports_for(&self, barrel_path: &str) -> Option<Vec<ReexportRef<'_>>>;
24+
25+
/// Return `true` if `file_path` contains a definition named `symbol`.
26+
fn has_definition(&self, file_path: &str, symbol: &str) -> bool;
27+
}
28+
29+
/// Recursively resolve a symbol through barrel reexport chains.
30+
///
31+
/// Mirrors `resolveBarrelExport()` in `resolve-imports.ts`.
32+
/// The caller provides a `visited` set to prevent infinite loops on circular
33+
/// reexport chains.
34+
pub fn resolve_barrel_export<'a, C: BarrelContext>(
35+
ctx: &'a C,
36+
barrel_path: &str,
37+
symbol_name: &str,
38+
visited: &mut HashSet<String>,
39+
) -> Option<String> {
40+
if visited.contains(barrel_path) {
41+
return None;
42+
}
43+
visited.insert(barrel_path.to_string());
44+
45+
let reexports = ctx.reexports_for(barrel_path)?;
46+
47+
for re in &reexports {
48+
// Named reexports (non-wildcard)
49+
if !re.names.is_empty() && !re.wildcard_reexport {
50+
if re.names.iter().any(|n| n == symbol_name) {
51+
if ctx.has_definition(re.source, symbol_name) {
52+
return Some(re.source.to_string());
53+
}
54+
let deeper = resolve_barrel_export(ctx, re.source, symbol_name, visited);
55+
if deeper.is_some() {
56+
return deeper;
57+
}
58+
// Fallback: return source even if no definition found
59+
return Some(re.source.to_string());
60+
}
61+
continue;
62+
}
63+
64+
// Wildcard or empty-names reexports
65+
if ctx.has_definition(re.source, symbol_name) {
66+
return Some(re.source.to_string());
67+
}
68+
let deeper = resolve_barrel_export(ctx, re.source, symbol_name, visited);
69+
if deeper.is_some() {
70+
return deeper;
71+
}
72+
}
73+
74+
None
75+
}
76+
77+
#[cfg(test)]
78+
mod tests {
79+
use super::*;
80+
use std::collections::HashMap;
81+
82+
struct TestContext {
83+
reexports: HashMap<String, Vec<(String, Vec<String>, bool)>>,
84+
definitions: HashMap<String, HashSet<String>>,
85+
}
86+
87+
impl BarrelContext for TestContext {
88+
fn reexports_for(&self, barrel_path: &str) -> Option<Vec<ReexportRef<'_>>> {
89+
self.reexports.get(barrel_path).map(|entries| {
90+
entries
91+
.iter()
92+
.map(|(source, names, wildcard)| ReexportRef {
93+
source: source.as_str(),
94+
names: names.as_slice(),
95+
wildcard_reexport: *wildcard,
96+
})
97+
.collect()
98+
})
99+
}
100+
101+
fn has_definition(&self, file_path: &str, symbol: &str) -> bool {
102+
self.definitions
103+
.get(file_path)
104+
.map_or(false, |defs| defs.contains(symbol))
105+
}
106+
}
107+
108+
#[test]
109+
fn resolves_named_reexport() {
110+
let mut reexports = HashMap::new();
111+
reexports.insert(
112+
"src/index.ts".to_string(),
113+
vec![("src/utils.ts".to_string(), vec!["foo".to_string()], false)],
114+
);
115+
let mut definitions = HashMap::new();
116+
definitions.insert(
117+
"src/utils.ts".to_string(),
118+
HashSet::from(["foo".to_string()]),
119+
);
120+
121+
let ctx = TestContext { reexports, definitions };
122+
let mut visited = HashSet::new();
123+
let result = resolve_barrel_export(&ctx, "src/index.ts", "foo", &mut visited);
124+
assert_eq!(result.as_deref(), Some("src/utils.ts"));
125+
}
126+
127+
#[test]
128+
fn resolves_wildcard_reexport() {
129+
let mut reexports = HashMap::new();
130+
reexports.insert(
131+
"src/index.ts".to_string(),
132+
vec![("src/utils.ts".to_string(), vec![], true)],
133+
);
134+
let mut definitions = HashMap::new();
135+
definitions.insert(
136+
"src/utils.ts".to_string(),
137+
HashSet::from(["bar".to_string()]),
138+
);
139+
140+
let ctx = TestContext { reexports, definitions };
141+
let mut visited = HashSet::new();
142+
let result = resolve_barrel_export(&ctx, "src/index.ts", "bar", &mut visited);
143+
assert_eq!(result.as_deref(), Some("src/utils.ts"));
144+
}
145+
146+
#[test]
147+
fn resolves_transitive_chain() {
148+
let mut reexports = HashMap::new();
149+
reexports.insert(
150+
"src/index.ts".to_string(),
151+
vec![("src/mid.ts".to_string(), vec![], true)],
152+
);
153+
reexports.insert(
154+
"src/mid.ts".to_string(),
155+
vec![("src/deep.ts".to_string(), vec!["baz".to_string()], false)],
156+
);
157+
let mut definitions = HashMap::new();
158+
definitions.insert(
159+
"src/deep.ts".to_string(),
160+
HashSet::from(["baz".to_string()]),
161+
);
162+
163+
let ctx = TestContext { reexports, definitions };
164+
let mut visited = HashSet::new();
165+
let result = resolve_barrel_export(&ctx, "src/index.ts", "baz", &mut visited);
166+
assert_eq!(result.as_deref(), Some("src/deep.ts"));
167+
}
168+
169+
#[test]
170+
fn prevents_circular_reexport() {
171+
let mut reexports = HashMap::new();
172+
reexports.insert(
173+
"src/a.ts".to_string(),
174+
vec![("src/b.ts".to_string(), vec![], true)],
175+
);
176+
reexports.insert(
177+
"src/b.ts".to_string(),
178+
vec![("src/a.ts".to_string(), vec![], true)],
179+
);
180+
181+
let ctx = TestContext {
182+
reexports,
183+
definitions: HashMap::new(),
184+
};
185+
let mut visited = HashSet::new();
186+
let result = resolve_barrel_export(&ctx, "src/a.ts", "missing", &mut visited);
187+
assert_eq!(result, None);
188+
}
189+
}

crates/codegraph-core/src/build_pipeline.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
1919
use crate::change_detection;
2020
use crate::config::{BuildConfig, BuildOpts, BuildPathAliases};
21+
use crate::constants::{FAST_PATH_MAX_CHANGED_FILES, FAST_PATH_MIN_EXISTING_FILES};
2122
use crate::file_collector;
2223
use crate::import_edges::{self, ImportEdgeContext};
2324
use crate::import_resolution;
@@ -492,7 +493,7 @@ pub fn run_pipeline(
492493
// reverse-dep files added for edge rebuilding, which inflates the count
493494
// and would skip the fast path even for single-file incremental builds.
494495
let use_fast_path =
495-
!change_result.is_full_build && parse_changes.len() <= 5 && existing_file_count > 20;
496+
!change_result.is_full_build && parse_changes.len() <= FAST_PATH_MAX_CHANGED_FILES && existing_file_count > FAST_PATH_MIN_EXISTING_FILES;
496497

497498
if use_fast_path {
498499
structure::update_changed_file_metrics(
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,30 @@
11
/// Maximum recursion depth for AST traversal to prevent stack overflow
22
/// on deeply nested trees. Used by extractors, complexity, CFG, and dataflow.
33
pub const MAX_WALK_DEPTH: usize = 200;
4+
5+
// ─── Louvain community detection ────────────────────────────────────
6+
7+
/// Maximum number of coarsening levels in the Louvain algorithm.
8+
pub const LOUVAIN_MAX_LEVELS: usize = 50;
9+
10+
/// Maximum number of local-move passes per level before stopping.
11+
pub const LOUVAIN_MAX_PASSES: usize = 20;
12+
13+
/// Minimum modularity gain to accept a node move (avoids floating-point noise).
14+
pub const LOUVAIN_MIN_GAIN: f64 = 1e-12;
15+
16+
/// Default random seed for deterministic community detection.
17+
pub const DEFAULT_RANDOM_SEED: u32 = 42;
18+
19+
// ─── Dataflow analysis ──────────────────────────────────────────────
20+
21+
/// Maximum character length for truncated dataflow expressions.
22+
pub const DATAFLOW_TRUNCATION_LIMIT: usize = 120;
23+
24+
// ─── Build pipeline ─────────────────────────────────────────────────
25+
26+
/// Maximum number of changed files eligible for the incremental fast path.
27+
pub const FAST_PATH_MAX_CHANGED_FILES: usize = 5;
28+
29+
/// Minimum existing file count required before the fast path is considered.
30+
pub const FAST_PATH_MIN_EXISTING_FILES: i64 = 20;

crates/codegraph-core/src/dataflow.rs

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use std::collections::HashMap;
22
use tree_sitter::{Node, Tree};
33

4-
use crate::constants::MAX_WALK_DEPTH;
4+
use crate::constants::{DATAFLOW_TRUNCATION_LIMIT, MAX_WALK_DEPTH};
55
use crate::types::{
66
DataflowArgFlow, DataflowAssignment, DataflowMutation, DataflowParam, DataflowResult,
77
DataflowReturn,
@@ -1196,7 +1196,7 @@ fn handle_var_declarator(
11961196
var_name: n.clone(),
11971197
caller_func: Some(func_name.clone()),
11981198
source_call_name: callee.clone(),
1199-
expression: truncate(node_text(node, source), 120),
1199+
expression: truncate(node_text(node, source), DATAFLOW_TRUNCATION_LIMIT),
12001200
line: node_line(node),
12011201
});
12021202
scope
@@ -1209,7 +1209,7 @@ fn handle_var_declarator(
12091209
var_name: var_name.clone(),
12101210
caller_func: Some(func_name),
12111211
source_call_name: callee.clone(),
1212-
expression: truncate(node_text(node, source), 120),
1212+
expression: truncate(node_text(node, source), DATAFLOW_TRUNCATION_LIMIT),
12131213
line: node_line(node),
12141214
});
12151215
scope.locals.insert(var_name, LocalSource::CallReturn { callee });
@@ -1245,7 +1245,7 @@ fn handle_assignment(
12451245
func_name: Some(func_name.clone()),
12461246
receiver_name: receiver,
12471247
binding_type: binding.as_ref().map(|b| b.binding_type.clone()),
1248-
mutating_expr: truncate(node_text(node, source), 120),
1248+
mutating_expr: truncate(node_text(node, source), DATAFLOW_TRUNCATION_LIMIT),
12491249
line: node_line(node),
12501250
});
12511251
}
@@ -1264,7 +1264,7 @@ fn handle_assignment(
12641264
var_name: var_name.clone(),
12651265
caller_func: Some(func_name),
12661266
source_call_name: callee.clone(),
1267-
expression: truncate(node_text(node, source), 120),
1267+
expression: truncate(node_text(node, source), DATAFLOW_TRUNCATION_LIMIT),
12681268
line: node_line(node),
12691269
});
12701270
if let Some(scope) = scope_stack.last_mut() {
@@ -1340,7 +1340,7 @@ fn handle_call_expr(
13401340
arg_name: Some(tracked.clone()),
13411341
binding_type: binding.as_ref().map(|b| b.binding_type.clone()),
13421342
confidence: conf,
1343-
expression: truncate(node_text(&arg_raw, source), 120),
1343+
expression: truncate(node_text(&arg_raw, source), DATAFLOW_TRUNCATION_LIMIT),
13441344
line: node_line(node),
13451345
});
13461346
}
@@ -1442,7 +1442,7 @@ fn handle_expr_stmt_mutation(
14421442
func_name,
14431443
receiver_name: recv,
14441444
binding_type: binding.as_ref().map(|b| b.binding_type.clone()),
1445-
mutating_expr: truncate(node_text(&expr, source), 120),
1445+
mutating_expr: truncate(node_text(&expr, source), DATAFLOW_TRUNCATION_LIMIT),
14461446
line: node_line(node),
14471447
});
14481448
}

0 commit comments

Comments
 (0)