Skip to content

js_printer: re-wrap delete of folded import/cjs-export identifier - #36740

Merged
Jarred-Sumner merged 7 commits into
mainfrom
farm/e9472152/printer-delete-rewrap-import-ident
Aug 4, 2026
Merged

js_printer: re-wrap delete of folded import/cjs-export identifier#36740
Jarred-Sumner merged 7 commits into
mainfrom
farm/e9472152/printer-delete-rewrap-import-ident

js_printer: re-wrap delete of folded import/cjs-export identifier

ee36712
Select commit
Loading
Failed to load commit list.
Claude / Claude Code Review completed Aug 1, 2026 in 19m 47s

Code review found 3 potential issues

Found 5 candidates, confirmed 3. See review comments for details.

Details

Severity Count
🔴 Important 0
🟡 Nit 2
🟣 Pre-existing 1
Severity File:Line Issue
🟡 Nit src/js_printer/lib.rs:1223-1226 Same-class siblings not swept: ERequireCallTarget / ERequireMain / ERequireResolveCallTarget / EImportMeta / EInlinedEnu
🟡 Nit src/js_printer/lib.rs:1225 ESpecial arm has no test coverage
🟣 Pre-existing src/js_printer/lib.rs:1218 Pre-existing: react_compiler codegen emits UnDelete with the flag unset → this helper's wrap neuters the delete

Annotations

Check warning on line 1226 in src/js_printer/lib.rs

See this annotation in the file changed.

@claude claude / Claude Code Review

Same-class siblings not swept: ERequireCallTarget / ERequireMain / ERequireResolveCallTarget / EImportMeta / EInlinedEnum

Same-class siblings still fall through to `_ => false`: `ERequireCallTarget` / `ERequireMain` / `ERequireResolveCallTarget` / `EImportMeta` all print as a bare identifier or property access, and `EInlinedEnum` can wrap an `ENumber(NaN|Infinity)` — so `delete (null ?? require)` bundles to `delete __require` and `const enum E { N = NaN }; delete (null ?? E.N)` bundles to `delete NaN`, both strict-mode SyntaxErrors via the identical fold path this PR fixes. Consider adding those variants alongside 

Check warning on line 1225 in src/js_printer/lib.rs

See this annotation in the file changed.

@claude claude / Claude Code Review

ESpecial arm has no test coverage

The `ESpecial(_)` arm has no test coverage — deleting `| ExprData::ESpecial(_)` breaks none of the 5 new tests, so per REVIEW.md ("confirm deleting each load-bearing clause of your fix breaks at least one test") it needs one, e.g. `delete (null ?? module.exports)` in a CJS entry. Minor: the comment says these "print as a bare identifier or a property access", but `E::Special::ResolvedSpecifierString` prints a string literal and `E::Special::HotDisabled` prints via `EUndefined` — over-wrapping th

Check notice on line 1218 in src/js_printer/lib.rs

See this annotation in the file changed.

@claude claude / Claude Code Review

Pre-existing: react_compiler codegen emits UnDelete with the flag unset → this helper's wrap neuters the delete

Pre-existing (not introduced or widened by this PR): `src/react_compiler/codegen.rs` emits `UnDelete` over an `EDot`/`EIndex` with `flags: E::UnaryFlags::empty()` at both `PropertyDelete` (:1994-2001) and `ComputedDelete` (:2047-2061), so the pre-existing `EDot | EIndex` arm here already re-wraps every `delete obj.prop` inside a React-compiled component into `delete (0, obj.prop)` — a no-op that returns `true`. The fix belongs in `react_compiler/codegen.rs` (set `WAS_ORIGINALLY_DELETE_OF_IDENTIF