Skip to content

fix(code-knowledge): resolve Swift symbols across files in the same module - #847

Open
Smilewithoutfalling wants to merge 4 commits into
Tencent:mainfrom
Smilewithoutfalling:fix/843-swift-module-scope
Open

Smilewithoutfalling wants to merge 4 commits into
Tencent:mainfrom
Smilewithoutfalling:fix/843-swift-module-scope

Conversation

@Smilewithoutfalling

@Smilewithoutfalling Smilewithoutfalling commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

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: LocalProto in B.swift with LocalProto declared in A.swift emitted 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:

  1. same file — unchanged;
  2. resolved imports — unchanged;
  3. new, Swift-only — a declaration elsewhere in the same module.

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.swift is Packages/A/Sources/App, not Sources/App, so two independent packages that both name a target App stay two modules. And a package can be vendored under a directory the outer package already named Tests or Sources — Tests/Fixtures/A/Sources/App is 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:

  • it is top-level — not a method, not a protocol requirement, not a type nested inside another type; and
  • it is not file-scoped — no private, no fileprivate.

Both facts are read from the declaration node while the walker still holds it, and surfaced as FileWalkResult.swiftModuleSymbols (Swift only, a subset of symbols). They cannot be recovered later: an AstSymbol is { id, kind, name, file, lineStart, lineEnd, exported }, and neither "is top-level" nor "is private" 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:

// A.swift
func work() -> Int { return 1 }

// B.swift
func run(work: () -> Int) -> Int { return work() }   // the parameter, not A.swift
func plain() -> Int { return work() }                // this one really is A.swift

Both call sites look identical to a resolver that only sees fromFile, line and calleeText, so the walker records the enclosing bindings — function and closure parameters, local let/var, and what a for / if let / guard let / catch let / case let introduces — and carries them on the call site as AstCallSite.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.

file
src/wiki-engine/code-knowledge/ast/module-scope.ts new — the scope rule and the module-wide lookup
src/wiki-engine/code-knowledge/ast/walk.ts compute per-declaration module eligibility, and the enclosing bindings per call site, while the node is in hand
src/wiki-engine/code-knowledge/ast/types.ts one optional field: AstCallSite.localBindings
src/wiki-engine/code-knowledge/ast/index.ts build the index once; feed it the module-visible subset; add the fallback to the conformance loop
src/wiki-engine/code-knowledge/ast/call-resolver.ts add the fallback to both call shapes (free and receiver), gated on the bindings
src/__tests__/ast-swift-module-scope.test.ts new — 24 cases

queries.ts and the shared AstSymbol type are untouched, and no existing call site changes shape: the scope helper returns undefined for any path that is not .swift, localBindings is optional and absent for every other language, and resolveCallSites takes 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.

