fix(code-knowledge): resolve Swift symbols across files in the same module - #847
Smilewithoutfalling wants to merge 4 commits into
Conversation
…odule Swift declarations are module-scoped, but the AST track resolved calls and conformances with a file-scoped model: declared in this file, or reachable through a resolved import. That model is correct for TS/Go/Python, where a cross-file symbol has to be imported. Swift needs no import inside a module, so a conformance or a call to a symbol in a sibling file emitted nothing. Add a third, Swift-only level of lookup after the existing two: a declaration elsewhere in the same module, with the module boundary read from the layout SwiftPM mandates (Sources/<Target>/, Tests/<Target>/). Outside that layout no scope is claimed, and a name declared more than once in the module emits nothing rather than picking one arbitrarily. Fixes Tencent#843
The PR description includes a sufficient representative real-CLI verification record, so there is no testing-documentation finding. |
Addresses the three P1 findings the automated review raised on the first revision of Tencent#847. All three were real. The fallback widened the candidate set without narrowing eligibility. The candidate set is "every declaration in the module"; what a bare name can actually reach is strictly smaller, and that has to be decided where the declaration and its modifiers are still in hand. - module-scope.ts: the module key now keeps the package root, so Packages/A/Sources/App and Packages/B/Sources/App stay two modules instead of merging into Sources/App. - walk.ts: a declaration enters the module index only when it is top-level and not private/fileprivate. Neither fact survives into AstSymbol, so it is computed in the walker and surfaced as FileWalkResult.swiftModuleSymbols. - index.ts: feeds that subset to the module index. queries.ts and the shared AstSymbol type are untouched, and the tie rule agreed in Tencent#843 is unchanged: only module-visible declarations can tie. Verification on the merge result (main a8ab8e0 + this change): - real-CLI e2e, 15/15 assertions, three new negative probes each in a file that declares nothing else; both cross-package directions stay clean while each package resolves to its own Proto.swift - four mutations, each removing one guard, each caught by exactly the matching test and nothing else - ast-swift-module-scope.test.ts 15 cases; related suite 68 -> 83 passed - oxlint --deny-warnings and tsc --noEmit: rc=0 on this change and rc=0 on unmodified main, same binary and flags, so the comparison is real
|
Findings
Resolved
Testing
|
…boundary Addresses the P1 the automated review raised on the previous head. Keeping the package root only fixes packages that sit side by side. A package vendored under the outer package's own Tests/ has two markers on its path, and the scan took the first: Tests/Fixtures/A/Sources/App and Tests/Fixtures/B/Sources/App were both scoped to Tests/Fixtures, which is the same merge as the earlier package-root finding, reached one level out. The scan now runs from the end, so the innermost marker wins. The direction also decides how a layout this function misreads can fail: an inner marker can only yield a scope nested inside the true module, which loses a resolution, while an outer one can span two real modules and fabricate an edge. The layer already prefers a missing edge to a wrong one, so the bias is deliberate and is stated in the docstring and in the tests. Verification on the merge result (main a8ab8e0 + this change): - real-CLI e2e, 20/20 assertions; both vendored fixture packages now resolve to their own Proto.swift while neither cross-package direction produces an edge - five mutations, each removing one guard, each caught by exactly the matching tests and nothing else - ast-swift-module-scope.test.ts 18 cases; related suite 68 -> 86 passed - oxlint --deny-warnings and tsc --noEmit: rc=0 here and rc=0 on unmodified main, same binary and flags
|
Findings
Resolved
Testing
|
jeff-r2026
left a comment
There was a problem hiding this comment.
Please resolve the P1 findings
…reference
Addresses the P1 the automated review raised on the previous head.
The fallback answered "which module does this bare call belong to" without
first asking whether the call is a module-level reference at all. With
A.swift declaring func work() and B.swift declaring
func run(work: () -> Void) { work() }, Swift calls the parameter, and the
lookup resolved it to A.swift and emitted a fabricated cross-file REFERENCES
edge. The receiver fallback had the same hole for a local value shadowing a
type name.
The walker now records, per call site, the names an enclosing scope binds:
function and closure parameters, local let/var, and what a
for / if let / guard let / catch let / case let introduces. Both module-wide
lookups decline when the callee or receiver is one of those names. The walk
stops at the call own ancestors, so a binding in an unrelated function of the
same file shadows nothing; it over-collects only within the scopes it does
visit, which costs a resolution rather than inventing an edge.
This is the same mistake as the earlier four findings on a different axis:
eligibility was narrowed at one end of the edge (can a sibling reach this
declaration) and not at the other (can this call reach anything module-level).
Verification on the merge result (main a8ab8e0 + this change):
- real-CLI e2e, 24/24 assertions; ShadowParam.swift (its only call bound by
its own parameter) ends with 0 outgoing edges, ShadowMixed.swift (one bound
call, one unbound) with exactly 1, and the heuristic track is byte-identical
- seven mutations, each removing one guard, each caught by exactly the matching
tests and nothing else; originals restored byte for byte
- ast-swift-module-scope.test.ts 24 cases; affected set derived from the
changed modules: 93/106 passed on unmodified main and 93/106 here
- Node 24.14.0, the runtime the matrix gained while this sat in review, on the
merge result (main 5fb316c, which carries Tencent#861): the two Swift AST test
files pass, 2/2. On this branch tree alone Node 24 aborts during the first
Swift parse (Fatal process out of memory: Zone) and a command-line
--wasm-tier-up-filter does not help, which is why Tencent#861 has to set the flag
inside the worker.
- oxlint --deny-warnings and tsc --noEmit: rc=0 here and rc=0 on unmodified
main, same binary and flags
|
Findings
Resolved
Testing
|
What this fixes
Fixes #843.
Swift declarations are module-scoped, but the AST track resolved calls and conformances with a file-scoped model: declared in this file, or reachable through a resolved import. That model is correct for TS/Go/Python, where a cross-file symbol has to be imported. Swift needs no import for a symbol in the same module, so
struct S: LocalProtoinB.swiftwithLocalProtodeclared inA.swiftemitted nothing — and the same held for a cross-file call.This gap was disclosed in #842 and split out there as its one open P1 finding. @jeff-r2026 approved this shape in #843: "Swift-only fallback after the existing lookup, and emit nothing on a tie." That is what this PR implements.
This head additionally addresses every P1 the automated review has raised on the way: three from the review of the first revision, one from the second, one from the third. All five were real; they are listed below. The fifth is the most interesting of the five, because it is not about which module a name belongs to but about whether the name is a module-level reference at all.
Change
Conformance and call resolution each gain a third level of lookup, after the two that already exist:
The module boundary is read from the layout SwiftPM mandates:
Sources/<Target>/**is one module,Tests/<Target>/**is another. Anything outside that layout yields no scope, so the resolver keeps its previous behaviour instead of inventing a boundary from an arbitrary directory tree — a fabricated edge is worse than a missing one, because the missing one still surfaces as a gap.The module key keeps the package root, and takes the innermost one. The scope of
Packages/A/Sources/App/Models.swiftisPackages/A/Sources/App, notSources/App, so two independent packages that both name a targetAppstay two modules. And a package can be vendored under a directory the outer package already namedTestsorSources—Tests/Fixtures/A/Sources/Appis package A's target, not a target of the outer package — so the marker that counts is the innermost, not the first.The module index holds only what a sibling can actually reach by name. A declaration is admitted when, and only when:
private, nofileprivate.Both facts are read from the declaration node while the walker still holds it, and surfaced as
FileWalkResult.swiftModuleSymbols(Swift only, a subset ofsymbols). They cannot be recovered later: anAstSymbolis{ id, kind, name, file, lineStart, lineEnd, exported }, and neither "is top-level" nor "isprivate" survives into it. Doing it in the query instead would need a second pass over the whole tree.A name an enclosing scope binds wins over every module-level one. The fallback answers which module a bare call belongs to; whether the call is a module-level reference at all is a prior question, and only the syntax tree can answer it:
Both call sites look identical to a resolver that only sees
fromFile,lineandcalleeText, so the walker records the enclosing bindings — function and closure parameters, locallet/var, and what afor/if let/guard let/catch let/case letintroduces — and carries them on the call site asAstCallSite.localBindings. Both module-wide lookups then decline when the callee (or the receiver) is one of those names. The walk stops at the call's own ancestors, so a binding in an unrelated function of the same file shadows nothing.src/wiki-engine/code-knowledge/ast/module-scope.tssrc/wiki-engine/code-knowledge/ast/walk.tssrc/wiki-engine/code-knowledge/ast/types.tsAstCallSite.localBindingssrc/wiki-engine/code-knowledge/ast/index.tssrc/wiki-engine/code-knowledge/ast/call-resolver.tssrc/__tests__/ast-swift-module-scope.test.tsqueries.tsand the sharedAstSymboltype are untouched, and no existing call site changes shape: the scope helper returnsundefinedfor any path that is not.swift,localBindingsis optional and absent for every other language, andresolveCallSitestakes the index as an optional argument, so existing callers are unaffected.The P1 findings from the automated review
Each was reproduced in the code before being fixed, and each now has a dedicated negative probe in the e2e fixture plus a dedicated mutation that must make its test fail.
module-scope.ts:28Sources/<Target>alone, soPackages/A/Sources/App/**andPackages/B/Sources/App/**both becameSources/App. Two unrelated packages merged into one symbol table: false cross-package edges, and — worse — false ties that suppressed a correct resolution.module-scope.ts:81private funcwas indexed, so a call in a sibling file bound to a declaration Swift says that file cannot see. The review's example — aprivate func maxshadowing the standard library — is exactly this.module-scope.ts:82kind: "function", indistinguishable from a top-level function. An unqualified call therefore bound to an unrelated method in another file; and two same-named members produced a false tie that suppressed resolution of an actual file-level function.module-scope.ts:33Tests/has two markers on its path, and taking the first scopedTests/Fixtures/A/Sources/AppandTests/Fixtures/B/Sources/Appboth toTests/Fixtures— the same merge as the first finding, reached one level out. The innermost marker is the boundary.call-resolver.ts:109,:153run(work:) { work() }calls its parameter, and the lookup resolved it to a sibling file'sfunc work(), emitting a fabricated cross-fileREFERENCESedge — the one failure mode the rest of this PR exists to avoid. The receiver fallback had the same hole for a local value shadowing a type name.The first four share one root cause: the fallback widened the candidate set without narrowing eligibility. The candidate set is "every declaration in the module"; eligibility for a bare name is a strictly smaller thing, and "which module" is a narrower thing still. Both have to be decided where the evidence still exists — the declaration's own placement and access control in the walker, and the innermost package marker in the path.
The fifth is the same shape of mistake on a different axis. Narrowing eligibility means asking "can a sibling reach this declaration", and the answer is not complete until you ask the mirror question — "can this call reach anything module-level at all". Access control is one reason it cannot; a local binding is the other. Both are properties of the two ends of the edge, and the fallback was only checking one end.
The base moved under this PR, and so did the test matrix
While this sat in review, main advanced to
5fb316c7andci.ymlgained a third matrix entry (#861):node-version: [20, 22]became[20, 22, 24]. Every green run on this PR so far was produced on the old matrix, so Node 24 had never executed this code. That is not a formality here: this PR is the first in the repo to parse Swift through tree-sitter WebAssembly, and Node 24 aborts on that parse before a single assertion runs.Measured on this branch's own tree — which does not carry #861 — with the same
node_modulesand the same two test files. Only the runtime, and the way the flag is set, differ:Fatal process out of memory: Zone— process abort--wasm-tier-up-filter=…setFlagsFromString(…)The third row is the one worth keeping: a command-line flag never reaches vitest's worker process, so "the flag did not help" is not evidence the flag is ineffective — it is exactly why #861 sets it inside the worker. #861 is in main now, so the row that decides this PR is measured on the merge result:
This PR therefore no longer depends on which ref CI checks out: the merge ref carries #861, and the runtime that used to abort passes.
Verification on this head
Everything below was measured on the merge result — current
main(a8ab8e00) with this change applied — because that is the tree CI tests, and it is deliberately not the same tree as this branch's merge base. #842 is the cautionary case: it was green on its own head, andmain'sLint & Testwent red once it merged, because the base had moved and the rule set had changed underneath it.Real CLI e2e — single-variable control
One Swift package, run through the built CLI with identical flags; the only variable is the source tree. Every probe lives in a file that declares nothing else, so a spurious edge into it cannot be masked by a legitimate one.
main12 calls (1 resolved),1 edges12 calls (5 resolved),10 edgesModels.swift→Protocols.swiftIMPLEMENTSRunner.swift→Networking.swiftREFERENCESRunner.swift→Helpers.swiftREFERENCES→ Secrets.swift(file-scoped declaration)→ Service.swift(method)ShadowParam.swift→Helpers.swift(callee bound by the caller's own parameter)ShadowParam.swiftShadowMixed.swiftPackages/A/**↔Packages/B/**Tests/Fixtures/A/**↔Tests/Fixtures/B/**…/Impl.swift→ its ownProto.swift, in all four packagesOther/UseAux.swift→SwiftDemo/*TEAMAI_SKIP_AST=1)24/24 assertions.
Three of the negative rows are decisive rather than merely green, which is why they are written as counts:
SharedProtoof the same name in a same-named file, so a merged module key — by the package root, or by the outer marker — would make each conformance ambiguous and produce no edge at all. Absence is the bug, not the fix, and the assertion catches that.ShadowMixed.swiftholds two calls tovisibleHelper(), one of them bound by its own signature. The file must end with one edge: two means the binding was ignored, zero means the binding rule was applied per file instead of per call site. A boolean assertion would have accepted both wrong answers.ShadowParam.swiftdeclares nothing else and calls nothing else, so its edge count is a direct read on the fabrication: 0 here, and the same fixture on the pre-fix head produced 1.Mutation testing
Every guard is load-bearing: removing exactly one makes exactly the matching test(s) fail, and nothing else moves. Originals are restored and compared byte for byte afterwards.
Each mutation changes one thing. (An earlier draft of M4 was discarded for changing two: pointing the fallback at
segments.slice(0, 2)on a two-segment fixture path yields the full path including the file name, giving every file its own module — nothing can resolve, so the test passed for the wrong reason.)Why mutation rather than a control tree for M6/M7. For the earlier findings a control tree worked: unmodified
mainplus the same test file, so the only variable wassrc. That cannot be done for the binding guard, because the test file importsmodule-scope.ts, which this PR adds — there is no upstream tree that can run it. The mutations are the honest substitute and a stricter one: they are the pre-fix behaviour, differing from the head by those two lines and nothing else.M6is also where an incomplete expectation nearly passed for a finding. The first list named only the four negative cases, so the fifth failure — the two-sided precision test — looked like overshoot. It is not: with the gate gone both calls resolve and the graph keeps one edge per resolved call, so the count goes to two. The expectation was incomplete, not the mutation.Tests
src/__tests__/ast-swift-module-scope.test.ts— 24 cases: cross-file conformance, cross-file call, cross-file receiver call, a symbol from a different target staying unresolved, a tie emitting nothing, a non-SPM layout not being guessed, same-file resolution unchanged, three boundary cases for the scope helper, a method / protocol requirement staying unresolved, a nested type staying unresolved, a file-scoped declaration staying unresolved, same-named targets of different packages not merging, resolution inside a nested package target, the same-named pair again for a package vendored under the outerTests/, and six cases for the binding guard (parameter, locallet,guard let, closure parameter, a receiver, and the per-call-site precision case).The negatives are built as single-variable controls: one sibling file, several calls differing only in the declaration's modifier or nesting; one layout, differing only in which package the conformance sits in; one name, several calls differing only in whether an enclosing scope binds it.
In every one of those, the shadowed call sits in its own file. Edges are file-to-file, so a shadowed call sharing a file with an unshadowed one of the same name would be masked: the resolved call supplies the very edge the unresolved one must not produce, and the assertion holds either way.
The earlier revisions of this PR quoted a hand-listed ten-file "related suite". That list was drawn from the AST work and could not have caught this head's fifth finding, which lives in
pull-adjacent territory by topic — so it is replaced by a set derived from the changed modules: every test file that mentionscode-knowledgeorextractStructuralGraphAsFacts.Static checks
I ran lint locally this time on purpose: #842 merged just after
maingainedoxlint --deny-warnings, and itsextractors/swift.tstrippedunicorn/prefer-string-starts-ends-with, leavingmainred until #846 fixed that line. Both trees above are checked with the same binary and flags so the comparison is real. Runningtscon the same two trees paid off twice on this head — it rejected the new walker helper (namedChildrenis typed(Node | null)[]in web-tree-sitter, so the child lookups need an explicit null guard), and the test runner, which strips types without checking them, was already green at that point.Known limitations (disclosed, not fixed here)
B.swiftdeclares a top-levelfunc work()and a parameter calledwork, the same-file lookup still prefers the declaration. That is pre-existing behaviour this PR does not widen, and making those levels binding-aware is a change to the resolution model rather than to the fallback — it deserves its own issue and its own verification, not a rider on this one.if letalso counts in theelsebranch it does not cover, and awhereclause sees thecase letbefore it. Each of those costs a resolution; none of them can fabricate an edge, which is the direction the asymmetry should fail in.Package.swift. A target with an explicitpath:, or any non-SPM layout, gets no scope and keeps the previous (empty) result. Two consequences worth naming:Sources/Testsis invisible to the fallback.Sources/App/Tests/Subis scoped as its own module rather than as part ofSources/App.Reading
Package.swift(or thePackage.swiftfiles already in the walk) to resolve this properly is a separate change.kind === "interface"only, so a cross-file superclass was never resolved either; the fallback deliberately keeps that behaviour identical rather than widening it here.self.member()across files stays unresolved. That needs the receiver's type, which is type inference rather than scope lookup.modifierschild.public private(set)narrows only the setter, so such a declaration stays module-visible; that is intentional.Foundationexports.Not in this PR
docs/product-overview.md,docs/usage-guide.mdand theskill-data/pages describe the AST track as resolving "imports, calls andimplementsclauses to file-to-file edges" — this change makes that promise true for Swift rather than altering it.CHANGELOG.md, whichstandard-versiongenerates.