From e9abc41ea3f0170659292ce1151a015300105999 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 20 Apr 2023 16:48:34 -0700 Subject: [PATCH] Fix evaluation order for JSX element tags When lowering a JSX element we were correctly lowering to a temporary in all but one case: the common case of an identifier. That is fine in practice but breaks in the presence of the tag identifier being reassigned in the props/children. This PR fixes to always lower the tag to a temporary. --- compiler/forget/src/HIR/BuildHIR.ts | 8 ++- .../_bug.jsx-tag-evaluation-order.expect.md | 68 ------------------- .../error.jsx-tag-evaluation-order.expect.md | 28 ++++++++ ...r.js => error.jsx-tag-evaluation-order.js} | 4 +- ...rror.mutate-after-aliased-freeze.expect.md | 2 +- .../error.mutate-after-freeze.expect.md | 2 +- .../jsx-tag-evaluation-order.expect.md | 48 +++++++++++++ .../compiler/jsx-tag-evaluation-order.js | 9 +++ 8 files changed, 95 insertions(+), 74 deletions(-) delete mode 100644 compiler/forget/src/__tests__/fixtures/compiler/_bug.jsx-tag-evaluation-order.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.jsx-tag-evaluation-order.expect.md rename compiler/forget/src/__tests__/fixtures/compiler/{_bug.jsx-tag-evaluation-order.js => error.jsx-tag-evaluation-order.js} (75%) create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/jsx-tag-evaluation-order.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/jsx-tag-evaluation-order.js diff --git a/compiler/forget/src/HIR/BuildHIR.ts b/compiler/forget/src/HIR/BuildHIR.ts index 0174beea2d..37553ada13 100644 --- a/compiler/forget/src/HIR/BuildHIR.ts +++ b/compiler/forget/src/HIR/BuildHIR.ts @@ -27,7 +27,6 @@ import { InstructionKind, InstructionValue, JsxAttribute, - makeInstructionId, ObjectPattern, ObjectProperty, Place, @@ -35,6 +34,7 @@ import { SourceLocation, SpreadPattern, ThrowTerminal, + makeInstructionId, } from "./HIR"; import HIRBuilder, { Bindings } from "./HIRBuilder"; @@ -2041,7 +2041,11 @@ function lowerJsxElementName( if (exprPath.isJSXIdentifier()) { const tag: string = exprPath.node.name; if (tag.match(/^[A-Z]/)) { - return lowerIdentifier(builder, exprPath); + return lowerValueToTemporary(builder, { + kind: "LoadLocal", + place: lowerIdentifier(builder, exprPath), + loc: exprLoc, + }); } else { if (tag.indexOf(":") !== -1) { builder.errors.push({ diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.jsx-tag-evaluation-order.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/_bug.jsx-tag-evaluation-order.expect.md deleted file mode 100644 index 4f9e5190aa..0000000000 --- a/compiler/forget/src/__tests__/fixtures/compiler/_bug.jsx-tag-evaluation-order.expect.md +++ /dev/null @@ -1,68 +0,0 @@ - -## Input - -```javascript -function Component(props) { - const maybeMutable = new MaybeMutable(); - let Tag = View; - // NOTE: the order of evaluation in the lowering is incorrect: - // the jsx element's tag observes `Tag` after reassignment, but should observe - // it before the reassignment. - return ( - - {((Tag = HScroll), maybeMutate(maybeMutable))} - - - ); -} - -``` - -## Code - -```javascript -import * as React from "react"; -function Component(props) { - const $ = React.unstable_useMemoCache(5); - let Tag; - let t0; - let t1; - if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - const maybeMutable = new MaybeMutable(); - - t0 = "\n "; - Tag = HScroll; - t1 = maybeMutate(maybeMutable); - $[0] = Tag; - $[1] = t0; - $[2] = t1; - } else { - Tag = $[0]; - t0 = $[1]; - t1 = $[2]; - } - let t2; - if ($[3] === Symbol.for("react.memo_cache_sentinel")) { - t2 = ; - $[3] = t2; - } else { - t2 = $[3]; - } - let t3; - if ($[4] === Symbol.for("react.memo_cache_sentinel")) { - t3 = ( - - {t0} - {t1} - {t2} - - ); - $[4] = t3; - } else { - t3 = $[4]; - } - return t3; -} - -``` - \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.jsx-tag-evaluation-order.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.jsx-tag-evaluation-order.expect.md new file mode 100644 index 0000000000..81e5631cf7 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.jsx-tag-evaluation-order.expect.md @@ -0,0 +1,28 @@ + +## Input + +```javascript +function Component(props) { + const maybeMutable = new MaybeMutable(); + let Tag = props.component; + // NOTE: the order of evaluation in the lowering is incorrect: + // the jsx element's tag observes `Tag` after reassignment, but should observe + // it before the reassignment. + return ( + + {((Tag = props.alternateComponent), maybeMutate(maybeMutable))} + + + ); +} + +``` + + +## Error + +``` +[ReactForget] Invariant: [Codegen] No value found for temporary. Value for 'read $33' was not set in the codegen context (8:8) +``` + + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.jsx-tag-evaluation-order.js b/compiler/forget/src/__tests__/fixtures/compiler/error.jsx-tag-evaluation-order.js similarity index 75% rename from compiler/forget/src/__tests__/fixtures/compiler/_bug.jsx-tag-evaluation-order.js rename to compiler/forget/src/__tests__/fixtures/compiler/error.jsx-tag-evaluation-order.js index d5af42bdaa..b2440232c8 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/_bug.jsx-tag-evaluation-order.js +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.jsx-tag-evaluation-order.js @@ -1,12 +1,12 @@ function Component(props) { const maybeMutable = new MaybeMutable(); - let Tag = View; + let Tag = props.component; // NOTE: the order of evaluation in the lowering is incorrect: // the jsx element's tag observes `Tag` after reassignment, but should observe // it before the reassignment. return ( - {((Tag = HScroll), maybeMutate(maybeMutable))} + {((Tag = props.alternateComponent), maybeMutate(maybeMutable))} ); diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.mutate-after-aliased-freeze.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.mutate-after-aliased-freeze.expect.md index ca7f01d8e0..e9627583ba 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/error.mutate-after-aliased-freeze.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.mutate-after-aliased-freeze.expect.md @@ -25,7 +25,7 @@ function Component(props) { ## Error ``` -[ReactForget] InvalidInput: InferReferenceEffects: inferred mutation of known immutable value. Found mutation of $43:TObject (frozen) (13:13) +[ReactForget] InvalidInput: InferReferenceEffects: inferred mutation of known immutable value. Found mutation of $46:TObject (frozen) (13:13) ``` \ No newline at end of file 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 25841c62e9..92d2441e96 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 $26:TObject (frozen) (7:7) +[ReactForget] InvalidInput: InferReferenceEffects: inferred mutation of known immutable value. Found mutation of $28:TObject (frozen) (7:7) ``` \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/jsx-tag-evaluation-order.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/jsx-tag-evaluation-order.expect.md new file mode 100644 index 0000000000..10ec0f6869 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/jsx-tag-evaluation-order.expect.md @@ -0,0 +1,48 @@ + +## Input + +```javascript +function Component(props) { + let Tag = View; + return ( + + {((Tag = HScroll), props.value)} + + + ); +} + +``` + +## Code + +```javascript +import * as React from "react"; +function Component(props) { + const $ = React.unstable_useMemoCache(3); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = ; + $[0] = t0; + } else { + t0 = $[0]; + } + const c_1 = $[1] !== props.value; + let t1; + if (c_1) { + t1 = ( + + {props.value} + {t0} + + ); + $[1] = props.value; + $[2] = t1; + } else { + t1 = $[2]; + } + return t1; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/jsx-tag-evaluation-order.js b/compiler/forget/src/__tests__/fixtures/compiler/jsx-tag-evaluation-order.js new file mode 100644 index 0000000000..51f6b51ff7 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/jsx-tag-evaluation-order.js @@ -0,0 +1,9 @@ +function Component(props) { + let Tag = View; + return ( + + {((Tag = HScroll), props.value)} + + + ); +}