-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[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 precense 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
1 parent
333a67e
commit dd946a7
Showing
3 changed files
with
269 additions
and
0 deletions.
There are no files selected for viewing
56 changes: 56 additions & 0 deletions
56
.../ember-template-compiler/lib/plugins/assert-local-variable-shadowing-helper-invocation.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.`; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
211 changes: 211 additions & 0 deletions
211
...template-compiler/tests/plugins/assert-local-variable-shadowing-helper-invocation-test.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,211 @@ | ||
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', | ||
}); | ||
} | ||
} | ||
); |