From f66ca05540309260a507b732b8d5a17ae88b5ada Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Thu, 18 Oct 2018 18:27:09 -0700 Subject: [PATCH] [BUGFIX beta] Assert when local variables shadow helper invocations ```hbs {{#let this.foo as |foo|}} {{concat (foo)}} ~~~ shadowed helper invocation! {{/let}} ``` Previously, this would have tried to resolve and invoke the helper `foo`, ignoring the presence of the `foo` local variable. This is inconsistent with our general lexical lookup rules. This is now an error (assertion). This paves the way for allowing "contextual helpers" in the future, where the local variable `foo` contains an invocable helper value. Partially addresses #17121, allowing the rest of the bugs to be fixed in Glimmer VM. --- ...al-variable-shadowing-helper-invocation.ts | 56 +++++ .../lib/plugins/index.ts | 2 + ...riable-shadowing-helper-invocation-test.js | 228 ++++++++++++++++++ 3 files changed, 286 insertions(+) create mode 100644 packages/ember-template-compiler/lib/plugins/assert-local-variable-shadowing-helper-invocation.ts create mode 100644 packages/ember-template-compiler/tests/plugins/assert-local-variable-shadowing-helper-invocation-test.js 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 new file mode 100644 index 00000000000..38bf1a11c7e --- /dev/null +++ b/packages/ember-template-compiler/lib/plugins/assert-local-variable-shadowing-helper-invocation.ts @@ -0,0 +1,56 @@ +import { assert } from '@ember/debug'; +import { AST, ASTPlugin, ASTPluginEnvironment } from '@glimmer/syntax'; +import calculateLocationDisplay from '../system/calculate-location-display'; + +export default function assertLocalVariableShadowingHelperInvocation( + env: ASTPluginEnvironment +): ASTPlugin { + let { moduleName } = env.meta; + let locals: string[][] = []; + + return { + name: 'assert-local-variable-shadowing-helper-invocation', + + visitor: { + BlockStatement: { + enter(node: AST.BlockStatement) { + locals.push(node.program.blockParams); + }, + + exit() { + locals.pop(); + }, + }, + + ElementNode: { + enter(node: AST.ElementNode) { + locals.push(node.blockParams); + }, + + exit() { + locals.pop(); + }, + }, + + SubExpression(node: AST.SubExpression) { + assert( + `${messageFor(node)} ${calculateLocationDisplay(moduleName, node.loc)}`, + !isLocalVariable(node.path, locals) + ); + }, + }, + }; +} + +function isLocalVariable(node: AST.PathExpression, locals: string[][]): boolean { + return !node.this && 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): string { + let name = node.path.parts[0]; + return `Cannot invoke the \`${name}\` 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.`; +} diff --git a/packages/ember-template-compiler/lib/plugins/index.ts b/packages/ember-template-compiler/lib/plugins/index.ts index aaa06b6fbfb..db5a90d864e 100644 --- a/packages/ember-template-compiler/lib/plugins/index.ts +++ b/packages/ember-template-compiler/lib/plugins/index.ts @@ -1,5 +1,6 @@ import AssertIfHelperWithoutArguments from './assert-if-helper-without-arguments'; import AssertInputHelperWithoutBlock from './assert-input-helper-without-block'; +import AssertLocalVariableShadowingHelperInvocation from './assert-local-variable-shadowing-helper-invocation'; import AssertReservedNamedArguments from './assert-reserved-named-arguments'; import AssertSplattributeExpressions from './assert-splattribute-expression'; import DeprecateSendAction from './deprecate-send-action'; @@ -34,6 +35,7 @@ const transforms: Array = [ TransformAttrsIntoArgs, TransformEachInIntoEach, TransformHasBlockSyntax, + AssertLocalVariableShadowingHelperInvocation, AssertInputHelperWithoutBlock, TransformInElement, AssertIfHelperWithoutArguments, 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 new file mode 100644 index 00000000000..7adb7f1e427 --- /dev/null +++ b/packages/ember-template-compiler/tests/plugins/assert-local-variable-shadowing-helper-invocation-test.js @@ -0,0 +1,228 @@ +import { compile } from '../../index'; +import { moduleFor, AbstractTestCase } from 'internal-test-helpers'; + +moduleFor( + 'ember-template-compiler: assert-local-variable-shadowing-helper-invocation', + class extends AbstractTestCase { + [`@test block statements shadowing sub-expression invocations`]() { + expectAssertion(() => { + compile( + ` + {{#let foo as |foo|}} + {{concat (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:C21) `); + + expectAssertion(() => { + compile( + ` + {{#let foo as |foo|}} + {{concat (foo bar baz)}} + {{/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:C21) `); + + // Not shadowed + + compile( + ` + {{#let foo as |foo|}}{{/let}} + {{concat (foo)}} + {{concat (foo bar baz)}}`, + { moduleName: 'baz/foo-bar' } + ); + + // Not an invocation + + compile( + ` + {{#let foo as |foo|}} + {{concat foo}} + {{/let}}`, + { moduleName: 'baz/foo-bar' } + ); + } + + [`@test element nodes shadowing sub-expression invocations`]() { + expectAssertion(() => { + compile( + ` + + {{concat (foo)}} + `, + { 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:C21) `); + + expectAssertion(() => { + compile( + ` + + {{concat (foo bar baz)}} + `, + { 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:C21) `); + + // Not shadowed + + compile( + ` + + {{concat (foo)}} + {{concat (foo bar baz)}}`, + { moduleName: 'baz/foo-bar' } + ); + + // Not an invocation + + compile( + ` + + {{concat foo}} + `, + { moduleName: 'baz/foo-bar' } + ); + } + + [`@test deeply nested sub-expression invocations`]() { + expectAssertion(() => { + compile( + ` + {{#let foo as |foo|}} + + {{#each items as |baz|}} + {{concat (foo)}} + {{/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:C25) `); + + expectAssertion(() => { + compile( + ` + {{#let foo as |foo|}} + + {{#each items as |baz|}} + {{concat (foo bar 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:C25) `); + + // Not shadowed + + compile( + ` + {{#let foo as |foo|}} + + {{#each items as |baz|}} + {{/each}} + {{concat (baz)}} + {{concat (baz bat)}} + + {{concat (bar)}} + {{concat (bar baz bat)}} + {{/let}} + {{concat (foo)}} + {{concat (foo bar baz bat)}}`, + { moduleName: 'baz/foo-bar' } + ); + + // Not an invocation + + compile( + ` + {{#let foo as |foo|}} + + {{#each items as |baz|}} + {{concat foo}} + {{/each}} + + {{/let}}`, + { moduleName: 'baz/foo-bar' } + ); + } + + [`@test block statements shadowing mustache invocations`](assert) { + // These are fine, because they should already be considered contextual + // component invocations, not helper invocations + assert.expect(0); + + compile( + ` + {{#let foo as |foo|}} + {{foo}} + {{/let}}`, + { moduleName: 'baz/foo-bar' } + ); + + compile( + ` + {{#let foo as |foo|}} + {{foo bar baz}} + {{/let}}`, + { moduleName: 'baz/foo-bar' } + ); + } + + [`@test element nodes shadowing mustache invocations`](assert) { + // These are fine, because they should already be considered contextual + // component invocations, not helper invocations + assert.expect(0); + + compile( + ` + + {{foo}} + `, + { moduleName: 'baz/foo-bar' } + ); + + compile( + ` + + {{foo bar baz}} + `, + { moduleName: 'baz/foo-bar' } + ); + } + + [`@test deeply nested mustache invocations`](assert) { + // These are fine, because they should already be considered contextual + // component invocations, not helper invocations + assert.expect(0); + + compile( + ` + {{#let foo as |foo|}} + + {{#each items as |baz|}} + {{foo}} + {{/each}} + + {{/let}}`, + { moduleName: 'baz/foo-bar' } + ); + + compile( + ` + {{#let foo as |foo|}} + + {{#each items as |baz|}} + {{foo bar baz}} + {{/each}} + + {{/let}}`, + { moduleName: 'baz/foo-bar' } + ); + } + } +);