Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove support for Partials #1547

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Remove support for Partials #1547

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
52b7b0b
Remove support for Partials
NullVoxPopuli Jan 19, 2024
7d07ae9
Revert "Remove support for Partials"
NullVoxPopuli Jan 19, 2024
e1887e9
Remove bindEval and evalScope
NullVoxPopuli Jan 19, 2024
95470fc
Remove more partial, rename PartialScope to Scope
NullVoxPopuli Jan 19, 2024
7f9a6c5
lintfix
NullVoxPopuli Jan 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion bin/opcodes.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,6 @@
"GetComponentSelf",
"GetComponentTagName",
"GetComponentLayout",
"SetupForEval",
"PopulateLayout",
"InvokeComponentLayout",
"BeginComponentTransaction",
Expand Down
45 changes: 0 additions & 45 deletions packages/@glimmer/debug/lib/opcode-metadata.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1187,36 +1187,6 @@ METADATA[Op.GetComponentLayout] = {
check: true,
};

METADATA[Op.BindEvalScope] = {
name: 'BindEvalScope',
mnemonic: 'eval_scope',
before: null,
stackChange: 0,
ops: [
{
name: 'state',
type: 'register',
},
],
operands: 1,
check: true,
};

METADATA[Op.SetupForEval] = {
name: 'SetupForEval',
mnemonic: 'eval_setup',
before: null,
stackChange: 0,
ops: [
{
name: 'state',
type: 'register',
},
],
operands: 1,
check: true,
};

