trending/teamai-cli
1
0
Fork 0
mirror of https://github.com/Tencent/teamai-cli.git synced 2026-10-02 03:14:40 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix(code-knowledge): collect Swift shadowing once per declaration, for any identifier

Browse Source 
Two follow-ups from review, both in the same set of names.

The character class guarding `addSwiftName` admitted Unicode letters after the
previous pass but still rejected escaped identifiers such as `repeat` -- which
Swift requires when a name collides with a keyword -- and symbol or emoji
names. A parameter with such a name never reached `localBindings`, so a sibling
`func` of the same name won the fallback and a fabricated cross-file edge was
emitted. Every caller passes the text of a `simple_identifier` or a
`type_identifier`, which is the grammar's own verdict that the token is a name,
so the class was the whole defect and nothing else is tested there now.

The set was also rebuilt for every call by re-walking that call's ancestors.
The union over those ancestors is exactly the top-level declaration that holds
the call -- every ancestor sits inside it, and the declaration is one of them --
so the declaration is now read once per file and each call site looks its answer
up. Same names, minus the quadratic term: a function holding N calls went from
roughly N^2 AST visits to a single traversal per declaration.

Reading the declaration whole is a shade coarser than the per-call walk: a name
occurring only inside the call now counts as well. That costs nothing where the
field is consumed, because the resolver asks about the callee and the receiver,
and both are callee positions, skipped below as uses. A name can only enter the
set through a position that is not the very call it would resolve.

Verification against the pushed head 891da0826b, same fixtures, same runtime:

- 16-shape before/after matrix (5 shadowed shapes, 2 of the dropped identifiers,
  2 scope cases, 7 controls). The only rows that move are the two identifier
  shapes, each 1 fabricated edge -> 0. Every other row is identical, including
  the controls for those two shapes, which still resolve.
- The two new identifier cases fail on the unmodified head (2 failed | 31 passed
  of 33) and pass here (33/33), so they pin the defect rather than the change.
- Five mutations, each removing one guard: reinstating the ASCII shape test,
  keeping the callee as evidence, scoping to the nearest function instead of the
  declaration, looking the declaration up by endIndex, collecting type
  positions. All five are killed; walk.ts restored byte for byte afterwards.
- ast-swift-module-scope.test.ts 29 -> 33 cases. The four AST suites: 65 passed.
- The quadratic term, measured on one function holding the same call N times:
  200 -> 800 calls is 477 ms -> 8428 ms on the head (x17.7, and it overruns the
  15 s test timeout) against 12.6 ms -> 29.5 ms here.
- oxlint --deny-warnings --report-unused-disable-directives and tsc --noEmit:
  rc=0. No reference to the two helpers removed here is left in src/.
