Repository navigation
[JSC] ModuleAnalyzer keeps the attributes list of the declaration behind each requested module - #670
dylan-conway wants to merge 2 commits into
Conversation
…ind each requested module
requestedModules() is deduplicated by (specifier, type) per phase, and a request
keeps only the parsed type. An embedder that reports every import attribute of a
request, such as node:vm's SourceTextModule.moduleRequests, had to find the
declaration again by re-deriving that key from the AST.
appendRequestedModule now takes the declaration's ImportAttributesListNode and,
under BUN_JSC_ADDITIONS, records it when the request is new.
requestedModuleAttributesLists() is index-aligned with requestedModules() and
covers import, export * from and export { } from alike.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs; since it changes the signature of a parser entry point in JavaScriptCore proper (a path listed under CODEOWNERS), a human look is still worthwhile before merging.
What was reviewed:
- Index alignment of
requestedModuleAttributesLists()withrequestedModules(): theJSModuleRecordis created fresh inModuleAnalyzer's constructor and, during analysis, onlyModuleAnalyzer::appendRequestedModuleappends to it, in the sameisNewEntrybranch as the new vector, so the two cannot diverge (theJSWebAssemblyInstancedirect call is on a different record). - Both
ModuleAnalyzeruse sites (JSModuleLoader.cppandCompletion.cpp) keep the analyzer andModuleProgramNodeas locals of one function, and the header comment documents the arena lifetime of the stored node pointers. - All three in-tree callers were updated for the new positional parameter; without
BUN_JSC_ADDITIONSthe parameter is onlyUNUSED_PARAM-ed, so upstream behaviour is unchanged.
Extended reasoning...
Overview
The PR touches three parser files in JavaScriptCore: Source/JavaScriptCore/parser/ModuleAnalyzer.h, ModuleAnalyzer.cpp, and NodesAnalyzeModule.cpp. ModuleAnalyzer::appendRequestedModule gains an ImportAttributesListNode* parameter, and under USE(BUN_JSC_ADDITIONS) the analyzer stores that pointer in a new vector whenever a request is new, exposed via requestedModuleAttributesLists(). The three analyzeModule callers (import, export * from, export {} from) pass their node's attributesList(). No consumer is in this diff; the stated consumer is Bun's node:vm SourceTextModule implementation.
Security risks
None identified. The change stores raw parser-arena pointers, which could dangle if an embedder held them past the ModuleProgramNode's lifetime, but that is a lifetime contract for the embedder and is documented in the header; the two in-tree constructions of ModuleAnalyzer (JSModuleLoader.cpp:1507 and Completion.cpp:94) keep the analyzer and the node as locals of the same function. No parsing, dedup, or module-record behaviour changes when the flag is off.
Level of scrutiny
Low to moderate. The diff is +22/-6, mechanical, and correctly gated. The one property that matters, index alignment with requestedModules(), holds by construction: JSModuleRecord is created inside ModuleAnalyzer's constructor, and within analysis the only appends to its m_requestedModules come through ModuleAnalyzer::appendRequestedModule, in the same isNewEntry branch that appends to the new vector. AbstractModuleRecord::appendRequestedModule is also called from JSWebAssemblyInstance.cpp, but on a different record that no analyzer touches. The dedup key ignoring non-type attributes ("first declaration wins") matches the existing requestedModules() semantics and is what the PR sets out to mirror. I did not approve outright because the repository's CODEOWNERS covers all of Source/JavaScriptCore, and the change alters a public signature of a parser class that upstream syncs will have to carry.
Other factors
There are no tests in this diff; the behaviour is only observable from Bun's embedder, so coverage would live in oven-sh/bun rather than here. The #if !USE(BUN_JSC_ADDITIONS) / UNUSED_PARAM split is slightly unusual stylistically but compiles cleanly under both configurations. The bug hunt exited on a dry streak after the four listed candidates were ruled out.
|
Preview build of df5b071: |
oven-sh/WebKit#670: ModuleAnalyzer records, for every requested module, the attributes list of the declaration that created it. createModuleRecord reads that list by index instead of searching the AST for a matching import. A request created by export ... from now reports that declaration's attributes, custom keys included, and not a later import's. An index-like attribute key no longer asserts.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. Walkthrough
ChangesModule attribute propagation
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk is identified in this change. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the change, motivation, affected behavior, and compatibility notes. It does not follow the repository template because it omits the bug title, Bugzilla URL, review status, and changed-file/function list. Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
…r-request-attributes-lists
What
ModuleAnalyzer::appendRequestedModuletakes the declaration'sImportAttributesListNode*. UnderBUN_JSC_ADDITIONSit records that pointer when the request is new, andrequestedModuleAttributesLists()exposes the result: a vector index-aligned with the module record'srequestedModules().Why
requestedModules()is deduplicated by (specifier,ScriptFetchParameters::Type) per phase, first declaration wins, and a request keeps only the parsed type.node:vm'sSourceTextModulereports every attribute of each request (moduleRequests, and theextra.attributesargument of thelink()callback), so Bun walks the AST again to find them. It paired the i-th import declaration with the i-th request, which is wrong as soon as a specifier repeats (oven-sh/bun#41654). Re-deriving JSC's dedup key from outside to fix that needs a copy oftryCreateAttributes, a way to readImportDeclarationNode's private phase, and still cannot coverexport * from/export { } from, becauseModuleDeclarationNodehas no way to tell those two apart.The analyzer already knows, at the moment it deduplicates, which declaration created the request. Recording it there makes the pairing correct by construction for import and both export-from forms.
Notes
ModuleProgramNodebeing analyzed; the vector is only meaningful while that node is alive, which is how Bun uses it (analyzer and node are both locals of one function).BUN_JSC_ADDITIONS; the new parameter is unused there.JSWebAssemblyInstancecallsAbstractModuleRecord::appendRequestedModuledirectly and is not affected.