From 1ec1a0ceb84de690e50dfd45fe476302ea954c59 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Wed, 4 Oct 2023 14:01:34 -0400 Subject: [PATCH] [fix] JSXElement identifiers now included in lambda capture deps --- `gatherCapturedDeps` previously did not visit JSXElements, so Forget did not read any local JSX identifiers as dependencies (in lambdas) --- .../src/HIR/BuildHIR.ts | 175 ++++++++++++------ ...g-ternary-value-in-jsx-attribute.expect.md | 27 --- ...ctive-local-variable-member-expr.expect.md | 52 ++++++ ...sx-reactive-local-variable-member-expr.tsx | 15 ++ .../jsx-ternary-local-variable.expect.md | 65 +++++++ ...ute.tsx => jsx-ternary-local-variable.tsx} | 2 +- 6 files changed, 252 insertions(+), 84 deletions(-) delete mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.bug-ternary-value-in-jsx-attribute.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/jsx-reactive-local-variable-member-expr.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/jsx-reactive-local-variable-member-expr.tsx create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/jsx-ternary-local-variable.expect.md rename compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/{error.bug-ternary-value-in-jsx-attribute.tsx => jsx-ternary-local-variable.tsx} (81%) diff --git a/compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts b/compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts index 0a8f57783f..d8ec0c0e6a 100644 --- a/compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts +++ b/compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts @@ -3457,78 +3457,141 @@ function gatherCapturedDeps( to: componentScope, }); - function visit(path: NodePath): void { - // Babel has a bug where it doesn't visit the LHS of an - // AssignmentExpression if it's an Identifier. Work around it by explicitly - // visiting it. - if (path.isAssignmentExpression()) { - const left = path.get("left"); - if (left.isIdentifier()) { - visit(left); + function addCapturedId(bindingIdentifier: t.Identifier): number { + if (!capturedIds.has(bindingIdentifier)) { + const index = capturedIds.size; + capturedIds.set(bindingIdentifier, index); + return index; + } else { + return capturedIds.get(bindingIdentifier)!; + } + } + + function handleMaybeDependency( + path: + | NodePath + | NodePath + | NodePath + ): void { + // Base context variable to depend on + let baseIdentifier: NodePath; + // Base expression to depend on, which (for now) may contain non side-effectful + // member expressions + let dependency: + | NodePath + | NodePath + | NodePath; + if (path.isJSXOpeningElement()) { + const name = path.get("name"); + if (!(name.isJSXMemberExpression() || name.isJSXIdentifier())) { + // TODO: should JSX namespaced names be handled here as well? + return; } - return; + let current: NodePath = name; + while (current.isJSXMemberExpression()) { + current = current.get("object"); + } + invariant( + current.isJSXIdentifier(), + "Invalid logic in gatherCapturedDeps" + ); + baseIdentifier = current; + dependency = current; + } else if (path.isMemberExpression()) { + // Calculate baseIdentifier + let current: NodePath = path; + while (current.isMemberExpression()) { + current = current.get("object"); + } + if (!current.isIdentifier()) { + return; + } + baseIdentifier = current; + + // Get the expression to depend on, which may involve PropertyLoads + // for member expressions + current = + path.parent.type === "CallExpression" && + path.parent.callee === path.node + ? path.get("object") + : path; + while (current.isMemberExpression() && current.node.computed) { + // computed nodes may contain side-effectful subexpressions + current = current.get("object"); + } + invariant( + current.isMemberExpression() || current.isIdentifier(), + "Internal invariant broken in BuildHIR, unexpected type for capturedDep" + ); + dependency = current; + } else { + baseIdentifier = path; + dependency = path; } - let obj = path; - while (obj.isMemberExpression()) { - obj = obj.get("object"); - } + /** + * Skip dependency path, as we already tried to recursively add it (+ all subexpressions) + * as a dependency. + */ + dependency.skip(); - if (!obj.isIdentifier()) { - return; - } - - const binding = obj.scope.getBinding(obj.node.name); + /** + * Add the base identifier binding as a dependency. + */ + const binding = baseIdentifier.scope.getBinding(baseIdentifier.node.name); if (binding === undefined || !pureScopes.has(binding.scope)) { return; } + const idKey = String(addCapturedId(binding.identifier)); - if (path.isMemberExpression()) { - // For CallExpression, we need to depend on the receiver, not the - // function itself. - if ( - path.parent.type === "CallExpression" && - path.parent.callee === path.node - ) { - path = path.get("object"); + /** + * Add the expression (potentially a memberexpr path) as a dependency. + */ + let exprKey = idKey; + if (dependency.isMemberExpression()) { + let pathTokens = []; + let current: NodePath = dependency; + while (current.isMemberExpression()) { + const property = current.get("property") as NodePath; + pathTokens.push(property.node.name); + current = current.get("object"); } - // Skip the computed part of the member expression. - while (path.isMemberExpression() && path.node.computed) { - path = path.get("object"); + exprKey += "." + pathTokens.reverse().join("."); + } + + if (!seenPaths.has(exprKey)) { + let loweredDep: Place; + if (dependency.isJSXIdentifier()) { + loweredDep = lowerValueToTemporary(builder, { + kind: "LoadLocal", + place: lowerIdentifier(builder, dependency), + loc: path.node.loc ?? GeneratedSource, + }); + } else { + loweredDep = lowerExpressionToTemporary(builder, dependency); } - - path.skip(); - } - - // Store the top-level identifiers that are captured as well as the list - // of Places (including PropertyLoad) - let index: number; - if (!capturedIds.has(binding.identifier)) { - index = capturedIds.size; - capturedIds.set(binding.identifier, index); - } else { - index = capturedIds.get(binding.identifier)!; - } - let pathTokens = []; - let current = path; - while (current.isMemberExpression()) { - const property = path.get("property") as NodePath; - pathTokens.push(property.node.name); - current = current.get("object"); - } - pathTokens.push(String(index)); - pathTokens.reverse(); - const pathKey = pathTokens.join("."); - if (!seenPaths.has(pathKey)) { - capturedRefs.add(lowerExpressionToTemporary(builder, path)); - seenPaths.add(pathKey); + capturedRefs.add(loweredDep); + seenPaths.add(exprKey); } } fn.traverse({ Expression(path) { - visit(path); + if (path.isAssignmentExpression()) { + // Babel has a bug where it doesn't visit the LHS of an + // AssignmentExpression if it's an Identifier. Work around it by explicitly + // visiting it. + const left = path.get("left"); + if (left.isIdentifier()) { + handleMaybeDependency(left); + } + return; + } else if (path.isJSXElement()) { + handleMaybeDependency(path.get("openingElement")); + } else if (path.isMemberExpression() || path.isIdentifier()) { + handleMaybeDependency(path); + } }, }); diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.bug-ternary-value-in-jsx-attribute.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.bug-ternary-value-in-jsx-attribute.expect.md deleted file mode 100644 index b91274569f..0000000000 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.bug-ternary-value-in-jsx-attribute.expect.md +++ /dev/null @@ -1,27 +0,0 @@ - -## Input - -```javascript -import { RenderPropAsChild, StaticText1, StaticText2 } from "shared-runtime"; - -function Component(props: { showText1: boolean }) { - const Foo = props.showText1 ? StaticText1 : StaticText2; - - return ]} />; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{ showText1: false }], -}; - -``` - - -## Error - -``` -[ReactForget] Invariant: Expected value for identifier `28` to be initialized. (6:6) -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/jsx-reactive-local-variable-member-expr.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/jsx-reactive-local-variable-member-expr.expect.md new file mode 100644 index 0000000000..48504a1171 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/jsx-reactive-local-variable-member-expr.expect.md @@ -0,0 +1,52 @@ + +## Input + +```javascript +import * as sharedRuntime from "shared-runtime"; + +function Component({ + something, +}: { + something: { StaticText1: React.ElementType }; +}) { + const Foo = something.StaticText1; + return () => ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ something: sharedRuntime }], +}; + +``` + +## Code + +```javascript +import { unstable_useMemoCache as useMemoCache } from "react"; +import * as sharedRuntime from "shared-runtime"; + +function Component(t13) { + const $ = useMemoCache(2); + const { something } = t13; + + const Foo = something.StaticText1; + const c_0 = $[0] !== Foo; + let t0; + if (c_0) { + t0 = () => ; + $[0] = Foo; + $[1] = t0; + } else { + t0 = $[1]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ something: sharedRuntime }], +}; + +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/jsx-reactive-local-variable-member-expr.tsx b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/jsx-reactive-local-variable-member-expr.tsx new file mode 100644 index 0000000000..223967b06f --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/jsx-reactive-local-variable-member-expr.tsx @@ -0,0 +1,15 @@ +import * as sharedRuntime from "shared-runtime"; + +function Component({ + something, +}: { + something: { StaticText1: React.ElementType }; +}) { + const Foo = something.StaticText1; + return () => ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ something: sharedRuntime }], +}; diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/jsx-ternary-local-variable.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/jsx-ternary-local-variable.expect.md new file mode 100644 index 0000000000..d234d17c75 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/jsx-ternary-local-variable.expect.md @@ -0,0 +1,65 @@ + +## Input + +```javascript +import { RenderPropAsChild, StaticText1, StaticText2 } from "shared-runtime"; + +function Component(props: { showText1: boolean }) { + const Foo = props.showText1 ? StaticText1 : StaticText2; + + return ]} />; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ showText1: false }], +}; + +``` + +## Code + +```javascript +import { unstable_useMemoCache as useMemoCache } from "react"; +import { RenderPropAsChild, StaticText1, StaticText2 } from "shared-runtime"; + +function Component(props) { + const $ = useMemoCache(6); + const Foo = props.showText1 ? StaticText1 : StaticText2; + const c_0 = $[0] !== Foo; + let t0; + if (c_0) { + t0 = () => ; + $[0] = Foo; + $[1] = t0; + } else { + t0 = $[1]; + } + const c_2 = $[2] !== t0; + let t1; + if (c_2) { + t1 = [t0]; + $[2] = t0; + $[3] = t1; + } else { + t1 = $[3]; + } + const c_4 = $[4] !== t1; + let t2; + if (c_4) { + t2 = ; + $[4] = t1; + $[5] = t2; + } else { + t2 = $[5]; + } + return t2; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ showText1: false }], +}; + +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.bug-ternary-value-in-jsx-attribute.tsx b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/jsx-ternary-local-variable.tsx similarity index 81% rename from compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.bug-ternary-value-in-jsx-attribute.tsx rename to compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/jsx-ternary-local-variable.tsx index 5f8010efe5..8dac84b2f0 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.bug-ternary-value-in-jsx-attribute.tsx +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/jsx-ternary-local-variable.tsx @@ -3,7 +3,7 @@ import { RenderPropAsChild, StaticText1, StaticText2 } from "shared-runtime"; function Component(props: { showText1: boolean }) { const Foo = props.showText1 ? StaticText1 : StaticText2; - return ]} />; + return ]} />; } export const FIXTURE_ENTRYPOINT = {