Skip to content

Commit

Permalink
[BUGFIX beta] Assert when local variables shadow modifier invocations
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
chancancode committed Oct 20, 2018
1 parent 2ed1416 commit 74b8f60
Show file tree
Hide file tree
Showing 2 changed files with 132 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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))
);
},
},
};
}
Expand All @@ -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';
}
Original file line number Diff line number Diff line change
Expand Up @@ -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' }
);
}
}
);

0 comments on commit 74b8f60

Please sign in to comment.