From 030f37388eda4de99dcaa7a5b6174432a424320d Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Thu, 20 Dec 2018 12:20:06 -0800 Subject: [PATCH] [BUGFIX release] local variable shadowing assert Previously we considered the block params to shadow the params, hash, attributes and named arguments on the same block/element, which was incorrect. For example: ```hbs {{#let (concat "foo" "bar") as |concat|}} ... {{/let}} ``` In this case the assertion code incorrectly believed the `concat` in the sub-expression invocation was being shadowed. This is now fixed. Also fixes a bug where we incorrectly transformed mustaches in attribute positions: ```hbs {{#let ... as |foo|}}
{{/let}} ``` ...became... ```hbs {{#let ... as |foo|}}
{{/let}} ``` This is clearly incorrect and has been fixed here as well. Fixes #17370 --- ...al-variable-shadowing-helper-invocation.ts | 63 ++-- .../plugins/transform-component-invocation.ts | 45 ++- ...riable-shadowing-helper-invocation-test.js | 316 +++++++++++++++++- 3 files changed, 385 insertions(+), 39 deletions(-) diff --git a/packages/ember-template-compiler/lib/plugins/assert-local-variable-shadowing-helper-invocation.ts b/packages/ember-template-compiler/lib/plugins/assert-local-variable-shadowing-helper-invocation.ts index 84b152d2698..80d75d924d1 100644 --- a/packages/ember-template-compiler/lib/plugins/assert-local-variable-shadowing-helper-invocation.ts +++ b/packages/ember-template-compiler/lib/plugins/assert-local-variable-shadowing-helper-invocation.ts @@ -12,9 +12,9 @@ export default function assertLocalVariableShadowingHelperInvocation( name: 'assert-local-variable-shadowing-helper-invocation', visitor: { - BlockStatement: { - enter(node: AST.BlockStatement) { - locals.push(node.program.blockParams); + Program: { + enter(node: AST.Program) { + locals.push(node.blockParams); }, exit() { @@ -23,29 +23,48 @@ export default function assertLocalVariableShadowingHelperInvocation( }, ElementNode: { - enter(node: AST.ElementNode) { - locals.push(node.blockParams); - }, + keys: { + children: { + enter(node: AST.ElementNode) { + locals.push(node.blockParams); + }, - exit() { - locals.pop(); + exit() { + locals.pop(); + }, + }, }, }, + MustacheStatement(node: AST.MustacheStatement) { + if (isPath(node.path) && hasArguments(node)) { + let name = node.path.parts[0]; + let type = 'helper'; + + assert( + `${messageFor(name, type)} ${calculateLocationDisplay(moduleName, node.loc)}`, + !isLocalVariable(node.path, locals) + ); + } + }, + SubExpression(node: AST.SubExpression) { + let name = node.path.parts[0]; + let type = 'helper'; + assert( - `${messageFor(node)} ${calculateLocationDisplay(moduleName, node.loc)}`, + `${messageFor(name, type)} ${calculateLocationDisplay(moduleName, node.loc)}`, !isLocalVariable(node.path, locals) ); }, ElementModifierStatement(node: AST.ElementModifierStatement) { - // The ElementNode get visited first, but modifiers are more of a sibling - // than a child in the lexical scope (we aren't evaluated in its "block") - // so any locals introduced by the last element doesn't count + let name = node.path.parts[0]; + let type = 'modifier'; + assert( - `${messageFor(node)} ${calculateLocationDisplay(moduleName, node.loc)}`, - !isLocalVariable(node.path, locals.slice(0, -1)) + `${messageFor(name, type)} ${calculateLocationDisplay(moduleName, node.loc)}`, + !isLocalVariable(node.path, locals) ); }, }, @@ -53,21 +72,21 @@ export default function assertLocalVariableShadowingHelperInvocation( } function isLocalVariable(node: AST.PathExpression, locals: string[][]): boolean { - return !node.this && hasLocalVariable(node.parts[0], locals); + return !node.this && node.parts.length === 1 && hasLocalVariable(node.parts[0], locals); } function hasLocalVariable(name: string, locals: string[][]): boolean { return locals.some(names => names.indexOf(name) !== -1); } -function messageFor(node: AST.SubExpression | AST.ElementModifierStatement): string { - let type = isSubExpression(node) ? 'helper' : 'modifier'; - let name = node.path.parts[0]; +function messageFor(name: string, type: string): string { return `Cannot invoke the \`${name}\` ${type} because it was shadowed by a local variable (i.e. a block param) with the same name. Please rename the local variable to resolve the conflict.`; } -function isSubExpression( - node: AST.SubExpression | AST.ElementModifierStatement -): node is AST.SubExpression { - return node.type === 'SubExpression'; +function isPath(node: AST.Node): node is AST.PathExpression { + return node.type === 'PathExpression'; +} + +function hasArguments(node: AST.MustacheStatement): boolean { + return node.params.length > 0 || node.hash.pairs.length > 0; } diff --git a/packages/ember-template-compiler/lib/plugins/transform-component-invocation.ts b/packages/ember-template-compiler/lib/plugins/transform-component-invocation.ts index 20d5314044f..5324c9648ea 100644 --- a/packages/ember-template-compiler/lib/plugins/transform-component-invocation.ts +++ b/packages/ember-template-compiler/lib/plugins/transform-component-invocation.ts @@ -124,13 +124,14 @@ export default function transformComponentInvocation(env: ASTPluginEnvironment): let { moduleName } = env.meta; let { builders: b } = env.syntax; let locals: string[][] = []; + let isAttrs = false; return { name: 'transform-component-invocation', visitor: { - ElementNode: { - enter(node: AST.ElementNode) { + Program: { + enter(node: AST.Program) { locals.push(node.blockParams); }, @@ -139,23 +140,39 @@ export default function transformComponentInvocation(env: ASTPluginEnvironment): }, }, - BlockStatement: { - enter(node: AST.BlockStatement) { - // check this before we push the new locals - if (isBlockInvocation(node, locals)) { - wrapInComponent(moduleName, node, b); - } - - locals.push(node.program.blockParams); + ElementNode: { + keys: { + attributes: { + enter() { + isAttrs = true; + }, + + exit() { + isAttrs = false; + }, + }, + + children: { + enter(node: AST.ElementNode) { + locals.push(node.blockParams); + }, + + exit() { + locals.pop(); + }, + }, }, + }, - exit() { - locals.pop(); - }, + BlockStatement(node: AST.BlockStatement) { + // check this before we push the new locals + if (isBlockInvocation(node, locals)) { + wrapInComponent(moduleName, node, b); + } }, MustacheStatement(node: AST.MustacheStatement): AST.Node | void { - if (isInlineInvocation(node, locals)) { + if (!isAttrs && isInlineInvocation(node, locals)) { wrapInComponent(moduleName, node, b); } }, diff --git a/packages/ember-template-compiler/tests/plugins/assert-local-variable-shadowing-helper-invocation-test.js b/packages/ember-template-compiler/tests/plugins/assert-local-variable-shadowing-helper-invocation-test.js index 387e9006c2d..313334fe2b0 100644 --- a/packages/ember-template-compiler/tests/plugins/assert-local-variable-shadowing-helper-invocation-test.js +++ b/packages/ember-template-compiler/tests/plugins/assert-local-variable-shadowing-helper-invocation-test.js @@ -35,7 +35,7 @@ moduleFor( { moduleName: 'baz/foo-bar' } ); - // Not an invocation + // Not invocations compile( ` @@ -44,6 +44,14 @@ moduleFor( {{/let}}`, { moduleName: 'baz/foo-bar' } ); + + compile( + ` + {{#let (concat foo) as |concat|}} + {{input value=concat}} + {{/let}}`, + { moduleName: 'baz/foo-bar' } + ); } [`@test element nodes shadowing sub-expression invocations`]() { @@ -77,7 +85,7 @@ moduleFor( { moduleName: 'baz/foo-bar' } ); - // Not an invocation + // Not invocations compile( ` @@ -86,6 +94,14 @@ moduleFor( `, { moduleName: 'baz/foo-bar' } ); + + compile( + ` + + {{input value=concat}} + `, + { moduleName: 'baz/foo-bar' } + ); } [`@test deeply nested sub-expression invocations`]() { @@ -136,7 +152,7 @@ moduleFor( { moduleName: 'baz/foo-bar' } ); - // Not an invocation + // Not invocations compile( ` @@ -149,6 +165,300 @@ moduleFor( {{/let}}`, { moduleName: 'baz/foo-bar' } ); + + compile( + ` + {{#let (foo foo) as |foo|}} + + {{#each (baz baz) as |baz|}} + {{concat foo bar baz}} + {{/each}} + + {{/let}}`, + { moduleName: 'baz/foo-bar' } + ); + } + + [`@test block statements shadowing attribute sub-expression invocations`]() { + expectAssertion(() => { + compile( + ` + {{#let foo as |foo|}} +
+ {{/let}}`, + { moduleName: 'baz/foo-bar' } + ); + }, `Cannot invoke the \`foo\` helper because it was shadowed by a local variable (i.e. a block param) with the same name. Please rename the local variable to resolve the conflict. ('baz/foo-bar' @ L3:C32) `); + + // Not shadowed + + compile( + ` + {{#let foo as |foo|}}{{/let}} +
+
`, + { moduleName: 'baz/foo-bar' } + ); + + // Not invocations + + compile( + ` + {{#let foo as |foo|}} +
+ {{/let}}`, + { moduleName: 'baz/foo-bar' } + ); + + compile( + ` + {{#let (foo foo) as |foo|}} +
+ {{/let}}`, + { moduleName: 'baz/foo-bar' } + ); + } + + [`@test element nodes shadowing attribute sub-expression invocations`]() { + expectAssertion(() => { + compile( + ` + +
+ `, + { moduleName: 'baz/foo-bar' } + ); + }, `Cannot invoke the \`foo\` helper because it was shadowed by a local variable (i.e. a block param) with the same name. Please rename the local variable to resolve the conflict. ('baz/foo-bar' @ L3:C32) `); + + // Not shadowed + + compile( + ` + +
+
`, + { moduleName: 'baz/foo-bar' } + ); + + // Not invocations + + compile( + ` + +
+ `, + { moduleName: 'baz/foo-bar' } + ); + + compile( + ` + +
+ `, + { moduleName: 'baz/foo-bar' } + ); + } + + [`@test deeply nested attribute sub-expression invocations`]() { + expectAssertion(() => { + compile( + ` + {{#let foo as |foo|}} + + {{#each items as |baz|}} +
+ {{/each}} + + {{/let}}`, + { moduleName: 'baz/foo-bar' } + ); + }, `Cannot invoke the \`foo\` helper because it was shadowed by a local variable (i.e. a block param) with the same name. Please rename the local variable to resolve the conflict. ('baz/foo-bar' @ L5:C36) `); + + // Not shadowed + + compile( + ` + {{#let foo as |foo|}} + + {{#each items as |baz|}} + {{/each}} +
+
+ +
+
+ {{/let}} +
+
`, + { moduleName: 'baz/foo-bar' } + ); + + // Not invocations + + compile( + ` + {{#let foo as |foo|}} + + {{#each items as |baz|}} +
+ {{/each}} + + {{/let}}`, + { moduleName: 'baz/foo-bar' } + ); + + compile( + ` + {{#let (foo foo) as |foo|}} + + {{#each (baz baz) as |baz|}} +
+ {{/each}} + + {{/let}}`, + { moduleName: 'baz/foo-bar' } + ); + } + + [`@test block statements shadowing attribute mustache invocations`]() { + expectAssertion(() => { + compile( + ` + {{#let foo as |foo|}} +
+ {{/let}}`, + { moduleName: 'baz/foo-bar' } + ); + }, `Cannot invoke the \`foo\` helper because it was shadowed by a local variable (i.e. a block param) with the same name. Please rename the local variable to resolve the conflict. ('baz/foo-bar' @ L3:C23) `); + + // Not shadowed + + compile( + ` + {{#let foo as |foo|}}{{/let}} +
+
`, + { moduleName: 'baz/foo-bar' } + ); + + // Not invocations + + compile( + ` + {{#let foo as |foo|}} +
+ {{/let}}`, + { moduleName: 'baz/foo-bar' } + ); + + compile( + ` + {{#let (concat foo) as |concat|}} +
+ {{/let}}`, + { moduleName: 'baz/foo-bar' } + ); + } + + [`@test element nodes shadowing attribute mustache invocations`]() { + expectAssertion(() => { + compile( + ` + +
+ `, + { moduleName: 'baz/foo-bar' } + ); + }, `Cannot invoke the \`foo\` helper because it was shadowed by a local variable (i.e. a block param) with the same name. Please rename the local variable to resolve the conflict. ('baz/foo-bar' @ L3:C23) `); + + // Not shadowed + + compile( + ` + +
+
`, + { moduleName: 'baz/foo-bar' } + ); + + // Not invocations + + compile( + ` + +
+ `, + { moduleName: 'baz/foo-bar' } + ); + + compile( + ` + +
+ `, + { moduleName: 'baz/foo-bar' } + ); + } + + [`@test deeply nested attribute mustache invocations`]() { + expectAssertion(() => { + compile( + ` + {{#let foo as |foo|}} + + {{#each items as |baz|}} +
+ {{/each}} + + {{/let}}`, + { moduleName: 'baz/foo-bar' } + ); + }, `Cannot invoke the \`foo\` helper because it was shadowed by a local variable (i.e. a block param) with the same name. Please rename the local variable to resolve the conflict. ('baz/foo-bar' @ L5:C27) `); + + // Not shadowed + + compile( + ` + {{#let foo as |foo|}} + + {{#each items as |baz|}} + {{/each}} +
+
+ +
+
+ {{/let}} +
+
`, + { moduleName: 'baz/foo-bar' } + ); + + // Not invocations + + compile( + ` + {{#let foo as |foo|}} + + {{#each items as |baz|}} +
+ {{/each}} + + {{/let}}`, + { moduleName: 'baz/foo-bar' } + ); + + compile( + ` + {{#let (foo foo) as |foo|}} + + {{#each (baz baz) as |baz|}} +
+ {{/each}} + + {{/let}}`, + { moduleName: 'baz/foo-bar' } + ); } [`@test block statements shadowing mustache invocations`](assert) {