Skip to content

Commit 571a94d

Browse files
NullVoxPopuliclaude
andcommitted
Revert PathExpression legacy-upgrade in convertLocations
The previous commit (83ccd8d) tried to address a review comment by upgrading every PathExpression through buildLegacyPath during convertLocations, so that setting `.original` would flow through to `head`/`tail` automatically. The auto-import-builtins plugin was then simplified back to `node.path.original = rewritten`. That combination passed the browser test suite locally but regressed two smoke-test scenarios: {{fn}} as keyword (but it is shadowed): it works {{on}} as keyword (but it is shadowed): it works These `.gjs` tests verify that a lexical JS binding (e.g. `const fn = ...`) shadows the built-in keyword inside the template. The regression isn't due to the simpler auto-import-builtins — the plugin correctly skips rewriting when `hasLocal('fn')` is true — but rather something about the buildLegacyPath-upgraded PathExpression interacts poorly with the lexical-scope path inside the v2 normalizer for this specific case. Without the upgrade, the previous commit's auto-import-builtins simplification also has to be reverted: without head/tail-rewriting in the plugin, the runtime compilation path (env.meta.emberRuntime) leaves `node.path.head` pointing at the original keyword name while `node.path.original` contains the rewritten `__ember_keywords__.<name>`, and the v2 normalizer reads head.name. So: - Revert convertLocations: drop the buildLegacyPath upgrade and the import. PathExpression stays as plain grammar-emitted data. - Restore the manual head/tail rewrite in auto-import-builtins, with a comment explaining why it's needed (the grammar emits PathExpression as a plain object whose `original`, `head`, and `tail` are independent data properties, so they must be kept in sync by hand). Verified locally: browser test suite (9138/9138 pass) and all 3 classic-basics smoke-test scenarios (pass 3, fail 0). The reviewer's architectural point — that setting `.original` should propagate to `head`/`tail` — is still valid, but realizing it requires having the Peggy grammar emit PathExpression nodes through buildLegacyPath directly (not as a post-process step). That's a larger change that can happen in a follow-up. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
1 parent b4b6665 commit 571a94d

2 files changed

Lines changed: 18 additions & 16 deletions

File tree

packages/@ember/template-compiler/lib/plugins/auto-import-builtins.ts

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,24 @@ function rewriteKeyword(
5151
name,
5252
});
5353
} else if (env.meta?.emberRuntime) {
54-
node.path.original = env.meta.emberRuntime.lookupKeyword(name);
54+
const rewritten = env.meta.emberRuntime.lookupKeyword(name);
55+
node.path.original = rewritten;
56+
// Also update head and tail so the v2 normalizer resolves correctly.
57+
// e.g., "__ember_keywords__.on" → head="__ember_keywords__", tail=["on"]
58+
// (The Peggy grammar emits PathExpression nodes as plain objects where
59+
// `original`, `head`, and `tail` are independent data properties, so we
60+
// need to keep them in sync by hand here.)
61+
const parts = rewritten.split('.');
62+
if (parts.length >= 2) {
63+
const headName = parts[0] as string;
64+
node.path.head = {
65+
type: 'VarHead',
66+
name: headName,
67+
original: headName,
68+
loc: node.path.head.loc,
69+
} as AST.VarHead;
70+
node.path.tail = parts.slice(1);
71+
}
5572
}
5673
}
5774

packages/@glimmer/syntax/lib/parser/tokenizer-event-handlers.ts

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import * as src from '../source/api';
1111
import { generateSyntaxError } from '../syntax-error';
1212
import traverse from '../traversal/traverse';
1313
import Walker from '../traversal/walker';
14-
import { buildLegacyPath } from '../v1/legacy-interop';
1514
import publicBuilder from '../v1/public-builders';
1615

1716
// ============================================================================
@@ -473,12 +472,6 @@ function isPeggySpan(
473472

474473
/**
475474
* Recursively convert plain {start, end} location objects to SourceSpan instances.
476-
*
477-
* PathExpression nodes are additionally upgraded via `buildLegacyPath` so that
478-
* `node.path.original` is a live getter/setter backed by `head` and `tail`.
479-
* This way downstream AST plugins (e.g. auto-import-builtins) can assign
480-
* `node.path.original = 'foo.bar'` and have `head`/`tail` update automatically,
481-
* matching the behavior of the legacy parser-builders-produced nodes.
482475
*/
483476
// eslint-disable-next-line @typescript-eslint/no-explicit-any
484477
function convertLocations(node: any, source: src.Source): any {
@@ -492,10 +485,6 @@ function convertLocations(node: any, source: src.Source): any {
492485
let result: any = {};
493486

494487
for (let key of Object.keys(node)) {
495-
// PathExpression owns its own `original` via buildLegacyPath's getter.
496-
// Drop the grammar-emitted data field so the getter wins.
497-
if (node.type === 'PathExpression' && key === 'original') continue;
498-
499488
let val = node[key];
500489
if ((key === 'loc' || key === 'openTag' || key === 'closeTag') && isPeggySpan(val)) {
501490
result[key] = peggySpanToSourceSpan(source, val);
@@ -506,10 +495,6 @@ function convertLocations(node: any, source: src.Source): any {
506495
}
507496
}
508497

509-
if (result.type === 'PathExpression') {
510-
return buildLegacyPath({ head: result.head, tail: result.tail, loc: result.loc });
511-
}
512-
513498
return result;
514499
}
515500

0 commit comments

Comments
 (0)