METADATA[Op.PopulateLayout] = {
name: 'PopulateLayout',
mnemonic: 'comp_layoutput',
Expand Down Expand Up @@ -1297,21 +1267,6 @@ METADATA[Op.DidRenderLayout] = {
check: true,
};

METADATA[Op.ResolveMaybeLocal] = {
name: 'ResolveMaybeLocal',
mnemonic: 'eval_varload',
before: null,
stackChange: 1,
ops: [
{
name: 'local',
type: 'str',
},
],
operands: 1,
check: true,
};

METADATA[Op.Debugger] = {
name: 'Debugger',
mnemonic: 'debugger',
Expand Down
10 changes: 1 addition & 9 deletions packages/@glimmer/interfaces/lib/runtime/scope.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { Dict, Nullable } from '../core.js';
import type { Nullable } from '../core.js';
import type { Reference } from '../references.js';
import type { CompilableBlock } from '../template.js';
import type { BlockSymbolTable } from '../tier1/symbol-table.js';
Expand All @@ -18,21 +18,13 @@ export interface Scope {
getSelf(): Reference;
getSymbol(symbol: number): Reference;
getBlock(symbol: number): Nullable<ScopeBlock>;
getEvalScope(): Nullable<Dict<ScopeSlot>>;
getPartialMap(): Nullable<Dict<Reference>>;
bind(symbol: number, value: ScopeSlot): void;
bindSelf(self: Reference): void;
bindSymbol(symbol: number, value: Reference): void;
bindBlock(symbol: number, value: Nullable<ScopeBlock>): void;
bindEvalScope(map: Nullable<Dict<ScopeSlot>>): void;
bindPartialMap(map: Dict<Reference>): void;
child(): Scope;
}

export interface PartialScope extends Scope {
bindEvalScope(scope: Nullable<Dict<ScopeSlot>>): void;
}

export interface DynamicScope {
get(key: string): Reference<unknown>;
set(key: string, reference: Reference<unknown>): Reference<unknown>;
Expand Down
6 changes: 0 additions & 6 deletions packages/@glimmer/interfaces/lib/vm-opcodes.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,15 +96,12 @@ export type VmPutComponentOperations = 89;
export type VmGetComponentSelf = 90;
export type VmGetComponentTagName = 91;
export type VmGetComponentLayout = 92;
export type VmBindEvalScope = 93;
export type VmSetupForEval = 94;
export type VmPopulateLayout = 95;
export type VmInvokeComponentLayout = 96;
export type VmBeginComponentTransaction = 97;
export type VmCommitComponentTransaction = 98;
export type VmDidCreateElement = 99;
export type VmDidRenderLayout = 100;
export type VmResolveMaybeLocal = 102;
export type VmDebugger = 103;
export type VmSize = 104;
export type VmStaticComponentAttr = 105;
Expand Down Expand Up @@ -194,15 +191,12 @@ export type VmOp =
| VmGetComponentSelf
| VmGetComponentTagName
| VmGetComponentLayout
| VmBindEvalScope
| VmSetupForEval
| VmPopulateLayout
| VmInvokeComponentLayout
| VmBeginComponentTransaction
| VmCommitComponentTransaction
| VmDidCreateElement
| VmDidRenderLayout
| VmResolveMaybeLocal
| VmDebugger
| VmSize
| VmStaticComponentAttr
Expand Down
1 change: 0 additions & 1 deletion packages/@glimmer/opcode-compiler/lib/opcode-builder/helpers/components.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,6 @@ export function invokePreparedComponent(

op(Op.VirtualRootScope, $s0);
op(Op.SetVariable, 0);
op(Op.SetupForEval, $s0);

if (bindableAtNames) op(Op.SetNamedVariables, $s0);
if (bindableBlocks) op(Op.SetBlocks, $s0);
Expand Down
2 changes: 1 addition & 1 deletion packages/@glimmer/runtime/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export { hash } from './lib/helpers/hash';
export { invokeHelper } from './lib/helpers/invoke';
export { on } from './lib/modifiers/on';
export { renderComponent, renderMain, renderSync } from './lib/render';
export { DynamicScopeImpl, PartialScopeImpl } from './lib/scope';
export { DynamicScopeImpl, ScopeImpl } from './lib/scope';
export type { SafeString } from './lib/upsert';
export { type InternalVM, VM as LowLevelVM, UpdatingVM } from './lib/vm';
export {
Expand Down
4 changes: 2 additions & 2 deletions packages/@glimmer/runtime/lib/compiled/opcodes/-debug-strip.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ import {
import { REFERENCE, UNDEFINED_REFERENCE } from '@glimmer/reference';
import { COMPUTE } from '@glimmer/validator';

import { PartialScopeImpl } from '../../scope';
import { ScopeImpl } from '../../scope';
import { VMArgumentsImpl } from '../../vm/arguments';
import { ComponentElementOperations } from './component';

Expand Down Expand Up @@ -92,7 +92,7 @@ export const CheckCapturedArguments: Checker<CapturedArguments> = CheckInterface
named: wrap(() => CheckDict(CheckReference)),
});

export const CheckScope: Checker<Scope> = wrap(() => CheckInstanceof(PartialScopeImpl));
export const CheckScope: Checker<Scope> = wrap(() => CheckInstanceof(ScopeImpl));

export const CheckComponentManager: Checker<InternalComponentManager<unknown>> = CheckInterface({
getCapabilities: CheckFunction,
Expand Down
9 changes: 0 additions & 9 deletions packages/@glimmer/runtime/lib/compiled/opcodes/component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -766,15 +766,6 @@ APPEND_OPCODES.add(Op.VirtualRootScope, (vm, { op1: _state }) => {
vm.pushRootScope(table.symbols.length + 1, owner);
});

APPEND_OPCODES.add(Op.SetupForEval, (vm, { op1: _state }) => {
let state = check(vm.fetchValue(_state), CheckFinishedComponentInstance);

if (state.table.hasEval) {
let lookup = (state.lookup = dict<ScopeSlot>());
vm.scope().bindEvalScope(lookup);
}
});

APPEND_OPCODES.add(Op.SetNamedVariables, (vm, { op1: _state }) => {
let state = check(vm.fetchValue(_state), CheckFinishedComponentInstance);
let scope = vm.scope();
Expand Down
5 changes: 1 addition & 4 deletions packages/@glimmer/runtime/lib/compiled/opcodes/debugger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,16 +52,13 @@ class ScopeInspector {
let { scope, locals } = this;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like we still generally treat the {{debugger}} statement as an "runtime eval", which is the same kind of thing as partial.

In order to support it, we have to keep around a lot more strings labels (the names of the variables) than we otherwise need to, just so when we "eval" code (in the case of {{debugger}}, calling the get("...") function in the console where "..." can be any local variables, named arguments, etc.

I think now that "implicit this" and partials are no longer supported, I think we should be able to accomplish the same by doing a bit more analysis at pre-compile time. We should be able to see all the in-scope local variables at that point, and we can internally rewrite:

{{#let this.foo as |foo|}}
  {{#let foo.bar as |bar|}} 
    {{!-- magically `get("foo")` and `get("bar")` has to work in the console --}}
    {{debugger}}
  {{/let}}
{{/let}}

into

{{#let this.foo as |foo|}}
  {{#let foo.bar as |bar|}} 
    {{debugger foo=foo bar=bar}}
  {{/let}}
{{/let}}

Basically we pass every known local variables (and there is no longer such thing as "unknown local variables" anymore) as named arguments, and that should give sufficient information for the {{debugger}} implementation to make get() work without any pervasive book-keeping everywhere.

let parts = path.split('.');
let [head, ...tail] = path.split('.') as [string, ...string[]];

let evalScope = scope.getEvalScope()!;
let ref: Reference;

if (head === 'this') {
ref = scope.getSelf();
} else if (locals[head]) {
ref = unwrap(locals[head]);
} else if (head.indexOf('@') === 0 && evalScope[head]) {
ref = evalScope[head] as Reference;
// FIXME restore get("@foo") functionality
} else {
ref = this.scope.getSelf();
tail = parts;
Expand Down
12 changes: 0 additions & 12 deletions packages/@glimmer/runtime/lib/compiled/opcodes/expressions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,18 +181,6 @@ APPEND_OPCODES.add(Op.SetBlock, (vm, { op1: symbol }) => {
vm.scope().bindBlock(symbol, [handle, scope, table]);
});

APPEND_OPCODES.add(Op.ResolveMaybeLocal, (vm, { op1: _name }) => {
let name = vm[CONSTANTS].getValue<string>(_name);
let locals = vm.scope().getPartialMap()!;

let ref = locals[name];
if (ref === undefined) {
ref = childRefFor(vm.getSelf(), name);
}

vm.stack.push(ref);
});

APPEND_OPCODES.add(Op.RootScope, (vm, { op1: symbols }) => {
vm.pushRootScope(symbols, vm.getOwner());
});
Expand Down
42 changes: 9 additions & 33 deletions packages/@glimmer/runtime/lib/scope.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import type {
DynamicScope,
Nullable,
Owner,
PartialScope,
Scope,
ScopeBlock,
ScopeSlot,
Expand Down Expand Up @@ -41,29 +40,28 @@ export function isScopeReference(s: ScopeSlot): s is Reference {
return true;
}

export class PartialScopeImpl implements PartialScope {
static root(self: Reference<unknown>, size = 0, owner: Owner): PartialScope {
export class ScopeImpl implements Scope {
static root(self: Reference<unknown>, size = 0, owner: Owner): Scope {
let refs: Reference<unknown>[] = new Array(size + 1).fill(UNDEFINED_REFERENCE);

return new PartialScopeImpl(refs, owner, null, null, null).init({ self });
return new ScopeImpl(refs, owner, null).init({ self });
}

static sized(size = 0, owner: Owner): Scope {
let refs: Reference<unknown>[] = new Array(size + 1).fill(UNDEFINED_REFERENCE);

return new PartialScopeImpl(refs, owner, null, null, null);
return new ScopeImpl(refs, owner, null);
}

constructor(
// the 0th slot is `self`
readonly slots: Array<ScopeSlot>,
readonly owner: Owner,
private callerScope: Scope | null,
// named arguments and blocks passed to a layout that uses eval
private evalScope: Dict<ScopeSlot> | null,
// locals in scope when the partial was invoked
private partialMap: Dict<Reference<unknown>> | null
private callerScope: Scope | null
) {}
getPartialMap(): Nullable<Dict<Reference<unknown>>> {
throw new Error('Method not implemented.');
}

init({ self }: { self: Reference<unknown> }): this {
this.slots[0] = self;
Expand All @@ -83,14 +81,6 @@ export class PartialScopeImpl implements PartialScope {
return block === UNDEFINED_REFERENCE ? null : (block as ScopeBlock);
}

getEvalScope(): Nullable<Dict<ScopeSlot>> {
return this.evalScope;
}

getPartialMap(): Nullable<Dict<Reference<unknown>>> {
return this.partialMap;
}

bind(symbol: number, value: ScopeSlot) {
this.set(symbol, value);
}
Expand All @@ -107,14 +97,6 @@ export class PartialScopeImpl implements PartialScope {
this.set<Nullable<ScopeBlock>>(symbol, value);
}

bindEvalScope(map: Nullable<Dict<ScopeSlot>>) {
this.evalScope = map;
}

bindPartialMap(map: Dict<Reference<unknown>>) {
this.partialMap = map;
}

bindCallerScope(scope: Nullable<Scope>): void {
this.callerScope = scope;
}
Expand All @@ -124,13 +106,7 @@ export class PartialScopeImpl implements PartialScope {
}

child(): Scope {
return new PartialScopeImpl(
this.slots.slice(),
this.owner,
this.callerScope,
this.evalScope,
this.partialMap
);
return new ScopeImpl(this.slots.slice(), this.owner, this.callerScope);
}

private get<T extends ScopeSlot>(index: number): T {
Expand Down
13 changes: 6 additions & 7 deletions packages/@glimmer/runtime/lib/vm/append.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import type {
Environment,
Nullable,
Owner,
PartialScope,
RenderResult,
ResolutionTimeConstants,
RichIteratorResult,
Expand Down Expand Up @@ -41,7 +40,7 @@ import {
JumpIfNotModifiedOpcode,
} from '../compiled/opcodes/vm';
import { APPEND_OPCODES } from '../opcodes';
import { PartialScopeImpl } from '../scope';
import { ScopeImpl } from '../scope';
import { ARGS, CONSTANTS, DESTROYABLE_STACK, HEAP, INNER_VM, REGISTERS, STACKS } from '../symbols';
import { VMArgumentsImpl } from './arguments';
import { LowLevelVM } from './low-level';
Expand Down Expand Up @@ -96,7 +95,7 @@ export interface InternalVM {
enterItem(item: OpaqueIterationItem): ListItemOpcode;
registerItem(item: ListItemOpcode): void;

pushRootScope(size: number, owner: Owner): PartialScope;
pushRootScope(size: number, owner: Owner): Scope;
pushChildScope(): void;
popScope(): void;
pushScope(scope: Scope): void;
Expand Down Expand Up @@ -303,7 +302,7 @@ export class VM implements PublicVM, InternalVM {
context: CompileTimeCompilationContext,
{ handle, self, dynamicScope, treeBuilder, numSymbols, owner }: InitOptions
) {
let scope = PartialScopeImpl.root(self, numSymbols, owner);
let scope = ScopeImpl.root(self, numSymbols, owner);
let state = vmState(runtime.program.heap.getaddr(handle), scope, dynamicScope);
let vm = initVM(context)(runtime, state, treeBuilder);
vm.pushUpdating();
Expand All @@ -319,7 +318,7 @@ export class VM implements PublicVM, InternalVM {
runtime,
vmState(
runtime.program.heap.getaddr(handle),
PartialScopeImpl.root(UNDEFINED_REFERENCE, 0, owner),
ScopeImpl.root(UNDEFINED_REFERENCE, 0, owner),
dynamicScope
),
treeBuilder
Expand Down Expand Up @@ -500,8 +499,8 @@ export class VM implements PublicVM, InternalVM {
return child;
}

pushRootScope(size: number, owner: Owner): PartialScope {
let scope = PartialScopeImpl.sized(size, owner);
pushRootScope(size: number, owner: Owner): Scope {
let scope = ScopeImpl.sized(size, owner);
this[STACKS].scope.push(scope);
return scope;
}
Expand Down
22 changes: 0 additions & 22 deletions packages/@glimmer/vm/lib/opcodes.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -834,16 +834,6 @@ operand-stack = [
["ProgramSymbolTable", "handle"]
]

[syscall.eval_scope]

format = ["BindEvalScope", "state:register"]
operation = "Populate the eval lookup if necessary."

[syscall.eval_setup]

format = ["SetupForEval", "state:register"]
operation = "Setup for eval"

[syscall.comp_layoutput]

format = ["PopulateLayout", "state:register"]
Expand Down Expand Up @@ -885,18 +875,6 @@ operation = "Invoke didCreateElement on the current component manager"
format = ["DidRenderLayout", "state:register"]
operation = "Invoke didRenderLayout on the current component manager"

[syscall.eval_varload]

format = ["ResolveMaybeLocal", "local:str"]
operation = """
Resolve {{foo}} inside a partial, which could be either a self-lookup
or a local variable that is in-scope for the caller.
"""
operand-stack = [
[],
["Reference"]
]

[syscall.debugger]

format = ["Debugger", "symbols:str-array", "debugInfo:array"]
Expand Down
Loading
Loading