Introduce keywords option for precompile

chancancode · chancancode · commit 025e7429bceb · 2024-04-04T16:51:25.000-07:00
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.
diff --git a/packages/@glimmer-workspace/integration-tests/lib/modes/jit/compilation-context.ts b/packages/@glimmer-workspace/integration-tests/lib/modes/jit/compilation-context.ts
@@ -23,8 +23,8 @@ export default class JitCompileTimeLookup implements CompileTimeResolver {
     return this.resolver.lookupComponent(name, owner);
   }
 
-  lookupBuiltInHelper(_name: string): Nullable<HelperDefinitionState> {
-    return null;
+  lookupBuiltInHelper(name: string): Nullable<HelperDefinitionState> {
+    return this.resolver.lookupHelper(`$keyword.${name}`);
   }
 
   lookupBuiltInModifier(_name: string): Nullable<ModifierDefinitionState> {
diff --git a/packages/@glimmer-workspace/integration-tests/lib/test-helpers/define.ts b/packages/@glimmer-workspace/integration-tests/lib/test-helpers/define.ts
@@ -96,6 +96,9 @@ export interface DefineComponentOptions {
 
   // defaults to true when some scopeValues are passed and false otherwise
   strictMode?: boolean;
+
+  // additional strict-mode keywords
+  keywords?: string[];
 }
 
 export function defineComponent(
@@ -110,8 +113,10 @@ export function defineComponent(
     strictMode = scopeValues !== null;
   }
 
+  let keywords = options.keywords ?? [];
+
   let definition = options.definition ?? templateOnlyComponent();
-  let templateFactory = createTemplate(templateSource, { strictMode }, scopeValues ?? {});
+  let templateFactory = createTemplate(templateSource, { strictMode, keywords }, scopeValues ?? {});
   setComponentTemplate(templateFactory, definition);
   return definition;
 }
diff --git a/packages/@glimmer-workspace/integration-tests/test/strict-mode-test.ts b/packages/@glimmer-workspace/integration-tests/test/strict-mode-test.ts
@@ -72,12 +72,16 @@ class GeneralStrictModeTest extends RenderTest {
   }
 
   @test
-  'Implicit this lookup does not work'() {
+  'Undefined references is an error'() {
+    this.registerHelper('bar', () => 'should not resolve this helper');
+
     this.assert.throws(
       () => {
         defineComponent({}, '{{bar}}', {
           definition: class extends GlimmerishComponent {
-            bar = 'Hello, world!';
+            get bar() {
+              throw new Error('should not fallback to this.bar');
+            }
           },
         });
       },
@@ -91,6 +95,28 @@ class GeneralStrictModeTest extends RenderTest {
     );
   }
 
+  @test
+  'Non-native keyword'() {
+    this.registerHelper('bar', () => {
+      throw new Error('should not resolve this helper');
+    });
+
+    this.registerHelper('$keyword.bar', () => 'bar keyword');
+
+    const Foo = defineComponent({}, '{{bar}}', {
+      keywords: ['bar'],
+      definition: class extends GlimmerishComponent {
+        get bar() {
+          throw new Error('should not fallback to this.bar');
+        }
+      },
+    });
+
+    this.renderComponent(Foo);
+    this.assertHTML('bar keyword');
+    this.assertStableRerender();
+  }
+
   @test
   '{{component}} throws an error if a string is used in strict (append position)'() {
     this.assert.throws(() => {
diff --git a/packages/@glimmer/compiler/lib/builder/builder.ts b/packages/@glimmer/compiler/lib/builder/builder.ts
@@ -632,7 +632,7 @@ export function buildVar(
   symbols: Symbols,
   path?: PresentArray<string>
 ): Expressions.GetPath | Expressions.GetVar {
-  let op: Expressions.GetVar[0] = Op.GetSymbol;
+  let op: Expressions.GetPath[0] | Expressions.GetVar[0] = Op.GetSymbol;
   let sym: number;
   switch (head.kind) {
     case VariableKind.Free:
@@ -665,6 +665,7 @@ export function buildVar(
   if (path === undefined || path.length === 0) {
     return [op, sym];
   } else {
+    assert(op !== Op.GetStrictKeyword, '[BUG] keyword with a path');
     return [op, sym, path];
   }
 }
diff --git a/packages/@glimmer/compiler/lib/passes/1-normalization/utils/is-node.ts b/packages/@glimmer/compiler/lib/passes/1-normalization/utils/is-node.ts
@@ -1,111 +1,4 @@
-import type { PresentArray } from '@glimmer/interfaces';
-import type { SourceSlice } from '@glimmer/syntax';
-import { ASTv2, generateSyntaxError } from '@glimmer/syntax';
-import { unreachable } from '@glimmer/util';
-
-export type HasPath<Node extends ASTv2.CallNode = ASTv2.CallNode> = Node & {
-  head: ASTv2.PathExpression;
-};
-
-export type HasArguments =
-  | {
-      params: PresentArray<ASTv2.ExpressionNode>;
-    }
-  | {
-      hash: {
-        pairs: PresentArray<ASTv2.NamedArgument>;
-      };
-    };
-
-export type HelperInvocation<Node extends ASTv2.CallNode = ASTv2.CallNode> = HasPath<Node> &
-  HasArguments;
-
-export function hasPath<N extends ASTv2.CallNode>(node: N): node is HasPath<N> {
-  return node.callee.type === 'Path';
-}
-
-export function isHelperInvocation<N extends ASTv2.CallNode>(
-  node: ASTv2.CallNode
-): node is HelperInvocation<N> {
-  if (!hasPath(node)) {
-    return false;
-  }
-
-  return !node.args.isEmpty();
-}
-
-export interface SimplePath extends ASTv2.PathExpression {
-  tail: [SourceSlice];
-  data: false;
-  this: false;
-}
-
-export type SimpleHelper<N extends HasPath> = N & {
-  path: SimplePath;
-};
-
-export function isSimplePath(path: ASTv2.ExpressionNode): path is SimplePath {
-  if (path.type === 'Path') {
-    let { ref: head, tail: parts } = path;
-
-    return head.type === 'Free' && !ASTv2.isStrictResolution(head.resolution) && parts.length === 0;
-  } else {
-    return false;
-  }
-}
-
-export function isStrictHelper(expr: HasPath): boolean {
-  if (expr.callee.type !== 'Path') {
-    return true;
-  }
-
-  if (expr.callee.ref.type !== 'Free') {
-    return true;
-  }
-
-  return ASTv2.isStrictResolution(expr.callee.ref.resolution);
-}
-
-export function assertIsValidModifier<N extends HasPath>(
-  helper: N
-): asserts helper is SimpleHelper<N> {
-  if (isStrictHelper(helper) || isSimplePath(helper.callee)) {
-    return;
-  }
-
-  throw generateSyntaxError(
-    `\`${printPath(helper.callee)}\` is not a valid name for a modifier`,
-    helper.loc
-  );
-}
-
-function printPath(path: ASTv2.ExpressionNode): string {
-  switch (path.type) {
-    case 'Literal':
-      return JSON.stringify(path.value);
-    case 'Path': {
-      let printedPath = [printPathHead(path.ref)];
-      printedPath.push(...path.tail.map((t) => t.chars));
-      return printedPath.join('.');
-    }
-    case 'Call':
-      return `(${printPath(path.callee)} ...)`;
-    case 'Interpolate':
-      throw unreachable('a concat statement cannot appear as the head of an expression');
-  }
-}
-
-function printPathHead(head: ASTv2.VariableReference): string {
-  switch (head.type) {
-    case 'Arg':
-      return head.name.chars;
-    case 'Free':
-    case 'Local':
-      return head.name;
-    case 'This':
-      return 'this';
-  }
-}
+import type { ASTv2 } from '@glimmer/syntax';
 
 /**
  * This function is checking whether an AST node is a triple-curly, which means that it's
diff --git a/packages/@glimmer/compiler/lib/passes/1-normalization/visitors/element/classified.ts b/packages/@glimmer/compiler/lib/passes/1-normalization/visitors/element/classified.ts
@@ -7,7 +7,6 @@ import { Ok, Result, ResultArray } from '../../../../shared/result';
 import { getAttrNamespace } from '../../../../utils';
 import * as mir from '../../../2-encoding/mir';
 import { MODIFIER_KEYWORDS } from '../../keywords';
-import { assertIsValidModifier, isHelperInvocation } from '../../utils/is-node';
 import { convertPathToCallIfKeyword, VISIT_EXPRS } from '../expressions';
 
 export type ValidAttr = mir.StaticAttr | mir.DynamicAttr | mir.SplatAttr;
@@ -75,10 +74,6 @@ export class ClassifiedElement {
   }
 
   private modifier(modifier: ASTv2.ElementModifier): Result<mir.Modifier> {
-    if (isHelperInvocation(modifier)) {
-      assertIsValidModifier(modifier);
-    }
-
     let translated = MODIFIER_KEYWORDS.translate(modifier, this.state);
 
     if (translated !== null) {
diff --git a/packages/@glimmer/compiler/lib/passes/1-normalization/visitors/expressions.ts b/packages/@glimmer/compiler/lib/passes/1-normalization/visitors/expressions.ts
@@ -8,13 +8,14 @@ import type { NormalizationState } from '../context';
 import { Ok, Result, ResultArray } from '../../../shared/result';
 import * as mir from '../../2-encoding/mir';
 import { CALL_KEYWORDS } from '../keywords';
-import { hasPath } from '../utils/is-node';
 
 export class NormalizeExpressions {
   visit(node: ASTv2.ExpressionNode, state: NormalizationState): Result<mir.ExpressionNode> {
     switch (node.type) {
       case 'Literal':
         return Ok(this.Literal(node));
+      case 'Keyword':
+        return Ok(this.Keyword(node));
       case 'Interpolate':
         return this.Interpolate(node, state);
       case 'Path':
@@ -78,6 +79,10 @@ export class NormalizeExpressions {
     return literal;
   }
 
+  Keyword(keyword: ASTv2.KeywordExpression): ASTv2.KeywordExpression {
+    return keyword;
+  }
+
   Interpolate(
     expr: ASTv2.InterpolateExpression,
     state: NormalizationState
@@ -93,8 +98,8 @@ export class NormalizeExpressions {
     expr: ASTv2.CallExpression,
     state: NormalizationState
   ): Result<mir.ExpressionNode> {
-    if (!hasPath(expr)) {
-      throw new Error(`unimplemented subexpression at the head of a subexpression`);
+    if (expr.callee.type === 'Call') {
+      throw new Error(`unimplemented: subexpression at the head of a subexpression`);
     } else {
       return Result.all(
         VISIT_EXPRS.visit(expr.callee, state),
diff --git a/packages/@glimmer/compiler/lib/passes/1-normalization/visitors/strict-mode.ts b/packages/@glimmer/compiler/lib/passes/1-normalization/visitors/strict-mode.ts
@@ -120,6 +120,7 @@ export default class StrictModeValidationPass {
   ): Result<null> {
     switch (expression.type) {
       case 'Literal':
+      case 'Keyword':
       case 'Missing':
       case 'This':
       case 'Arg':
diff --git a/packages/@glimmer/compiler/lib/passes/2-encoding/expressions.ts b/packages/@glimmer/compiler/lib/passes/2-encoding/expressions.ts
@@ -1,6 +1,6 @@
 import type { PresentArray, WireFormat } from '@glimmer/interfaces';
 import type { ASTv2 } from '@glimmer/syntax';
-import { assertPresentArray, isPresentArray, mapPresentArray } from '@glimmer/util';
+import { assert, assertPresentArray, isPresentArray, mapPresentArray } from '@glimmer/util';
 import { SexpOpcodes } from '@glimmer/wire-format';
 
 import type * as mir from './mir';
@@ -14,6 +14,8 @@ export class ExpressionEncoder {
         return undefined;
       case 'Literal':
         return this.Literal(expr);
+      case 'Keyword':
+        return this.Keyword(expr);
       case 'CallExpression':
         return this.CallExpression(expr);
       case 'PathExpression':
@@ -86,9 +88,13 @@ export class ExpressionEncoder {
     return [isTemplateLocal ? SexpOpcodes.GetLexicalSymbol : SexpOpcodes.GetSymbol, symbol];
   }
 
+  Keyword({ symbol }: ASTv2.KeywordExpression): WireFormat.Expressions.GetStrictFree {
+    return [SexpOpcodes.GetStrictKeyword, symbol];
+  }
+
   PathExpression({ head, tail }: mir.PathExpression): WireFormat.Expressions.GetPath {
     let getOp = EXPR.expr(head) as WireFormat.Expressions.GetVar;
-
+    assert(getOp[0] !== SexpOpcodes.GetStrictKeyword, '[BUG] keyword in a PathExpression');
     return [...getOp, EXPR.Tail(tail)];
   }
 
diff --git a/packages/@glimmer/compiler/lib/passes/2-encoding/mir.ts b/packages/@glimmer/compiler/lib/passes/2-encoding/mir.ts
@@ -180,9 +180,10 @@ export class Tail extends node('Tail').fields<{ members: PresentArray<SourceSlic
 
 export type ExpressionNode =
   | ASTv2.LiteralExpression
+  | ASTv2.KeywordExpression
+  | ASTv2.VariableReference
   | Missing
   | PathExpression
-  | ASTv2.VariableReference
   | InterpolateExpression
   | CallExpression
   | Not
diff --git a/packages/@glimmer/compiler/lib/wire-format-debug.ts b/packages/@glimmer/compiler/lib/wire-format-debug.ts
@@ -170,7 +170,7 @@ export default class WireFormatDebugger {
           return ['concat', this.formatParams(opcode[1] as WireFormat.Core.Params)];
 
         case Op.GetStrictKeyword:
-          return ['get-strict-free', this.upvars[opcode[1]], opcode[2]];
+          return ['get-strict-free', this.upvars[opcode[1]]];
 
         case Op.GetFreeAsComponentOrHelperHead:
           return ['GetFreeAsComponentOrHelperHead', this.upvars[opcode[1]], opcode[2]];
diff --git a/packages/@glimmer/interfaces/lib/compile/wire-format/api.d.ts b/packages/@glimmer/interfaces/lib/compile/wire-format/api.d.ts
@@ -111,7 +111,6 @@ export namespace Expressions {
 
   export type GetPathSymbol = [GetSymbolOpcode, number, Path];
   export type GetPathTemplateSymbol = [GetLexicalSymbolOpcode, number, Path];
-  export type GetPathStrictFree = [GetStrictKeywordOpcode, number, Path];
   export type GetPathFreeAsComponentOrHelperHead = [
     GetFreeAsComponentOrHelperHeadOpcode,
     number,
@@ -126,8 +125,7 @@ export namespace Expressions {
     | GetPathFreeAsHelperHead
     | GetPathFreeAsModifierHead
     | GetPathFreeAsComponentHead;
-  export type GetPathFree = GetPathStrictFree | GetPathContextualFree;
-  export type GetPath = GetPathSymbol | GetPathTemplateSymbol | GetPathFree;
+  export type GetPath = GetPathSymbol | GetPathTemplateSymbol | GetPathContextualFree;
 
   export type Get = GetVar | GetPath;
 
diff --git a/packages/@glimmer/syntax/lib/parser/tokenizer-event-handlers.ts b/packages/@glimmer/syntax/lib/parser/tokenizer-event-handlers.ts
@@ -661,6 +661,22 @@ export interface TemplateIdFn {
 
 export interface PrecompileOptions extends PreprocessOptions {
   id?: TemplateIdFn;
+
+  /**
+   * Additional non-native keywords.
+   *
+   * Local variables (block params or lexical scope) always takes precedence,
+   * but otherwise, suitable free variable candidates (e.g. those are not part
+   * of a path) are matched against this list and turned into keywords.
+   *
+   * In strict mode compilation, keywords suppresses the undefined reference
+   * error and will be resolved by the runtime environment.
+   *
+   * In loose mode, keywords are currently ignored and since all free variables
+   * are already resolved by the runtime environment.
+   */
+  keywords?: readonly string[];
+
   customizeComponentName?: ((input: string) => string) | undefined;
 }
 
diff --git a/packages/@glimmer/syntax/lib/symbol-table.ts b/packages/@glimmer/syntax/lib/symbol-table.ts
diff --git a/packages/@glimmer/syntax/lib/v2/builders.ts b/packages/@glimmer/syntax/lib/v2/builders.ts
diff --git a/packages/@glimmer/syntax/lib/v2/normalize.ts b/packages/@glimmer/syntax/lib/v2/normalize.ts
diff --git a/packages/@glimmer/syntax/lib/v2/objects/base.ts b/packages/@glimmer/syntax/lib/v2/objects/base.ts
diff --git a/packages/@glimmer/syntax/lib/v2/objects/expr.ts b/packages/@glimmer/syntax/lib/v2/objects/expr.ts
diff --git a/packages/@glimmer/syntax/lib/v2/serialize/serialize.ts b/packages/@glimmer/syntax/lib/v2/serialize/serialize.ts

Original file line number	Diff line number	Diff line change
`@@ -23,8 +23,8 @@ export default class JitCompileTimeLookup implements CompileTimeResolver {`
`23`	`23`	`return this.resolver.lookupComponent(name, owner);`
`24`	`24`	`}`
`25`	`25`
`26`		`- lookupBuiltInHelper(_name: string): Nullable<HelperDefinitionState> {`
`27`		`- return null;`
	`26`	`+ lookupBuiltInHelper(name: string): Nullable<HelperDefinitionState> {`
	`27`	+ return this.resolver.lookupHelper(`$keyword.${name}`);
`28`	`28`	`}`
`29`	`29`
`30`	`30`	`lookupBuiltInModifier(_name: string): Nullable<ModifierDefinitionState> {`
Original file line number	Diff line number	Diff line change
`@@ -96,6 +96,9 @@ export interface DefineComponentOptions {`
`96`	`96`
`97`	`97`	`// defaults to true when some scopeValues are passed and false otherwise`
`98`	`98`	`strictMode?: boolean;`
	`99`	`+`
	`100`	`+ // additional strict-mode keywords`
	`101`	`+ keywords?: string[];`
`99`	`102`	`}`
`100`	`103`
`101`	`104`	`export function defineComponent(`
`@@ -110,8 +113,10 @@ export function defineComponent(`
`110`	`113`	`strictMode = scopeValues !== null;`
`111`	`114`	`}`
`112`	`115`
	`116`	`+ let keywords = options.keywords ?? [];`
	`117`	`+`
`113`	`118`	`let definition = options.definition ?? templateOnlyComponent();`
`114`		`- let templateFactory = createTemplate(templateSource, { strictMode }, scopeValues ?? {});`
	`119`	`+ let templateFactory = createTemplate(templateSource, { strictMode, keywords }, scopeValues ?? {});`
`115`	`120`	`setComponentTemplate(templateFactory, definition);`
`116`	`121`	`return definition;`
`117`	`122`	`}`