@samitouri / QOS-React / commits / 894cf6e37b

First-class representation of builtin jsx tags

We previously represented JsxExpressions using builtin tags - `<div>`, `<b>` etc - by lowering the tag name to a Primitive with the string name of the tag. However, by lowering into an independent value, it was possible that the lowered tag name could be grouped into a different memo slot, such that we ended up with output like: ```javascript let t0; if (c_1) { ... t0 = "div" ... } else { ... } return <t0>{children}</t0> ``` This is obviously wrong. It's also wrong to rename `t0` -> `T0`, because React treats that as a custom component, not a builtin. The right thing is to explicitly model builtin components, which this PR does by making `JsxExpression.tag` be a union of Place | BuiltinTag.

Joe Savona committed Apr 26, 2023 at 11:27 UTC 894cf6e37b7b34c95076328a10efbbc2117f89bc
13 files changed +186 -175
compiler/forget/src/HIR/BuildHIR.ts
+6 -13
@@ -17,6 +17,7 @@ import {
17 ArrayPattern,
18 BlockId,
19 BranchTerminal,
20 + BuiltinTag,
21 Case,
22 Effect,
23 GeneratedSource,
@@ -2035,7 +2036,7 @@ function lowerJsxElementName(
2036 exprPath: NodePath<
2037 t.JSXIdentifier | t.JSXMemberExpression | t.JSXNamespacedName
2038 >
2038 -): Place {
2039 +): Place | BuiltinTag {
2040 const exprNode = exprPath.node;
2041 const exprLoc = exprNode.loc ?? GeneratedSource;
2042 if (exprPath.isJSXIdentifier()) {
@@ -2047,19 +2048,11 @@ function lowerJsxElementName(
2048 loc: exprLoc,
2049 });
2050 } else {
2050 - if (tag.indexOf(":") !== -1) {
2051 - builder.errors.push({
2052 - reason: `(BuildHIR::lowerJsxElementName) JSXIdentifier to have no colons, got '${tag}'`,
2053 - severity: ErrorSeverity.InvalidInput,
2054 - nodePath: exprPath,
2055 - });
2056 - }
2057 - const place = lowerValueToTemporary(builder, {
2058 - kind: "Primitive",
2059 - value: tag,
2051 + return {
2052 + kind: "BuiltinTag",
2053 + name: tag,
2054 loc: exprLoc,
2061 - });
2062 - return place;
2055 + };
2056 }
2057 } else if (exprPath.isJSXMemberExpression()) {
2058 return lowerJsxMemberExpression(builder, exprPath);
compiler/forget/src/HIR/HIR.ts
+7 -1
@@ -608,7 +608,7 @@ export type InstructionValue =
608 }
609 | {
610 kind: "JsxExpression";
611 - tag: Place;
611 + tag: Place | BuiltinTag;
612 props: Array<JsxAttribute>;
613 children: Array<Place> | null; // null === no children
614 loc: SourceLocation;
@@ -752,6 +752,12 @@ export type LoadGlobal = {
752 loc: SourceLocation;
753 };
754
755 +export type BuiltinTag = {
756 + kind: "BuiltinTag";
757 + name: string;
758 + loc: SourceLocation;
759 +};
760 +
761 /*
762 * Range in which an identifier is mutable. Start and End refer to Instruction.id.
763 *
compiler/forget/src/HIR/PrintHIR.ts
+7 -5
@@ -317,18 +317,20 @@ export function printInstructionValue(instrValue: ReactiveValue): string {
317 propItems.push(`...${printPlace(attribute.argument)}`);
318 }
319 }
320 + const tag =
321 + instrValue.tag.kind === "Identifier"
322 + ? printPlace(instrValue.tag)
323 + : instrValue.tag.name;
324 const props = propItems.length !== 0 ? " " + propItems.join(" ") : "";
325 if (instrValue.children !== null) {
326 const children = instrValue.children.map((child) => {
327 return `{${printPlace(child)}}`;
328 });
325 - value = `JSX <${printPlace(instrValue.tag)}${props}${
329 + value = `JSX <${tag}${props}${
330 props.length > 0 ? " " : ""
327 - }>${children.join("")}</${printPlace(instrValue.tag)}>`;
331 + }>${children.join("")}</${tag}>`;
332 } else {
329 - value = `JSX <${printPlace(instrValue.tag)}${props}${
330 - props.length > 0 ? " " : ""
331 - }/>`;
333 + value = `JSX <${tag}${props}${props.length > 0 ? " " : ""}/>`;
334 }
335 break;
336 }
compiler/forget/src/HIR/visitors.ts
+6 -2
@@ -110,7 +110,9 @@ export function* eachInstructionValueOperand(
110 break;
111 }
112 case "JsxExpression": {
113 - yield instrValue.tag;
113 + if (instrValue.tag.kind === "Identifier") {
114 + yield instrValue.tag;
115 + }
116 for (const attribute of instrValue.props) {
117 switch (attribute.kind) {
118 case "JsxAttribute": {
@@ -370,7 +372,9 @@ export function mapInstructionOperands(
372 break;
373 }
374 case "JsxExpression": {
373 - instrValue.tag = fn(instrValue.tag);
375 + if (instrValue.tag.kind === "Identifier") {
376 + instrValue.tag = fn(instrValue.tag);
377 + }
378 for (const attribute of instrValue.props) {
379 switch (attribute.kind) {
380 case "JsxAttribute": {
compiler/forget/src/ReactiveScopes/CodegenReactiveFunction.ts
+4 -1
@@ -752,7 +752,10 @@ function codegenInstructionValue(
752 for (const attribute of instrValue.props) {
753 attributes.push(codegenJsxAttribute(cx, attribute));
754 }
755 - let tagValue = codegenPlace(cx, instrValue.tag);
755 + let tagValue =
756 + instrValue.tag.kind === "Identifier"
757 + ? codegenPlace(cx, instrValue.tag)
758 + : t.stringLiteral(instrValue.tag.name);
759 let tag: t.JSXIdentifier | t.JSXNamespacedName | t.JSXMemberExpression;
760 if (tagValue.type === "Identifier") {
761 tag = createJsxIdentifier(instrValue.tag.loc, tagValue.name);
compiler/forget/src/ReactiveScopes/MemoizeFbtOperandsInSameScope.ts
+20 -5
@@ -10,9 +10,13 @@ import {
10 makeInstructionId,
11 ReactiveFunction,
12 ReactiveInstruction,
13 + ReactiveValue,
14 } from "../HIR";
14 -import { eachInstructionValueOperand } from "../HIR/visitors";
15 -import { ReactiveFunctionVisitor, visitReactiveFunction } from "./visitors";
15 +import {
16 + eachReactiveValueOperand,
17 + ReactiveFunctionVisitor,
18 + visitReactiveFunction,
19 +} from "./visitors";
20
21 /**
22 * This pass supports the `fbt` translation system (https://facebook.github.io/fbt/).
@@ -57,15 +61,14 @@ class Transform extends ReactiveFunctionVisitor<void> {
61 // Record references to `fbt` as a global
62 this.fbtValues.add(lvalue.identifier.id);
63 } else if (
60 - (value.kind === "JsxExpression" &&
61 - this.fbtValues.has(value.tag.identifier.id)) ||
64 + isFbtJsxExpression(this.fbtValues, value) ||
65 (value.kind === "CallExpression" &&
66 this.fbtValues.has(value.callee.identifier.id))
67 ) {
68 // if the JSX element's tag was `fbt`, mark all its operands
69 // to ensure that they end up in the same scope as the jsx element
70 // itself.
68 - for (const operand of eachInstructionValueOperand(value)) {
71 + for (const operand of eachReactiveValueOperand(value)) {
72 operand.identifier.scope = lvalue.identifier.scope;
73 operand.identifier.mutableRange.end =
74 lvalue.identifier.mutableRange.end;
@@ -81,3 +84,15 @@ class Transform extends ReactiveFunctionVisitor<void> {
84 }
85 }
86 }
87 +
88 +function isFbtJsxExpression(
89 + fbtValues: Set<IdentifierId>,
90 + value: ReactiveValue
91 +): boolean {
92 + return (
93 + value.kind === "JsxExpression" &&
94 + ((value.tag.kind === "Identifier" &&
95 + fbtValues.has(value.tag.identifier.id)) ||
96 + (value.tag.kind === "BuiltinTag" && value.tag.name === "fbt"))
97 + );
98 +}
compiler/forget/src/ReactiveScopes/PromoteUsedTemporaries.ts
+1 -1
@@ -56,7 +56,7 @@ class CollectJsxTagsVisitor extends ReactiveFunctionVisitor<JsxExpressionTags> {
56 value: ReactiveValue,
57 state: JsxExpressionTags
58 ): void {
59 - if (value.kind === "JsxExpression") {
59 + if (value.kind === "JsxExpression" && value.tag.kind === "Identifier") {
60 state.add(value.tag.identifier.id);
61 }
62 }
compiler/forget/src/ReactiveScopes/PruneNonEscapingScopes.ts
+3 -1
@@ -381,7 +381,9 @@ function computeMemoizationInputs(
381 }
382 case "JsxExpression": {
383 const operands: Array<Place> = [];
384 - operands.push(value.tag);
384 + if (value.tag.kind === "Identifier") {
385 + operands.push(value.tag);
386 + }
387 for (const prop of value.props) {
388 if (prop.kind === "JsxAttribute") {
389 operands.push(prop.place);
compiler/forget/src/__tests__/fixtures/compiler/builtin-jsx-tag-lowered-between-mutations.expect.md renamed
+11 -15
@@ -14,27 +14,23 @@ function Component(props) {
14 ```javascript
15 import { unstable_useMemoCache as useMemoCache } from "react";
16 function Component(props) {
17 - const $ = useMemoCache(3);
18 - let T0;
19 - let t1;
17 + const $ = useMemoCache(2);
18 + let t0;
19 if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
20 const maybeMutable = new MaybeMutable();
22 - T0 = "div";
23 - t1 = maybeMutate(maybeMutable);
24 - $[0] = T0;
25 - $[1] = t1;
21 + t0 = maybeMutate(maybeMutable);
22 + $[0] = t0;
23 } else {
27 - T0 = $[0];
28 - t1 = $[1];
24 + t0 = $[0];
25 }
30 - let t2;
31 - if ($[2] === Symbol.for("react.memo_cache_sentinel")) {
32 - t2 = <T0>{t1}</T0>;
33 - $[2] = t2;
26 + let t1;
27 + if ($[1] === Symbol.for("react.memo_cache_sentinel")) {
28 + t1 = <div>{t0}</div>;
29 + $[1] = t1;
30 } else {
35 - t2 = $[2];
31 + t1 = $[1];
32 }
37 - return t2;
33 + return t1;
34 }
35
36 ```
compiler/forget/src/__tests__/fixtures/compiler/builtin-jsx-tag-lowered-between-mutations.js renamed
compiler/forget/src/__tests__/fixtures/compiler/error.mutate-after-freeze.expect.md
+1 -1
@@ -19,7 +19,7 @@ function Component(props) {
19 ## Error
20
21 ```
22 -[ReactForget] InvalidInput: InferReferenceEffects: inferred mutation of known immutable value. Found mutation of $28:TObject<BuiltInArray> (frozen) (7:7)
22 +[ReactForget] InvalidInput: InferReferenceEffects: inferred mutation of known immutable value. Found mutation of $27:TObject<BuiltInArray> (frozen) (7:7)
23 ```
24
25
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/inner-memo-value-not-promoted-to-outer-scope-dynamic.expect.md
+71 -77
@@ -24,107 +24,101 @@ function Component(props) {
24 ```javascript
25 import { unstable_useMemoCache as useMemoCache } from "react";
26 function Component(props) {
27 - const $ = useMemoCache(23);
27 + const $ = useMemoCache(21);
28 const item = useFragment(FRAGMENT, props.item);
29 useFreeze(item);
30 const c_0 = $[0] !== item;
31 - let T1;
32 - let t2;
33 - let T3;
34 - let t4;
31 + let t1;
32 + let T2;
33 + let t3;
34 let t0;
36 - let t5;
37 - let T6;
38 - let t7;
35 + let t4;
36 + let T5;
37 + let t6;
38 if (c_0) {
39 const count = new MaybeMutable(item);
40
42 - T6 = View;
43 - t7 = "\n ";
44 - T3 = View;
45 - t4 = "\n ";
46 - if ($[9] === Symbol.for("react.memo_cache_sentinel")) {
41 + T5 = View;
42 + t6 = "\n ";
43 + T2 = View;
44 + t3 = "\n ";
45 + if ($[8] === Symbol.for("react.memo_cache_sentinel")) {
46 t0 = <span>Text</span>;
48 - $[9] = t0;
47 + $[8] = t0;
48 } else {
50 - t0 = $[9];
49 + t0 = $[8];
50 }
52 - t5 = "\n ";
53 - T1 = "span";
54 - t2 = maybeMutate(count);
51 + t4 = "\n ";
52 + t1 = maybeMutate(count);
53 $[0] = item;
56 - $[1] = T1;
57 - $[2] = t2;
58 - $[3] = T3;
59 - $[4] = t4;
60 - $[5] = t0;
61 - $[6] = t5;
62 - $[7] = T6;
63 - $[8] = t7;
54 + $[1] = t1;
55 + $[2] = T2;
56 + $[3] = t3;
57 + $[4] = t0;
58 + $[5] = t4;
59 + $[6] = T5;
60 + $[7] = t6;
61 } else {
65 - T1 = $[1];
66 - t2 = $[2];
67 - T3 = $[3];
68 - t4 = $[4];
69 - t0 = $[5];
70 - t5 = $[6];
71 - T6 = $[7];
72 - t7 = $[8];
62 + t1 = $[1];
63 + T2 = $[2];
64 + t3 = $[3];
65 + t0 = $[4];
66 + t4 = $[5];
67 + T5 = $[6];
68 + t6 = $[7];
69 }
74 - const c_10 = $[10] !== T1;
75 - const c_11 = $[11] !== t2;
76 - let t8;
77 - if (c_10 || c_11) {
78 - t8 = <T1>{t2}</T1>;
79 - $[10] = T1;
80 - $[11] = t2;
81 - $[12] = t8;
70 + const c_9 = $[9] !== t1;
71 + let t7;
72 + if (c_9) {
73 + t7 = <span>{t1}</span>;
74 + $[9] = t1;
75 + $[10] = t7;
76 } else {
83 - t8 = $[12];
77 + t7 = $[10];
78 }
85 - const c_13 = $[13] !== T3;
79 + const c_11 = $[11] !== T2;
80 + const c_12 = $[12] !== t3;
81 + const c_13 = $[13] !== t0;
82 const c_14 = $[14] !== t4;
87 - const c_15 = $[15] !== t0;
88 - const c_16 = $[16] !== t5;
89 - const c_17 = $[17] !== t8;
90 - let t9;
91 - if (c_13 || c_14 || c_15 || c_16 || c_17) {
92 - t9 = (
93 - <T3>
94 - {t4}
83 + const c_15 = $[15] !== t7;
84 + let t8;
85 + if (c_11 || c_12 || c_13 || c_14 || c_15) {
86 + t8 = (
87 + <T2>
88 + {t3}
89 {t0}
96 - {t5}
97 - {t8}
98 - </T3>
90 + {t4}
91 + {t7}
92 + </T2>
93 );
100 - $[13] = T3;
94 + $[11] = T2;
95 + $[12] = t3;
96 + $[13] = t0;
97 $[14] = t4;
102 - $[15] = t0;
103 - $[16] = t5;
104 - $[17] = t8;
105 - $[18] = t9;
98 + $[15] = t7;
99 + $[16] = t8;
100 } else {
107 - t9 = $[18];
101 + t8 = $[16];
102 }
109 - const c_19 = $[19] !== T6;
110 - const c_20 = $[20] !== t7;
111 - const c_21 = $[21] !== t9;
112 - let t10;
113 - if (c_19 || c_20 || c_21) {
114 - t10 = (
115 - <T6>
116 - {t7}
117 - {t9}
118 - </T6>
103 + const c_17 = $[17] !== T5;
104 + const c_18 = $[18] !== t6;
105 + const c_19 = $[19] !== t8;
106 + let t9;
107 + if (c_17 || c_18 || c_19) {
108 + t9 = (
109 + <T5>
110 + {t6}
111 + {t8}
112 + </T5>
113 );
120 - $[19] = T6;
121 - $[20] = t7;
122 - $[21] = t9;
123 - $[22] = t10;
114 + $[17] = T5;
115 + $[18] = t6;
116 + $[19] = t8;
117 + $[20] = t9;
118 } else {
125 - t10 = $[22];
119 + t9 = $[20];
120 }
127 - return t10;
121 + return t9;
122 }
123
124 ```
compiler/forget/src/__tests__/fixtures/compiler/inner-memo-value-not-promoted-to-outer-scope-static.expect.md
+49 -53
@@ -21,52 +21,62 @@ function Component(props) {
21 ```javascript
22 import { unstable_useMemoCache as useMemoCache } from "react";
23 function Component(props) {
24 - const $ = useMemoCache(12);
25 - let T1;
26 - let t2;
27 - let T3;
28 - let t4;
24 + const $ = useMemoCache(11);
25 + let t1;
26 + let T2;
27 + let t3;
28 let t0;
30 - let t5;
31 - let T6;
32 - let t7;
29 + let t4;
30 + let T5;
31 + let t6;
32 if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
33 const count = new MaybeMutable();
34
36 - T6 = View;
37 - t7 = "\n ";
38 - T3 = View;
39 - t4 = "\n ";
40 - if ($[8] === Symbol.for("react.memo_cache_sentinel")) {
35 + T5 = View;
36 + t6 = "\n ";
37 + T2 = View;
38 + t3 = "\n ";
39 + if ($[7] === Symbol.for("react.memo_cache_sentinel")) {
40 t0 = <span>Text</span>;
42 - $[8] = t0;
41 + $[7] = t0;
42 } else {
44 - t0 = $[8];
43 + t0 = $[7];
44 }
46 - t5 = "\n ";
47 - T1 = "span";
48 - t2 = maybeMutate(count);
49 - $[0] = T1;
50 - $[1] = t2;
51 - $[2] = T3;
52 - $[3] = t4;
53 - $[4] = t0;
54 - $[5] = t5;
55 - $[6] = T6;
56 - $[7] = t7;
45 + t4 = "\n ";
46 + t1 = maybeMutate(count);
47 + $[0] = t1;
48 + $[1] = T2;
49 + $[2] = t3;
50 + $[3] = t0;
51 + $[4] = t4;
52 + $[5] = T5;
53 + $[6] = t6;
54 } else {
58 - T1 = $[0];
59 - t2 = $[1];
60 - T3 = $[2];
61 - t4 = $[3];
62 - t0 = $[4];
63 - t5 = $[5];
64 - T6 = $[6];
65 - t7 = $[7];
55 + t1 = $[0];
56 + T2 = $[1];
57 + t3 = $[2];
58 + t0 = $[3];
59 + t4 = $[4];
60 + T5 = $[5];
61 + t6 = $[6];
62 + }
63 + let t7;
64 + if ($[8] === Symbol.for("react.memo_cache_sentinel")) {
65 + t7 = <span>{t1}</span>;
66 + $[8] = t7;
67 + } else {
68 + t7 = $[8];
69 }
70 let t8;
71 if ($[9] === Symbol.for("react.memo_cache_sentinel")) {
69 - t8 = <T1>{t2}</T1>;
72 + t8 = (
73 + <T2>
74 + {t3}
75 + {t0}
76 + {t4}
77 + {t7}
78 + </T2>
79 + );
80 $[9] = t8;
81 } else {
82 t8 = $[9];
@@ -74,30 +84,16 @@ function Component(props) {
84 let t9;
85 if ($[10] === Symbol.for("react.memo_cache_sentinel")) {
86 t9 = (
77 - <T3>
78 - {t4}
79 - {t0}
80 - {t5}
87 + <T5>
88 + {t6}
89 {t8}
82 - </T3>
90 + </T5>
91 );
92 $[10] = t9;
93 } else {
94 t9 = $[10];
95 }
88 - let t10;
89 - if ($[11] === Symbol.for("react.memo_cache_sentinel")) {
90 - t10 = (
91 - <T6>
92 - {t7}
93 - {t9}
94 - </T6>
95 - );
96 - $[11] = t10;
97 - } else {
98 - t10 = $[11];
99 - }
100 - return t10;
96 + return t9;
97 }
98
99 ```