Skip to content

Commit

Permalink
[BUGFIX beta] Assert when local variables shadow helper invocations
Browse files Browse the repository at this point in the history
```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.
  • Loading branch information
chancancode committed Oct 19, 2018
1 parent 333a67e commit d581fd5
Show file tree
Hide file tree
Showing 3 changed files with 286 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -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[][]) {
return !node.this && hasLocalVariable(node.parts[0], locals);
}

function hasLocalVariable(name: string, locals: string[][]) {
return locals.some(names => names.includes(name));
}

function messageFor(node: AST.SubExpression) {
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.`;
}
2 changes: 2 additions & 0 deletions packages/ember-template-compiler/lib/plugins/index.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -34,6 +35,7 @@ const transforms: Array<APluginFunc> = [
TransformAttrsIntoArgs,
TransformEachInIntoEach,
TransformHasBlockSyntax,
AssertLocalVariableShadowingHelperInvocation,
AssertInputHelperWithoutBlock,
TransformInElement,
AssertIfHelperWithoutArguments,
Expand Down
Original file line number Diff line number Diff line change
@@ -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' }
);
}
}
);

0 comments on commit d581fd5

Please sign in to comment.