[valueblocks] Join blocks instead of replacing operands (fix edge case)
There's a bug in the HIR->ReactiveFunction conversion for certain categories of compound value blocks where we replace operands (which must be a Place) with a ReactiveValue. This approach worked in practice for lots of cases so I thought a type coercion was safe, but then I found a case where this assumption breaks (see new test). The updated logic fixes the bug and is simpler. When a value block gets split up (because there was a nested value block), instead of replacing the earlier value in the later instructions, we append the instructions together. This can result in some extra nesting (which if we wanted we could flatten away) but ensures that we maintain type-safety.
Joe Savona committed
Feb 3, 2023 at 14:16 UTC
812e0ce7010ca9475a10981609467a9f5b6b5eac
4 files changed
+128
-96
compiler/forget/src/ReactiveScopes/BuildReactiveFunction.ts
+83
-94
@@ -18,12 +18,10 @@ import {
18
} from "../HIR";
19
import {
20
HIRFunction,
21
- Instruction,
21
InstructionKind,
22
ReactiveBreakTerminal,
23
ReactiveContinueTerminal,
24
ReactiveFunction,
26
- ReactiveInstruction,
25
ReactiveLogicalValue,
26
ReactiveSequenceValue,
27
ReactiveTerminalStatement,
@@ -31,7 +29,6 @@ import {
29
ReactiveValue,
30
Terminal,
31
} from "../HIR/HIR";
34
-import { mapInstructionOperands } from "../HIR/visitors";
32
import { assertExhaustive } from "../Utils/utils";
33
34
/**
@@ -471,62 +468,74 @@ class Driver {
468
visitValueBlock(
469
id: BlockId,
470
loc: SourceLocation
474
- ): { block: BlockId; value: ReactiveValue; place: Place } {
475
- let block: BasicBlock;
476
- let value: ReactiveValue | null = null;
477
- let place: Place | null = null;
471
+ ): { block: BlockId; value: ReactiveValue; place: Place; id: InstructionId } {
472
const defaultBlock = this.cx.ir.blocks.get(id)!;
473
if (
474
defaultBlock.terminal.kind === "goto" ||
475
defaultBlock.terminal.kind === "branch"
476
) {
483
- block = defaultBlock;
484
- } else {
485
- const result = this.visitValueBlockTerminal(defaultBlock.terminal);
486
- block = this.cx.ir.blocks.get(result.fallthrough)!;
487
- place = result.place;
488
- value = result.value;
489
- }
490
- const instructions: Array<ReactiveInstruction> = block.instructions;
491
- if (place !== null && value !== null) {
492
- instructions.forEach((instr) =>
493
- mapInstructionOperands(instr as Instruction, (place) => {
494
- return place.identifier === place.identifier
495
- ? (value as Place)
496
- : place;
497
- })
498
- );
499
- }
500
- if (instructions.length === 0) {
501
- invariant(
502
- block.terminal.kind === "branch",
503
- "Expected instructions for non-branch terminal"
504
- );
505
- return {
506
- block: block.id,
507
- place: block.terminal.test,
508
- value: value ?? block.terminal.test,
509
- };
510
- } else if (instructions.length === 1) {
511
- const instr = instructions[0]!;
512
- return {
513
- block: block.id,
514
- place: instr.lvalue!.place,
515
- value: instr.value,
516
- };
477
+ const instructions = defaultBlock.instructions;
478
+ if (instructions.length === 0) {
479
+ invariant(
480
+ defaultBlock.terminal.kind === "branch",
481
+ "Expected instructions for non-branch terminal"
482
+ );
483
+ return {
484
+ block: defaultBlock.id,
485
+ place: defaultBlock.terminal.test,
486
+ value: defaultBlock.terminal.test,
487
+ id: defaultBlock.terminal.id,
488
+ };
489
+ } else if (defaultBlock.instructions.length === 1) {
490
+ const instr = defaultBlock.instructions[0]!;
491
+ return {
492
+ block: defaultBlock.id,
493
+ place: instr.lvalue!.place,
494
+ value: instr.value,
495
+ id: instr.id,
496
+ };
497
+ } else {
498
+ const instr = defaultBlock.instructions.at(-1)!;
499
+ const sequence: ReactiveSequenceValue = {
500
+ kind: "SequenceExpression",
501
+ instructions: defaultBlock.instructions.slice(0, -1),
502
+ id: instr.id,
503
+ value: instr.value,
504
+ loc: loc,
505
+ };
506
+ return {
507
+ block: defaultBlock.id,
508
+ place: instr.lvalue!.place,
509
+ value: sequence,
510
+ id: instr.id,
511
+ };
512
+ }
513
} else {
518
- const instr = instructions.at(-1)!;
514
+ // The value block ended in a value terminal, recurse to get the value
515
+ // of that terminal
516
+ const init = this.visitValueBlockTerminal(defaultBlock.terminal);
517
+ // Code following the logical terminal
518
+ const final = this.visitValueBlock(init.fallthrough, loc);
519
+ // Stitch the two together...
520
const sequence: ReactiveSequenceValue = {
521
kind: "SequenceExpression",
521
- instructions: instructions.slice(0, -1),
522
- id: instr.id,
523
- value: instr.value,
524
- loc: loc,
522
+ instructions: [
523
+ {
524
+ id: init.id,
525
+ loc,
526
+ lvalue: { kind: InstructionKind.Const, place: init.place },
527
+ value: init.value,
528
+ },
529
+ ],
530
+ id: final.id,
531
+ value: final.value,
532
+ loc,
533
};
534
return {
527
- block: block.id,
528
- place: instr.lvalue!.place,
535
+ block: init.fallthrough,
536
value: sequence,
537
+ place: final.place,
538
+ id: final.id,
539
};
540
}
541
}
@@ -535,77 +544,56 @@ class Driver {
544
value: ReactiveValue;
545
place: Place;
546
fallthrough: BlockId;
547
+ id: InstructionId;
548
} {
549
switch (terminal.kind) {
550
case "logical": {
541
- let testBlock: BasicBlock;
542
- let leftPlace: Place | null = null;
543
- let leftValue: ReactiveValue | null = null;
544
- const defaultTestBlock = this.cx.ir.blocks.get(terminal.test)!;
545
- if (defaultTestBlock.terminal.kind === "branch") {
546
- testBlock = defaultTestBlock;
547
- } else {
548
- const leftResult = this.visitValueBlockTerminal(
549
- defaultTestBlock.terminal
550
- );
551
- testBlock = this.cx.ir.blocks.get(leftResult.fallthrough)!;
552
- leftPlace = leftResult.place;
553
- leftValue = leftResult.value;
554
- }
555
-
551
+ const test = this.visitValueBlock(terminal.test, terminal.loc);
552
+ const testBlock = this.cx.ir.blocks.get(test.block)!;
553
invariant(
554
testBlock.terminal.kind === "branch",
555
"Unexpected terminal kind '%s' for logical test block",
556
testBlock.terminal.kind
557
);
561
- const leftInstructions: Array<ReactiveInstruction> =
562
- testBlock.instructions;
563
- const leftBlock = this.cx.ir.blocks.get(testBlock.terminal.consequent)!;
564
- leftInstructions.push(...leftBlock.instructions);
565
- if (leftPlace !== null && leftValue !== null) {
566
- leftInstructions.forEach((instr) =>
567
- mapInstructionOperands(instr as Instruction, (place) => {
568
- return place.identifier === leftPlace!.identifier
569
- ? (leftValue as Place)
570
- : place;
571
- })
572
- );
573
- }
574
- const lastInstruction = leftInstructions.at(-1)!;
575
- const place = lastInstruction.lvalue!.place;
558
577
- let left: ReactiveValue;
578
- if (leftInstructions.length === 1) {
579
- left = leftInstructions[0]!.value;
580
- } else {
581
- const sequence: ReactiveSequenceValue = {
582
- kind: "SequenceExpression",
583
- instructions: leftInstructions.slice(0, -1),
584
- id: lastInstruction.id,
585
- value: lastInstruction.value,
586
- loc: terminal.loc,
587
- };
588
- left = sequence;
589
- }
559
+ const leftFinal = this.visitValueBlock(
560
+ testBlock.terminal.consequent,
561
+ terminal.loc
562
+ );
563
+ const left: ReactiveSequenceValue = {
564
+ kind: "SequenceExpression",
565
+ instructions: [
566
+ {
567
+ id: test.id,
568
+ loc: terminal.loc,
569
+ lvalue: { kind: InstructionKind.Const, place: test.place },
570
+ value: test.value,
571
+ },
572
+ ],
573
+ id: leftFinal.id,
574
+ value: leftFinal.value,
575
+ loc: terminal.loc,
576
+ };
577
const right = this.visitValueBlock(
578
testBlock.terminal.alternate,
579
terminal.loc
580
);
581
invariant(
595
- place.identifier === right.place.identifier,
582
+ leftFinal.place.identifier === right.place.identifier,
583
"Expected the left and right side of a logical expression to store a value to the same place"
584
);
585
const value: ReactiveLogicalValue = {
586
kind: "LogicalExpression",
587
operator: terminal.operator,
601
- left,
588
+ left: left,
589
right: right.value,
590
loc: terminal.loc,
591
};
592
return {
606
- place: { ...place },
593
+ place: { ...leftFinal.place },
594
value,
595
fallthrough: terminal.fallthrough,
596
+ id: terminal.id,
597
};
598
}
599
case "ternary": {
@@ -639,6 +627,7 @@ class Driver {
627
place: { ...consequent.place },
628
value,
629
fallthrough: terminal.fallthrough,
630
+ id: terminal.id,
631
};
632
}
633
default: {
compiler/forget/src/ReactiveScopes/PrintReactiveFunction.ts
+1
-1
@@ -218,7 +218,7 @@ function printTerminal(writer: Writer, terminal: ReactiveTerminal): void {
218
break;
219
}
220
case "for": {
221
- writer.writeLine("[${terminal.id}] for (");
221
+ writer.writeLine(`[${terminal.id}] for (`);
222
printReactiveValue(writer, terminal.init);
223
writer.writeLine(";");
224
printReactiveValue(writer, terminal.test);
compiler/forget/src/__tests__/fixtures/hir/for-logical.expect.md
new
+44
@@ -0,0 +1,44 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function foo(props) {
6
+ let y = 0;
7
+ for (
8
+ let x = 0;
9
+ x > props.min && x < props.max;
10
+ x += props.cond ? props.increment : 2
11
+ ) {
12
+ x *= 2;
13
+ y += x;
14
+ }
15
+ return y;
16
+}
17
+
18
+```
19
+
20
+## Code
21
+
22
+```javascript
23
+function foo(props) {
24
+ const $ = React.useMemoCache();
25
+ let y;
26
+ if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
27
+ y = 0;
28
+ $[0] = y;
29
+ } else {
30
+ y = $[0];
31
+ }
32
+ for (
33
+ let x = 0;
34
+ x > props.min && x < props.max;
35
+ x = x$0 + (props.cond ? props.increment : (2, 2)), x
36
+ ) {
37
+ const x$0 = x * 2;
38
+ const y$1 = y + x$0;
39
+ }
40
+ return y;
41
+}
42
+
43
+```
44
+
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/hir/for-logical.js
-1
@@ -1,4 +1,3 @@
1
-// @skip
1
function foo(props) {
2
let y = 0;
3
for (