Skip to content

Commit

Permalink
[ENHANCEMENT+CLEANUP] Better compile time errors for strict mode
Browse files Browse the repository at this point in the history 
Removing support for implicit this fallback {{foo}} -> {{this.foo}}

When a strict mode template references an undefined variable, it
really should produce a compile-time error, arguably that is the
point of the feature.

However, it seems like previously a lot of these errors ended up
propagating and is actually handled by the runtime compiler.

The original/primary goal here started out as cleaning up all the
code for implicit this fallback, but as I delete code it eventually
revaled all the places where strict mode was inappropiately using
the same infrastructure ("unresolved free variables", which isn't
really a sensible thing).

Also:

* Removes "deprecated call" @foo={{bar}}
* Removes unused opcodes
  • Loading branch information
@chancancode
chancancode committed Mar 9, 2024
1 parent 974fdaa commit 8685023
Show file tree
Hide file tree
Showing 33 changed files with 738 additions and 819 deletions.
2 changes: 1 addition & 1 deletion packages/@glimmer-workspace/integration-tests/test/modifiers/dynamic-modifiers-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ class DynamicModifiersResolutionModeTest extends RenderTest {
this.registerComponent('TemplateOnly', 'Bar', '<div {{x.foo}}></div>');
},
syntaxErrorFor(
'You attempted to invoke a path (`{{#x.foo}}`) as a modifier, but x was not in scope. Try adding `this` to the beginning of the path',
'You attempted to invoke a path (`{{x.foo}}`) as a modifier, but x was not in scope',
'{{x.foo}}',
'an unknown module',
1,
Expand Down
219 changes: 115 additions & 104 deletions packages/@glimmer-workspace/integration-tests/test/strict-mode-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,15 +73,22 @@ class GeneralStrictModeTest extends RenderTest {

@test
'Implicit this lookup does not work'() {
const Foo = defineComponent({}, '{{bar}}', {
definition: class extends GlimmerishComponent {
bar = 'Hello, world!';
this.assert.throws(
() => {
defineComponent({}, '{{bar}}', {
definition: class extends GlimmerishComponent {
bar = 'Hello, world!';
},
});
},
});

this.assert.throws(() => {
this.renderComponent(Foo);
}, /Attempted to resolve a value in a strict mode template, but that value was not in scope: bar/u);
syntaxErrorFor(
'Attempted to resolve a value in a strict mode template, but that value was not in scope: bar',
'{{bar}}',
'an unknown module',
1,
0
)
);
}

@test
Expand Down Expand Up @@ -395,132 +402,136 @@ class StaticStrictModeTest extends RenderTest {

@test
'Throws an error if value in append position is not in scope'() {
const Bar = defineComponent({}, '{{foo}}');

this.assert.throws(() => {
this.renderComponent(Bar);
}, /Attempted to resolve a value in a strict mode template, but that value was not in scope: foo/u);
this.assert.throws(
() => {
defineComponent({}, '{{foo}}');
},
syntaxErrorFor(
'Attempted to resolve a value in a strict mode template, but that value was not in scope: foo',
'{{foo}}',
'an unknown module',
1,
0
)
);
}

@test
'Throws an error if component or helper in append position is not in scope'() {
const Bar = defineComponent({}, '{{foo "bar"}}');

this.assert.throws(() => {
this.renderComponent(Bar);
}, /Attempted to resolve a component or helper in a strict mode template, but that value was not in scope: foo/u);
this.assert.throws(
() => {
defineComponent({}, '{{foo "bar"}}');
},
syntaxErrorFor(
'Attempted to resolve a component or helper in a strict mode template, but that value was not in scope: foo',
'{{foo "bar"}}',
'an unknown module',
1,
0
)
);
}

@test
'Throws an error if a value in argument position is not in scope'() {
const Foo = defineComponent({}, '{{@foo}}');
const Bar = defineComponent({ Foo }, '<Foo @foo={{bar}}/>');

this.assert.throws(() => {
this.renderComponent(Bar);
}, /Attempted to resolve a value in a strict mode template, but that value was not in scope: bar/u);
}

@test
'Throws an error if helper in argument position (with args) is not in scope'() {
const Foo = defineComponent({}, '{{@foo}}');
const Bar = defineComponent({ Foo }, '<Foo @foo={{bar "aoeu"}}/>');

this.assert.throws(() => {
this.renderComponent(Bar);
}, /Attempted to resolve a helper in a strict mode template, but that value was not in scope: bar/u);
}

@test
'Throws an error if helper in subexpression position is not in scope'() {
const foo = defineSimpleHelper((value: string) => value);
const Bar = defineComponent({ foo }, '{{foo (bar)}}');

this.assert.throws(() => {
this.renderComponent(Bar);
}, /Attempted to resolve a helper in a strict mode template, but that value was not in scope: bar/u);
}

@test
'Throws an error if value in append position is not in scope, and component is registered'() {
this.registerComponent('TemplateOnly', 'foo', 'Hello, world!');
const Bar = defineComponent({}, '{{foo}}');

this.assert.throws(() => {
this.renderComponent(Bar);
}, /Attempted to resolve a value in a strict mode template, but that value was not in scope: foo/u);
}

@test
'Throws an error if value in append position is not in scope, and helper is registered'() {
this.registerHelper('foo', () => 'Hello, world!');
const Bar = defineComponent({}, '{{foo}}');

this.assert.throws(() => {
this.renderComponent(Bar);
}, /Attempted to resolve a value in a strict mode template, but that value was not in scope: foo/u);
this.assert.throws(
() => {
defineComponent({ Foo }, '<Foo @foo={{bar}}/>');
},
syntaxErrorFor(
'Attempted to resolve a value in a strict mode template, but that value was not in scope: bar',
'@foo={{bar}}',
'an unknown module',
1,
5
)
);
}

@test
'Throws an error if component or helper in append position is not in scope, and helper is registered'() {
this.registerHelper('foo', () => 'Hello, world!');
const Bar = defineComponent({}, '{{foo "bar"}}');

this.assert.throws(() => {
this.renderComponent(Bar);
}, /Attempted to resolve a component or helper in a strict mode template, but that value was not in scope: foo/u);
'Throws an error if a value in attribute position is not in scope'() {
this.assert.throws(
() => {
defineComponent({}, '<div class={{foo}} />');
},
syntaxErrorFor(
'Attempted to resolve a value in a strict mode template, but that value was not in scope: foo',
'class={{foo}}',
'an unknown module',
1,
5
)
);
}

@test
'Throws an error if a value in argument position is not in scope, and helper is registered'() {
this.registerHelper('bar', () => 'Hello, world!');
'Throws an error if helper in argument position (with args) is not in scope'() {
const Foo = defineComponent({}, '{{@foo}}');
const Bar = defineComponent({ Foo }, '<Foo @foo={{bar}}/>');

this.assert.throws(() => {
this.renderComponent(Bar);
}, /Attempted to resolve a value in a strict mode template, but that value was not in scope: bar/u);
this.assert.throws(
() => {
defineComponent({ Foo }, '<Foo @foo={{bar "aoeu"}}/>');
},
syntaxErrorFor(
'Attempted to resolve a helper in a strict mode template, but that value was not in scope: bar',
'@foo={{bar "aoeu"}}',
'an unknown module',
1,
5
)
);
}

@test
'Throws an error if helper in argument position (with args) is not in scope, and helper is registered'() {
this.registerHelper('bar', () => 'Hello, world!');
const Foo = defineComponent({}, '{{@foo}}');
const Bar = defineComponent({ Foo }, '<Foo @foo={{bar "aoeu"}}/>');

this.assert.throws(() => {
this.renderComponent(Bar);
}, /Attempted to resolve a helper in a strict mode template, but that value was not in scope: bar/u);
'Throws an error if helper in attribute position (with args) is not in scope'() {
this.assert.throws(
() => {
defineComponent({}, '<div class={{foo "bar"}} />');
},
syntaxErrorFor(
'Attempted to resolve a helper in a strict mode template, but that value was not in scope: foo',
'class={{foo "bar"}}',
'an unknown module',
1,
5
)
);
}

@test
'Throws an error if helper in subexpression position is not in scope, and helper is registered'() {
this.registerHelper('bar', () => 'Hello, world!');
'Throws an error if helper in subexpression position is not in scope'() {
const foo = defineSimpleHelper((value: string) => value);
const Bar = defineComponent({ foo }, '{{foo (bar)}}');

this.assert.throws(() => {
this.renderComponent(Bar);
}, /Attempted to resolve a helper in a strict mode template, but that value was not in scope: bar/u);
this.assert.throws(
() => {
defineComponent({ foo }, '{{foo (bar)}}');
},
syntaxErrorFor(
'Attempted to resolve a helper in a strict mode template, but that value was not in scope: bar',
'(bar)',
'an unknown module',
1,
6
)
);
}

@test
'Throws an error if modifier is not in scope'() {
const Bar = defineComponent({}, '<div {{foo}}></div>');

this.assert.throws(() => {
this.renderComponent(Bar);
}, /Attempted to resolve a modifier in a strict mode template, but it was not in scope: foo/u);
}

@test
'Throws an error if modifier is not in scope, and modifier is registred'() {
this.registerModifier('name', class {});
const Bar = defineComponent({}, '<div {{foo}}></div>');

this.assert.throws(() => {
this.renderComponent(Bar);
}, /Attempted to resolve a modifier in a strict mode template, but it was not in scope: foo/u);
this.assert.throws(
() => {
defineComponent({}, '<div {{foo}}></div>');
},
syntaxErrorFor(
'Attempted to resolve a modifier in a strict mode template, but that value was not in scope: foo',
'{{foo}}',
'an unknown module',
1,
5
)
);
}

@test
Expand Down
36 changes: 17 additions & 19 deletions ...er-workspace/integration-tests/test/syntax/argument-less-helper-paren-less-invoke-test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,29 +1,27 @@
import { defineSimpleHelper, jitSuite, RenderTest, test } from '../..';
import { defineSimpleHelper, jitSuite, preprocess, RenderTest, syntaxErrorFor, test } from '../..';

class ArgumentLessHelperParenLessInvokeTest extends RenderTest {
static suiteName = 'argument-less helper paren-less invoke';

@test
'invoking an argument-less helper without parens in named argument position is deprecated'(
'invoking an argument-less helper without parens in named argument position is a syntax error'(
assert: Assert
) {
this.registerHelper('is-string', ([value]: readonly unknown[]) => typeof value === 'string');

this.registerHelper('foo', () => 'Hello, world!');
this.registerComponent('TemplateOnly', 'Bar', '[{{is-string @content}}][{{@content}}]');

this.render('<Bar @content={{foo}} />', { foo: 'Not it!' });
this.assertHTML('[true][Hello, world!]');
this.assertStableRerender();

assert.validateDeprecations(
new RegExp(
/The `foo` helper was used in the `\(unknown template module\)` template as /.source +
/`@content=\{\{foo\}\}`\. This is ambigious between wanting the `@content` argument /
.source +
/to be the `foo` helper itself, or the result of invoking the `foo` helper /.source +
/\(current behavior\)\. This implicit invocation behavior has been deprecated\./.source,
'u'
assert.throws(
() => {
preprocess('<Bar @content={{foo}} />', {
meta: { moduleName: 'test-module' },
});
},
syntaxErrorFor(
'You attempted to pass a path as argument (`@content={{foo}}`) but foo was not in scope. Try:\n' +
'* `@content={{this.foo}}` if this is meant to be a property lookup, or\n' +
'* `@content={{(foo)}}` if this is meant to invoke the resolved helper, or\n' +
'* `@content={{helper "foo"}}` if this is meant to pass the resolved helper by value',
`@content={{foo}}`,
'test-module',
1,
5
)
);
}
Expand Down
8 changes: 4 additions & 4 deletions packages/@glimmer-workspace/integration-tests/test/syntax/keyword-errors-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ for (let keyword of KEYWORDS) {
'keyword can be yielded as a parameter in other keywords in non-strict mode'() {
preprocess(
`
{{#let value as |${keyword}|}}
{{#let this.value as |${keyword}|}}
{{some-helper ${keyword}}}
{{/let}}
`,
Expand All @@ -46,7 +46,7 @@ for (let keyword of KEYWORDS) {
'keyword can be yielded as a parameter in other keywords in strict mode'() {
preprocess(
`
{{#let value as |${keyword}|}}
{{#let this.value as |${keyword}|}}
{{some-helper ${keyword}}}
{{/let}}
`,
Expand All @@ -58,7 +58,7 @@ for (let keyword of KEYWORDS) {
'keyword can be yielded as a parameter in curly invocation in non-strict mode'() {
preprocess(
`
{{#my-component value as |${keyword}|}}
{{#my-component this.value as |${keyword}|}}
{{some-helper ${keyword}}}
{{/my-component}}
`,
Expand All @@ -70,7 +70,7 @@ for (let keyword of KEYWORDS) {
'keyword can be yielded as a parameter in curly invocation in strict mode'() {
preprocess(
`
{{#my-component value as |${keyword}|}}
{{#my-component this.value as |${keyword}|}}
{{some-helper ${keyword}}}
{{/my-component}}
`,
Expand Down
Loading

0 comments on commit 8685023

Please sign in to comment.