-
-
Notifications
You must be signed in to change notification settings - Fork 158
fix(hir): late-bind new X() to a class declared later; name the ReferenceError (#8882)
#8892
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
Closed
Closed
Changes from all commits
Commits
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
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Fix a `new <Identifier>()` whose constructor is not statically resolvable at lowering time throwing a nameless `ReferenceError: identifier is not defined` at module init (#8882, the second instance of #8730's class). The unresolved-`new` guard added in #8643 decided the miss at compile time, but JS binds the constructor reference when the `new` executes, and two shapes are invisible to the lowering-time lookups: a class declared in a function body lowered LATER, and a global that exists only at runtime. The CJS wrap produces the first one routinely — it hoists top-level classes out of the module IIFE but leaves some inside (here Next's `server/lib/lru-cache.js` `SentinelNode`, whose doc comment closes on the `class` line so the textual hoister never sees it), so the hoisted `LRUCache` constructor's `new SentinelNode()` was lowered before the IIFE body registered `SentinelNode`, and the whole Next.js App Route application died at init under Coop. A module-wide pre-scan now records every class declaration name at any depth; a name in that set keeps the late-bound by-name construction codegen resolves through the module class table (the pre-#8643 behaviour), and any other name is read off `globalThis` when the `new` runs via `js_global_get_or_throw_unresolved`, so a runtime-created constructor works and a true miss throws the spec `ReferenceError: <name> is not defined` — with the identifier, as #8730 asked. The bare-identifier arm shares the same helper so the two cannot drift. |
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| //! Pre-scan for every `class X { … }` DECLARATION name in the module, at any | ||
| //! nesting depth. | ||
| //! | ||
| //! #8882: `lower_new`'s unresolved-constructor guard (#8643) decides at | ||
| //! lowering time whether `new X()` can bind at all. Its lookups only see | ||
| //! bindings registered so far, but a class declared inside a function body | ||
| //! that is lowered LATER is still a legitimate late-bound target — JS | ||
| //! resolves the constructor reference when the `new` executes, not when the | ||
| //! enclosing method is compiled. The CJS wrap makes this shape common: it | ||
| //! hoists most top-level classes out of the module IIFE but leaves some | ||
| //! inside (a class it did not recognise textually, or one that reads an | ||
| //! IIFE-local), so a hoisted class's constructor can `new` a sibling that is | ||
| //! now nested in the `__perry_cjs_factory` closure and registered only when | ||
| //! that closure body is lowered. Next's `server/lib/lru-cache.js` has exactly | ||
| //! this: `LRUCache` (hoisted) constructs `SentinelNode` (left in the IIFE | ||
| //! because its doc comment closes on the `class` line). | ||
| //! | ||
| //! The guard consults this set: a name declared as a class anywhere in the | ||
| //! module keeps the by-name `Expr::New` lowering that codegen resolves through | ||
| //! the module class table (the pre-#8643 behaviour); anything else is a | ||
| //! runtime `globalThis` lookup that throws the spec `ReferenceError: X is not | ||
| //! defined`. Only DECLARATIONS count — a named class EXPRESSION's name binds | ||
| //! inside its own body alone. | ||
|
|
||
| use swc_ecma_ast as ast; | ||
| use swc_ecma_visit::{Visit, VisitWith}; | ||
|
|
||
| use crate::lower::*; | ||
|
|
||
| pub(crate) fn pre_scan_class_decl_names(ast_module: &ast::Module, ctx: &mut LoweringContext) { | ||
| struct Collector<'a> { | ||
| names: &'a mut std::collections::HashSet<String>, | ||
| } | ||
| impl Visit for Collector<'_> { | ||
| fn visit_class_decl(&mut self, class_decl: &ast::ClassDecl) { | ||
| self.names.insert(class_decl.ident.sym.to_string()); | ||
| class_decl.visit_children_with(self); | ||
| } | ||
| } | ||
| let mut collector = Collector { | ||
| names: &mut ctx.class_decl_names_any_depth, | ||
| }; | ||
| ast_module.visit_with(&mut collector); | ||
| } |
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.
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.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve lexical scope when exempting class names.
class_decl_names_any_depthrecords nested class names without their binding scope. As a result,new X()in one function can emitExpr::Newmerely because another function declaresclass X; codegen may then select that unrelated class instead of resolvingglobalThis.Xor throwingReferenceErrorwhen the global is absent.Restrict the late-binding exemption to class declarations visible at the constructor site, and add a regression covering unrelated functions that use and declare the same class name.
📍 Affects 2 files
crates/perry-hir/src/lower/expr_new.rs#L1613-L1614(this comment)crates/perry-hir/src/lower/lowering_context.rs#L819-L827🤖 Prompt for AI Agents