-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Implement static import defer (TC39 Stage 3)
#30975
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
e52b8c3
wtf-bindings: add missing <cassert> include
robobun b8e5f91
Implement static `import defer * as ns from "..."`
robobun 4fa979e
[autofix.ci] apply automated fixes
autofix-ci[bot] 34c4f6b
Harden phase-byte validation when building JSModuleRecord
robobun daced86
Reject escaped `def\u0065r` as a phase keyword
robobun baef48d
Reject `import defer * as` outside module scope
robobun 810e569
Don't strip the namespace binding from `import defer` during unused-i…
robobun bfdda47
Reject `export import defer * as` in TypeScript
robobun c0ddc41
ci: retrigger
robobun 1a98bc5
Assert the `import defer` shape invariant in process_import_statement
robobun 6307ae8
Add `--compile --bytecode` tests for `import defer`
robobun File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we turn this into an enum of phases? to support source phase later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reasonable — I went back and forth on this. A few notes on why I landed on the bool here but kept the enum where it matters for forward-compat:
ModuleInfoalready uses aModulePhaseenum (Evaluation = 0,Defer = 1) for the per-requested-module phase byte, so the wire format is trivially extensible toSource = 2without another cache-version bump.import source x from "..."has a different grammar shape (default binding only, no* as), so it wouldn't share thisS::Importbranch inparse_stmtanyway — it'd get its own handling and probably its ownS::Importfield (or a sharedphase: ImportPhaseat that point).ImportRecord::Flagsis now full (u16, bit 15 isPHASE_DEFER). AddingPHASE_SOURCEwould mean widening tou32or moving phase to a separateu8on the record — either is fine but felt like it belonged with the change that actually needs it.Happy to switch
phase_defer: bool→phase: ImportPhase { Evaluation, Defer }onS::Importnow if you'd rather have it in place — it's a small mechanical change. Let me know.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty reasonable - how can we make sure we are not introducing strange bugs by theoretically allowing an invalid state though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The invariant (
phase_defer⟹star_name_loc.is_some()∧default_name.is_none()∧items.is_empty()) is held by construction right now — the only place that setsphase_defer: trueis the parser branch that builds a freshS::Import { star_name_loc: Some(_), ..Default::default() }, and every otherS::Importconstructor (7 of them) explicitly passesphase_defer: false.For defence-in-depth:
defertoken onPHASE_DEFER && CONTAINS_IMPORT_STAR, so even a malformedS::Importcan't produceimport defer"./x";.phase_deferis set.debug_assert!inprocess_import_statement(1a98bc5) that checks the full shape invariant at theS::Import→ import-record boundary, so any future producer that violates it trips immediately in debug builds rather than surfacing as odd output downstream.An enum wouldn't actually prevent the cross-field invalid state (you could still have
phase: ImportPhase::Deferwithdefault_name: Some(_)), so the assert is the more direct guard. Whenimport sourcelands and needs its own shape invariant (default-binding-only), that's the natural point to liftphase_defer: bool→phase: ImportPhaseand widen the record flags.