From 74b8f60faac072943f967a15659abe63ddea1778 Mon Sep 17 00:00:00 2001
From: Godfrey Chan <godfreykfc@gmail.com>
Date: Fri, 19 Oct 2018 18:24:55 -0700
Subject: [PATCH] [BUGFIX beta] Assert when local variables shadow modifier
 invocations

Expanding #17132 to cover modifiers.

```hbs
{{#let this.foo as |foo|}}
  <div {{foo}} />
         ~~~ shadowed modifier invocation!
{{/let}}
```

Previously, this would have tried to resolve and invoke the modifier `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 modifiers" in the future, where
the local variable `foo` contains an invocable modifier value.
---
 ...al-variable-shadowing-helper-invocation.ts |  21 +++-
 ...riable-shadowing-helper-invocation-test.js | 113 ++++++++++++++++++
 2 files changed, 132 insertions(+), 2 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 38bf1a11c7e..84b152d2698 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
@@ -38,6 +38,16 @@ export default function assertLocalVariableShadowingHelperInvocation(
           !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
+        assert(
+          `${messageFor(node)} ${calculateLocationDisplay(moduleName, node.loc)}`,
+          !isLocalVariable(node.path, locals.slice(0, -1))
+        );
+      },
     },
   };
 }
@@ -50,7 +60,14 @@ function hasLocalVariable(name: string, locals: string[][]): boolean {
   return locals.some(names => names.indexOf(name) !== -1);
 }
 
-function messageFor(node: AST.SubExpression): string {
+function messageFor(node: AST.SubExpression | AST.ElementModifierStatement): string {
+  let type = isSubExpression(node) ? 'helper' : 'modifier';
   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.`;
+  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';
 }
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 7adb7f1e427..387e9006c2d 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
@@ -224,5 +224,118 @@ moduleFor(
         { moduleName: 'baz/foo-bar' }
       );
     }
+
+    [`@test block statements shadowing modifier invocations`]() {
+      expectAssertion(() => {
+        compile(
+          `
+          {{#let foo as |foo|}}
+            <div {{foo}} />
+          {{/let}}`,
+          { moduleName: 'baz/foo-bar' }
+        );
+      }, `Cannot invoke the \`foo\` modifier 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:C17) `);
+
+      expectAssertion(() => {
+        compile(
+          `
+          {{#let foo as |foo|}}
+            <div {{foo bar baz}} />
+          {{/let}}`,
+          { moduleName: 'baz/foo-bar' }
+        );
+      }, `Cannot invoke the \`foo\` modifier 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:C17) `);
+
+      // Not shadowed
+
+      compile(
+        `
+        {{#let foo as |foo|}}{{/let}}
+        <div {{foo}} />
+        <div {{foo bar baz}} />`,
+        { moduleName: 'baz/foo-bar' }
+      );
+    }
+
+    [`@test element nodes shadowing modifier invocations`]() {
+      expectAssertion(() => {
+        compile(
+          `
+          <Foo as |foo|>
+            <div {{foo}} />
+          </Foo>`,
+          { moduleName: 'baz/foo-bar' }
+        );
+      }, `Cannot invoke the \`foo\` modifier 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:C17) `);
+
+      expectAssertion(() => {
+        compile(
+          `
+          <Foo as |foo|>
+            <div {{foo bar baz}} />
+          </Foo>`,
+          { moduleName: 'baz/foo-bar' }
+        );
+      }, `Cannot invoke the \`foo\` modifier 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:C17) `);
+
+      // Not shadowed
+
+      compile(
+        `
+        <Foo as |foo|></Foo>
+        <div {{foo}} />
+        <div {{foo bar baz}} />`,
+        { moduleName: 'baz/foo-bar' }
+      );
+    }
+
+    [`@test deeply nested modifier invocations`]() {
+      expectAssertion(() => {
+        compile(
+          `
+          {{#let foo as |foo|}}
+            <FooBar as |bar|>
+              {{#each items as |baz|}}
+                <div {{foo}} />
+              {{/each}}
+            </FooBar>
+          {{/let}}`,
+          { moduleName: 'baz/foo-bar' }
+        );
+      }, `Cannot invoke the \`foo\` modifier 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:C21) `);
+
+      expectAssertion(() => {
+        compile(
+          `
+          {{#let foo as |foo|}}
+            <FooBar as |bar|>
+              {{#each items as |baz|}}
+                <div {{foo bar baz}} />
+              {{/each}}
+            </FooBar>
+          {{/let}}`,
+          { moduleName: 'baz/foo-bar' }
+        );
+      }, `Cannot invoke the \`foo\` modifier 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:C21) `);
+
+      // Not shadowed
+
+      compile(
+        `
+        {{#let foo as |foo|}}
+          <FooBar as |bar|>
+            {{#each items as |baz|}}
+            {{/each}}
+            <div {{baz}} />
+            <div {{baz bat}} />
+          </FooBar>
+          <div {{bar}} />
+          <div {{bar baz bat}} />
+        {{/let}}
+        <div {{foo}} />
+        <div {{foo bar baz bat}} />`,
+        { moduleName: 'baz/foo-bar' }
+      );
+    }
   }
 );