From b609f1df47cc7d540e3291c54507bde0d921b0ff Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 28 Aug 2024 15:16:02 -0700 Subject: [PATCH] [compiler] Inferred deps must match exact optionality of manual deps To prevent any difference in behavior, we check that the optionality of the inferred deps exactly matches the optionality of the manual dependencies. This required a fix, I was incorrectly inferring optionality of manual deps (they're only optional if OptionalTerminal.optional is true) - for nested cases of mixed optional/non-optional. ghstack-source-id: afd49e89cc3194eb3c317ca7434d3fa948896bff Pull Request resolved: https://github.com/facebook/react/pull/30840 --- .../src/Inference/DropManualMemoization.ts | 2 +- .../ValidatePreservedManualMemoization.ts | 2 +- ...as-memo-dep-non-optional-in-body.expect.md | 38 ++++++++++++++ ...ssion-as-memo-dep-non-optional-in-body.js} | 0 ...as-memo-dep-non-optional-in-body.expect.md | 50 ------------------- ...ession-single-with-unconditional.expect.md | 33 ++++++------ ...er-expression-single-with-unconditional.js | 4 +- 7 files changed, 57 insertions(+), 72 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-optional-member-expression-as-memo-dep-non-optional-in-body.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{optional-member-expression-as-memo-dep-non-optional-in-body.js => error.invalid-optional-member-expression-as-memo-dep-non-optional-in-body.js} (100%) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-as-memo-dep-non-optional-in-body.expect.md diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts index fdd9bcc968aef1..4dcdc21e15ac5e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts @@ -488,7 +488,7 @@ export function dropManualMemoization(func: HIRFunction): void { function findOptionalPlaces(fn: HIRFunction): Set { const optionals = new Set(); for (const [, block] of fn.body.blocks) { - if (block.terminal.kind === 'optional') { + if (block.terminal.kind === 'optional' && block.terminal.optional) { const optionalTerminal = block.terminal; let testBlock = fn.body.blocks.get(block.terminal.test)!; loop: while (true) { diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts index 4f0c756585cf09..e7615320c7b950 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts @@ -170,7 +170,7 @@ function compareDeps( if (inferred.path[i].property !== source.path[i].property) { isSubpath = false; break; - } else if (inferred.path[i].optional && !source.path[i].optional) { + } else if (inferred.path[i].optional !== source.path[i].optional) { /** * The inferred path must be at least as precise as the manual path: * if the inferred path is optional, then the source path must have diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-optional-member-expression-as-memo-dep-non-optional-in-body.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-optional-member-expression-as-memo-dep-non-optional-in-body.expect.md new file mode 100644 index 00000000000000..ba5b30418069a4 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-optional-member-expression-as-memo-dep-non-optional-in-body.expect.md @@ -0,0 +1,38 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees +function Component(props) { + const data = useMemo(() => { + // actual code is non-optional + return props.items.edges.nodes ?? []; + // deps are optional + }, [props.items?.edges?.nodes]); + return ; +} + +``` + + +## Error + +``` + 1 | // @validatePreserveExistingMemoizationGuarantees + 2 | function Component(props) { +> 3 | const data = useMemo(() => { + | ^^^^^^^ +> 4 | // actual code is non-optional + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +> 5 | return props.items.edges.nodes ?? []; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +> 6 | // deps are optional + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +> 7 | }, [props.items?.edges?.nodes]); + | ^^^^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. The inferred dependencies did not match the manually specified dependencies, which could cause the value to change more or less frequently than expected (3:7) + 8 | return ; + 9 | } + 10 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-as-memo-dep-non-optional-in-body.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-optional-member-expression-as-memo-dep-non-optional-in-body.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-as-memo-dep-non-optional-in-body.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-optional-member-expression-as-memo-dep-non-optional-in-body.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-as-memo-dep-non-optional-in-body.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-as-memo-dep-non-optional-in-body.expect.md deleted file mode 100644 index 7cf1bcd90b3399..00000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-as-memo-dep-non-optional-in-body.expect.md +++ /dev/null @@ -1,50 +0,0 @@ - -## Input - -```javascript -// @validatePreserveExistingMemoizationGuarantees -function Component(props) { - const data = useMemo(() => { - // actual code is non-optional - return props.items.edges.nodes ?? []; - // deps are optional - }, [props.items?.edges?.nodes]); - return ; -} - -``` - -## Code - -```javascript -import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees -function Component(props) { - const $ = _c(4); - - props.items?.edges?.nodes; - let t0; - let t1; - if ($[0] !== props.items.edges.nodes) { - t1 = props.items.edges.nodes ?? []; - $[0] = props.items.edges.nodes; - $[1] = t1; - } else { - t1 = $[1]; - } - t0 = t1; - const data = t0; - let t2; - if ($[2] !== data) { - t2 = ; - $[2] = data; - $[3] = t2; - } else { - t2 = $[3]; - } - return t2; -} - -``` - -### Eval output -(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single-with-unconditional.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single-with-unconditional.expect.md index d0c4afe459da2b..46767056bdcdff 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single-with-unconditional.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single-with-unconditional.expect.md @@ -10,8 +10,8 @@ function Component(props) { x.push(props?.items); x.push(props.items); return x; - }, [props?.items]); - return ; + }, [props.items]); + return ; } ``` @@ -23,8 +23,6 @@ import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMe import { ValidateMemoization } from "shared-runtime"; function Component(props) { const $ = _c(7); - - props?.items; let t0; let x; if ($[0] !== props.items) { @@ -38,25 +36,24 @@ function Component(props) { } t0 = x; const data = t0; - const t1 = props?.items; - let t2; - if ($[2] !== t1) { - t2 = [t1]; - $[2] = t1; - $[3] = t2; + let t1; + if ($[2] !== props.items) { + t1 = [props.items]; + $[2] = props.items; + $[3] = t1; } else { - t2 = $[3]; + t1 = $[3]; } - let t3; - if ($[4] !== t2 || $[5] !== data) { - t3 = ; - $[4] = t2; + let t2; + if ($[4] !== t1 || $[5] !== data) { + t2 = ; + $[4] = t1; $[5] = data; - $[6] = t3; + $[6] = t2; } else { - t3 = $[6]; + t2 = $[6]; } - return t3; + return t2; } ``` diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single-with-unconditional.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single-with-unconditional.js index c630dd6bb4b9d5..8e6275bf921eb8 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single-with-unconditional.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single-with-unconditional.js @@ -6,6 +6,6 @@ function Component(props) { x.push(props?.items); x.push(props.items); return x; - }, [props?.items]); - return ; + }, [props.items]); + return ; }