Skip to content

build: fix private-to-property rewrite bugs, add syntax check#308724

Merged
jrieken merged 2 commits intomainfrom
joh/fix-private-to-property
Apr 9, 2026
Merged

build: fix private-to-property rewrite bugs, add syntax check#308724
jrieken merged 2 commits intomainfrom
joh/fix-private-to-property

Conversation

@jrieken
Copy link
Copy Markdown
Member

@jrieken jrieken commented Apr 9, 2026

Summary

Fix two correctness bugs in the convertPrivateFields rewrite step that could produce invalid or semantically broken JavaScript output, and add a build-time syntax check as a safety net.

Session Context

Key decisions from the development session:

  • Test-first approach: Tests were written first to confirm the bugs, then the logic was fixed to make them pass. Tests were not modified after the fix.
  • Heritage clause fix strategy: Instead of a post-hoc fixup, the walk was restructured to process heritage clauses before pushing the inner class scope. This matches JS lexical scoping where the extends expression is evaluated in the enclosing scope.
  • Name collision fix strategy: A simple do/while loop skips generated names ($a, $b, …) that collide with existing public member names on the same class. This avoids silent property shadowing.
  • Syntax check uses esbuild.transform(): Chosen over node --check (which doesn't handle ESM .js files without "type": "module") and vm.Script (which only parses CJS). esbuild is already imported, handles ESM natively, runs in-process with no child process overhead, and checks all files in parallel.
  • Syntax check scope: Only post-processed files (those touched by mangle-privates or NLS) are checked — esbuild's own output is trusted.

Changes

build/next/private-to-property.ts

  • Heritage clause scope resolution: Walk extends expressions before pushing the inner class's scope onto the stack. Previously, a nested class declaring the same #field as its outer class would cause resolvePrivateName in the heritage clause to pick the inner scope — wrong, since extends is evaluated in the enclosing lexical scope. This could cause TypeError: Class extends value undefined at runtime.
  • Name collision avoidance: Collect public member names before generating replacement names. Skip any $a, $b, … that already exist as public members. Previously, a class with both $a (public) and #x (private) would get two $a declarations, silently breaking runtime behavior.
  • Extracted walkInClass into createWalkInClass() so heritage clauses and members can be walked separately with different stack states.

build/next/test/private-to-property.test.ts

  • 5 new tests: heritage clause resolution (string + runtime), name collision (string + runtime), brand check in heritage clause.

build/next/index.ts

  • Added post-processing syntax check: after mangle-privates and NLS string surgery, all affected JS files are parsed via esbuild.transform() to catch any syntax corruption at build time rather than at runtime.

…ax check

- Fix heritage clause scope resolution: walk extends expressions before
  pushing the inner class scope so #field references resolve to the
  enclosing class (matching JS lexical scoping)
- Fix name collision: skip generated names that collide with existing
  public member names on the same class
- Add syntax validation for post-processed JS bundles using esbuild
- Add 5 tests covering both bugs and the brand-check variant
Copilot AI review requested due to automatic review settings April 9, 2026 09:19
@jrieken jrieken self-assigned this Apr 9, 2026
@jrieken jrieken added this to the 1.116.0 milestone Apr 9, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes correctness issues in the build-time convertPrivateFields transformation (private #x → short public properties like $a) and adds a post-processing syntax validation step to catch bundle corruption during the build pipeline.

Changes:

  • Fix private-name resolution in extends/heritage clauses so nested classes resolve to the correct (outer) lexical private scope.
  • Avoid generated replacement-name collisions with existing public class member names.
  • Add a build-time syntax check for JS bundles that were post-processed (mangle-privates and/or NLS edits) using esbuild.transform() parsing.
Show a summary per file
File Description
build/next/private-to-property.ts Adjusts class walking order for heritage clauses and adds collision avoidance during replacement-name generation.
build/next/test/private-to-property.test.ts Adds regression tests covering heritage-clause scope resolution, name-collision behavior, and brand checks in heritage clauses.
build/next/index.ts Adds a post-processing syntax parse check for affected output JS files to fail builds early on syntax corruption.

Copilot's findings

  • Files reviewed: 3/3 changed files
  • Comments generated: 3

@jrieken jrieken enabled auto-merge April 9, 2026 09:38
@jrieken jrieken merged commit a6a1847 into main Apr 9, 2026
37 of 38 checks passed
@jrieken jrieken deleted the joh/fix-private-to-property branch April 9, 2026 12:19
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.

3 participants