Skip to content

fix(cjs_wrap): flat-emit module.exports = <Class> that closes over a top-level binding (#4933) - #4947

Merged
proggeramlug merged 2 commits into
mainfrom
fix-4933-cjs-class-methods
Jun 10, 2026
Merged

fix(cjs_wrap): flat-emit module.exports = <Class> that closes over a top-level binding (#4933)#4947
proggeramlug merged 2 commits into
mainfrom
fix-4933-cjs-class-methods

Conversation

@proggeramlug

Copy link
Copy Markdown
Contributor

Summary

Fixes #4933 — a compiled (compilePackages) CommonJS module whose default export is a class that reads a top-level const/let/var lost its entire method table: both static (Ctor.staticFn) and prototype (Ctor.prototype.method) methods read undefined on the consumer side, and new Ctor() threw whenever the constructor relied on one of those statics. This is the ink wall after #4902/#4931stack-utils<ErrorOverview>:

import StackUtils from 'stack-utils';
typeof StackUtils;                  // 'function'   ✓
typeof StackUtils.nodeInternals;    // was undefined → now 'function' ✓
typeof StackUtils.prototype?.clean; // was undefined → now 'function' ✓
new StackUtils();                   // was TypeError: undefined is not iterable → now OK ✓

Root cause

The class could not be hoisted out of the CJS wrap IIFE: the #2310 guard refuses to lift a class that references an IIFE-local binding (hoisting would sever the closure → Undefined variable in update expression). With the class trapped inside the IIFE, the module's default degraded to the opaque export default _cjs (the IIFE result), so compile.rs never registered class identity. The consumer then got a value whose statics, .prototype, and closure were all gone.

stack-utils trips this because its class body reads top-level consts as bare identifiers (natives, cwd, re, methodRe), unlike the textbook module.exports = class {} shape which hoists cleanly.

Fix

When the single module.exports = <Ident> target is a top-level class that did not hoist, and the body has no top-level return, drop the IIFE and run the CommonJS body at ESM module scope. The class becomes a real top-level declaration (export default <Class> resolves to it with full identity), and every sibling top-level binding it closes over stays in scope.

Because the class and its bindings now sit at the same (module) scope, this also supersedes the #2310 IIFE-retention mitigation for the default-export-class case — the mutable-closure shape keeps working:

// ws/lib/sender.js shape — verified byte-identical to Node
let pointer = 0;
class Sender { static next() { return pointer++; } }
module.exports = Sender;        // next() → 0, 1, 2 ✓  (and statics are now reachable cross-module)

Details

  • New top_level_class_names / source_has_top_level_return helpers gate the flat path. The latter tracks only {/} depth (matching the sibling collect_top_level_let_const_var_names) so a regex literal's brackets can't mis-flag a function-body return as top-level.
  • The CommonJS runtime preamble is factored into a shared cjs_preamble string; the IIFE template embeds it verbatim, leaving existing output byte-for-byte unchanged.
  • Flat emission triggers only for the previously-broken non-hoisting case, so packages that already hoisted (or that keep the IIFE for a top-level return) are unaffected.

Testing

Out of scope (pre-existing, separate)

Surfaced while verifying, not part of the method-table-absence this fixes:

Closes #4933.

Ralph Küpper and others added 2 commits June 10, 2026 21:57
…a top-level binding (#4933)

A compiled CommonJS module whose default export is a class that reads a
top-level `const`/`let`/`var` lost its entire method table: both static
(`Ctor.staticFn`) and prototype (`Ctor.prototype.method`) methods read
`undefined` on the consumer side, and `new Ctor()` threw whenever the
constructor relied on one of those statics. stack-utils → ink hit this:
`new StackUtils()` → `TypeError: undefined is not iterable`.

Root cause: the class could not be hoisted out of the CJS wrap IIFE — the
#2310 guard refuses to lift a class that references an IIFE-local binding,
since hoisting would sever the closure. With the class trapped inside the
IIFE, the module's default degraded to the opaque `export default _cjs`
(the IIFE result), so compile.rs never registered class identity. The
consumer then saw a value whose statics, `.prototype`, and closure were all
gone.

Fix: when the single `module.exports = <Ident>` target is a top-level class
that did not hoist, and the body has no top-level `return`, drop the IIFE
and run the CommonJS body at ESM module scope. The class becomes a real
top-level declaration (`export default <Class>` resolves to it with full
identity), and every sibling top-level binding it closes over stays in
scope — including mutable ones, so this also *supersedes* the #2310
IIFE-retention mitigation for the default-export-class case (verified: the
ws/sender `static next() { return pointer++; }` shape keeps working).

- `top_level_class_names` / `source_has_top_level_return` helpers gate the
  flat path; the latter tracks only `{`/`}` depth (matching the sibling
  `collect_top_level_let_const_var_names`) so a regex literal's brackets
  can't mis-flag a function-body `return` as top-level.
- The CommonJS runtime preamble is factored into a shared `cjs_preamble`
  string; the IIFE template embeds it verbatim, leaving its output
  byte-for-byte unchanged.
- Flat emission only triggers for the previously-broken non-hoisting case,
  so packages that already hoisted (or keep the IIFE for a top-level
  `return`) are unaffected.

Out of scope (pre-existing, separate bugs surfaced while verifying): a
cross-module class *instance* method reading a module-level const returns
garbage (#838 family), and `require('module').builtinModules` is a partial
stub. Both are independent of the method-table-absence this fixes.
@proggeramlug
proggeramlug merged commit 68068a9 into main Jun 10, 2026
12 of 13 checks passed
@proggeramlug
proggeramlug deleted the fix-4933-cjs-class-methods branch June 10, 2026 19:59
proggeramlug added a commit that referenced this pull request Jun 11, 2026
… survive (#4976) (#4981)

The `ExportDefaultDecl(DefaultDecl::Class)` arm only recorded the export
name and dropped the class body entirely: the class never entered
`module.classes`, the importer's `exported_classes` lookup missed, and
`import Widget from 'pkg'; new Widget()` fell through to the synthetic
default / empty-object placeholder — an instance whose prototype holds
only `constructor`, with every method and field initializer gone
(ink's `export default class Ink { render() {…} }`, the ESM sibling of
the CJS `module.exports = class` bug fixed in #4947).

Synthesize a ClassDecl (ident `default` for the anonymous form,
mirroring the anonymous-default-function branch) and run it through the
same flow as `ExportDecl::Class` — methods, field initializers, statics,
computed members, and decorators all install normally — then register
`Export::Named { local: Name, exported: "default" }` so the existing
#485/#665 alias machinery hands importers full class metadata. Also
pre-register named default classes in the declaration-order pass so
same-file static cross-references resolve.

Verified: issue repro now byte-identical to Node; static fields,
anonymous default class, and `extends`+`super` over a default-imported
class all match; ink hello clears the `instance.render is not a
function` wall (next gate is terminal-size/yoga, pre-existing).

Fixes #4976

Co-authored-by: Ralph Küpper <ralph@skelpo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

compilePackages: module.exports = class loses the class method table — static AND prototype methods read undefined (breaks stack-utils → ink)

1 participant