[codegen] validate that temporary values are set
In codegen when we lower an operand we check to see if we have an already lowered value for it (stored in `cx.temp`). Currently we silently handle missing values by emitting a raw identifier, but this is really an error. This PR adds validation, which uncovered a few places where we legitimately won't have a value - things like function parameters that got swapped for temporaries bc of destructuring. We populate those as `null` values now, and fail if a temporary had a truly missing value.
Joe Savona committed
Apr 20, 2023 at 16:48 UTC
c72452895a43d3317590b03092ebf7c2fc9e3d34
5 files changed
+68
-14
compiler/forget/src/HIR/PrintHIR.ts
+5
-1
@@ -293,7 +293,11 @@ export function printInstructionValue(instrValue: ReactiveValue): string {
293
}
294
case "JSXText":
295
case "Primitive": {
296
- value = JSON.stringify(instrValue.value);
296
+ if (instrValue.value === undefined) {
297
+ value = "<undefined>";
298
+ } else {
299
+ value = JSON.stringify(instrValue.value);
300
+ }
301
break;
302
}
303
case "TypeCastExpression": {
compiler/forget/src/ReactiveScopes/CodegenReactiveFunction.ts
+22
-1
@@ -27,6 +27,7 @@ import {
27
SourceLocation,
28
SpreadPattern,
29
} from "../HIR/HIR";
30
+import { printPlace } from "../HIR/PrintHIR";
31
import { eachPatternOperand } from "../HIR/visitors";
32
import { Err, Ok, Result } from "../Utils/Result";
33
import { assertExhaustive } from "../Utils/utils";
@@ -35,6 +36,13 @@ export function codegenReactiveFunction(
36
fn: ReactiveFunction
37
): Result<t.FunctionDeclaration, CompilerError> {
38
const cx = new Context();
39
+ if (fn.id !== null) {
40
+ cx.temp.set(fn.id.id, null);
41
+ }
42
+ for (const param of fn.params) {
43
+ cx.temp.set(param.identifier.id, null);
44
+ }
45
+
46
const params = fn.params.map((param) => convertIdentifier(param.identifier));
47
const body = codegenBlock(cx, fn.body);
48
const statements = body.body;
@@ -457,6 +465,12 @@ function codegenInstructionNullable(
465
} else {
466
lvalue = instr.value.lvalue.pattern;
467
for (const place of eachPatternOperand(lvalue)) {
468
+ if (
469
+ kind !== InstructionKind.Reassign &&
470
+ place.identifier.name === null
471
+ ) {
472
+ cx.temp.set(place.identifier.id, null);
473
+ }
474
if (cx.hasDeclared(place.identifier)) {
475
kind = InstructionKind.Reassign;
476
break;
@@ -586,7 +600,7 @@ const createJsxText = withLoc(t.jsxText);
600
const createJsxClosingElement = withLoc(t.jsxClosingElement);
601
const createStringLiteral = withLoc(t.stringLiteral);
602
589
-type Temporaries = Map<IdentifierId, t.Expression>;
603
+type Temporaries = Map<IdentifierId, t.Expression | null>;
604
605
function codegenLabel(id: BlockId): string {
606
return `bb${id}`;
@@ -1163,6 +1177,13 @@ function codegenPlace(cx: Context, place: Place): t.Expression {
1177
if (tmp != null) {
1178
return tmp;
1179
}
1180
+ if (place.identifier.name === null && tmp === undefined) {
1181
+ CompilerError.invariant(
1182
+ `[Codegen] No value found for temporary`,
1183
+ place.loc,
1184
+ `Value for '${printPlace(place)}' was not set in the codegen context`
1185
+ );
1186
+ }
1187
const identifier = convertIdentifier(place.identifier);
1188
identifier.loc = place.loc as any;
1189
return identifier;
compiler/forget/src/ReactiveScopes/PrintReactiveFunction.ts
+4
-2
@@ -23,10 +23,12 @@ import { assertExhaustive } from "../Utils/utils";
23
24
export function printReactiveFunction(fn: ReactiveFunction): string {
25
const writer = new Writer();
26
- writer.writeLine(`function ${fn.id?.name ?? "<unknown>"}(`);
26
+ writer.writeLine(
27
+ `function ${fn.id !== null ? printIdentifier(fn.id) : "<unknown>"}(`
28
+ );
29
writer.indented(() => {
30
for (const param of fn.params) {
29
- writer.writeLine(`${param.identifier.name ?? "<param>"},`);
31
+ writer.writeLine(`${printPlace(param)},`);
32
}
33
});
34
writer.writeLine(") {");
compiler/forget/src/__tests__/fixtures/compiler/_bug.jsx-tag-evaluation-order.expect.md
+31
-9
@@ -8,7 +8,12 @@ function Component(props) {
8
// NOTE: the order of evaluation in the lowering is incorrect:
9
// the jsx element's tag observes `Tag` after reassignment, but should observe
10
// it before the reassignment.
11
- return <Tag>{((Tag = HScroll), maybeMutae(maybeMutable))}</Tag>;
11
+ return (
12
+ <Tag>
13
+ {((Tag = HScroll), maybeMutate(maybeMutable))}
14
+ <Tag />
15
+ </Tag>
16
+ );
17
}
18
19
```
@@ -18,28 +23,45 @@ function Component(props) {
23
```javascript
24
import * as React from "react";
25
function Component(props) {
21
- const $ = React.unstable_useMemoCache(3);
26
+ const $ = React.unstable_useMemoCache(5);
27
let Tag;
28
let t0;
29
+ let t1;
30
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
31
const maybeMutable = new MaybeMutable();
32
33
+ t0 = "\n ";
34
Tag = HScroll;
28
- t0 = maybeMutae(maybeMutable);
35
+ t1 = maybeMutate(maybeMutable);
36
$[0] = Tag;
37
$[1] = t0;
38
+ $[2] = t1;
39
} else {
40
Tag = $[0];
41
t0 = $[1];
42
+ t1 = $[2];
43
}
35
- let t1;
36
- if ($[2] === Symbol.for("react.memo_cache_sentinel")) {
37
- t1 = <Tag>{t0}</Tag>;
38
- $[2] = t1;
44
+ let t2;
45
+ if ($[3] === Symbol.for("react.memo_cache_sentinel")) {
46
+ t2 = <Tag />;
47
+ $[3] = t2;
48
} else {
40
- t1 = $[2];
49
+ t2 = $[3];
50
+ }
51
+ let t3;
52
+ if ($[4] === Symbol.for("react.memo_cache_sentinel")) {
53
+ t3 = (
54
+ <Tag>
55
+ {t0}
56
+ {t1}
57
+ {t2}
58
+ </Tag>
59
+ );
60
+ $[4] = t3;
61
+ } else {
62
+ t3 = $[4];
63
}
42
- return t1;
64
+ return t3;
65
}
66
67
```
compiler/forget/src/__tests__/fixtures/compiler/_bug.jsx-tag-evaluation-order.js
+6
-1
@@ -4,5 +4,10 @@ function Component(props) {
4
// NOTE: the order of evaluation in the lowering is incorrect:
5
// the jsx element's tag observes `Tag` after reassignment, but should observe
6
// it before the reassignment.
7
- return <Tag>{((Tag = HScroll), maybeMutae(maybeMutable))}</Tag>;
7
+ return (
8
+ <Tag>
9
+ {((Tag = HScroll), maybeMutate(maybeMutable))}
10
+ <Tag />
11
+ </Tag>
12
+ );
13
}