From 894cf6e37b7b34c95076328a10efbbc2117f89bc Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 26 Apr 2023 11:27:35 -0700 Subject: [PATCH] First-class representation of builtin jsx tags We previously represented JsxExpressions using builtin tags - `
`, `` 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 {children} ``` 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. --- compiler/forget/src/HIR/BuildHIR.ts | 19 +-- compiler/forget/src/HIR/HIR.ts | 8 +- compiler/forget/src/HIR/PrintHIR.ts | 12 +- compiler/forget/src/HIR/visitors.ts | 8 +- .../ReactiveScopes/CodegenReactiveFunction.ts | 5 +- .../MemoizeFbtOperandsInSameScope.ts | 25 ++- .../ReactiveScopes/PromoteUsedTemporaries.ts | 2 +- .../ReactiveScopes/PruneNonEscapingScopes.ts | 4 +- ...x-tag-lowered-between-mutations.expect.md} | 26 ++- ...ltin-jsx-tag-lowered-between-mutations.js} | 0 .../error.mutate-after-freeze.expect.md | 2 +- ...-promoted-to-outer-scope-dynamic.expect.md | 154 +++++++++--------- ...t-promoted-to-outer-scope-static.expect.md | 102 ++++++------ 13 files changed, 189 insertions(+), 178 deletions(-) rename compiler/forget/src/__tests__/fixtures/compiler/{_bug.builtin-jsx-tag-lowered-between-mutations.expect.md => builtin-jsx-tag-lowered-between-mutations.expect.md} (63%) rename compiler/forget/src/__tests__/fixtures/compiler/{_bug.builtin-jsx-tag-lowered-between-mutations.js => builtin-jsx-tag-lowered-between-mutations.js} (100%) diff --git a/compiler/forget/src/HIR/BuildHIR.ts b/compiler/forget/src/HIR/BuildHIR.ts index 37553ada13..292ea6ee1d 100644 --- a/compiler/forget/src/HIR/BuildHIR.ts +++ b/compiler/forget/src/HIR/BuildHIR.ts @@ -17,6 +17,7 @@ import { ArrayPattern, BlockId, BranchTerminal, + BuiltinTag, Case, Effect, GeneratedSource, @@ -2035,7 +2036,7 @@ function lowerJsxElementName( exprPath: NodePath< t.JSXIdentifier | t.JSXMemberExpression | t.JSXNamespacedName > -): Place { +): Place | BuiltinTag { const exprNode = exprPath.node; const exprLoc = exprNode.loc ?? GeneratedSource; if (exprPath.isJSXIdentifier()) { @@ -2047,19 +2048,11 @@ function lowerJsxElementName( loc: exprLoc, }); } else { - if (tag.indexOf(":") !== -1) { - builder.errors.push({ - reason: `(BuildHIR::lowerJsxElementName) JSXIdentifier to have no colons, got '${tag}'`, - severity: ErrorSeverity.InvalidInput, - nodePath: exprPath, - }); - } - const place = lowerValueToTemporary(builder, { - kind: "Primitive", - value: tag, + return { + kind: "BuiltinTag", + name: tag, loc: exprLoc, - }); - return place; + }; } } else if (exprPath.isJSXMemberExpression()) { return lowerJsxMemberExpression(builder, exprPath); diff --git a/compiler/forget/src/HIR/HIR.ts b/compiler/forget/src/HIR/HIR.ts index 5f5e5b9db4..4c5f024ca1 100644 --- a/compiler/forget/src/HIR/HIR.ts +++ b/compiler/forget/src/HIR/HIR.ts @@ -608,7 +608,7 @@ export type InstructionValue = } | { kind: "JsxExpression"; - tag: Place; + tag: Place | BuiltinTag; props: Array; children: Array | null; // null === no children loc: SourceLocation; @@ -752,6 +752,12 @@ export type LoadGlobal = { loc: SourceLocation; }; +export type BuiltinTag = { + kind: "BuiltinTag"; + name: string; + loc: SourceLocation; +}; + /* * Range in which an identifier is mutable. Start and End refer to Instruction.id. * diff --git a/compiler/forget/src/HIR/PrintHIR.ts b/compiler/forget/src/HIR/PrintHIR.ts index f98a2ca74a..d7d9856057 100644 --- a/compiler/forget/src/HIR/PrintHIR.ts +++ b/compiler/forget/src/HIR/PrintHIR.ts @@ -317,18 +317,20 @@ export function printInstructionValue(instrValue: ReactiveValue): string { propItems.push(`...${printPlace(attribute.argument)}`); } } + const tag = + instrValue.tag.kind === "Identifier" + ? printPlace(instrValue.tag) + : instrValue.tag.name; const props = propItems.length !== 0 ? " " + propItems.join(" ") : ""; if (instrValue.children !== null) { const children = instrValue.children.map((child) => { return `{${printPlace(child)}}`; }); - value = `JSX <${printPlace(instrValue.tag)}${props}${ + value = `JSX <${tag}${props}${ props.length > 0 ? " " : "" - }>${children.join("")}`; + }>${children.join("")}`; } else { - value = `JSX <${printPlace(instrValue.tag)}${props}${ - props.length > 0 ? " " : "" - }/>`; + value = `JSX <${tag}${props}${props.length > 0 ? " " : ""}/>`; } break; } diff --git a/compiler/forget/src/HIR/visitors.ts b/compiler/forget/src/HIR/visitors.ts index bbaa67d83d..8164be1342 100644 --- a/compiler/forget/src/HIR/visitors.ts +++ b/compiler/forget/src/HIR/visitors.ts @@ -110,7 +110,9 @@ export function* eachInstructionValueOperand( break; } case "JsxExpression": { - yield instrValue.tag; + if (instrValue.tag.kind === "Identifier") { + yield instrValue.tag; + } for (const attribute of instrValue.props) { switch (attribute.kind) { case "JsxAttribute": { @@ -370,7 +372,9 @@ export function mapInstructionOperands( break; } case "JsxExpression": { - instrValue.tag = fn(instrValue.tag); + if (instrValue.tag.kind === "Identifier") { + instrValue.tag = fn(instrValue.tag); + } for (const attribute of instrValue.props) { switch (attribute.kind) { case "JsxAttribute": { diff --git a/compiler/forget/src/ReactiveScopes/CodegenReactiveFunction.ts b/compiler/forget/src/ReactiveScopes/CodegenReactiveFunction.ts index 3d8b246c6d..e00340025e 100644 --- a/compiler/forget/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/forget/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -752,7 +752,10 @@ function codegenInstructionValue( for (const attribute of instrValue.props) { attributes.push(codegenJsxAttribute(cx, attribute)); } - let tagValue = codegenPlace(cx, instrValue.tag); + let tagValue = + instrValue.tag.kind === "Identifier" + ? codegenPlace(cx, instrValue.tag) + : t.stringLiteral(instrValue.tag.name); let tag: t.JSXIdentifier | t.JSXNamespacedName | t.JSXMemberExpression; if (tagValue.type === "Identifier") { tag = createJsxIdentifier(instrValue.tag.loc, tagValue.name); diff --git a/compiler/forget/src/ReactiveScopes/MemoizeFbtOperandsInSameScope.ts b/compiler/forget/src/ReactiveScopes/MemoizeFbtOperandsInSameScope.ts index 4c2e45c04e..f326fac6b7 100644 --- a/compiler/forget/src/ReactiveScopes/MemoizeFbtOperandsInSameScope.ts +++ b/compiler/forget/src/ReactiveScopes/MemoizeFbtOperandsInSameScope.ts @@ -10,9 +10,13 @@ import { makeInstructionId, ReactiveFunction, ReactiveInstruction, + ReactiveValue, } from "../HIR"; -import { eachInstructionValueOperand } from "../HIR/visitors"; -import { ReactiveFunctionVisitor, visitReactiveFunction } from "./visitors"; +import { + eachReactiveValueOperand, + ReactiveFunctionVisitor, + visitReactiveFunction, +} from "./visitors"; /** * This pass supports the `fbt` translation system (https://facebook.github.io/fbt/). @@ -57,15 +61,14 @@ class Transform extends ReactiveFunctionVisitor { // Record references to `fbt` as a global this.fbtValues.add(lvalue.identifier.id); } else if ( - (value.kind === "JsxExpression" && - this.fbtValues.has(value.tag.identifier.id)) || + isFbtJsxExpression(this.fbtValues, value) || (value.kind === "CallExpression" && this.fbtValues.has(value.callee.identifier.id)) ) { // if the JSX element's tag was `fbt`, mark all its operands // to ensure that they end up in the same scope as the jsx element // itself. - for (const operand of eachInstructionValueOperand(value)) { + for (const operand of eachReactiveValueOperand(value)) { operand.identifier.scope = lvalue.identifier.scope; operand.identifier.mutableRange.end = lvalue.identifier.mutableRange.end; @@ -81,3 +84,15 @@ class Transform extends ReactiveFunctionVisitor { } } } + +function isFbtJsxExpression( + fbtValues: Set, + value: ReactiveValue +): boolean { + return ( + value.kind === "JsxExpression" && + ((value.tag.kind === "Identifier" && + fbtValues.has(value.tag.identifier.id)) || + (value.tag.kind === "BuiltinTag" && value.tag.name === "fbt")) + ); +} diff --git a/compiler/forget/src/ReactiveScopes/PromoteUsedTemporaries.ts b/compiler/forget/src/ReactiveScopes/PromoteUsedTemporaries.ts index 21249d3b0a..07ffdfd56d 100644 --- a/compiler/forget/src/ReactiveScopes/PromoteUsedTemporaries.ts +++ b/compiler/forget/src/ReactiveScopes/PromoteUsedTemporaries.ts @@ -56,7 +56,7 @@ class CollectJsxTagsVisitor extends ReactiveFunctionVisitor { value: ReactiveValue, state: JsxExpressionTags ): void { - if (value.kind === "JsxExpression") { + if (value.kind === "JsxExpression" && value.tag.kind === "Identifier") { state.add(value.tag.identifier.id); } } diff --git a/compiler/forget/src/ReactiveScopes/PruneNonEscapingScopes.ts b/compiler/forget/src/ReactiveScopes/PruneNonEscapingScopes.ts index b430a32d0a..5bce281193 100644 --- a/compiler/forget/src/ReactiveScopes/PruneNonEscapingScopes.ts +++ b/compiler/forget/src/ReactiveScopes/PruneNonEscapingScopes.ts @@ -381,7 +381,9 @@ function computeMemoizationInputs( } case "JsxExpression": { const operands: Array = []; - operands.push(value.tag); + if (value.tag.kind === "Identifier") { + operands.push(value.tag); + } for (const prop of value.props) { if (prop.kind === "JsxAttribute") { operands.push(prop.place); diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.builtin-jsx-tag-lowered-between-mutations.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/builtin-jsx-tag-lowered-between-mutations.expect.md similarity index 63% rename from compiler/forget/src/__tests__/fixtures/compiler/_bug.builtin-jsx-tag-lowered-between-mutations.expect.md rename to compiler/forget/src/__tests__/fixtures/compiler/builtin-jsx-tag-lowered-between-mutations.expect.md index aecd9f6404..cb00fa2a35 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/_bug.builtin-jsx-tag-lowered-between-mutations.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/builtin-jsx-tag-lowered-between-mutations.expect.md @@ -14,27 +14,23 @@ function Component(props) { ```javascript import { unstable_useMemoCache as useMemoCache } from "react"; function Component(props) { - const $ = useMemoCache(3); - let T0; - let t1; + const $ = useMemoCache(2); + let t0; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { const maybeMutable = new MaybeMutable(); - T0 = "div"; - t1 = maybeMutate(maybeMutable); - $[0] = T0; + t0 = maybeMutate(maybeMutable); + $[0] = t0; + } else { + t0 = $[0]; + } + let t1; + if ($[1] === Symbol.for("react.memo_cache_sentinel")) { + t1 =
{t0}
; $[1] = t1; } else { - T0 = $[0]; t1 = $[1]; } - let t2; - if ($[2] === Symbol.for("react.memo_cache_sentinel")) { - t2 = {t1}; - $[2] = t2; - } else { - t2 = $[2]; - } - return t2; + return t1; } ``` diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.builtin-jsx-tag-lowered-between-mutations.js b/compiler/forget/src/__tests__/fixtures/compiler/builtin-jsx-tag-lowered-between-mutations.js similarity index 100% rename from compiler/forget/src/__tests__/fixtures/compiler/_bug.builtin-jsx-tag-lowered-between-mutations.js rename to compiler/forget/src/__tests__/fixtures/compiler/builtin-jsx-tag-lowered-between-mutations.js diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.mutate-after-freeze.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.mutate-after-freeze.expect.md index 92d2441e96..6fcb57cbde 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/error.mutate-after-freeze.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.mutate-after-freeze.expect.md @@ -19,7 +19,7 @@ function Component(props) { ## Error ``` -[ReactForget] InvalidInput: InferReferenceEffects: inferred mutation of known immutable value. Found mutation of $28:TObject (frozen) (7:7) +[ReactForget] InvalidInput: InferReferenceEffects: inferred mutation of known immutable value. Found mutation of $27:TObject (frozen) (7:7) ``` \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/inner-memo-value-not-promoted-to-outer-scope-dynamic.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/inner-memo-value-not-promoted-to-outer-scope-dynamic.expect.md index 1c3c2a0a8b..32ac199577 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/inner-memo-value-not-promoted-to-outer-scope-dynamic.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/inner-memo-value-not-promoted-to-outer-scope-dynamic.expect.md @@ -24,107 +24,101 @@ function Component(props) { ```javascript import { unstable_useMemoCache as useMemoCache } from "react"; function Component(props) { - const $ = useMemoCache(23); + const $ = useMemoCache(21); const item = useFragment(FRAGMENT, props.item); useFreeze(item); const c_0 = $[0] !== item; - let T1; - let t2; - let T3; - let t4; + let t1; + let T2; + let t3; let t0; - let t5; - let T6; - let t7; + let t4; + let T5; + let t6; if (c_0) { const count = new MaybeMutable(item); - T6 = View; - t7 = "\n "; - T3 = View; - t4 = "\n "; - if ($[9] === Symbol.for("react.memo_cache_sentinel")) { + T5 = View; + t6 = "\n "; + T2 = View; + t3 = "\n "; + if ($[8] === Symbol.for("react.memo_cache_sentinel")) { t0 = Text; - $[9] = t0; + $[8] = t0; } else { - t0 = $[9]; + t0 = $[8]; } - t5 = "\n "; - T1 = "span"; - t2 = maybeMutate(count); + t4 = "\n "; + t1 = maybeMutate(count); $[0] = item; - $[1] = T1; - $[2] = t2; - $[3] = T3; - $[4] = t4; - $[5] = t0; - $[6] = t5; - $[7] = T6; - $[8] = t7; + $[1] = t1; + $[2] = T2; + $[3] = t3; + $[4] = t0; + $[5] = t4; + $[6] = T5; + $[7] = t6; } else { - T1 = $[1]; - t2 = $[2]; - T3 = $[3]; - t4 = $[4]; - t0 = $[5]; - t5 = $[6]; - T6 = $[7]; - t7 = $[8]; + t1 = $[1]; + T2 = $[2]; + t3 = $[3]; + t0 = $[4]; + t4 = $[5]; + T5 = $[6]; + t6 = $[7]; } - const c_10 = $[10] !== T1; - const c_11 = $[11] !== t2; - let t8; - if (c_10 || c_11) { - t8 = {t2}; - $[10] = T1; - $[11] = t2; - $[12] = t8; + const c_9 = $[9] !== t1; + let t7; + if (c_9) { + t7 = {t1}; + $[9] = t1; + $[10] = t7; } else { - t8 = $[12]; + t7 = $[10]; } - const c_13 = $[13] !== T3; + const c_11 = $[11] !== T2; + const c_12 = $[12] !== t3; + const c_13 = $[13] !== t0; const c_14 = $[14] !== t4; - const c_15 = $[15] !== t0; - const c_16 = $[16] !== t5; - const c_17 = $[17] !== t8; - let t9; - if (c_13 || c_14 || c_15 || c_16 || c_17) { - t9 = ( - - {t4} + const c_15 = $[15] !== t7; + let t8; + if (c_11 || c_12 || c_13 || c_14 || c_15) { + t8 = ( + + {t3} {t0} - {t5} - {t8} - - ); - $[13] = T3; - $[14] = t4; - $[15] = t0; - $[16] = t5; - $[17] = t8; - $[18] = t9; - } else { - t9 = $[18]; - } - const c_19 = $[19] !== T6; - const c_20 = $[20] !== t7; - const c_21 = $[21] !== t9; - let t10; - if (c_19 || c_20 || c_21) { - t10 = ( - + {t4} {t7} - {t9} - + ); - $[19] = T6; - $[20] = t7; - $[21] = t9; - $[22] = t10; + $[11] = T2; + $[12] = t3; + $[13] = t0; + $[14] = t4; + $[15] = t7; + $[16] = t8; } else { - t10 = $[22]; + t8 = $[16]; } - return t10; + const c_17 = $[17] !== T5; + const c_18 = $[18] !== t6; + const c_19 = $[19] !== t8; + let t9; + if (c_17 || c_18 || c_19) { + t9 = ( + + {t6} + {t8} + + ); + $[17] = T5; + $[18] = t6; + $[19] = t8; + $[20] = t9; + } else { + t9 = $[20]; + } + return t9; } ``` diff --git a/compiler/forget/src/__tests__/fixtures/compiler/inner-memo-value-not-promoted-to-outer-scope-static.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/inner-memo-value-not-promoted-to-outer-scope-static.expect.md index cb89a15a15..76109bdee7 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/inner-memo-value-not-promoted-to-outer-scope-static.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/inner-memo-value-not-promoted-to-outer-scope-static.expect.md @@ -21,52 +21,62 @@ function Component(props) { ```javascript import { unstable_useMemoCache as useMemoCache } from "react"; function Component(props) { - const $ = useMemoCache(12); - let T1; - let t2; - let T3; - let t4; + const $ = useMemoCache(11); + let t1; + let T2; + let t3; let t0; - let t5; - let T6; - let t7; + let t4; + let T5; + let t6; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { const count = new MaybeMutable(); - T6 = View; - t7 = "\n "; - T3 = View; - t4 = "\n "; - if ($[8] === Symbol.for("react.memo_cache_sentinel")) { + T5 = View; + t6 = "\n "; + T2 = View; + t3 = "\n "; + if ($[7] === Symbol.for("react.memo_cache_sentinel")) { t0 = Text; - $[8] = t0; + $[7] = t0; } else { - t0 = $[8]; + t0 = $[7]; } - t5 = "\n "; - T1 = "span"; - t2 = maybeMutate(count); - $[0] = T1; - $[1] = t2; - $[2] = T3; - $[3] = t4; - $[4] = t0; - $[5] = t5; - $[6] = T6; - $[7] = t7; + t4 = "\n "; + t1 = maybeMutate(count); + $[0] = t1; + $[1] = T2; + $[2] = t3; + $[3] = t0; + $[4] = t4; + $[5] = T5; + $[6] = t6; } else { - T1 = $[0]; - t2 = $[1]; - T3 = $[2]; - t4 = $[3]; - t0 = $[4]; - t5 = $[5]; - T6 = $[6]; - t7 = $[7]; + t1 = $[0]; + T2 = $[1]; + t3 = $[2]; + t0 = $[3]; + t4 = $[4]; + T5 = $[5]; + t6 = $[6]; + } + let t7; + if ($[8] === Symbol.for("react.memo_cache_sentinel")) { + t7 = {t1}; + $[8] = t7; + } else { + t7 = $[8]; } let t8; if ($[9] === Symbol.for("react.memo_cache_sentinel")) { - t8 = {t2}; + t8 = ( + + {t3} + {t0} + {t4} + {t7} + + ); $[9] = t8; } else { t8 = $[9]; @@ -74,30 +84,16 @@ function Component(props) { let t9; if ($[10] === Symbol.for("react.memo_cache_sentinel")) { t9 = ( - - {t4} - {t0} - {t5} + + {t6} {t8} - + ); $[10] = t9; } else { t9 = $[10]; } - let t10; - if ($[11] === Symbol.for("react.memo_cache_sentinel")) { - t10 = ( - - {t7} - {t9} - - ); - $[11] = t10; - } else { - t10 = $[11]; - } - return t10; + return t9; } ```