Merge pull request #17132 from emberjs/assert-local-variable-shadowin…

…g-helper-invocation

[BUGFIX beta] Assert when local variables shadow helper invocations

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.`;
+}

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<APluginFunc> = [
   TransformAttrsIntoArgs,
   TransformEachInIntoEach,
   TransformHasBlockSyntax,
+  AssertLocalVariableShadowingHelperInvocation,
   AssertInputHelperWithoutBlock,
   TransformInElement,
   AssertIfHelperWithoutArguments,

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(
+          `
+          <Foo as |foo|>
+            {{concat (foo)}}
+          </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(
+          `
+          <Foo as |foo|>
+            {{concat (foo bar baz)}}
+          </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) `);
+
+      // Not shadowed
+
+      compile(
+        `
+        <Foo as |foo|></Foo>
+        {{concat (foo)}}
+        {{concat (foo bar baz)}}`,
+        { moduleName: 'baz/foo-bar' }
+      );
+
+      // Not an invocation
+
+      compile(
+        `
+        <Foo as |foo|>
+          {{concat foo}}
+        </Foo>`,
+        { moduleName: 'baz/foo-bar' }
+      );
+    }
+
+    [`@test deeply nested sub-expression invocations`]() {
+      expectAssertion(() => {
+        compile(
+          `
+          {{#let foo as |foo|}}
+            <FooBar as |bar|>
+              {{#each items as |baz|}}
+                {{concat (foo)}}
+              {{/each}}
+            </FooBar>
+          {{/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|}}
+            <FooBar as |bar|>
+              {{#each items as |baz|}}
+                {{concat (foo bar baz)}}
+              {{/each}}
+            </FooBar>
+          {{/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|}}
+          <FooBar as |bar|>
+            {{#each items as |baz|}}
+            {{/each}}
+            {{concat (baz)}}
+            {{concat (baz bat)}}
+          </FooBar>
+          {{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|}}
+          <FooBar as |bar|>
+            {{#each items as |baz|}}
+              {{concat foo}}
+            {{/each}}
+          </FooBar>
+        {{/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 as |foo|>
+          {{foo}}
+        </Foo>`,
+        { moduleName: 'baz/foo-bar' }
+      );
+
+      compile(
+        `
+        <Foo as |foo|>
+          {{foo bar baz}}
+        </Foo>`,
+        { 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|}}
+          <FooBar as |bar|>
+            {{#each items as |baz|}}
+              {{foo}}
+            {{/each}}
+          </FooBar>
+        {{/let}}`,
+        { moduleName: 'baz/foo-bar' }
+      );
+
+      compile(
+        `
+        {{#let foo as |foo|}}
+          <FooBar as |bar|>
+            {{#each items as |baz|}}
+              {{foo bar baz}}
+            {{/each}}
+          </FooBar>
+        {{/let}}`,
+        { moduleName: 'baz/foo-bar' }
+      );
+    }
+  }
+);