fix(graph): bind builder method callers through typed locals (#1355) - #1361
ScriptedAlchemy wants to merge 6 commits into
Conversation
|
Emit Type::method for receiver calls when the local's type is known, keep qualified refs off the ubiquitous-name blocklist, and resolve associated methods at seal so WalkBuilder::build callers are not empty. Closes #1355 Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
34573f0 to
1194a08
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1194a08346
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Walk locals in lexical order with mut/ref unwrap, infer only from Type::new or annotations, and resolve full module paths at seal. Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
…nt/1355-false-complete-callers
Preserve source-ordered lexical binding from backup/1361-local-snapshot-59f5165102 on origin/agent/1355-false-complete-callers @ 000bf20 without force-push.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c1936c48d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let (type_path, ctor) = callee_name.rsplit_once("::")?; | ||
| if ctor != "new" || type_path.is_empty() { | ||
| return None; | ||
| } | ||
| Some(type_path.to_owned()) |
There was a problem hiding this comment.
new return types before qualifying locals
The fresh constructor_type_path implementation still assumes every Type::new() returns Type, but Rust gives new no special return-type semantics. For Factory::new() -> Product, let value = Factory::new(); value.build() emits Factory::build instead of Product::build; if Factory also has a build method, sealing records a false caller edge, and otherwise the blocklisted bare build leaves a complete-looking miss. Abstain unless the declared return type establishes Self/Type.
Useful? React with 👍 / 👎.
| let expected = format!("{type_name}::{method}"); | ||
| if relative != expected && !relative.ends_with(&format!("::{expected}")) { | ||
| return false; |
There was a problem hiding this comment.
When a constructor uses an imported alias, such as use dep::WalkBuilder as Builder; let b = Builder::new(); b.build(), extraction emits Builder::build, but this comparison rejects the real WalkBuilder::build candidate before the alias's import binding is consulted below. Consequently this common aliased form still returns no cross-file caller; use the binding's imported_name when constructing the expected owner name.
Useful? React with 👍 / 👎.
| let module_after_crate = module_prefix[1..].join("/"); | ||
| return if module_after_crate.is_empty() { | ||
| target_module.is_empty() | ||
| } else { | ||
| target_module == module_after_crate | ||
| || target_module.starts_with(&format!("{module_after_crate}/")) |
There was a problem hiding this comment.
For a valid external path like dep::a::Builder::new() where a is an inline module in dep/src/lib.rs, the target symbol is a::Builder::build but target_module is empty because it is derived only from the file path. The nonempty module_after_crate therefore rejects the candidate, so calls to builder types defined in inline modules never bind. Account for inline module segments from the target symbol's qualified path rather than requiring them to appear in the filename.
Useful? React with 👍 / 👎.
Performance Comparison
|
2 participants
Summary
Fixes #1355:
tracedecay_callersforWalkBuilder::buildreturned[]while the generation read as complete, even though other files callbuilder.build()through a typed local.Measured failure class: extractor/resolver (class 1). Bare
buildis on the ubiquitous-name blocklist; dottedbuilder.buildis dropped at retention; noWalkBuilder::buildcandidate reached seal.Fix
LocalTypeScopeframes; recordletafter the initializer; push/pop on blocks/closures).mut/refbinding patterns before recording names.Type::new(or explicit type annotations) — not arbitrary associated calls likeFactory::make().Type::method/path::Type::methodwith full module-segment validation (crate path or same-crate module).Codex P1 follow-up
Addressed review P1s on mut-only builders, shadowing/order, and
Factory::makefalse edges; P2 module-segment resolve included.Tests
cross_file_builder_method_calls_bind_through_typed_locals(mut-only fixtures)callers_of_cross_file_builder_method_include_typed_local_sitesCampaign base only (
codex/tracedecay-total-redesign-plan-reopened/ #707). Does not target master or #745.Closes #1355.
Order 25b — #1361 binding redesign hold
HOLD MERGE. Four valid review findings require redesign (not four patches):
Owner
e31ae27f: lexical scope stack + source-order walk; unwrap single-id patterns only; no constructor guessing; full module segments; tests that fail on old head. Subtract/close-as-unsafe if a large inference engine would be required.Order 25 drain — lexical redesign on head
1c1936c48d(FF from000bf2037c; no force): tipLocalTypeScoperedesign restored over 59f516 function-wide map.rust::31/31 green — Haulercc-22331.Closure decision (2026-09-16)
Closed unmerged as unsafe after two independent exact-head reviews. The +886-line local-type redesign still has five P1 semantic classes: unknown/control-flow bindings leak prior types;
let … elsealternatives are skipped;Type::new()is treated as fabricated return-type evidence; aliases/relative/inline module paths are not canonically resolved; and unanchored qualified matching can seal unrelated cross-crate methods. These defects can create convincing false graph edges, which is worse than abstaining. #1355 remains open and must be fixed through existing canonical signature/import/re-export/type authorities in a smaller PR. The branch and backup snapshot remain investigation evidence; do not merge piecemeal.