Skip to content

Commit b540854

Browse files
wycatswycats
committed
Fixes Ember keyword shadowing
This commit consistently fixes the interaction between keywords and lexical variables (JS or Handlebars) to match the specified behavior: keywords are **always** superseded by in-scope lexical variables. Ember keywords are implemented as AST transformations, and there were two sources of bugs that made implementation of these keywords inconsistent with the specified behavior: 1. In some cases, AST transforms applied to situations where the keyword in question was shadowed by a Handlebars block param. This means that the variable would be silently overridden by the AST transform. 2. In all cases, AST transforms applied when the keyword was shadowed by a lexical variable (i.e. in-scope JS variable) of the same name. This isn't supposed to happen, as lexical JS variables are supposed to have the same rules as variables bound via JS block params. This commit has tests for many of these cases. Some additional tests are forthcoming, mostly in cases where the transform in question wasn't tested at all before this change.
1 parent e1abff5 commit b540854

12 files changed

+127
-63
lines changed

‎packages/ember-template-compiler/lib/plugins/assert-against-attrs.ts

Lines changed: 6 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { assert, deprecate } from '@ember/debug';
22
import type { AST, ASTPlugin } from '@glimmer/syntax';
33
import calculateLocationDisplay from '../system/calculate-location-display';
44
import type { EmberASTPluginEnvironment } from '../types';
5+
import { trackLocals } from './utils';
56

