js_parser: track template_tag so wrapper folds don't rebind this - #36735
Open
robobun wants to merge 8 commits into
Open
js_parser: track template_tag so wrapper folds don't rebind this#36735robobun wants to merge 8 commits into
this#36735robobun wants to merge 8 commits into
Claude / Claude Code Review
completed
Aug 1, 2026 in 26m 56s
Code review found 2 potential issues
Found 2 candidates, confirmed 2. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | src/js_parser/visit/visit_binary.rs:120-121 |
is_template_tag captured after left-child visit; nested tag in left arm clobbers p.template_tag |
| 🟡 Nit | src/js_parser/visit/visit_expr.rs:1505-1512 |
e_if CouldHaveSideEffects early-return bypasses is_template_tag guard |
Annotations
Check warning on line 121 in src/js_parser/visit/visit_binary.rs
claude / Claude Code Review
is_template_tag captured after left-child visit; nested tag in left arm clobbers p.template_tag
`is_template_tag` is captured in `visit_right_and_finish`, which runs *after* the left child has been visited — so a nested tagged template in the left subtree (e.g. ``(false && a`x`, obj.m)`y```) overwrites `p.template_tag` before this check, and the comma folds to bare ``obj.m`y```, rebinding `this`. This is the same pre-existing structural weakness as `is_call_target` two lines up (the PR faithfully copies that pattern), so not a regression — capturing both flags in `check_and_prepare` before
Check warning on line 1512 in src/js_parser/visit/visit_expr.rs
claude / Claude Code Review
e_if CouldHaveSideEffects early-return bypasses is_template_tag guard
The new `(is_call_target || is_template_tag)` guard here is unreachable when the ternary test is an array/object/class literal: `to_boolean` on `[]`/`{}` returns `CouldHaveSideEffects`, so the branch at 1498-1503 fires first, and when `simplify_unused_expr` returns `None` (e.g. `[]`, `{a:1}`), `E::Missing.join_with_comma(e_.yes)` short-circuits to bare `e_.yes` — so `` ([] ? obj.m : 0)`x` `` still folds to `` obj.m`x` `` (esbuild emits `(0, obj.m)` here). Same hole exists in the `no` arm at 1527
Loading