Repository navigation
fix(rust): a call through a type a path names reaches that type's own function, not a namesake's - #3203
fix(rust): a call through a type a path names reaches that type's own function, not a namesake's#3203bob-codedaptive wants to merge 1 commit into
Conversation
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: b57434f41d
ℹ️ 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".
| segs = [...via, ...segs.slice(1)]; | ||
| head = segs[0]!; | ||
| bare = head !== 'crate' && head !== 'self' && head !== 'super'; |
There was a problem hiding this comment.
When a block-scoped import names another local import, this single substitution can produce a wrong call edge. For example, with sibling modules a and b both exposing make, use crate::a as b; { use b as a; a::make() } calls crate::a::make. The substitution instead produces b::make, and the subsequent module walk ignores the outer local binding of b and selects the sibling module's function. The parent revision resolves this example correctly, and the later Rust scope gate accepts the incorrect target. Resolve the imported head recursively in the import site's lexical scope before walking modules.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed on the published commit; the parent resolves this example correctly. Local alias heads are now expanded repeatedly at each import's lexical scope before the module walk. Two regression tests cover the chained import and a nearer binding at the call site that must not change what the earlier import names.
| // module of the file, or one a function declares for itself) does not. | ||
| const within = (n: Node): boolean => type.kind === 'trait' | ||
| ? n.filePath === type.filePath && n.startLine >= type.startLine && n.startLine <= (type.endLine ?? type.startLine) | ||
| : rustImplSelfItems(n, context)?.has(type.id) === true; |
There was a problem hiding this comment.
For a valid inherent impl such as impl<F: Fn() -> usize> Cache<F>, this newly mandatory ownership check rejects all its methods. The generic-stripping regex in rustImplSelfType treats the > in -> as the closing generic bracket, leaving usize> Cache<F> for rustPathHead, which returns null. Consequently, a previously resolved call such as crate::a::Cache::open(|| 1) loses its call edge even though the indexed method unambiguously belongs to Cache; the name-matching fallbacks do not resolve that full path. Parse the impl's generic parameters with an arrow-aware balanced-bracket reader before using the header to reject candidates.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed: the ownership check exposed the header reader's treatment of the > in ->. Impl generics now use the existing balanced reader, which skips return arrows and preserves nested generic return types. The regression test verifies both Fn() -> usize and a callback returning Option<Result<usize, ()>>.
b57434f to
fd87326
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd87326544
ℹ️ 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".
| if (rustUsesOf(ref.filePath, context).blocks.some((b) => b.name === head && b.scope !== undefined && b.parent === from.block && | ||
| b.scope.open < at && at < b.scope.close)) return null; |
There was a problem hiding this comment.
When an inner block declares mod b while an enclosing block has use crate::a as b;, b::make() names the inner module, not the import. The new alias-expansion loop runs before this guard and changes head to crate, so the guard never sees the local b and the resolver emits a false edge to a::make. The parent leaves this example unresolved. Check for a module declaration nearer than the selected import before expanding each head.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed: with use crate::a as b; in an outer block and mod b { … } in an inner one, b::make() was expanded through the import and linked to a::make. The alias expansion now stops at a head that a block mod declares nearer than the use, so the call no longer takes the import's target. __tests__/rust-path-call-edge-cases.test.ts pins it, with a control where no nearer mod exists and the import is still followed.
| const key = `${head}\0${via.at}`; | ||
| if (seen.has(key) || bound.paths.some((p) => p.segs.join('::') !== via.segs.join('::'))) return null; | ||
| seen.add(key); | ||
| segs = [...via.segs, ...segs.slice(1)]; |
There was a problem hiding this comment.
For a Rust 2021 call such as ::dep::make() with a project path dependency named dep, a block-local use crate::local as dep; must not change the callee: the leading :: selects the external crate. Because that marker was discarded before this new substitution, the call is rewritten to crate::local::make and produces a false edge to the local function instead of the dependency's function. Preserve the absolute-root information and bypass lexical alias expansion for such paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I checked this one against the code: with a path dependency dep, a block-local use crate::local as dep; and a call ::dep::make(), the PR produces no edge to the local make. The reference keeps its leading :: (::dep::make), and the resolver's existence pre-filter drops a name that starts with :: before the path resolver runs, so the alias expansion never sees it. Leading-:: Rust calls are unresolved today with or without this PR; that is an existing gap, not a false edge from the alias change.
| // module of the file, or one a function declares for itself) does not. | ||
| const within = (n: Node): boolean => type.kind === 'trait' | ||
| ? n.filePath === type.filePath && n.startLine >= type.startLine && n.startLine <= (type.endLine ?? type.startLine) | ||
| : rustImplSelfItems(n, context)?.has(type.id) === true; |
There was a problem hiding this comment.
For a valid const-generic implementation such as impl Cache<{ 1 + 1 }>, this newly mandatory ownership check rejects every associated function. readRustScopes treats the const argument's first { as the impl body's opening brace, so its recorded impl ends before the methods and rustImplSelfItems returns null. Consequently, a previously resolved call such as crate::a::Cache::<2>::open() loses its call edge, and the name-matching fallback does not recover the fully qualified path. Make the impl-body reader skip balanced const-argument blocks before enforcing this check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed: the impl-body reader took the { of a const-generic argument for the body, so the impl ended before its methods and the new ownership check dropped Cache::<2>::open(). The header scan now jumps over balanced { … } blocks inside <…>, both in the generics list and in the implementing type, and the header's type reader skips them too, so <<, >> or a comparison inside the block counts no angle bracket. __tests__/rust-path-call-edge-cases.test.ts pins { 1 + 1 } in the type and in a bound, { 1 << 1 }, { 4 >> 1 }, { 2 > 1 } and a two-argument type.
… function, not a namesake's
A call written through a type's path, `crate::a::Cache::open()`, took the
function of any type of that name in the crate: rustTypeMember matched by
qualified name and crate alone. A namesake type in another inline module of
the same file, one a function declares in its own body, and an `impl` for a
reference to the type (`impl Open for &Cache`, the reference's, not the
type's) could each be linked instead of the type the path walks to. The
walk itself also took a head wrongly: a `use` in a function's body that
binds it (`use crate::b as a;`) was ignored, and so was a `mod a { … }` the
function declares, which rustSubmodule also took for the module's own `a`
when it was written before the module's `pub mod a`.
- rustTypeMember takes a function, `const` or variant of a struct, enum or
union only from an `impl` whose header names that type (rustImplSelfItems,
walked from the module the `impl` is in); a trait's own declaration is
still the trait body's.
- rustImplSelfItems reads the `impl` header with the new rustImplSelfType,
which rustImplSelfArgs shares; a header naming a reference to the type,
or a bare name in an `impl` inside a function's body, names no type here.
- rustSubmodule skips a `mod` that sits in a function's body or another
block (RustModBlock.scope, read with rustUseScopes), which is no item of
its module.
- rustPathTarget reads a head that a `use` in the body binds as the path
that `use` names, and declines one a `mod` in the body declares.
fd87326 to
6321cfc
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6321cfcc65
ℹ️ 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".
| if (seen.has(key) || bound.paths.some((p) => p.segs.join('::') !== via.segs.join('::'))) return null; | ||
| seen.add(key); | ||
| segs = [...via.segs, ...segs.slice(1)]; | ||
| at = via.at; |
There was a problem hiding this comment.
When the first import immediately follows {, as in fn run(){use b as c; use crate::a as b; c::make()}, rustUsesOf stores the opening brace as via.at because it records the regex match's leading delimiter. This assignment therefore moves the recursive lookup outside the import's block, hiding the local binding of b. With root modules a and b both defining make, the resolver emits an incorrect edge to b::make, although Rust resolves the call to a::make; inserting whitespace after { changes the result. Use the actual use keyword offset for the recursive lookup.
Useful? React with 👍 / 👎.
| head = segs[0]!; | ||
| bare = head !== 'crate' && head !== 'self' && head !== 'super'; | ||
| if (!bare) break; | ||
| bound = rustVisibleUse(head, ref.filePath, at, context); |
There was a problem hiding this comment.
For a valid local import such as fn run() { use b; b::make(); }, where b is a project module, expanding the import leaves the head as b, and this lookup selects the exact same import again. The next iteration hits seen and returns null, dropping a call edge that the parent resolves and the name-matching fallbacks do not recover. Resolve the imported head without re-selecting the binding currently being expanded, rather than treating this self-named import as an alias cycle.
Useful? React with 👍 / 👎.
| // module of the file, or one a function declares for itself) does not. | ||
| const within = (n: Node): boolean => type.kind === 'trait' | ||
| ? n.filePath === type.filePath && n.startLine >= type.startLine && n.startLine <= (type.endLine ?? type.startLine) | ||
| : rustImplSelfItems(n, context)?.has(type.id) === true; |
There was a problem hiding this comment.
For valid bounds such as impl<T: Into<[u8; 4]>> Cache<T>, readRustScopes never records the impl: its angleClose helper stops at the array type's semicolon even though it is nested inside [...]. The new mandatory rustImplSelfItems check therefore discards every method from that impl, so a previously resolved call such as crate::a::Cache::<[u8; 4]>::open() loses its call edge. The generic reader needs to skip balanced array brackets before it can serve as this hard ownership gate.
Useful? React with 👍 / 👎.
#3025 made
Type::membercalls go through the module walk, but the member lookup (rustTypeMember) still matched a type's function by its qualified name and crate alone, and a few heads the walk read wrongly. The result was a wrong link, which is worse than none, in six shapes:crate::a::Cache::open()tookb::Cache::openwhena::Cachehas only a trait's providedopen.fn f() { struct Cache; impl Cache { fn open() … } }) took the call to the module-levelCache::open.implfor a reference to the type (impl Open for &Cache { fn open() … }) is the reference's, notCache's, and was linked asCache::open.usein the function's body that binds the path's head (use crate::b as a; a::Cache::open()) was ignored, so the call went to the moduleabeside it.mod a { … }the function declares in its body was ignored the same way.mod aof its own before the module'spub mod amaderustSubmoduletake the function'safor the module's, because it took the firstmod ain source order.What changes:
rustTypeMembertakes a struct, enum or union's function,constor variant only from animplwhose header names that type, walked from the module theimplis written in (rustImplSelfItems). Animplin another file than its type, by path or through ause, still counts. A trait's own declaration is still the trait body's.rustImplSelfTypeis the header reader thatrustImplSelfArgsshares; its balanced generic reader preserves callback bounds such asimpl<F: Fn() -> usize> Cache<F>, including nested return types.rustSubmoduleignores amodwritten in a function's body or another block (RustModBlock.scope, found with the existingrustUseScopes); it is no item of the module around it.rustPathTargetreads a head that ausein the body binds as the path thatusenames, following further local aliases at each import's scope rather than the call's scope, and returns nothing for a head amodin the body declares. This applies to module paths (a::make()) as well as type paths. Amoddeclared in a block nearer than theuse(use crate::a as b; { mod b { … } b::make() }) is what the head names, so the import is not expanded there.readRustScopesreads animplheader past a const-generic block argument, both in the generics list (impl<T: Bound<{ 1 + 1 }>> Cache<T>) and in the implementing type (impl Cache<{ 1 + 1 }> {): the block's braces are neither the end of the generics nor the body, so the impl spans its methods andcrate::a::Cache::<2>::open()keeps its edge under the header check above. The header's type reader (rustAngleEnd) likewise skips a balanced{ … }, so a block holding<<,>>or a comparison (Cache<{ 1 << 1 }>,Flag<{ 2 > 1 }>,Pair<{ 1 << 1 }, u8>) counts no angle bracket.Tests:
npx vitest run __tests__/rust-path-call-edge-cases.test.ts: all 17 cases pass.npx tsc --noEmit -p .passes.Seven of the eight cases added last fail against the previous revision of this PR: the nearer block
mod(the call went toa::make) and the six const-generic impls (no edge). The eighth, the same import with no nearermod, passes before and after.Three cases pin what an earlier revision of this PR got wrong: chained local aliases selected the sibling module's function in both lexical-scope cases, and the callback-bound impl lost its method edge. With only the resolver restored to parent 47a9c7d, all three pass (6 other tests filtered out), so those faults came from the PR's own change. The six original cases still pass with the fix.
Only this test file and the TypeScript typecheck were run, not the broader suites or the native kernel.
Not measured: the cost of
rustImplSelfItemsper candidate on a large Rust repository. It runs once per (type, member) pair, after the name and crate filters, and reads the scopes of the candidate's file (cached).Testing
🤖 Generated with Claude Code