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.

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

Fix issues with "loose free variable" lookup by dropping unneeded GetFreeAsFallback op code #1362

Merged
merged 1 commit into from
Nov 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
25 changes: 7 additions & 18 deletions packages/@glimmer/compiler/lib/builder/builder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ function buildKeyword(
return [Op.If, expect(params, 'if requires params')[0], block, inverse];
case 'each':
let keyExpr = normalized.hash ? normalized.hash['key'] : null;
let key = keyExpr ? buildExpression(keyExpr, 'Generic', symbols) : null;
let key = keyExpr ? buildExpression(keyExpr, 'Strict', symbols) : null;

return [Op.Each, expect(params, 'if requires params')[0], key, block, inverse];
default:
Expand Down Expand Up @@ -489,7 +489,6 @@ type ExprResolution =
| 'TrustedAppend'
| 'AttrValue'
| 'SubExpression'
| 'Generic'
| 'Strict';

function varContext(context: ExprResolution, bare: boolean): VarResolution {
Expand Down Expand Up @@ -528,7 +527,7 @@ export function buildExpression(
let builtHash = buildHash(expr.hash, symbols);
let builtExpr = buildCallHead(
expr.head,
context === 'Generic' ? 'SubExpression' : varContext(context, false),
context === 'Strict' ? 'SubExpression' : varContext(context, false),
symbols
);

Expand All @@ -540,7 +539,7 @@ export function buildExpression(
Op.HasBlock,
buildVar(
{ kind: VariableKind.Block, name: expr.name, mode: 'loose' },
VariableResolutionContext.LooseFreeVariable,
VariableResolutionContext.Strict,
symbols
),
];
Expand All @@ -551,7 +550,7 @@ export function buildExpression(
Op.HasBlockParams,
buildVar(
{ kind: VariableKind.Block, name: expr.name, mode: 'loose' },
VariableResolutionContext.LooseFreeVariable,
VariableResolutionContext.Strict,
symbols
),
];
Expand Down Expand Up @@ -583,12 +582,7 @@ export function buildCallHead(
}

export function buildGetPath(head: NormalizedPath, symbols: Symbols): Expressions.GetPath {
return buildVar(
head.path.head,
VariableResolutionContext.LooseFreeVariable,
symbols,
head.path.tail
);
return buildVar(head.path.head, VariableResolutionContext.Strict, symbols, head.path.tail);
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

}

type VarResolution =
Expand All @@ -600,7 +594,6 @@ type VarResolution =
| 'AttrValueBare'
| 'AttrValueInvoke'
| 'SubExpression'
| 'Generic'
| 'Strict';

export function buildVar(
Expand Down Expand Up @@ -640,8 +633,6 @@ export function buildVar(
op = SexpOpcodes.GetFreeAsHelperHead;
} else if (context === 'SubExpression') {
op = SexpOpcodes.GetFreeAsHelperHead;
} else if (context === 'Generic') {
op = SexpOpcodes.GetFreeAsFallback;
} else {
op = expressionContextOp(context);
}
Expand Down Expand Up @@ -688,8 +679,6 @@ export function expressionContextOp(context: VariableResolutionContext): GetCont
return Op.GetFreeAsComponentOrHelperHead;
case VariableResolutionContext.AmbiguousInvoke:
return Op.GetFreeAsHelperHeadOrThisFallback;
case VariableResolutionContext.LooseFreeVariable:
return Op.GetFreeAsFallback;
case VariableResolutionContext.ResolveAsCallHead:
return Op.GetFreeAsHelperHead;
case VariableResolutionContext.ResolveAsModifierHead:
Expand All @@ -707,7 +696,7 @@ export function buildParams(
): Option<WireFormat.Core.Params> {
if (exprs === null || !isPresent(exprs)) return null;

return exprs.map((e) => buildExpression(e, 'Generic', symbols)) as WireFormat.Core.ConcatParams;
return exprs.map((e) => buildExpression(e, 'Strict', symbols)) as WireFormat.Core.ConcatParams;
}

export function buildConcat(
Expand All @@ -724,7 +713,7 @@ export function buildHash(exprs: Option<NormalizedHash>, symbols: Symbols): Wire

Object.keys(exprs).forEach((key) => {
out[0].push(key);
out[1].push(buildExpression(exprs[key], 'Generic', symbols));
out[1].push(buildExpression(exprs[key], 'Strict', symbols));
});

return out as WireFormat.Core.Hash;
Expand Down
3 changes: 0 additions & 3 deletions packages/@glimmer/compiler/lib/wire-format-debug.ts
Original file line number Diff line number Diff line change
Expand Up @@ -171,9 +171,6 @@ export default class WireFormatDebugger {
case Op.GetStrictFree:
return ['get-strict-free', this.upvars[opcode[1]], opcode[2]];

case Op.GetFreeAsFallback:
return ['GetFreeAsFallback', this.upvars[opcode[1]], opcode[2]];

case Op.GetFreeAsComponentOrHelperHeadOrThisFallback:
return [
'GetFreeAsComponentOrHelperHeadOrThisFallback',
Expand Down
9 changes: 0 additions & 9 deletions packages/@glimmer/interfaces/lib/compile/wire-format.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ export const enum VariableResolutionContext {
AmbiguousAppend = 1,
AmbiguousAppendInvoke = 2,
AmbiguousInvoke = 3,
LooseFreeVariable = 4,
ResolveAsCallHead = 5,
ResolveAsModifierHead = 6,
ResolveAsComponentHead = 7,
Expand Down Expand Up @@ -69,9 +68,6 @@ export const enum SexpOpcodes {
// Free variables are only keywords in strict mode
GetStrictFree = 31,

// falls back to `this.` (or locals in the case of partials), but
// never turns into a component or helper invocation
GetFreeAsFallback = 33,
// `{{x}}` in append position (might be a helper or component invocation, otherwise fall back to `this`)
GetFreeAsComponentOrHelperHeadOrThisFallback = 34,
// a component or helper (`{{<expr> x}}` in append position)
Expand Down Expand Up @@ -117,7 +113,6 @@ export type GetContextualFreeOp =
| SexpOpcodes.GetFreeAsHelperHead
| SexpOpcodes.GetFreeAsModifierHead
| SexpOpcodes.GetFreeAsComponentHead
| SexpOpcodes.GetFreeAsFallback
| SexpOpcodes.GetStrictFree;

export type AttrOp =
Expand Down Expand Up @@ -167,7 +162,6 @@ export namespace Expressions {
export type GetSymbol = [SexpOpcodes.GetSymbol, number];
export type GetTemplateSymbol = [SexpOpcodes.GetTemplateSymbol, number];
export type GetStrictFree = [SexpOpcodes.GetStrictFree, number];
export type GetFreeAsFallback = [SexpOpcodes.GetFreeAsFallback, number];
export type GetFreeAsComponentOrHelperHeadOrThisFallback = [
SexpOpcodes.GetFreeAsComponentOrHelperHeadOrThisFallback,
number
Expand All @@ -186,7 +180,6 @@ export namespace Expressions {
export type GetFreeAsComponentHead = [SexpOpcodes.GetFreeAsComponentHead, number];

export type GetContextualFree =
| GetFreeAsFallback
| GetFreeAsComponentOrHelperHeadOrThisFallback
| GetFreeAsComponentOrHelperHead
| GetFreeAsHelperHeadOrThisFallback
Expand All @@ -200,7 +193,6 @@ export namespace Expressions {
export type GetPathSymbol = [SexpOpcodes.GetSymbol, number, Path];
export type GetPathTemplateSymbol = [SexpOpcodes.GetTemplateSymbol, number, Path];
export type GetPathStrictFree = [SexpOpcodes.GetStrictFree, number, Path];
export type GetPathFreeAsFallback = [SexpOpcodes.GetFreeAsFallback, number, Path];
export type GetPathFreeAsComponentOrHelperHeadOrThisFallback = [
SexpOpcodes.GetFreeAsComponentOrHelperHeadOrThisFallback,
number,
Expand All @@ -226,7 +218,6 @@ export namespace Expressions {
export type GetPathFreeAsComponentHead = [SexpOpcodes.GetFreeAsComponentHead, number, Path];

export type GetPathContextualFree =
| GetPathFreeAsFallback
| GetPathFreeAsComponentOrHelperHeadOrThisFallback
| GetPathFreeAsComponentOrHelperHead
| GetPathFreeAsHelperHeadOrThisFallback
Expand Down
4 changes: 0 additions & 4 deletions packages/@glimmer/opcode-compiler/lib/syntax/expressions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,6 @@ EXPRESSIONS.add(SexpOpcodes.GetStrictFree, (op, [, sym, _path]) => {
});
});

EXPRESSIONS.add(SexpOpcodes.GetFreeAsFallback, (op, [, , path]) => {
withPath(op, path);
});

EXPRESSIONS.add(SexpOpcodes.GetFreeAsComponentOrHelperHeadOrThisFallback, () => {
// TODO: The logic for this opcode currently exists in STATEMENTS.Append, since
// we want different wrapping logic depending on if we are invoking a component,
Expand Down
Loading