finding verdict what was actually wrong
P1 Preserve the package root in the module key — module-scope.ts:28 real The key was Sources/<Target> alone, so Packages/A/Sources/App/** and Packages/B/Sources/App/** both became Sources/App. Two unrelated packages merged into one symbol table: false cross-package edges, and — worse — false ties that suppressed a correct resolution.
P1 Exclude file-scoped declarations from module lookup — module-scope.ts:81 real A top-level private func was indexed, so a call in a sibling file bound to a declaration Swift says that file cannot see. The review's example — a private func max shadowing the standard library — is exactly this.
P1 Do not treat every extracted function as module-level — module-scope.ts:82 real Swift methods and protocol requirements are extracted as kind: "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.
P1 Use the SwiftPM marker of the nested package, not the first one — module-scope.ts:33 real 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 taking the first scoped Tests/Fixtures/A/Sources/App and Tests/Fixtures/B/Sources/App both to Tests/Fixtures — the same merge as the first finding, reached one level out. The innermost marker is the boundary.
P1 Respect local bindings before the module-wide fallback — call-resolver.ts:109, :153 real The fallback claimed a name an enclosing scope had already bound. run(work:) { work() } calls its parameter, and the lookup resolved it to a sibling file's func work(), emitting a fabricated cross-file REFERENCES edge — 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 5fb316c7 and ci.yml gained 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_modules and the same two test files. Only the runtime, and the way the flag is set, differ:

runtime how the flag is set result
Node 22.22.2 — 2 files, 32 tests, pass
Node 24.14.0 none Fatal process out of memory: Zone — process abort
Node 24.14.0 command line --wasm-tier-up-filter=… still aborts
Node 24.14.0 in-process setFlagsFromString(…) 2 files, 32 tests, pass

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:

main@5fb316c7 + this change, Node 24.14.0, the two Swift AST test files
  Test Files  2 passed (2)

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, and main's Lint & Test went 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.

Sources/SwiftDemo/Protocols.swift   protocol Describable, Persistent, DemoProto
Sources/SwiftDemo/Models.swift      struct Point: Persistent              <- cross-file conformance
Sources/SwiftDemo/Networking.swift  func makeRequest(), class Client
Sources/SwiftDemo/Runner.swift      calls Client(), makeRequest(), and the probes below
Sources/SwiftDemo/Helpers.swift     func visibleHelper()                  <- probe: top-level, public
Sources/SwiftDemo/Secrets.swift     private func hiddenHelper()           <- probe: file-scoped
Sources/SwiftDemo/Service.swift     struct Service { func handle() }      <- probe: method
Sources/SwiftDemo/ShadowParam.swift  func shadowParam(visibleHelper:)     <- probe: the callee is bound here
Sources/SwiftDemo/ShadowMixed.swift  one bound + one unbound call to the same name
Sources/Other/Aux.swift             protocol AuxProto
Sources/Other/UseAux.swift          struct UsesDemoProto: DemoProto       <- cross target, must NOT resolve
Packages/A/Sources/App/{Proto,Impl}.swift   two packages with a same-named target
Packages/B/Sources/App/{Proto,Impl}.swift   and a same-named protocol, on purpose
Tests/Fixtures/A/Sources/App/{Proto,Impl}.swift   the same two packages, vendored
Tests/Fixtures/B/Sources/App/{Proto,Impl}.swift   under the outer package's own Tests/
unmodified main this branch
AST track 12 calls (1 resolved), 1 edges 12 calls (5 resolved), 10 edges
Facts 66 (relation 25) 75 (relation 34)
Graph 34 nodes, 7 edges 34 nodes, 15 edges
Models.swift → Protocols.swift IMPLEMENTS absent present
Runner.swift → Networking.swift REFERENCES absent present
Runner.swift → Helpers.swift REFERENCES absent present
→ Secrets.swift (file-scoped declaration) absent absent
→ Service.swift (method) absent absent
ShadowParam.swift → Helpers.swift (callee bound by the caller's own parameter) absent absent
outgoing edges from ShadowParam.swift 0 0
outgoing edges from ShadowMixed.swift 0 exactly 1
Packages/A/** ↔ Packages/B/** absent absent
Tests/Fixtures/A/** ↔ Tests/Fixtures/B/** absent absent
each …/Impl.swift → its own Proto.swift, in all four packages absent present (four)
Other/UseAux.swift → SwiftDemo/* absent absent (targets not bridged)
heuristic track (TEAMAI_SKIP_AST=1) 6 edges 6 edges, byte-identical

24/24 assertions.

Three of the negative rows are decisive rather than merely green, which is why they are written as counts:

  • The two cross-package pairs each have both packages declaring a SharedProto of 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.swift holds two calls to visibleHelper(), 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.swift declares 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.

M1  module key falls back to the Sources/<Target> tail
      -> keeps the package root in the module boundary
         does not merge same-named targets of different packages
         takes the innermost marker, so a package vendored under Tests/ keeps its own root
         does not merge two packages vendored under the same Tests directory
M2  top-level restriction removed
      -> does not resolve a method or a protocol requirement from another file
         does not resolve a type nested inside another file
M3  file-scoped restriction removed
      -> does not resolve a file-scoped declaration from another file
M4  layout restriction removed
      -> does not guess a module outside a SwiftPM layout
         refuses to invent a module where the layout states none
M5  innermost-marker rule reverted (the scan takes the first marker again)
      -> takes the innermost marker, so a package vendored under Tests/ keeps its own root
         does not merge two packages vendored under the same Tests directory
M6  free-call binding gate removed
      -> does not resolve a call to a parameter of the enclosing function
         does not resolve a call to a local binding
         does not resolve a call to a guard binding, which is a sibling statement
         does not resolve a call to a closure parameter
         still resolves when the binding belongs to a different function of the same file
M7  receiver binding gate removed
      -> does not resolve a receiver that an enclosing scope binds

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 main plus the same test file, so the only variable was src. That cannot be done for the binding guard, because the test file imports module-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.

M6 is 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 outer Tests/, and six cases for the binding guard (parameter, local let, 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 mentions code-knowledge or extractStructuralGraphAsFacts.

affected set (13 files, derived from the changed modules)
  unmodified main   93/106 passed
  this branch       93/106 passed        no regression, no expectation moved
  the 13 red-on-both are environment-bound (offline-enrich fixture, unix permissions)
  and identical on both trees
new test file       24/24 on this branch

Static checks

oxlint --deny-warnings --report-unused-disable-directives
  unmodified main        rc=0  Found 0 warnings and 0 errors   (675 files)
  this branch            rc=0  Found 0 warnings and 0 errors   (677 files)
  main@5fb316c7 + this   rc=0  Found 0 warnings and 0 errors   (677 files)

tsc --noEmit
  unmodified main        rc=0  0 diagnostics
  this branch            rc=0  0 diagnostics
  main@5fb316c7 + this   rc=0  0 diagnostics

I ran lint locally this time on purpose: #842 merged just after main gained oxlint --deny-warnings, and its extractors/swift.ts tripped unicorn/prefer-string-starts-ends-with, leaving main red until #846 fixed that line. Both trees above are checked with the same binary and flags so the comparison is real. Running tsc on the same two trees paid off twice on this head — it rejected the new walker helper (namedChildren is 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)

  • The binding rule applies to the module-wide fallback only. The same-file and resolved-import levels are still position-blind, exactly as they were before this PR: if B.swift declares a top-level func work() and a parameter called work, 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.
  • The binding set over-collects inside the scopes it visits. A binding introduced by an if let also counts in the else branch it does not cover, and a where clause sees the case let before it. Each of those costs a resolution; none of them can fabricate an edge, which is the direction the asymmetry should fail in.
  • The module boundary is still a layout inference. It reads the path shape, not Package.swift. A target with an explicit path:, or any non-SPM layout, gets no scope and keeps the previous (empty) result. Two consequences worth naming:
    • A package whose target directory is not under Sources/Tests is invisible to the fallback.
    • The innermost-marker rule is biased towards under-scoping: a marker this function mistakes for a package root can only yield a scope nested inside the true module, which loses a resolution, never one that spans two real modules, which would fabricate an edge. Sources/App/Tests/Sub is scoped as its own module rather than as part of Sources/App.
      Reading Package.swift (or the Package.swift files already in the walk) to resolve this properly is a separate change.
  • Only protocol targets resolve for conformance. The same-file branch already matched 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.
  • Eligibility is read from the declaration's own modifiers child. public private(set) narrows only the setter, so such a declaration stays module-visible; that is intentional.
  • Uniqueness is checked against the module's own visible declarations only. A name that also exists in a framework is not distinguished, because the repo cannot see what Foundation exports.
  • Symbols in non-SPM Swift files are invisible to the fallback. They keep the old per-file behaviour; nothing regresses, but nothing improves either.

Not in this PR

  • No documentation change. docs/product-overview.md, docs/usage-guide.md and the skill-data/ pages describe the AST track as resolving "imports, calls and implements clauses to file-to-file edges" — this change makes that promise true for Swift rather than altering it.
  • No change to CHANGELOG.md, which standard-version generates.

…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
@github-actions

Copy link
Copy Markdown
  • [P1 blocking] Preserve the package root in the module key — src/wiki-engine/code-knowledge/ast/module-scope.ts:28. Both Packages/A/Sources/App/... and Packages/B/Sources/App/... become Sources/App, so same-named targets in separate nested Swift packages are merged and can produce false cross-package edges.

  • [P1 blocking] Exclude file-scoped declarations from module lookup — src/wiki-engine/code-knowledge/ast/module-scope.ts:81. The index ignores Swift access control. For example, a private func max in A.swift causes a standard-library max() call in B.swift to resolve to A.swift, although that declaration is not visible outside its file.

  • [P1 blocking] Do not treat every extracted function as module-level — src/wiki-engine/code-knowledge/ast/module-scope.ts:82. Swift methods and protocol requirements are currently stored as kind: "function", so an unqualified call in one type can resolve to an unrelated method in another file. They can also create false ties that suppress resolution of an actual file-level function.

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
@github-actions

Copy link
Copy Markdown

Findings

  • [P1 blocking] Use the SwiftPM marker associated with the nested package, not the first Sources/Tests segment — src/wiki-engine/code-knowledge/ast/module-scope.ts:33. For example, Tests/Fixtures/A/Sources/App/Proto.swift and Tests/Fixtures/B/Sources/App/Model.swift both receive the scope Tests/Fixtures, merging two fixture packages and producing false cross-package resolutions or false ambiguity. This recreates the package-boundary failure whenever a package is stored beneath a directory named Tests or Sources.

Resolved

  • The three findings from the earlier review are otherwise resolved: ordinary nested package roots remain distinct, file-scoped declarations are excluded, and non-top-level declarations are excluded.

Testing

  • The PR description includes a sufficient representative real-CLI verification record.

…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
@github-actions

Copy link
Copy Markdown

Findings

  • [P1 blocking] Respect local bindings before the module-wide call fallback — src/wiki-engine/code-knowledge/ast/call-resolver.ts:109. With A.swift declaring func work() and B.swift declaring func run(work: () -> Void) { work() }, Swift calls the parameter, but this lookup resolves it to A.swift and emits a fabricated cross-file REFERENCES edge. The receiver fallback at line 153 has the same issue when a local value shadows a type name.

Resolved

  • All four findings from the earlier review passes are resolved in the current diff.

Testing

  • The PR description includes a sufficient representative real-CLI verification record.

@jeff-r2026 jeff-r2026 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@github-actions

Copy link
Copy Markdown

Findings

  • [P1 blocking] Treat enclosing instance properties as bindings before module fallback — src/wiki-engine/code-knowledge/ast/walk.ts:318. With A.swift declaring func work() and B.swift containing struct S { let work: () -> Void; func run() { work() } }, Swift calls self.work, but the collector records no binding for the stored property and emits a fabricated B.swift → A.swift edge.
  • [P1 blocking] Include generic parameters in receiver shadowing — src/wiki-engine/code-knowledge/ast/walk.ts:319. For func run<Factory: Maker>() { Factory.make() }, a sibling top-level type named Factory is selected by the fallback even though Swift resolves the receiver to the generic parameter, producing a false cross-file edge.
  • [P1 blocking] Collect initialized closure-capture names — src/wiki-engine/code-knowledge/ast/walk.ts:330. In { [work = makeWork()] in work() }, work is introduced by the capture list, but the lambda branch examines only parameters; a sibling func work() is therefore incorrectly recorded as the call target.

Resolved

  • The exact findings from all earlier passes are resolved in the current diff, including parameter/local-variable shadowing.

Testing

  • The PR description includes a sufficient representative real-CLI verification record.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Swift AST: cross-file IMPLEMENTS/REFERENCES edges are dropped (module-scope symbols need no import)

2 participants