Skip to content

Commit

Permalink
refactor(compiler): Fix i18nExp moving phase in Template Pipeline
Browse files Browse the repository at this point in the history 
Previously, our i18n slot moving process was buggy. Specifically, it was not resilient to cases in which a create op consumed a slot, but no update ops depended on that slot.

The new algorithm fixes this issue, and is also easier to understand.
  • Loading branch information
@dylhunn
dylhunn committed Dec 6, 2023
1 parent 423692a commit ca08fd2
Show file tree
Hide file tree
Showing 4 changed files with 102 additions and 62 deletions.
10 changes: 8 additions & 2 deletions ...ompiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/nested_nodes/TEST_CASES.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,13 +148,19 @@
],
"expectations": [
{
"files": [
{
"generated": "nested_elements_with_i18n_attributes.js",
"expected": "nested_elements_with_i18n_attributes_template.js",
"templatePipelineExpected": "nested_elements_with_i18n_attributes_template.pipeline.js"
}
],
"extraChecks": [
"verifyPlaceholdersIntegrity",
"verifyUniqueConsts"
]
}
],
"skipForTemplatePipeline": true
]
},
{
"description": "should handle i18n attributes in nested templates",
Expand Down
0 ...s/nested_elements_with_i18n_attributes.js → ...elements_with_i18n_attributes_template.js
View file
File renamed without changes.
48 changes: 48 additions & 0 deletions ...view_compiler_i18n/nested_nodes/nested_elements_with_i18n_attributes_template.pipeline.js
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
decls: 9,
vars: 7,
consts: () => {
__i18nMsg__('Span title {$interpolation} and {$interpolation_1}', [['interpolation', String.raw`\uFFFD0\uFFFD`], ['interpolation_1', String.raw`\uFFFD1\uFFFD`]], {original_code: {'interpolation': '{{ valueB }}', 'interpolation_1': '{{ valueC }}'}}, {})
__i18nMsg__('Span title {$interpolation}', [['interpolation', String.raw`\uFFFD0\uFFFD`]], {original_code: {'interpolation': '{{ valueE }}'}}, {})
__i18nMsg__(' My i18n block #1 with value: {$interpolation} {$startTagSpan} Plain text in nested element (block #1) {$closeTagSpan}',[['closeTagSpan', String.raw`\uFFFD/#2\uFFFD`], ['interpolation', String.raw`\uFFFD0\uFFFD`], ['startTagSpan', String.raw`\uFFFD#2\uFFFD`]], {original_code: {'closeTagSpan': '</span>', 'interpolation': '{{ valueA }}', 'startTagSpan': '<span i18n-title title="Span title {{ valueB }} and {{ valueC }}">'}}, {})
__i18nMsg__(' My i18n block #2 with value {$interpolation} {$startTagSpan} Plain text in nested element (block #2) {$closeTagSpan}',[['closeTagSpan', String.raw`\uFFFD/#7\uFFFD`], ['interpolation', String.raw`\uFFFD0\uFFFD`], ['startTagSpan', String.raw`\uFFFD#7\uFFFD`]], {original_code: {'closeTagSpan': '</span>', 'interpolation': '{{ valueD | uppercase }}', 'startTagSpan': '<span i18n-title title="Span title {{ valueE }}">'}}, {})
return [
$i18n_0$,
$i18n_2$,
["title", $i18n_1$],
["title", $i18n_3$],
[__AttributeMarker.I18n__, "title"]
];
},
template: function MyComponent_Template(rf, ctx) {
if (rf & 1) {
$r3$.ɵɵelementStart(0, "div");
$r3$.ɵɵi18nStart(1, 0);
$r3$.ɵɵelementStart(2, "span", 4);
$r3$.ɵɵi18nAttributes(3, 2);
$r3$.ɵɵelementEnd();
$r3$.ɵɵi18nEnd();
$r3$.ɵɵelementEnd();
$r3$.ɵɵelementStart(4, "div");
$r3$.ɵɵi18nStart(5, 1);
$r3$.ɵɵpipe(6, "uppercase");
$r3$.ɵɵelementStart(7, "span", 4);
$r3$.ɵɵi18nAttributes(8, 3);
$r3$.ɵɵelementEnd();
$r3$.ɵɵi18nEnd();
$r3$.ɵɵelementEnd();
}
if (rf & 2) {
$r3$.ɵɵadvance(2);
$r3$.ɵɵi18nExp(ctx.valueB)(ctx.valueC);
$r3$.ɵɵi18nApply(3);
$r3$.ɵɵadvance(1);
$r3$.ɵɵi18nExp(ctx.valueA);
$r3$.ɵɵi18nApply(1);
$r3$.ɵɵadvance(4);
$r3$.ɵɵi18nExp(ctx.valueE);
$r3$.ɵɵi18nApply(8);
$r3$.ɵɵadvance(1);
$r3$.ɵɵi18nExp($r3$.ɵɵpipeBind1(6, 5, ctx.valueD));
$r3$.ɵɵi18nApply(5);
}
}
106 changes: 46 additions & 60 deletions packages/compiler/src/template/pipeline/src/phases/assign_i18n_slot_dependencies.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,80 +9,66 @@
import * as ir from '../../ir';
import {CompilationJob} from '../compilation';

interface BlockState {
blockXref: ir.XrefId;
lastSlotConsumer: ir.XrefId;
}

/**
* Updates i18n expression ops to target the last slot in their owning i18n block, and moves them
* after the last update instruction that depends on that slot.
*/
export function assignI18nSlotDependencies(job: CompilationJob) {
const i18nLastSlotConsumers = new Map<ir.XrefId, ir.XrefId>();
let lastSlotConsumer: ir.XrefId|null = null;
let currentI18nOp: ir.I18nStartOp|null = null;

for (const unit of job.units) {
// Record the last consumed slot before each i18n end instruction.
for (const op of unit.create) {
if (ir.hasConsumesSlotTrait(op)) {
lastSlotConsumer = op.xref;
// The first update op.
let updateOp = unit.update.head;

// I18n expressions currently being moved during the iteration.
let i18nExpressionsInProgress: ir.I18nExpressionOp[] = [];

// Non-null while we are iterating through an i18nStart/i18nEnd pair
let state: BlockState|null = null;

for (const createOp of unit.create) {
if (createOp.kind === ir.OpKind.I18nStart) {
state = {
blockXref: createOp.xref,
lastSlotConsumer: createOp.xref,
};
} else if (createOp.kind === ir.OpKind.I18nEnd) {
for (const op of i18nExpressionsInProgress) {
op.target = state!.lastSlotConsumer;
ir.OpList.insertBefore(op as ir.UpdateOp, updateOp!);
}
i18nExpressionsInProgress.length = 0;
state = null;
}

switch (op.kind) {
case ir.OpKind.I18nStart:
currentI18nOp = op;
break;
case ir.OpKind.I18nEnd:
if (currentI18nOp === null) {
throw new Error(
'AssertionError: Expected an active I18n block while calculating last slot consumers');
if (ir.hasConsumesSlotTrait(createOp)) {
if (state !== null) {
state.lastSlotConsumer = createOp.xref;
}

while (true) {
if (updateOp.next === null) {
break;
}
if (lastSlotConsumer === null) {
throw new Error(
'AssertionError: Expected a last slot consumer while calculating last slot consumers');

if (ir.hasDependsOnSlotContextTrait(updateOp) && updateOp.target !== createOp.xref) {
break;
}
i18nLastSlotConsumers.set(currentI18nOp.xref, lastSlotConsumer);
currentI18nOp = null;
break;
}
}

// Expresions that are currently being moved.
let opsToMove: ir.I18nExpressionOp[] = [];
// Previously we found the last slot-consuming create mode op in the i18n block. That op will be
// the new target for any moved i18n expresion inside the i18n block, and that op's slot is
// stored here.
let moveAfterTarget: ir.XrefId|null = null;
// This is the last target in the create IR that we saw during iteration. Eventally, it will be
// equal to moveAfterTarget. But wait! We need to find the *last* such op whose target is equal
// to `moveAfterTarget`.
let previousTarget: ir.XrefId|null = null;
for (const op of unit.update) {
if (ir.hasDependsOnSlotContextTrait(op)) {
// We've found an op that depends on another slot other than the one that we want to move
// the expressions to, after previously having seen the one we want to move to.
if (moveAfterTarget !== null && previousTarget === moveAfterTarget &&
op.target !== previousTarget) {
ir.OpList.insertBefore<ir.UpdateOp>(opsToMove, op);
moveAfterTarget = null;
opsToMove = [];
}
previousTarget = op.target;
}
if (state !== null && updateOp.kind === ir.OpKind.I18nExpression &&
updateOp.usage === ir.I18nExpressionFor.I18nText) {
const opToRemove = updateOp;
updateOp = updateOp.prev!;
ir.OpList.remove<ir.UpdateOp>(opToRemove);
i18nExpressionsInProgress.push(opToRemove);
}

if (op.kind === ir.OpKind.I18nExpression && op.usage === ir.I18nExpressionFor.I18nText) {
// This is an I18nExpressionOps that is used for text (not attributes).
ir.OpList.remove<ir.UpdateOp>(op);
opsToMove.push(op);
const target = i18nLastSlotConsumers.get(op.i18nOwner);
if (target === undefined) {
throw new Error(
'AssertionError: Expected to find a last slot consumer for an I18nExpressionOp');
updateOp = updateOp.next!;
}
op.target = target;
moveAfterTarget = op.target;
}
}

if (moveAfterTarget !== null) {
unit.update.push(opsToMove);
}
}
}

0 comments on commit ca08fd2

Please sign in to comment.