Resolve an external fn by its declaration rather than its bare name - #427
Open
0xGeorgii wants to merge 1 commit into
Open
Resolve an external fn by its declaration rather than its bare name#4270xGeorgii wants to merge 1 commit into
0xGeorgii wants to merge 1 commit into
Conversation
An `external fn`'s identity is the declaration a call site names, reachable through the scope that call is written in: the declaring file, and the `spec` block within it. Three whole-program maps keyed it on the bare name instead, so at most one declaration of a given name could exist anywhere in a program. Three other passes had already been migrated to declaration identity, so the compiler held both models at once and they disagreed. The bare-name maps were `collect_top_level_extern_decls`, the local `imports` accumulator in `collect_extern_bindings`, and `Compiler::extern_name_to_idx`. All three are gone. Resolution now runs through one shared `ExternIndex`, promoted out of the specification translator into `core/type-checker` and keyed by `(module_path, spec, name)`, which analysis rule A024, the specification translator, and code generation all consult. Fixed, each reproduced by execution: - A bound `external fn` in any file captured every same-file bare-identifier call of that name in the whole program. An entry file defining its own `fn scale` and calling it reached a sibling file's linked external instead, returning 1998 where 20 was correct, with its own function emitted and left dead and no diagnostic at any stage. - An unrelated, never-called, never-bound sibling declaration broke a working bound call, with the calling file byte-identical. - A `use … from` clause in one file bound another file's declaration, and linked, and executed. - Two files that each declared and bound one name were rejected, whether they named different modules or the same one. - The cross-file ambiguity error was stamped with the first file naming the field rather than the offending one. - A spec-inner declaration could be displaced by a same-named top-level import. - Extern binding diagnostics were emitted in hash order. Widening the binding made two declarations able to share a `(module, field)` pair, which `extern_origins()` deduplicated on: one declaration's signature would then never reach validation, and the linker satisfies an import on `(module, field)` alone without comparing signatures, so a disagreement would have shipped as a silently mis-linked artifact. The dedup key now carries the declaration, and a disagreement is rejected. Two declarations binding the same module and field share one WASM import. A free function and an `external fn` sharing one bare name is now the hard error `ExternFunctionNameCollision`, naming both declarations and their locations, in one file or across files. `AmbiguousExternModule` is correspondingly a within-file rule. The name-keyed `TypedContext::extern_origin` and `is_extern_function` are removed; they walked every scope ignoring file boundaries and had no production callers. No committed `.wasm`, `.wat` or `.v` golden moves: every extern golden is single-file, where the scope key resolves exactly as the bare name did.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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.
Closes #423.
An
external fn's identity is the declaration a call site names, reachable through the scope that call is written in — the declaring file, and thespecblock within it. Three whole-program maps keyed it on the bare name instead, so at most one declaration of a given name could exist anywhere in a program. Analysis rule A024, the specification translator and the link driver had already been migrated to declaration identity, so the compiler held both models at once and they disagreed.The three bare-name maps —
collect_top_level_extern_decls, theimportsaccumulator insidecollect_extern_bindings, andCompiler::extern_name_to_idx— are gone. Resolution now runs through a singleExternIndex, promoted out of the specification translator intocore/type-checkerand keyed by(module_path, spec, name), which analysis, the specification translator and code generation all consult.core/type-checkeris the unique common ancestor ofcore/analysisandcore/wasm-codegen, and it already ownedExternOrigin, so no new dependency edge was needed.What was wrong
The issue reported two problems. Enumerating and executing every shape of (same/different file) x (extern/local) x (bound/unbound) x (same/different module) found nine.
external fnin any file captured every same-file bare-identifier call of that name program-wideentry(2)returned 1998 where 20 was correct, the entry file's ownfn scaleemitted and left dead, silent at type check, analysis, codegen, link and validationA024fired on ituse … fromin one file bound another file's declarationTypedContext::extern_origin/is_extern_functionwalked all scopes ignoring file boundariesDefect 1 is not arity-safe either: a two-argument call into a one-parameter import still validates, because
returnis stack-polymorphic.Defect 6 did not exist before this change and was created by it. Re-keying the binding lets two declarations share a
(module, field)pair, whichSymbolTable::extern_originsdeduplicated on. One declaration's signature would then never reachvalidate_extern, and the linker satisfies an import on(module, field)alone without comparing signatures — so a disagreement would have shipped as a silently mis-linked artifact rather than a rejection. The dedup key now carries the declaration, and a disagreement is rejected. This is why the binding re-key and the validation fix land together rather than in sequence.Defect 3 is the one a narrower fix misses. It is the only defect whose correct outcome is a new rejection, so it could have stayed broken behind an otherwise-green suite.
Behaviour changes
Now compiles:
external fn fand bind it to a different module, with each file's calls reaching its own binding.external fn fto the same module; they share one WASM import.Now errors:
use … fromclause no longer reaches across files, so one naming an extern its own file does not declare isExternImportNotDeclared.external fnsharing one bare name is the newExternFunctionNameCollision, naming both declarations and their locations, in one file or across files.AmbiguousExternModuleis correspondingly a within-file rule. The name-keyedTypedContext::extern_originandis_extern_functionare removed and their test call sites migrated toextern_origin_by_decl.Note that the collision rule rejects a program the compiler now resolves correctly — each file's call reaches its own function, verified by execution before the rule existed. It is a deliberate rule about the spelling, because a call site does not say whether its callee is compiled here or linked in, not a limit on resolution. The diagnostic and changelog both say so.
Ordering
The codegen re-key had to land before the binding re-key. The binding change makes two same-named bound externs reachable; with codegen still bare-name-keyed at that moment, a hard
AmbiguousExternModuledegrades into the miscompile.Verification
cargo test --no-fail-fast: 6035 passed, 0 failed, 161 ignored, 51 suites, against a 5995-passing baseline captured in a worktree pinned atd869f9e. Every test of the +40 accounted for individually; three renamed tests appear as a removal plus an addition and no test was dropped or ignored.No committed
.wasm,.wator.vgolden moves. That was predicted rather than observed: all 13 extern goldens are single-file, where(module_path=[], spec=None, name)resolves on exactly the names the old bare-name lookup did.Each fix was neutralised to prove the tests have teeth. Reverting only the call-lowering probe fails the regression at
left: 1998, right: 20.Coverage went to the gap that let this ship: the multi-file x extern cell was empty across
tests/— all 16 A024 tests, all 13 extern goldens and every linker test were single-file. New coverage spans the merged-arena, project-mode, type-checker and CLI seams, including execution through Wasmtime of the two-files-two-modules shape, asserting each file's call reaches its own import.Documentation
Prose asserting properties the code no longer has was corrected alongside it, including two statements that were false before this work:
core/wasm-linker/src/merge.rsclaimed the merge keys on the export field alone, contradicted byfind_exporttwo hundred lines below it, and the A024 row incore/analysis/README.mdstill described extern calls as unimplemented in codegen, untrue since the linker landed.Confidence Score: 5/5
The PR appears safe to merge; no concrete changed-code defect remains after tracing scope identity through type checking, analysis, code generation, validation, and linking.
The new declaration-keyed resolution uses consistent file and spec identities across all consumers, call lowering remains aligned with parameter escape handling, and every shared module-field declaration is independently signature-validated before linker input is deduplicated.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR Call["Call site<br/>(module path, optional spec, name)"] Index["ExternIndex<br/>(module path, spec, name → DefId)"] Binding["Extern binding<br/>(DefId → module + field)"] Analysis["A024 bound/unbound check"] Spec["Specification translation"] Codegen["WASM import and call lowering"] Validate["Per-declaration signature validation"] Link["Static linker<br/>(module + field)"] Call --> Index Index --> Binding Index --> Analysis Index --> Spec Index --> Codegen Binding --> Codegen Binding --> Validate Codegen --> Link Validate --> LinkReviews (1): Last reviewed commit: "Resolve an external fn by its declaratio..." | Re-trigger Greptile
Context used (4)