From 0691f5f180aa8c72d3d9ee6f9f90f2e02ab1b85a Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 28 Aug 2024 15:15:58 -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-poisoned] --- .../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 fdd9bcc968aef..4dcdc21e15ac5 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 4f0c756585cf0..e7615320c7b95 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 0000000000000..ba5b30418069a --- /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 7cf1bcd90b339..0000000000000 --- 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 d0c4afe459da2..46767056bdcdf 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 c630dd6bb4b9d..8e6275bf921eb 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 ; }