67
/**
78
@module ember
@@ -23,48 +24,16 @@ import type { EmberASTPluginEnvironment } from '../types';
2324
export default function assertAgainstAttrs(env: EmberASTPluginEnvironment): ASTPlugin {
2425
let { builders: b } = env.syntax;
2526
let moduleName = env.meta?.moduleName;
26-
27-
let stack: string[][] = [[]];
28-
29-
function updateBlockParamsStack(blockParams: string[]) {
30-
let parent = stack[stack.length - 1];
31-
assert('has parent', parent);
32-
stack.push(parent.concat(blockParams));
33-
}
27+
let { hasLocal, visitor } = trackLocals(env);
3428

3529
return {
3630
name: 'assert-against-attrs',
3731

3832
visitor: {
39-
Template: {
40-
enter(node: AST.Template) {
41-
updateBlockParamsStack(node.blockParams);
42-
},
43-
exit() {
44-
stack.pop();
45-
},
46-
},
47-
48-
Block: {
49-
enter(node: AST.Block) {
50-
updateBlockParamsStack(node.blockParams);
51-
},
52-
exit() {
53-
stack.pop();
54-
},
55-
},
56-
57-
ElementNode: {
58-
enter(node: AST.ElementNode) {
59-
updateBlockParamsStack(node.blockParams);
60-
},
61-
exit() {
62-
stack.pop();
63-
},
64-
},
33+
...visitor,
6534

6635
PathExpression(node: AST.PathExpression): AST.Node | void {
67-
if (isAttrs(node, stack[stack.length - 1]!)) {
36+
if (isAttrs(node, hasLocal)) {
6837
assert(
6938
`Using {{attrs}} to reference named arguments is not supported. {{${
7039
node.original
@@ -106,12 +75,8 @@ export default function assertAgainstAttrs(env: EmberASTPluginEnvironment): ASTP
10675
};
10776
}
10877

109-
function isAttrs(node: AST.PathExpression, symbols: string[]) {
110-
return (
111-
node.head.type === 'VarHead' &&
112-
node.head.name === 'attrs' &&
113-
symbols.indexOf(node.head.name) === -1
114-
);
78+
function isAttrs(node: AST.PathExpression, hasLocal: (k: string) => boolean) {
79+
return node.head.type === 'VarHead' && node.head.name === 'attrs' && !hasLocal(node.head.name);
11580
}
11681

11782
function isThisDotAttrs(node: AST.PathExpression) {

‎packages/ember-template-compiler/lib/plugins/assert-against-named-outlets.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { assert } from '@ember/debug';
22
import type { AST, ASTPlugin } from '@glimmer/syntax';
33
import calculateLocationDisplay from '../system/calculate-location-display';
44
import type { EmberASTPluginEnvironment } from '../types';
5+
import { trackLocals } from './utils';
56

67
/**
78
@module ember
@@ -15,16 +16,19 @@ import type { EmberASTPluginEnvironment } from '../types';
1516
*/
1617
export default function assertAgainstNamedOutlets(env: EmberASTPluginEnvironment): ASTPlugin {
1718
let moduleName = env.meta?.moduleName;
19+
let { hasLocal, visitor } = trackLocals(env);
1820

1921
return {
2022
name: 'assert-against-named-outlets',
2123

2224
visitor: {
25+
...visitor,
2326
MustacheStatement(node: AST.MustacheStatement) {
2427
if (
2528
node.path.type === 'PathExpression' &&
2629
node.path.original === 'outlet' &&
27-
node.params[0]
30+
node.params[0] &&
31+
!hasLocal('outlet')
2832
) {
2933
let sourceInformation = calculateLocationDisplay(moduleName, node.loc);
3034
assert(

‎packages/ember-template-compiler/lib/plugins/assert-input-helper-without-block.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,16 +2,20 @@ import { assert } from '@ember/debug';
22
import type { AST, ASTPlugin } from '@glimmer/syntax';
33
import calculateLocationDisplay from '../system/calculate-location-display';
44
import type { EmberASTPluginEnvironment } from '../types';
5-
import { isPath } from './utils';
5+
import { isPath, trackLocals } from './utils';
66

77
export default function errorOnInputWithContent(env: EmberASTPluginEnvironment): ASTPlugin {
88
let moduleName = env.meta?.moduleName;
9+
let { hasLocal, visitor } = trackLocals(env);
910

1011
return {
1112
name: 'assert-input-helper-without-block',
1213

1314
visitor: {
15+
...visitor,
1416
BlockStatement(node: AST.BlockStatement) {
17+
if (hasLocal('input')) return;
18+
1519
if (isPath(node.path) && node.path.original === 'input') {
1620
assert(assertMessage(moduleName, node));
1721
}

‎packages/ember-template-compiler/lib/plugins/transform-action-syntax.ts

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import type { AST, ASTPlugin } from '@glimmer/syntax';
22
import type { Builders, EmberASTPluginEnvironment } from '../types';
3-
import { isPath } from './utils';
3+
import { isPath, trackLocals } from './utils';
44

55
/**
66
@module ember
@@ -27,36 +27,41 @@ import { isPath } from './utils';
2727
@class TransformActionSyntax
2828
*/
2929

30-
export default function transformActionSyntax({ syntax }: EmberASTPluginEnvironment): ASTPlugin {
31-
let { builders: b } = syntax;
30+
export default function transformActionSyntax(env: EmberASTPluginEnvironment): ASTPlugin {
31+
let { builders: b } = env.syntax;
32+
let { hasLocal, visitor } = trackLocals(env);
3233

3334
return {
3435
name: 'transform-action-syntax',
3536

3637
visitor: {
38+
...visitor,
3739
ElementModifierStatement(node: AST.ElementModifierStatement) {
38-
if (isAction(node)) {
40+
if (isAction(node, hasLocal)) {
3941
insertThisAsFirstParam(node, b);
4042
}
4143
},
4244

4345
MustacheStatement(node: AST.MustacheStatement) {
44-
if (isAction(node)) {
46+
if (isAction(node, hasLocal)) {
4547
insertThisAsFirstParam(node, b);
4648
}
4749
},
4850

4951
SubExpression(node: AST.SubExpression) {
50-
if (isAction(node)) {
52+
if (isAction(node, hasLocal)) {
5153
insertThisAsFirstParam(node, b);
5254
}
5355
},
5456
},
5557
};
5658
}
5759

58-
function isAction(node: AST.ElementModifierStatement | AST.MustacheStatement | AST.SubExpression) {
59-
return isPath(node.path) && node.path.original === 'action';
60+
function isAction(
61+
node: AST.ElementModifierStatement | AST.MustacheStatement | AST.SubExpression,
62+
hasLocal: (k: string) => boolean
63+
) {
64+
return isPath(node.path) && node.path.original === 'action' && !hasLocal('action');
6065
}
6166

6267
function insertThisAsFirstParam(

‎packages/ember-template-compiler/lib/plugins/transform-each-track-array.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import type { AST, ASTPlugin } from '@glimmer/syntax';
22
import { assert } from '@ember/debug';
33
import type { EmberASTPluginEnvironment } from '../types';
4-
import { isPath } from './utils';
4+
import { isPath, trackLocals } from './utils';
55

66
/**
77
@module ember
@@ -25,13 +25,15 @@ import { isPath } from './utils';
2525
*/
2626
export default function transformEachTrackArray(env: EmberASTPluginEnvironment): ASTPlugin {
2727
let { builders: b } = env.syntax;
28+
let { hasLocal, visitor } = trackLocals(env);
2829

2930
return {
3031
name: 'transform-each-track-array',
3132

3233
visitor: {
34+
...visitor,
3335
BlockStatement(node: AST.BlockStatement): AST.Node | void {
34-
if (isPath(node.path) && node.path.original === 'each') {
36+
if (isPath(node.path) && node.path.original === 'each' && !hasLocal('each')) {
3537
let firstParam = node.params[0];
3638
assert('has firstParam', firstParam);
3739

‎packages/ember-template-compiler/lib/plugins/transform-resolutions.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ const TARGETS = Object.freeze(['helper', 'modifier']);
6767
export default function transformResolutions(env: EmberASTPluginEnvironment): ASTPlugin {
6868
let { builders: b } = env.syntax;
6969
let moduleName = env.meta?.moduleName;
70-
let { hasLocal, node: tracker } = trackLocals();
70+
let { hasLocal, node: tracker } = trackLocals(env);
7171
let seen: Set<AST.Node> | undefined;
7272

7373
return {

‎packages/ember-template-compiler/lib/plugins/transform-wrap-mount-and-outlet.ts

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -37,15 +37,13 @@ import { isPath, trackLocals } from './utils';
3737
export default function transformWrapMountAndOutlet(env: EmberASTPluginEnvironment): ASTPlugin {
3838
let { builders: b } = env.syntax;
3939

40-
let { hasLocal, node } = trackLocals();
40+
let { hasLocal, visitor } = trackLocals(env);
4141

4242
return {
4343
name: 'transform-wrap-mount-and-outlet',
4444

4545
visitor: {
46-
Template: node,
47-
ElementNode: node,
48-
Block: node,
46+
...visitor,
4947

5048
MustacheStatement(node: AST.MustacheStatement): AST.Node | void {
5149
if (

‎packages/ember-template-compiler/lib/plugins/utils.ts

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import type { AST } from '@glimmer/syntax';
2+
import type { EmberASTPluginEnvironment } from '../types';
23

34
export function isPath(node: AST.Node): node is AST.PathExpression {
45
return node.type === 'PathExpression';
@@ -20,7 +21,11 @@ function getLocalName(node: string | AST.VarHead): string {
2021
}
2122
}
2223

23-
export function trackLocals() {
24+
export function inScope(env: EmberASTPluginEnvironment, name: string): boolean {
25+
return Boolean(env.lexicalScope?.(name));
26+
}
27+
28+
export function trackLocals(env: EmberASTPluginEnvironment) {
2429
let locals = new Map();
2530

2631
let node = {
@@ -49,7 +54,12 @@ export function trackLocals() {
4954
};
5055

5156
return {
52-
hasLocal: (key: string) => locals.has(key),
57+
hasLocal: (key: string) => locals.has(key) || inScope(env, key),
5358
node,
59+
visitor: {
60+
Template: node,
61+
ElementNode: node,
62+
Block: node,
63+
},
5464
};
5565
}

‎packages/ember-template-compiler/lib/types.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ export interface EmberPrecompileOptions extends PrecompileOptions {
2323
isProduction?: boolean;
2424
moduleName?: string;
2525
plugins?: Plugins;
26+
lexicalScope?: (name: string) => boolean;
2627
}
2728

2829
export type EmberASTPluginEnvironment = ASTPluginEnvironment & EmberPrecompileOptions;

‎packages/ember-template-compiler/tests/plugins/assert-against-attrs-test.js

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import TransformTestCase from '../utils/transform-test-case';
2-
import { moduleFor, RenderingTestCase } from 'internal-test-helpers';
2+
import { defineComponent, moduleFor, RenderingTestCase } from 'internal-test-helpers';
33

44
moduleFor(
55
'ember-template-compiler: assert against attrs',
@@ -64,6 +64,14 @@ moduleFor(
6464
this.assertComponentElement(this.firstChild, { content: 'foo' });
6565
}
6666

67+
["@test it doesn't assert lexical scope values"]() {
68+
let component = defineComponent({ attrs: 'just a string' }, `It's {{attrs}}`);
69+
this.registerComponent('root', { ComponentClass: component });
70+
this.render('<Root />');
71+
this.assertHTML("It's just a string");
72+
this.assertStableRerender();
73+
}
74+
6775
["@test it doesn't assert component block params"]() {
6876
this.registerComponent('foo', {
6977
template: '{{yield "foo"}}',

0 commit comments

Comments
 (0)