fix(resolution): follow alias bindings to the function they name - #1485
Open
valkyriweb wants to merge 1 commit into
Open
fix(resolution): follow alias bindings to the function they name#1485valkyriweb wants to merge 1 commit into
valkyriweb wants to merge 1 commit into
Conversation
A name bound to nothing but another symbol resolved to the BINDING, one hop
short of the callable:
export const alias = fn; // consumer calls alias()
export { fn as alias }; // declaration has is_exported = 0
export const api = { run: fn }; // consumer calls api.run()
const local = fn; // same-file indirection
The edge existed, so nothing looked broken — but `callers fn` omitted every
caller that went through the alias and reported a confident zero, while
`callers alias` found them. `impact` under-reported the same way.
`resolveOne` now takes one extra hop when the resolved target is an alias
binding, retargeting `calls` refs only (a `references` edge to the binding is
correct as-is). The initializer is already stored in `Node.signature`, so no
re-parsing is needed.
Ambiguity yields no hop: a same-file callable is preferred, a cross-file one is
accepted only when it is unique project-wide, and confidence is capped at 0.85.
A wrong edge is worse than a missing one.
Refs: colbymchenry#1482
Author
|
Companion: #1484 ( |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the second of the two root causes behind #1482.
The bug
When a name is bound to nothing but another symbol, resolution stops on the binding — one hop short of the callable:
An edge is produced, so nothing looks broken. But
callers fnomits every caller that went through the alias and reports a confident 0, whilecallers aliasfinds them.impactunder-reports the same way. That is the failure mode from #1482 — the graph is silently wrong rather than visibly incomplete, which is the expensive kind.The fix
Two parts:
1.
src/resolution/alias-binding.ts(new)aliasTargetName— reads the initializer already stored inNode.signature(= implFn,= { run: implFn }) and returns the aliased name. No re-parsing.resolveAliasBinding— resolves that name to a callable.extractLocalExportAliases— parses localexport { impl as alias }clauses.2.
resolveOnetakes one extra hop when the resolved target turns out to be an alias binding.The hop is applied once in
resolveOnerather than per-strategy, because any strategy can land on an alias.3.
getFileExportIndexnow indexes all declarations plus local export clauses, soexport { impl as alias }binds toimpl— previously the declaration carried noexportkeyword, wasn't flaggedisExported, and the importer's request foraliasfell through to the name-matcher.Safety
The conservative choices are deliberate — a wrong edge is worse than a missing one:
callsrefs only. Areferencesedge to the binding is correct as-is and is left alone.export const wrapper = () => fn()is a real function; the call site calls it.Tests
__tests__/alias-binding-resolution.test.ts, 6 cases: the four shapes above, plus two negative guards (genuine wrapper stays put; ambiguous name yields no hop).On
mainthe four positive cases fail and both negative guards pass. With the fix all six pass.Specifiers in these fixtures are extensionless on purpose, so this PR stands completely independent of the
.js-specifier companion.Verification
kernel-*-parityfiles, expected without a native kernel build (CODEGRAPH_KERNEL=0)Not covered
unresolved_refstable withstatus = 'failed'that already records misses like these, andcallers/impactnever surface it. Turning silent wrong answers into caveated ones looks like the cheapest remaining trust win, but it's out of scope here.Companion PR for the first root cause (
.jsspecifiers) is independent and can land in either order; both touchimport-resolver.tsin different regions, so whichever lands second may need a trivial rebase.