Elgato_dark/angular
1
0
Fork 0
mirror of https://github.com/angular/angular synced 2026-05-24 09:28:37 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

refactor(compiler): Fix a bug in which temporaries were being declared in the wrong places (#51950)

Browse source 
Previously, we always generated temporary variable declarations at the beginning of each view's update block. This is wrong, for two reasons:
1. Temporaries can be used in the create block
2. When listeners use temporaries, we should declare them inside the listener.

Now, we always place temporaries at the beginning of the enclosing OpList, and recursively try to generate them when we find a listener.

PR Close #51950
This commit is contained in:
Dylan Hunn 2023-09-28 12:44:25 -07:00 committed by Alex Rickabaugh
parent 706838950c
commit aa6bb8ee95
2 changed files with 73 additions and 48 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

15
packages/compiler-cli/test/compliance/test_cases/r3_view_compiler/nullish_coalescing/TEST_CASES.json
View file

@ -3,7 +3,9 @@
"cases": [
{
"description": "should handle nullish coalescing inside interpolations",
"inputFiles": ["nullish_coalescing_interpolation.ts"],
"inputFiles": [
"nullish_coalescing_interpolation.ts"
],
"expectations": [
{
"files": [
@ -18,7 +20,9 @@
},
{
"description": "should handle nullish coalescing inside property bindings",
"inputFiles": ["nullish_coalescing_property.ts"],
"inputFiles": [
"nullish_coalescing_property.ts"
],
"expectations": [
{
"files": [
@ -33,7 +37,9 @@
},
{
"description": "should handle nullish coalescing inside host bindings",
"inputFiles": ["nullish_coalescing_host.ts"],
"inputFiles": [
"nullish_coalescing_host.ts"
],
"expectations": [
{
"files": [
@ -44,8 +50,7 @@
],
"failureMessage": "Incorrect host bindings"
}
],
"skipForTemplatePipeline": true
]
}
]
}

106
packages/compiler/src/template/pipeline/src/phases/temporary_variables.ts
View file

@ -8,7 +8,7 @@
import * as o from '../../../../output/output_ast';
import * as ir from '../../ir';
import {CompilationJob, ComponentCompilationJob} from '../compilation';
import type {CompilationJob, CompilationUnit} from '../compilation';
/**
* Find all assignments and usages of temporary variables, which are linked to each other with cross
@ -21,51 +21,71 @@ import {CompilationJob, ComponentCompilationJob} from '../compilation';
*/
export function phaseTemporaryVariables(cpl: CompilationJob): void {
for (const unit of cpl.units) {
let opCount = 0;
let generatedStatements: Array<ir.StatementOp<ir.UpdateOp>> = [];
for (const op of unit.ops()) {
// Identify the final time each temp var is read.
const finalReads = new Map<ir.XrefId, ir.ReadTemporaryExpr>();
ir.visitExpressionsInOp(op, expr => {
if (expr instanceof ir.ReadTemporaryExpr) {
finalReads.set(expr.xref, expr);
}
});
// Name the temp vars, accounting for the fact that a name can be reused after it has been
// read for the final time.
let count = 0;
const assigned = new Set<ir.XrefId>();
const released = new Set<ir.XrefId>();
const defs = new Map<ir.XrefId, string>();
ir.visitExpressionsInOp(op, expr => {
if (expr instanceof ir.AssignTemporaryExpr) {
if (!assigned.has(expr.xref)) {
assigned.add(expr.xref);
// TODO: Exactly replicate the naming scheme used by `TemplateDefinitionBuilder`.
// It seems to rely on an expression index instead of an op index.
defs.set(expr.xref, `tmp_${opCount}_${count++}`);
}
assignName(defs, expr);
} else if (expr instanceof ir.ReadTemporaryExpr) {
if (finalReads.get(expr.xref) === expr) {
released.add(expr.xref);
count--;
}
assignName(defs, expr);
}
});
// Add declarations for the temp vars.
generatedStatements.push(
...Array.from(new Set(defs.values()))
.map(name => ir.createStatementOp<ir.UpdateOp>(new o.DeclareVarStmt(name))));
opCount++;
}
unit.update.prepend(generatedStatements);
unit.create.prepend(generateTemporaries(unit.create) as Array<ir.StatementOp<ir.CreateOp>>);
unit.update.prepend(generateTemporaries(unit.update) as Array<ir.StatementOp<ir.UpdateOp>>);
}
}
function generateTemporaries(ops: ir.OpList<ir.CreateOp|ir.UpdateOp>):
Array<ir.StatementOp<ir.CreateOp|ir.UpdateOp>> {
let opCount = 0;
let generatedStatements: Array<ir.StatementOp<ir.UpdateOp>> = [];
// For each op, search for any variables that are assigned or read. For each variable, generate a
// name and produce a `DeclareVarStmt` to the beginning of the block.
for (const op of ops) {
// Identify the final time each temp var is read.
const finalReads = new Map<ir.XrefId, ir.ReadTemporaryExpr>();
ir.visitExpressionsInOp(op, (expr, flag) => {
if (flag & ir.VisitorContextFlag.InChildOperation) {
return;
}
if (expr instanceof ir.ReadTemporaryExpr) {
finalReads.set(expr.xref, expr);
}
});
// Name the temp vars, accounting for the fact that a name can be reused after it has been
// read for the final time.
let count = 0;
const assigned = new Set<ir.XrefId>();
const released = new Set<ir.XrefId>();
const defs = new Map<ir.XrefId, string>();
ir.visitExpressionsInOp(op, (expr, flag) => {
if (flag & ir.VisitorContextFlag.InChildOperation) {
return;
}
if (expr instanceof ir.AssignTemporaryExpr) {
if (!assigned.has(expr.xref)) {
assigned.add(expr.xref);
// TODO: Exactly replicate the naming scheme used by `TemplateDefinitionBuilder`.
// It seems to rely on an expression index instead of an op index.
defs.set(expr.xref, `tmp_${opCount}_${count++}`);
}
assignName(defs, expr);
} else if (expr instanceof ir.ReadTemporaryExpr) {
if (finalReads.get(expr.xref) === expr) {
released.add(expr.xref);
count--;
}
assignName(defs, expr);
}
});
// Add declarations for the temp vars.
generatedStatements.push(
...Array.from(new Set(defs.values()))
.map(name => ir.createStatementOp<ir.UpdateOp>(new o.DeclareVarStmt(name))));
opCount++;
if (op.kind === ir.OpKind.Listener) {
op.handlerOps.prepend(generateTemporaries(op.handlerOps) as ir.UpdateOp[]);
}
}
return generatedStatements;
}
/**
* Assigns a name to the temporary variable in the given temporary variable expression.
*/