[ENHANCEMENT+CLEANUP] Better compile time errors for strict mode #1557

chancancode · 2024-02-01T14:07:40Z

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 {{#with}}
Removes "deprecated call" @foo={{bar}}
Removes unused opcodes
Syncs keyword list with Ember

chancancode · 2024-02-01T14:13:42Z

packages/@glimmer/syntax/lib/v2/normalize.ts

+        `You attempted to render a path (\`{{${resolution.path}}}\`), but ${resolution.head} was not in scope`,
+        loc
+      );
+    }


TODO: consider deferring these to the validation pass

Basically reconsider all the syntax errors this whole file

chancancode · 2024-02-01T14:25:31Z

packages/@glimmer/syntax/lib/v2/normalize.ts

+        `* \`${arg.name}={{(${resolution.path})}}\` if this is meant to invoke the resolved helper, or\n` +
+        `* \`${arg.name}={{helper "${resolution.path}"}}\` if this is meant to pass the resolved helper by value`,
+        arg.loc
+      );


Note: this is actually a loose mode only error

NullVoxPopuli · 2024-02-01T14:11:06Z

packages/@glimmer-workspace/integration-tests/test/strict-mode-test.ts

-    const Foo = defineComponent({}, '{{bar}}', {
-      definition: class extends GlimmerishComponent {
-        bar = 'Hello, world!';
+    this.assert.throws(


NullVoxPopuli · 2024-02-01T14:11:23Z

packages/@glimmer-workspace/integration-tests/test/helpers/array-test.ts

@@ -58,11 +58,11 @@ class ArrayTest extends RenderTest {
  @test
  'binds multiple values when variables are used'() {
    this.render(
-      strip`{{#with (array this.personOne this.personTwo) as |people|}}


I'm glad I never used with 😅

NullVoxPopuli · 2024-02-01T14:12:42Z

...immer-workspace/integration-tests/test/syntax/argument-less-helper-paren-less-invoke-test.ts

+          meta: { moduleName: 'test-module' },
+        });
+      },
+      syntaxErrorFor(


this is a really nice error! folks will appreciate this!

NullVoxPopuli · 2024-02-01T14:13:40Z

packages/@glimmer/compiler/lib/builder/builder.ts

@@ -691,14 +693,11 @@ function getSymbolForVar(
 export function expressionContextOp(context: VariableResolutionContext): GetContextualFreeOpcode {
  switch (context) {
    case VariableResolutionContext.Strict:
+      debugger;


stray debugger

packages/@glimmer/compiler/lib/builder/builder.ts

NullVoxPopuli · 2024-02-01T14:15:18Z

packages/@glimmer/compiler/lib/passes/1-normalization/visitors/expressions.ts

@@ -112,13 +110,6 @@ export class NormalizeExpressions {
    }
  }

-  DeprecaedCallExpression(


no need to fix a typo when we can just delete 🎉

NullVoxPopuli · 2024-02-01T14:17:26Z

packages/@glimmer/compiler/lib/passes/1-normalization/visitors/strict-mode.ts

+
+  InElement(inElement: mir.InElement): Result<null> {
+    return this.Expression(inElement.destination)
+      // Unfortunately we lost the `insertBefore=` part of the span


why did we lose it?

There is no AST/mir node (and hence span) for the “name=” part of named arguments; there is AST/mir node for the entire “name=value” thing, but the InElement mir node did not preserve it since it happen to know it will always have (or add one during normalization) the insertBefore key. So we just have the expression node and the entry node was thrown away in earlier normalization pass

NullVoxPopuli · 2024-02-01T14:20:09Z

packages/@glimmer/opcode-compiler/lib/opcode-builder/helpers/resolution.ts

@@ -101,6 +84,8 @@ export function resolveComponent(
  let type = expr[0];

  if (import.meta.env.DEV && expr[0] === SexpOpcodes.GetStrictKeyword) {
+    assert(!meta.isStrictMode, 'Strict mode errors should already be handled at compile time');


glimmer's assert matches node's assert, which is opposite ember's assert? 😅

Yeah 🤷🏻‍♂️

NullVoxPopuli · 2024-02-01T14:26:30Z

packages/@glimmer/syntax/lib/v2/normalize.ts

-      trusting: false,
-    };
+    if (resolution.result === 'error' && resolution.path !== 'has-block') {
+      throw generateSyntaxError(


this is a great error!! thanks!

NullVoxPopuli · 2024-02-01T14:28:15Z

packages/@glimmer/compiler/lib/passes/1-normalization/visitors/strict-mode.ts

+  ComponentOrHelper = 'component or helper',
+}
+
+export default class StrictModeValidationPass {


does this have tests? I didn't seem the explicitly -- but maybe I missed them?

This is what’s generating the syntax errors in the strict mode tests

This was dropped on Ember side a while back, just never cleaned up Ironically this probably meant when Ember dropped the deprecation/ error, nothing was actually stopping `{{#with}}` from anymore, so perhaps for a while we have actually _re-introduced_ support for the syntax for some time? Extracted from #1557

@foo

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

wycats

I think these changes are solid.

Recently in #1557 we started emitting build time error for strict mode templates containing undefined references. Previously these were not handled until runtime and so we can only emit runtime errors. However, the previous setup allowed for the host environment to implement additional keywords – these are resolved at runtime with the `lookupBuildInHelper` and `lookupBuiltInModifier` hooks, and the error is only emitted if these hooks returned `null`. To allow for this possibility, `precompile` now accept an option for additional strict mode keywords that the host environment is expected to provide.

chancancode force-pushed the cleanup-this-fallback branch from 6500ab8 to da81f20 Compare February 1, 2024 14:09

chancancode commented Feb 1, 2024

View reviewed changes

NullVoxPopuli reviewed Feb 1, 2024

View reviewed changes

chancancode mentioned this pull request Mar 9, 2024

[CLEANUP] Drop support for {{#with}} keyword #1574

Merged

chancancode force-pushed the cleanup-this-fallback branch from da81f20 to 8685023 Compare March 9, 2024 03:39

NullVoxPopuli approved these changes Mar 20, 2024

View reviewed changes

wycats self-assigned this Mar 20, 2024

wycats approved these changes Mar 20, 2024

View reviewed changes

chancancode merged commit 8f79790 into main Mar 20, 2024
5 of 6 checks passed

chancancode deleted the cleanup-this-fallback branch March 20, 2024 17:42

chancancode mentioned this pull request Apr 4, 2024

Introduce keywords option for precompile #1585

Merged

github-actions bot mentioned this pull request Oct 15, 2024

Prepare Release patricklx/glimmer-vm#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENHANCEMENT+CLEANUP] Better compile time errors for strict mode #1557

[ENHANCEMENT+CLEANUP] Better compile time errors for strict mode #1557

chancancode commented Feb 1, 2024

chancancode Feb 1, 2024

chancancode Feb 1, 2024

chancancode Feb 1, 2024

NullVoxPopuli Feb 1, 2024

NullVoxPopuli Feb 1, 2024

NullVoxPopuli Feb 1, 2024

NullVoxPopuli Feb 1, 2024

NullVoxPopuli Feb 1, 2024

NullVoxPopuli Feb 1, 2024

chancancode Feb 2, 2024

NullVoxPopuli Feb 1, 2024

chancancode Feb 2, 2024

NullVoxPopuli Feb 1, 2024

NullVoxPopuli Feb 1, 2024

chancancode Feb 2, 2024

wycats left a comment

[ENHANCEMENT+CLEANUP] Better compile time errors for strict mode #1557

[ENHANCEMENT+CLEANUP] Better compile time errors for strict mode #1557

Conversation

chancancode commented Feb 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wycats left a comment

Choose a reason for hiding this comment