Repository navigation
Code review found 1 important issue
Found 5 candidates, confirmed 4. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 3 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/js_parser/visit/visit_expr.rs:1226-1228 |
UnDelete arm ports p.deleteTarget assignment but not esbuild's post-visit reference-shape check |
| 🟡 Nit | src/js_parser/visit/visit_expr.rs:1227 |
e_dot dot-define substitution not gated on is_delete_target |
| 🟡 Nit | src/js_parser/visit/visit_expr.rs:1062-1066 |
[x][0] fold still fires when the index is a tagged-template tag |
| 🟡 Nit | src/js_parser/fold.rs:495-496 |
Sibling arms in maybe_rewrite_property_access still ignore is_delete_target/assign_target |
Annotations
Check failure on line 1228 in src/js_parser/visit/visit_expr.rs
claude / Claude Code Review
UnDelete arm ports p.deleteTarget assignment but not esbuild's post-visit reference-shape check
The `Op::UnDelete` arm now sets `p.delete_target` (matching esbuild's `p.deleteTarget = e.Value.Data`) but omits the paired post-visit check from the same esbuild block — the `canBeDeletedBefore/After` wrap that re-inserts `(0, …)` when a fold turns a *value* operand into a *reference*. So the mirror direction of the class this PR guards at :1060 is still miscompiled: `delete (0, o.p)` / `delete (null ?? o.p)` / `delete (0 || o.p)` / `delete (1 && o.p)` / `delete (true ? o.p : z)` all fold to `d
Check warning on line 1227 in src/js_parser/visit/visit_expr.rs
claude / Claude Code Review
e_dot dot-define substitution not gated on is_delete_target
The dot-define substitution at `e_dot` (visit_expr.rs:1353) checks only `in_.assign_target == None` and not `!is_delete_target`, so `delete process.env.NODE_ENV` with a string-valued `--define` still emits `delete "development"` — the direct sibling of the `delete FOO` → `delete 1` case the PR description cites and the new `delete user_undefined` test covers. Pre-existing output (unchanged by this PR), but `is_delete_target` is now live at :1343 and the `e_identifier` arm at :263 already carries
Check warning on line 1066 in src/js_parser/visit/visit_expr.rs
claude / Claude Code Review
[x][0] fold still fires when the index is a tagged-template tag
The guard now covers delete/call/assign but not tagged-template tags — the fourth position where a property Reference's base is observable via `this` (ECMA-262 §13.3.11 uses `EvaluateCall` with the tag Reference, identical to regular calls). `e_template` visits its tag via bare `p.visit_expr(...)` without setting `p.call_target`, and `p.template_tag` was dropped from the port (only the commented-out stubs at :1044/:1439 remain), so ``[o.m][0]`x` `` folds to ``o.m`x` `` and changes `this` from th
Check warning on line 496 in src/js_parser/fold.rs
claude / Claude Code Review
Sibling arms in maybe_rewrite_property_access still ignore is_delete_target/assign_target
Two sibling arms in this same `match` still fold a property reference to a value without checking `identifier_opts`: the `EString` arm (`"foo".length` → number) and the `ESpecial::HotEnabled|HotDisabled` arm (`import.meta.hot.<method>` → `undefined`). Under minify, `"foo".length = 5` emits `3 = 5` (invalid JS) and `delete import.meta.hot.accept` still emits `delete undefined` — the exact case this `can_inline` guard was added to prevent one arm above. Both are pre-existing and the triggers are c