From 4686800bf2ab21b3bbd953b1eabd31318a685b9e Mon Sep 17 00:00:00 2001 From: Yehuda Katz Date: Thu, 23 Jan 2025 21:59:07 -0800 Subject: [PATCH] Split modifier and component to resolved variants --- .../test/debug-render-tree-test.ts | 3 +- .../test/strict-mode-test.ts | 2 +- .../@glimmer/compiler/lib/builder/builder.ts | 35 ++++++++------- .../compiler/lib/passes/2-encoding/content.ts | 30 ++++++++----- .../compiler/lib/wire-format-debug.ts | 10 +++-- .../interfaces/lib/compile/wire-format/api.ts | 36 ++++++++++----- .../lib/compile/wire-format/opcodes.d.ts | 6 ++- .../lib/opcode-builder/helpers/resolution.ts | 5 ++- .../opcode-compiler/lib/syntax/statements.ts | 44 +++++++++++-------- packages/@glimmer/wire-format/index.ts | 3 +- packages/@glimmer/wire-format/lib/opcodes.ts | 12 +++-- 11 files changed, 114 insertions(+), 72 deletions(-) diff --git a/packages/@glimmer-workspace/integration-tests/test/debug-render-tree-test.ts b/packages/@glimmer-workspace/integration-tests/test/debug-render-tree-test.ts index 5cecd7d08..c831c05c6 100644 --- a/packages/@glimmer-workspace/integration-tests/test/debug-render-tree-test.ts +++ b/packages/@glimmer-workspace/integration-tests/test/debug-render-tree-test.ts @@ -116,6 +116,7 @@ class DebugRenderTreeTest extends RenderTest { const HelloWorld = defComponent('

{{@arg}}

'); const noopFn = () => {}; const noop = defineSimpleModifier(noopFn); + const Root = defComponent( `{{#if state.showSecond}}{{/if}}`, { scope: { HelloWorld, state, noop }, emit: { moduleName: 'root.hbs' } } @@ -144,7 +145,7 @@ class DebugRenderTreeTest extends RenderTest { children: [ { type: 'modifier', - name: 'noop', + name: 'noopFn', args: { positional: [], named: {} }, instance: (modifier: unknown) => modifier && Reflect.get(modifier, 'fn') === noopFn, template: null, diff --git a/packages/@glimmer-workspace/integration-tests/test/strict-mode-test.ts b/packages/@glimmer-workspace/integration-tests/test/strict-mode-test.ts index 8251b28c9..cf5f5250b 100644 --- a/packages/@glimmer-workspace/integration-tests/test/strict-mode-test.ts +++ b/packages/@glimmer-workspace/integration-tests/test/strict-mode-test.ts @@ -586,7 +586,7 @@ class StaticStrictModeTest extends RenderTest { this.assert.throws(() => { this.renderComponent(Bar); - }, /Attempted to load a modifier, but there wasn't a modifier manager associated with the definition. The definition was:/u); + }, /Expected a dynamic modifier definition, but received an object or function that did not have a modifier manager associated with it. The dynamic invocation was `\{\{false\}\}`/u); } } diff --git a/packages/@glimmer/compiler/lib/builder/builder.ts b/packages/@glimmer/compiler/lib/builder/builder.ts index 9a241fa1f..68e262593 100644 --- a/packages/@glimmer/compiler/lib/builder/builder.ts +++ b/packages/@glimmer/compiler/lib/builder/builder.ts @@ -232,16 +232,14 @@ export function buildNormalizedStatements( export function buildAppend( trusted: boolean, expr: Expressions.Expression -): WireFormat.Statement[] { +): WireFormat.Statements.SomeAppend { if (Array.isArray(expr) && expr[0] === Op.GetFreeAsComponentOrHelperHead) { - return [ - trusted - ? [Op.UnknownTrustingAppend, expr as Expressions.GetUnknownAppend] - : [Op.UnknownAppend, expr as Expressions.GetUnknownAppend], - ]; + return trusted + ? [Op.UnknownTrustingAppend, expr as Expressions.GetUnknownAppend] + : [Op.UnknownAppend, expr as Expressions.GetUnknownAppend]; } - return [[trusted ? Op.TrustingAppend : Op.Append, expr]]; + return [trusted ? Op.TrustingAppend : Op.Append, expr]; } export function buildStatement( @@ -250,14 +248,16 @@ export function buildStatement( ): WireFormat.Statement[] { switch (normalized.kind) { case APPEND_PATH_HEAD: { - return buildAppend(normalized.trusted, buildGetPath(normalized.path, symbols)); + return [buildAppend(normalized.trusted, buildGetPath(normalized.path, symbols))]; } case APPEND_EXPR_HEAD: { - return buildAppend( - normalized.trusted, - buildExpression(normalized.expr, normalized.trusted ? 'TrustedAppend' : 'Append', symbols) - ); + return [ + buildAppend( + normalized.trusted, + buildExpression(normalized.expr, normalized.trusted ? 'TrustedAppend' : 'Append', symbols) + ), + ]; } case CALL_HEAD: { @@ -818,20 +818,21 @@ export function invokeType( } export function callType(expr: Expressions.Expression): CallLexicalOpcode | CallResolvedOpcode { - if (!Array.isArray(expr)) throw Error('Something is suspicious @fixme'); + if (!Array.isArray(expr)) { + throw Error('Something is suspicious @fixme'); + } let type = expr[0]; switch (type) { - case Op.GetLexicalSymbol: - case Op.GetSymbol: - return Op.CallLexical; case Op.GetFreeAsHelperHead: + case Op.GetFreeAsModifierHead: + case Op.GetFreeAsComponentHead: case Op.GetFreeAsComponentOrHelperHead: case Op.GetStrictKeyword: return Op.CallResolved; default: - throw Error(`Something is suspicious (unexpected ${type} opcode in call) @fixme`); + return Op.CallLexical; } } diff --git a/packages/@glimmer/compiler/lib/passes/2-encoding/content.ts b/packages/@glimmer/compiler/lib/passes/2-encoding/content.ts index 144c09036..b1752d5ef 100644 --- a/packages/@glimmer/compiler/lib/passes/2-encoding/content.ts +++ b/packages/@glimmer/compiler/lib/passes/2-encoding/content.ts @@ -17,7 +17,7 @@ import { SexpOpcodes } from '@glimmer/wire-format'; import type { OptionalList } from '../../shared/list'; import type * as mir from './mir'; -import { buildAppend } from '../../builder/builder'; +import { buildAppend, callType } from '../../builder/builder'; import { deflateAttrName, deflateTagName } from '../../utils'; import { EXPR } from './expressions'; @@ -119,10 +119,8 @@ export class ContentEncoder { return [SexpOpcodes.TrustingAppend, EXPR.expr(html)]; } - AppendTextNode({ - text, - }: mir.AppendTextNode): WireFormat.Statements.Append | WireFormat.Statements.UnknownAppend { - return buildAppend(false, EXPR.expr(text))[0]; + AppendTextNode({ text }: mir.AppendTextNode): WireFormat.Statements.SomeAppend { + return buildAppend(false, EXPR.expr(text)); } AppendComment({ value }: mir.AppendComment): WireFormat.Statements.Comment { @@ -168,8 +166,16 @@ export class ContentEncoder { return [dynamicAttrOp(param.kind), ...dynamicAttr(param)]; case 'StaticAttr': return [staticAttrOp(param.kind), ...staticAttr(param)]; - case 'Modifier': - return [SexpOpcodes.Modifier, EXPR.expr(param.callee), ...EXPR.Args(param.args)]; + case 'Modifier': { + const expr = EXPR.expr(param.callee); + return [ + callType(expr) === SexpOpcodes.CallLexical + ? SexpOpcodes.LexicalModifier + : SexpOpcodes.ResolvedModifier, + EXPR.expr(param.callee), + ...EXPR.Args(param.args), + ]; + } } } @@ -226,10 +232,14 @@ export class ContentEncoder { definition, args, blocks, - }: mir.InvokeComponent): WireFormat.Statements.InvokeComponent { + }: mir.InvokeComponent): WireFormat.Statements.SomeInvokeComponent { + const expr = EXPR.expr(definition); + return [ - SexpOpcodes.InvokeComponent, - EXPR.expr(definition), + typeof expr === 'string' || callType(expr) === SexpOpcodes.CallLexical + ? SexpOpcodes.InvokeLexicalComponent + : SexpOpcodes.InvokeResolvedComponent, + expr, EXPR.Positional(args.positional), EXPR.NamedArguments(args.named), blocks ? CONTENT.NamedBlocks(blocks) : null, diff --git a/packages/@glimmer/compiler/lib/wire-format-debug.ts b/packages/@glimmer/compiler/lib/wire-format-debug.ts index 3f429e758..1a40b49f1 100644 --- a/packages/@glimmer/compiler/lib/wire-format-debug.ts +++ b/packages/@glimmer/compiler/lib/wire-format-debug.ts @@ -124,9 +124,10 @@ export default class WireFormatDebugger { case Op.Comment: return ['comment', opcode[1]]; - case Op.Modifier: + case Op.LexicalModifier: + case Op.ResolvedModifier: return [ - 'modifier', + opcode[0] === Op.ResolvedModifier ? 'modifier:resolved' : 'modifier', this.formatOpcode(opcode[1]), this.formatParams(opcode[2]), this.formatHash(opcode[3]), @@ -233,9 +234,10 @@ export default class WireFormatDebugger { case Op.GetDynamicVar: return ['-get-dynamic-vars', this.formatOpcode(opcode[1])]; - case Op.InvokeComponent: + case Op.InvokeLexicalComponent: + case Op.InvokeResolvedComponent: return [ - 'component', + opcode[0] === Op.InvokeLexicalComponent ? 'component' : 'component:resolved', this.formatOpcode(opcode[1]), this.formatParams(opcode[2]), this.formatHash(opcode[3]), diff --git a/packages/@glimmer/interfaces/lib/compile/wire-format/api.ts b/packages/@glimmer/interfaces/lib/compile/wire-format/api.ts index a8788a75c..97d0dd55c 100644 --- a/packages/@glimmer/interfaces/lib/compile/wire-format/api.ts +++ b/packages/@glimmer/interfaces/lib/compile/wire-format/api.ts @@ -33,13 +33,15 @@ import type { IfInlineOpcode, IfOpcode, InElementOpcode, - InvokeComponentOpcode, + InvokeLexicalComponentOpcode, + InvokeResolvedComponentOpcode, LetOpcode, + LexicalModifierOpcode, LogOpcode, - ModifierOpcode, NotOpcode, OpenElementOpcode, OpenElementWithSplatOpcode, + ResolvedModifierOpcode, StaticArgOpcode, StaticAttrOpcode, StaticComponentAttrOpcode, @@ -220,12 +222,17 @@ export namespace Statements { export type Blocks = Core.Blocks; export type Path = Core.Path; + export type SomeAppend = Append | TrustingAppend | UnknownAppend | UnknownTrustingAppend; + export type SomeModifier = LexicalModifier | ResolvedModifier; + export type SomeInvokeComponent = InvokeLexicalComponent | InvokeResolvedComponent; + export type UnknownAppend = [UnknownAppendOpcode, Expressions.GetUnknownAppend]; export type UnknownTrustingAppend = [UnknownTrustingAppendOpcode, Expressions.GetUnknownAppend]; export type Append = [AppendOpcode, Expression]; export type TrustingAppend = [TrustingAppendOpcode, Expression]; export type Comment = [CommentOpcode, string]; - export type Modifier = [ModifierOpcode, Expression, Params, Hash]; + export type LexicalModifier = [LexicalModifierOpcode, Expression, Params, Hash]; + export type ResolvedModifier = [ResolvedModifierOpcode, Expression, Params, Hash]; export type Block = [BlockOpcode, Expression, Params, Hash, Blocks]; export type Component = [ op: ComponentOpcode, @@ -304,8 +311,16 @@ export namespace Statements { block: SerializedInlineBlock, ]; - export type InvokeComponent = [ - op: InvokeComponentOpcode, + export type InvokeLexicalComponent = [ + op: InvokeLexicalComponentOpcode, + definition: Expression, + positional: Core.Params, + named: Core.Hash, + blocks: Blocks | null, + ]; + + export type InvokeResolvedComponent = [ + op: InvokeResolvedComponentOpcode, definition: Expression, positional: Core.Params, named: Core.Hash, @@ -316,12 +331,9 @@ export namespace Statements { * A Handlebars statement */ export type Statement = - | Append - | TrustingAppend - | UnknownAppend - | UnknownTrustingAppend + | SomeAppend + | SomeModifier | Comment - | Modifier | Block | Component | OpenElement @@ -339,7 +351,7 @@ export namespace Statements { | Each | Let | WithDynamicVars - | InvokeComponent; + | SomeInvokeComponent; export type Attribute = | StaticAttr @@ -349,7 +361,7 @@ export namespace Statements { | ComponentAttr | TrustingComponentAttr; - export type ComponentFeature = Modifier | AttrSplat; + export type ComponentFeature = SomeModifier | AttrSplat; export type Argument = StaticArg | DynamicArg; export type ElementParameter = Attribute | Argument | ComponentFeature; diff --git a/packages/@glimmer/interfaces/lib/compile/wire-format/opcodes.d.ts b/packages/@glimmer/interfaces/lib/compile/wire-format/opcodes.d.ts index 07374a724..5c3e4db76 100644 --- a/packages/@glimmer/interfaces/lib/compile/wire-format/opcodes.d.ts +++ b/packages/@glimmer/interfaces/lib/compile/wire-format/opcodes.d.ts @@ -4,7 +4,8 @@ export type UnknownAppendOpcode = 2; export type UnknownTrustingAppendOpcode = 3; export type TrustingAppendOpcode = 4; export type CommentOpcode = 5; -export type ModifierOpcode = 6; +export type LexicalModifierOpcode = 6; +export type ResolvedModifierOpcode = 55; export type StrictModifierOpcode = 7; export type BlockOpcode = 8; export type StrictBlockOpcode = 9; @@ -57,7 +58,8 @@ export type IfOpcode = 41; export type EachOpcode = 42; export type LetOpcode = 44; export type WithDynamicVarsOpcode = 45; -export type InvokeComponentOpcode = 46; +export type InvokeLexicalComponentOpcode = 46; +export type InvokeResolvedComponentOpcode = 47; // Keyword Expressions export type HasBlockOpcode = 48; diff --git a/packages/@glimmer/opcode-compiler/lib/opcode-builder/helpers/resolution.ts b/packages/@glimmer/opcode-compiler/lib/opcode-builder/helpers/resolution.ts index e8a8a301f..3b6752539 100644 --- a/packages/@glimmer/opcode-compiler/lib/opcode-builder/helpers/resolution.ts +++ b/packages/@glimmer/opcode-compiler/lib/opcode-builder/helpers/resolution.ts @@ -80,7 +80,10 @@ export function resolveComponent( expr: Expressions.Expression, then: (component: CompileTimeComponent) => void ): void { - localAssert(isGetFreeComponent(expr), 'Attempted to resolve a component with incorrect opcode'); + localAssert( + isGetFreeComponent(expr), + `Attempted to resolve a component with incorrect opcode (${JSON.stringify(expr)})` + ); let type = expr[0]; diff --git a/packages/@glimmer/opcode-compiler/lib/syntax/statements.ts b/packages/@glimmer/opcode-compiler/lib/syntax/statements.ts index e5d54824f..ffac9beb1 100644 --- a/packages/@glimmer/opcode-compiler/lib/syntax/statements.ts +++ b/packages/@glimmer/opcode-compiler/lib/syntax/statements.ts @@ -54,7 +54,7 @@ import { } from '../opcode-builder/helpers/components'; import { Replayable, ReplayableIf, SwitchCases } from '../opcode-builder/helpers/conditional'; import { expr } from '../opcode-builder/helpers/expr'; -import { isGetFreeComponent, isGetFreeModifier } from '../opcode-builder/helpers/resolution'; +import { isGetFreeComponent } from '../opcode-builder/helpers/resolution'; import { CompilePositional, SimpleArgs } from '../opcode-builder/helpers/shared'; import { Call, @@ -91,22 +91,22 @@ STATEMENTS.add(SexpOpcodes.Comment, (op, [, comment]) => Comment(op, comment)); STATEMENTS.add(SexpOpcodes.CloseElement, (op) => CloseElement(op)); STATEMENTS.add(SexpOpcodes.FlushElement, (op) => FlushElement(op)); -STATEMENTS.add(SexpOpcodes.Modifier, (encode, [, expression, positional, named]) => { - if (isGetFreeModifier(expression)) { - encode.modifier(expression, (handle: number) => { - encode.op(VM_PUSH_FRAME_OP); - SimpleArgs(encode, positional, named, false); - encode.op(VM_MODIFIER_OP, handle); - encode.op(VM_POP_FRAME_OP); - }); - } else { - expr(encode, expression); +STATEMENTS.add(SexpOpcodes.ResolvedModifier, (encode, [, expression, positional, named]) => { + encode.modifier(expression, (handle: number) => { encode.op(VM_PUSH_FRAME_OP); SimpleArgs(encode, positional, named, false); - encode.op(VM_DUP_OP, $fp, 1); - encode.op(VM_DYNAMIC_MODIFIER_OP); + encode.op(VM_MODIFIER_OP, handle); encode.op(VM_POP_FRAME_OP); - } + }); +}); + +STATEMENTS.add(SexpOpcodes.LexicalModifier, (encode, [, expression, positional, named]) => { + expr(encode, expression); + encode.op(VM_PUSH_FRAME_OP); + SimpleArgs(encode, positional, named, false); + encode.op(VM_DUP_OP, $fp, 1); + encode.op(VM_DYNAMIC_MODIFIER_OP); + encode.op(VM_POP_FRAME_OP); }); STATEMENTS.add(SexpOpcodes.StaticAttr, (encode, [, name, value, namespace]) => { @@ -408,15 +408,21 @@ STATEMENTS.add(SexpOpcodes.WithDynamicVars, (encode, [, named, block]) => { } }); -STATEMENTS.add(SexpOpcodes.InvokeComponent, (encode, [, expr, positional, named, blocks]) => { - if (isGetFreeComponent(expr)) { +STATEMENTS.add( + SexpOpcodes.InvokeLexicalComponent, + (encode, [, expr, positional, named, blocks]) => { + InvokeDynamicComponent(encode, expr, null, positional, named, blocks, false, false); + } +); + +STATEMENTS.add( + SexpOpcodes.InvokeResolvedComponent, + (encode, [, expr, positional, named, blocks]) => { encode.component(expr, (component: CompileTimeComponent) => { InvokeComponent(encode, component, null, positional, hashToArgs(named), blocks); }); - } else { - InvokeDynamicComponent(encode, expr, null, positional, named, blocks, false, false); } -}); +); function hashToArgs(hash: WireFormat.Core.Hash | null): WireFormat.Core.Hash | null { if (hash === null) return null; diff --git a/packages/@glimmer/wire-format/index.ts b/packages/@glimmer/wire-format/index.ts index 4c3c6d6f1..c17ad4dcd 100644 --- a/packages/@glimmer/wire-format/index.ts +++ b/packages/@glimmer/wire-format/index.ts @@ -25,7 +25,8 @@ export function isAttribute(val: Statement): val is Statements.Attribute { val[0] === opcodes.StaticComponentAttr || val[0] === opcodes.TrustingComponentAttr || val[0] === opcodes.AttrSplat || - val[0] === opcodes.Modifier + val[0] === opcodes.LexicalModifier || + val[0] === opcodes.ResolvedModifier ); } diff --git a/packages/@glimmer/wire-format/lib/opcodes.ts b/packages/@glimmer/wire-format/lib/opcodes.ts index 24aeeed2e..e462619b5 100644 --- a/packages/@glimmer/wire-format/lib/opcodes.ts +++ b/packages/@glimmer/wire-format/lib/opcodes.ts @@ -28,13 +28,15 @@ import type { IfInlineOpcode, IfOpcode, InElementOpcode, - InvokeComponentOpcode, + InvokeLexicalComponentOpcode, + InvokeResolvedComponentOpcode, LetOpcode, + LexicalModifierOpcode, LogOpcode, - ModifierOpcode, NotOpcode, OpenElementOpcode, OpenElementWithSplatOpcode, + ResolvedModifierOpcode, StaticArgOpcode, StaticAttrOpcode, StaticComponentAttrOpcode, @@ -57,7 +59,8 @@ export const opcodes = { UnknownTrustingAppend: 3 satisfies UnknownTrustingAppendOpcode, TrustingAppend: 4 satisfies TrustingAppendOpcode, Comment: 5 satisfies CommentOpcode, - Modifier: 6 satisfies ModifierOpcode, + LexicalModifier: 6 satisfies LexicalModifierOpcode, + ResolvedModifier: 55 satisfies ResolvedModifierOpcode, StrictModifier: 7 satisfies StrictModifierOpcode, Block: 8 satisfies BlockOpcode, StrictBlock: 9 satisfies StrictBlockOpcode, @@ -94,7 +97,8 @@ export const opcodes = { Each: 42 satisfies EachOpcode, Let: 44 satisfies LetOpcode, WithDynamicVars: 45 satisfies WithDynamicVarsOpcode, - InvokeComponent: 46 satisfies InvokeComponentOpcode, + InvokeLexicalComponent: 46 satisfies InvokeLexicalComponentOpcode, + InvokeResolvedComponent: 47 satisfies InvokeResolvedComponentOpcode, HasBlock: 48 satisfies HasBlockOpcode, HasBlockParams: 49 satisfies HasBlockParamsOpcode, Curry: 50 satisfies CurryOpcode,