This commit is contained in:
Smilewithoutfalling
2026-09-29 21:12:12 +08:00
parent 891da0826b
commit da6f894e68
3 changed files with 175 additions and 87 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/__tests__/ast-swift-module-scope.test.ts
+65
View File
@@ -510,4 +510,69 @@ describe('Swift module-scope resolution yields to enclosing bindings', () => {
// the parameter and lets the sibling `func π()` win the fallback.
expect(result.edges.filter((e) => e.relation === 'REFERENCES')).toHaveLength(0);
});
it('recognises a binding whose name has to be escaped', async () => {
const { result } = await extractFiles([
['Sources/App/Worker.swift', 'func `repeat`() -> Int { return 1 }\n'],
['Sources/App/Shadowed.swift', 'func shadowed(`repeat`: () -> Int) -> Int {\n return `repeat`()\n}\n'],
['Sources/App/Plain.swift', 'func plain() -> Int {\n return `repeat`()\n}\n'],
]);
// A name that collides with a keyword is written between backticks, and the
// grammar reports it that way on both sides — the declaration and the call
// carry the same token, so leaving the escaped form out of the set is the
// only thing that breaks the pair.
const references = result.edges.filter((e) => e.relation === 'REFERENCES');
expect(references).toHaveLength(1);
expect(references[0]?.from).toBe('Sources/App/Plain.swift');
expect(references[0]?.to).toBe('Sources/App/Worker.swift');
});
it('recognises a binding whose name is a symbol', async () => {
// U+1F680, written as an escape so the case stays readable in an editor.
const rocket = '\u{1F680}';
const { result } = await extractFiles([
['Sources/App/Worker.swift', `func ${rocket}() -> Int { return 1 }\n`],
['Sources/App/Shadowed.swift', `func shadowed(${rocket}: () -> Int) -> Int {\n return ${rocket}()\n}\n`],
['Sources/App/Plain.swift', `func plain() -> Int {\n return ${rocket}()\n}\n`],
]);
// Swift admits symbol names, emoji included. A class built out of Unicode
// *letters* still excludes every one of them.
const references = result.edges.filter((e) => e.relation === 'REFERENCES');
expect(references).toHaveLength(1);
expect(references[0]?.from).toBe('Sources/App/Plain.swift');
expect(references[0]?.to).toBe('Sources/App/Worker.swift');
});
it('resolves every call of a declaration that binds the name nowhere', async () => {
const body = Array.from({ length: 12 }, () => ' work()').join('\n');
const { result } = await extractFiles([
['Sources/App/Worker.swift', 'func work() -> Int { return 1 }\n'],
['Sources/App/Many.swift', `func run() -> Int {\n${body}\n return 0\n}\n`],
]);
// The shadowing set is read off the declaration once and handed to every
// call inside it. Nothing binds `work` here, so all twelve calls resolve --
// a set looked up for the wrong declaration, or emptied after the first
// call, would show up as a shortfall rather than as a wrong edge.
const references = result.edges.filter((e) => e.relation === 'REFERENCES');
expect(references).toHaveLength(12);
for (const reference of references) expect(reference.to).toBe('Sources/App/Worker.swift');
});
it('does not let a type position stand in for a binding', async () => {
const { result } = await extractFiles([
['Sources/App/Factory.swift', 'func Logger() -> Int { return 1 }\n'],
['Sources/App/Use.swift', 'func run(logger: Logger) -> Int {\n return Logger()\n}\n'],
]);
// `Logger` in the parameter list is a *type*, and only a value binding can
// shadow a call. Counting the type position would suppress the resolution
// and drop the edge to the sibling function that actually answers for it.
const references = result.edges.filter((e) => e.relation === 'REFERENCES');
expect(references).toHaveLength(1);
expect(references[0]?.from).toBe('Sources/App/Use.swift');
expect(references[0]?.to).toBe('Sources/App/Factory.swift');
});
});
src/wiki-engine/code-knowledge/ast/types.ts
+12 -3
View File
@@ -29,15 +29,24 @@ export interface AstCallSite {
calleeText: string;
receiver?: string;
/**
* Swift only. The names bound by an enclosing scope at this call site:
* Swift only. The names that could bind this call's callee (or receiver),
* gathered from the top-level declaration that encloses the call site:
* function and closure parameters, local `let`/`var`, and whatever a
* `for` / `if let` / `guard let` / `catch let` / `case let` introduces.
* `for` / `if let` / `guard let` / `catch let` / `case let` introduces, plus
* stored properties, generic parameters and closure capture lists.
*
* A call whose callee (or receiver) is one of these names refers to that
* binding, so a module-wide lookup must not claim it: `run(work:) { work() }`
* calls the parameter, not a sibling file's `func work()`. Only the syntax
* tree knows this, which is why it is recorded here and not recomputed in the
* resolver. Absent for non-Swift files and for sites with no such binding.
* resolver.
*
* Collected per declaration rather than per call site, so it is deliberately
* coarse: the declaration is read whole, and a name that merely occurs in it —
* passed around as an argument rather than bound — counts. Both directions of
* error are not equal here: an extra name costs a resolution, a missing one
* invents a cross-file edge. Absent for non-Swift files and for sites with no
* such binding.
*/
localBindings?: string[];
resolvedTargetId?: string;
src/wiki-engine/code-knowledge/ast/walk.ts
+98 -84
View File
@@ -70,6 +70,10 @@ export function walkFile(file: CodeCollectedFile): FileWalkResult {
try {
const query = getQuery(variant);
const exportLineStarts = collectExportLineStarts(variant, tree.rootNode);
// One traversal per file, shared by every call site below: doing this per
// call would re-walk the enclosing declaration once for each of its calls.
const swiftShadowedNames =
variant === "swift" ? buildSwiftShadowedNames(tree.rootNode) : undefined;
for (const match of query.matches(tree.rootNode)) {
const byName = new Map(match.captures.map((c) => [c.name, c.node]));
@@ -129,7 +133,8 @@ export function walkFile(file: CodeCollectedFile): FileWalkResult {
const receiver = byName.get("call.receiver")?.text;
const member = byName.get("call.member")?.text;
const calleeText = callee ?? (receiver && member ? `${receiver}.${member}` : callNode.text);
const localBindings = variant === "swift" ? swiftLocalBindingsAt(callNode) : [];
const localBindings =
swiftShadowedNames === undefined ? [] : swiftShadowedNamesAt(callNode, swiftShadowedNames);
callSites.push({
fromFile: file.relativePath,
line,
@@ -215,21 +220,25 @@ function namedChildrenOf(node: Node): Node[] {
return node.namedChildren.filter((child): child is Node => child !== null);
}
// Swift identifiers admit any Unicode letter, so an ASCII-only class would drop
// a parameter named `π` and let a sibling `func π()` win the fallback.
const SWIFT_IDENTIFIER = /^[\p{L}_][\p{L}\p{N}_]*$/u;
function addSwiftName(name: string, names: Set<string>): void {
// `_` is the "no internal name" placeholder, not a binding.
if (name === "_" || !SWIFT_IDENTIFIER.test(name)) {
return;
// `_` is the "no internal name" placeholder, not a binding. Nothing else is
// tested here: every caller passes the text of a `simple_identifier` or a
// `type_identifier`, which is the grammar's own verdict that the token is a
// name, so there is no shape left to check. The character class that used to
// guard this was the defect — widened to Unicode letters it still dropped
// escaped identifiers such as `` `repeat` `` (which Swift requires when a name
// collides with a keyword) and symbol or emoji names, so a parameter with such
// a name never reached `localBindings` and a same-named sibling function won
// the fallback.
if (name !== "_") {
names.add(name);
}
names.add(name);
}
/**
* The names that stop `node`'s callee from resolving through Swift module scope.
* The names in one top-level declaration that stop a call inside it from
* resolving through Swift module scope.
*
* `call-resolver` decides *which module* a bare call belongs to, but only the
* syntax tree knows whether the callee is genuinely a module-level declaration:
@@ -239,92 +248,97 @@ function addSwiftName(name: string, names: Set<string>): void {
* names are gathered here — while the tree is still in hand — and carried on the
* call site.
*
* The scopes that can shadow the name are the call's own ancestors, which is a
* closed set: nothing binds a name without appearing somewhere on that chain.
* What changed is that no code enumerates *kinds* of scope any more. The earlier
* version switched on `scope.type` and pulled named fields out of the handful of
* node types it recognised, while the doc comment above it promised to
* over-collect. That promise only ever held "inside the scopes it does visit",
* so every binding form the switch did not know — an instance property, a
* generic parameter, a closure capture list, `if let` without `else`, `catch
* let`, `case let` — stayed a live bug, and there was always one more. Reading
* whole scopes instead of their recognised fields closes that door: every way of
* introducing a name puts the name in the tree.
* The question asked is deliberately *closed*: does the name occur anywhere else
* in this declaration? Every construct that can introduce a name — a parameter, a
* local `let`, `if let`, `guard let`, `for … in`, `catch let`, `case let`, a
* stored property, a generic parameter, a closure capture list — puts that name
* somewhere else in the declaration, whether or not this walk understands the
* construct. Enumerating the constructs instead is precisely what makes a
* whitelist the defect: there is always one more binding form, and that was the
* shape of every review round this file has had.
*
* The cost is paid on the other side, deliberately. A name that merely *appears*
* in an enclosing scope — a nested function's local, a value passed around
* rather than bound — suppresses the resolution too. Over-suppressing costs a
* in the declaration — a value passed around rather than bound, a sibling
* statement's local — suppresses the resolution too. Over-suppressing costs a
* resolution; the opposite error invents an edge, and that asymmetry is the
* point.
*/
function swiftLocalBindingsAt(node: Node): string[] {
const names = new Set<string>();
const call = enclosingCallExpression(node);
const start = call.startIndex;
const end = call.endIndex;
const collect = (current: Node): void => {
// The call is not evidence about itself: `work()` contains `work`, and
// counting that occurrence would suppress every cross-file resolution.
if (current.startIndex >= start && current.endIndex <= end) {
return;
}
if (current.type === "simple_identifier" || current.type === "type_identifier") {
addSwiftName(current.text, names);
return;
}
// A type position such as `Int` in `(work: Int)` names a type, not a value;
// collecting it would shadow every call to a same-named function. A generic
// parameter is a `type_identifier` too, but it never sits under `user_type`,
// so it still lands in the set.
if (current.type === "user_type") {
return;
}
// The name a *call* goes through is a use, not a binding. Without this,
// `work(); work()` has each call count the other one's identifier as
// evidence, and both cross-file resolutions are suppressed. Arguments are
// still walked, so a closure that does bind a name — `handler { work in
// work() }` — keeps counting.
if (current.type === "call_expression") {
for (const child of namedChildrenOf(current)) {
if (child.type === "navigation_expression" || child.type === "simple_identifier") {
continue;
}
collect(child);
function collectSwiftShadowedNames(node: Node, names: Set<string>): void {
if (node.type === "simple_identifier" || node.type === "type_identifier") {
addSwiftName(node.text, names);
return;
}
// A type position such as `Int` in `(work: Int)` names a type, not a value;
// collecting it would shadow every call to a same-named function. A generic
// parameter is a `type_identifier` too, but it never sits under `user_type`,
// so it still lands in the set.
if (node.type === "user_type") {
return;
}
// The name a *call* goes through is a use, not a binding. Without this,
// `work(); work()` has each call count the other one's identifier as evidence,
// and both cross-file resolutions are suppressed. Arguments are still walked,
// so a closure that does bind a name — `handler { work in work() }` — keeps
// counting.
if (node.type === "call_expression") {
for (const child of namedChildrenOf(node)) {
if (child.type === "navigation_expression" || child.type === "simple_identifier") {
continue;
}
return;
}
for (const child of namedChildrenOf(current)) {
collect(child);
collectSwiftShadowedNames(child, names);
}
};
// Up to but not including the file: the module level is what the fallback
// resolves *against*, so it cannot also be what shadows the name.
let scope: Node | null = call.parent;
while (scope && scope.type !== "source_file") {
collect(scope);
scope = scope.parent;
return;
}
for (const child of namedChildrenOf(node)) {
collectSwiftShadowedNames(child, names);
}
return [...names];
}
/**
* The `call_expression` a captured node belongs to.
* The shadowing names of every top-level declaration in a file.
*
* The Swift query binds `@call.member` twice — once to the outer
* `call_expression`, once to the `simple_identifier` in its `navigation_suffix`.
* A capture map keyed by name keeps the last one, so the node handed to
* `swiftLocalBindingsAt` is often just the member identifier (`make` in
* `Service.make()`) rather than the whole call. Walking up is what makes the
* "inside the call" test exact: excluding only the identifier would leave the
* receiver (`Service`) looking like an ordinary mention of the name.
* Built once per file, not once per call. The ancestors a call could be shadowed
* by are the scopes between it and the file, and their union is exactly the
* top-level declaration that holds the call — so reading that declaration whole
* yields the same names, at one traversal per declaration instead of a subtree
* walk per call. The per-call version made a function holding N calls cost
* O(N²) AST visits.
*
* The set is a shade coarser than the per-call walk: a name occurring *only*
* inside the call — an argument, a closure body — now counts as well. That costs
* nothing where the field is consumed, because the resolver asks about the callee
* and the receiver, and callee positions are skipped below: a name can only reach
* this set through a position that is not the very call it would resolve.
*
* Keyed by `startIndex`: top-level declarations do not overlap, and tree-sitter
* hands out a fresh wrapper on every navigation, so node identity is not
* something a `Map` can be built on.
*/
function enclosingCallExpression(node: Node): Node {
let current: Node = node;
while (current.type !== "call_expression" && current.parent) {
current = current.parent;
function buildSwiftShadowedNames(root: Node): Map<number, string[]> {
const byDeclaration = new Map<number, string[]>();
for (const declaration of namedChildrenOf(root)) {
const names = new Set<string>();
collectSwiftShadowedNames(declaration, names);
byDeclaration.set(declaration.startIndex, [...names]);
}
return current;
return byDeclaration;
}
/**
* The shadowing names for one call site, read off the map built above.
*
* The scope that can shadow the name is the top-level declaration holding the
* call: it is the outermost ancestor below the file, and the file's own module
* level is what the fallback resolves *against*, so it cannot also be what
* shadows the name. A call that *is* a top-level statement has no such
* declaration above it, and the map holds only its own bare callee — which is
* skipped as a callee position, leaving the empty set the per-call walk
* produced.
*/
function swiftShadowedNamesAt(node: Node, byDeclaration: Map<number, string[]>): string[] {
let scope: Node = node;
while (scope.parent && scope.parent.type !== "source_file") {
scope = scope.parent;
}
return byDeclaration.get(scope.startIndex) ?? [];
}