Improve Rust callgraph resolution for direct self fields - #196
Conversation
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/aft/tests/integration/callgraph_store_name_match_test.rs">
<violation number="1" location="crates/aft/tests/integration/callgraph_store_name_match_test.rs:769">
P2: The nested same-named-type test only asserts the absence of precise type_match edges; it predicates on resolved_by=="type_match" && approximate==false, so a regression that recreates a name_match edge to nested::Engine::start (the false edge this PR exists to prevent) would still pass. Consider also asserting that no edge at all targets nested::Engine::start (e.g. counting edges for that symbol, or checking the child is present with resolved==false), not just that no precise type_match edge exists.</violation>
</file>
<file name="crates/aft/src/callgraph_store/mod.rs">
<violation number="1" location="crates/aft/src/callgraph_store/mod.rs:9811">
P2: Inline comments between a direct self field and method bypass inference and re-enable `name_match`, so `self.path /* ... */ .as_path()` can falsely resolve to an unrelated same-named method. Normalize Rust trivia here or derive direct-field identity from the parsed field-expression before allowing fallback.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| 1, | ||
| true, | ||
| )); | ||
| let precise_tree_edge = tree["children"].as_array().unwrap().iter().any(|child| { |
There was a problem hiding this comment.
P2: The nested same-named-type test only asserts the absence of precise type_match edges; it predicates on resolved_by=="type_match" && approximate==false, so a regression that recreates a name_match edge to nested::Engine::start (the false edge this PR exists to prevent) would still pass. Consider also asserting that no edge at all targets nested::Engine::start (e.g. counting edges for that symbol, or checking the child is present with resolved==false), not just that no precise type_match edge exists.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/aft/tests/integration/callgraph_store_name_match_test.rs, line 769:
<comment>The nested same-named-type test only asserts the absence of precise type_match edges; it predicates on resolved_by=="type_match" && approximate==false, so a regression that recreates a name_match edge to nested::Engine::start (the false edge this PR exists to prevent) would still pass. Consider also asserting that no edge at all targets nested::Engine::start (e.g. counting edges for that symbol, or checking the child is present with resolved==false), not just that no precise type_match edge exists.</comment>
<file context>
@@ -458,6 +726,69 @@ export function entry(permissionRuleEngine: PermissionRuleEngine) {
+ 1,
+ true,
+ ));
+ let precise_tree_edge = tree["children"].as_array().unwrap().iter().any(|child| {
+ child["name"] == "Engine::start"
+ && child["resolved_by"] == "type_match"
</file context>
| fn rust_direct_self_field_name(receiver_expression: &str) -> Option<&str> { | ||
| let (base, field) = receiver_expression.split_once('.')?; | ||
| let base = base.trim(); | ||
| let field = field.trim(); |
There was a problem hiding this comment.
P2: Inline comments between a direct self field and method bypass inference and re-enable name_match, so self.path /* ... */ .as_path() can falsely resolve to an unrelated same-named method. Normalize Rust trivia here or derive direct-field identity from the parsed field-expression before allowing fallback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/aft/src/callgraph_store/mod.rs, line 9811:
<comment>Inline comments between a direct self field and method bypass inference and re-enable `name_match`, so `self.path /* ... */ .as_path()` can falsely resolve to an unrelated same-named method. Normalize Rust trivia here or derive direct-field identity from the parsed field-expression before allowing fallback.</comment>
<file context>
@@ -9619,19 +9700,200 @@ fn is_code_ident_char(ch: char) -> bool {
+fn rust_direct_self_field_name(receiver_expression: &str) -> Option<&str> {
+ let (base, field) = receiver_expression.split_once('.')?;
+ let base = base.trim();
+ let field = field.trim();
+ (base == "self" && rust_direct_nominal_type_name(field).is_some()).then_some(field)
+}
</file context>
The nested-type regression test asserted only that no PRECISE edge exists to nested::Engine::start, but the implemented behavior is stronger: the scope rejection suppresses the name-match fallback too, leaving the call with no dispatch edge at all. A regression re-creating the old name_match false edge would have passed the precise-only assertions. Count edges of any dispatch provenance and require zero. Review follow-up from PR #196.
Summary
While testing Rust callgraph output, I ran into a case where
Holder::checkcallsPathBuf::as_path, but AFT could still produce an approximate edge to an unrelatedPrinterPath::as_pathelsewhere in the project. So i create this pr and explain what I do.The existing
name_matchfallback is useful when the receiver type is genuinely unknown, so I didn't want to remove or weaken that behavior. The issue here is that for a simple field receiver likeself.path, we can sometimes recover enough local type information to make a better decision — and when we can't, guessing by method name can be misleading.What changed
This adds a conservative path for direct
self.field.method()calls.When the field can be tied to a concrete local type in the same lexical scope, AFT uses the existing precise
type_matchpath. For example, ifself.engineis known to be anEngine,Car::run -> Engine::startis resolved precisely.If the field is clearly present but its type can't be resolved safely, the call is left unresolved instead of being connected to an unrelated same-named method. Precise candidates are also checked against the field type's lexical module scope, so a same-named type inside a nested module can't accidentally become the target.
I intentionally kept this narrow. This isn't meant to add general Rust type or module resolution. Imported or scoped types, nested fields, generics, trait impls, references, aliases, wrappers, deref/coercion, and cross-file type identities stay conservative unless AFT already knows how to resolve them.
Tests
The tests cover the original
PathBuf/PrinterPathcase, precise resolution for a local field type, preservation of the existingname_matchfallback for genuinely unknown receivers, and the scoped/imported/nested-module cases that could otherwise create false precise edges.The relevant unit and integration tests pass, along with:
The full crate suite still has four failures in this local environment. I reproduced all four against a clean worktree at the same HEAD with the same failure signatures, so they appear to be baseline/environment-related rather than introduced by this change.(I guess it's because of my base environment.)
Thanks for your project. It helps a lot in my coding time.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Improves Rust callgraph accuracy for direct
self.field.method()calls, including spaced forms likeself .engine.start(). Resolves only when the field’s type is a proven local concrete struct in the same file and module, and suppresses name-only matches when the field is known but unsafe to resolve.type_matchonly when the field’s type is a concrete, local, non-generic struct in the same file and lexical module; validates inherentimplby checking the candidate’sstart_line, file, and module scope; rejects trait and generic impls.receiver_expression, so forms likeself .engineandself. engineare detected.name_match; genuinely unknown receivers keep the fallback.Written for commit 952522a. Summary will update on new commits.
Greptile Summary
The PR improves Rust callgraph resolution for direct
self.field.method()calls while preserving conservative behavior when the field type cannot be proven.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Reviews (5): Last reviewed commit: "Clarify unresolved Rust field test names" | Re-trigger Greptile