-
-
Notifications
You must be signed in to change notification settings - Fork 161
fix(cjs): preserve live getter re-export bindings #10156
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| ### Fixed | ||
|
|
||
| - Recognize CommonJS exports installed with `Object.defineProperty`, including | ||
| Babel's getter re-exports, as value exports. ESM named and namespace imports | ||
| now read the current property through the existing getter-aware runtime | ||
| lookup and call the returned value, instead of referencing a function symbol | ||
| that the CommonJS barrel never defined. | ||
| - Avoid evaluating synthetic CommonJS property exports during initialization, | ||
| preserving accessor side effects and exports assigned or replaced later. | ||
| Regression coverage includes named imports, namespace calls, ESM barrels, | ||
| materialized and dynamic namespaces, writable descriptors, and late | ||
| assignments (#10153). |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,123 @@ | ||
| //! Turn the CJS wrapper's synthetic property exports into live value getters. | ||
|
|
||
| use std::borrow::Cow; | ||
| use std::collections::{HashMap, HashSet}; | ||
|
|
||
| use perry_hir::{Expr, Module, Stmt}; | ||
|
|
||
| use crate::module::LlModule; | ||
| use crate::types::{DOUBLE, I32, I64, PTR}; | ||
|
|
||
| pub(super) type PropertyExports = HashMap<String, (u32, String)>; | ||
|
|
||
| /// The wrapper emits `export const x = _cjs.x` to register value-export | ||
| /// metadata. Do not execute that read during module initialization: it can | ||
| /// invoke a getter too early and freezes exports assigned after initialization. | ||
| /// Keep the caller's HIR immutable (it is also the object-cache input). | ||
| pub(super) fn prepare(hir: &Module) -> (Cow<'_, Module>, PropertyExports) { | ||
| // Pair the wrapper's private binding with its compiler-owned preamble; | ||
| // an ordinary ESM variable named `_cjs` still has snapshot semantics. | ||
| let has_preamble = hir.imports.iter().any(|import| { | ||
| import | ||
| .source | ||
| .strip_prefix("node:") | ||
| .unwrap_or(&import.source) | ||
| == "module" | ||
| && import.specifiers.iter().any(|specifier| { | ||
| matches!(specifier, perry_hir::ImportSpecifier::Named { imported, local } | ||
| if imported == "createRequire" && local == "__perry_cjs_create_require") | ||
| }) | ||
| }); | ||
| if !has_preamble { | ||
| return (Cow::Borrowed(hir), HashMap::new()); | ||
| } | ||
| let entry = super::entry_outline::logical_entry_stmts(hir); | ||
| let Some(object_id) = entry.iter().find_map(|stmt| match stmt { | ||
| Stmt::Let { id, name, .. } if name == "_cjs" => Some(*id), | ||
| _ => None, | ||
| }) else { | ||
| return (Cow::Borrowed(hir), HashMap::new()); | ||
| }; | ||
| let exported: HashSet<&str> = hir | ||
| .exports | ||
| .iter() | ||
| .filter_map(|export| match export { | ||
| perry_hir::Export::Named { local, .. } => Some(local.as_str()), | ||
| _ => None, | ||
| }) | ||
| .collect(); | ||
| let mut properties = HashMap::new(); | ||
| let mut bindings = HashSet::new(); | ||
| for stmt in entry { | ||
| if let Stmt::Let { | ||
| id, | ||
| name, | ||
| init: Some(Expr::PropertyGet { | ||
| object, property, .. | ||
| }), | ||
| .. | ||
| } = stmt | ||
| { | ||
| if exported.contains(name.as_str()) | ||
| && matches!(object.as_ref(), Expr::LocalGet(id) if *id == object_id) | ||
| { | ||
| properties.insert(name.clone(), (object_id, property.clone())); | ||
| bindings.insert(*id); | ||
| } | ||
| } | ||
| } | ||
| if properties.is_empty() { | ||
| return (Cow::Borrowed(hir), properties); | ||
| } | ||
| let mut owned = hir.clone(); | ||
| let clear_snapshots = |stmts: &mut [Stmt]| { | ||
| for stmt in stmts { | ||
| if let Stmt::Let { id, init, .. } = stmt { | ||
| if bindings.contains(id) { | ||
| *init = None; | ||
| } | ||
| } | ||
| } | ||
| }; | ||
| clear_snapshots(&mut owned.init); | ||
| for function in &mut owned.functions { | ||
| if super::entry_outline::is_entry_chunk(function) { | ||
| clear_snapshots(&mut function.body); | ||
| } | ||
| } | ||
| (Cow::Owned(owned), properties) | ||
| } | ||
|
|
||
| pub(super) fn emit_getter( | ||
| llmod: &mut LlModule, | ||
| getter_name: &str, | ||
| module_prefix: &str, | ||
| object_id: u32, | ||
| property: &str, | ||
| ) { | ||
| let (key_global, key_len) = llmod.add_string_constant(property); | ||
| let getter = llmod.define_function(getter_name, DOUBLE, vec![]); | ||
| getter.create_block("entry"); | ||
| let blk = getter.block_mut(0).unwrap(); | ||
| // Allocate the key before loading the GC-rooted namespace. No raw object | ||
| // pointer survives that allocation; the boxed property helper handles | ||
| // accessors, prototypes and function-valued module.exports. | ||
| let key = blk.call( | ||
| I64, | ||
| "js_string_from_bytes", | ||
| &[ | ||
| (PTR, &format!("@{key_global}")), | ||
| (I32, &key_len.to_string()), | ||
| ], | ||
| ); | ||
| let object = blk.load( | ||
| DOUBLE, | ||
| &format!("@perry_global_{module_prefix}__{object_id}"), | ||
| ); | ||
| let value = blk.call( | ||
| DOUBLE, | ||
| "js_object_get_field_by_name_boxed", | ||
| &[(DOUBLE, &object), (I64, &key)], | ||
| ); | ||
| blk.ret(DOUBLE, &value); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| //! Closure-ABI adapters for live exports in materialized static namespaces. | ||
|
|
||
| use super::helpers::sanitize_member; | ||
| use super::opts::CrossModuleCtx; | ||
| use crate::module::LlModule; | ||
| use crate::types::{DOUBLE, I64}; | ||
|
|
||
| pub(crate) fn symbol(module_prefix: &str, namespace: &str, member: &str) -> String { | ||
| format!( | ||
| "__perry_namespace_value_{}__{}", | ||
| module_prefix, | ||
| sanitize_member(&format!("{namespace}:{member}")) | ||
| ) | ||
| } | ||
|
|
||
| pub(super) fn emit(llmod: &mut LlModule, module_prefix: &str, imports: &CrossModuleCtx) { | ||
| let mut members: Vec<_> = imports.namespace_member_prefixes.iter().collect(); | ||
| members.sort_by_key(|(key, _)| *key); | ||
| for ((namespace, member), source_prefix) in members { | ||
| let wrapper_name = symbol(module_prefix, namespace, member); | ||
| // Lowering declares an adapter only when a namespace value actually | ||
| // escapes. Direct `ns.x` reads need no additional wrapper. | ||
| if !llmod.is_declared(&wrapper_name) || llmod.has_function(&wrapper_name) { | ||
| continue; | ||
| } | ||
| let origin = crate::expr::import_origin_suffix_ns( | ||
| &imports.import_function_origin_names, | ||
| &imports.namespace_member_origin_names, | ||
| namespace, | ||
| member, | ||
| ); | ||
| let getter_name = format!("perry_fn_{source_prefix}__{origin}"); | ||
| llmod.declare_function(&getter_name, DOUBLE, &[]); | ||
| let wrapper = | ||
| llmod.define_function(wrapper_name, DOUBLE, vec![(I64, "%closure".to_string())]); | ||
| wrapper.create_block("entry"); | ||
| let block = wrapper.block_mut(0).unwrap(); | ||
| let value = block.call(DOUBLE, &getter_name, &[]); | ||
| block.ret(DOUBLE, &value); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,7 +62,11 @@ pub(in crate::commands::compile) fn is_commonjs(source: &str) -> bool { | |
| // — no `exports.X =`, no `require(`. Without this arm they fall | ||
| // through to the ESM pipeline, where the bare `exports` identifier | ||
| // throws a ReferenceError at module init. | ||
| || stripped.contains("defineProperty(exports,") | ||
| || perry_perex::tooling::Regex::new( | ||
| r"(?:^|[^A-Za-z0-9_$.])Object\s*\.\s*defineProperty\s*\(\s*(?:module\s*\.\s*)?exports\s*,", | ||
| ) | ||
| .unwrap() | ||
| .is_match(&stripped) | ||
|
Comment on lines
+65
to
+69
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Detect bare
Add a boundary-safe bare-call detector, extend descriptor extraction to accept both callee forms, and add regression coverage for detection and name extraction. 🤖 Prompt for AI Agents |
||
| { | ||
| return true; | ||
| } | ||
|
|
||
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
Use explicit wrapper metadata instead of user-spellable names.
An ordinary ESM module can import
createRequireas__perry_cjs_create_requireand declare_cjs. This condition then identifies the module as a generated CommonJS wrapper.prepareclears matching property initializers. Local reads can becomeundefined, and exported values become live property reads instead of ESM initializer snapshots. This violates the required ordinary ESM semantics.Mark generated wrappers explicitly in HIR and require that marker before this transformation.
🤖 Prompt for AI Agents