From a2bc92a8be751d3f780aec40aaa29330c5cb0b0f Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 28 Aug 2025 14:52:57 -0700 Subject: [PATCH 1/2] [compiler] Handle empty list of eslint suppression rules --- .../src/Entrypoint/Options.ts | 5 ++ .../src/Entrypoint/Suppression.ts | 27 +++++++--- ...empty-eslint-suppressions-config.expect.md | 53 +++++++++++++++++++ .../empty-eslint-suppressions-config.js | 15 ++++++ 4 files changed, 92 insertions(+), 8 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/empty-eslint-suppressions-config.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/empty-eslint-suppressions-config.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Options.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Options.ts index c13940ed10a21..1ebdb68f9501e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Options.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Options.ts @@ -135,7 +135,12 @@ export type PluginOptions = { */ eslintSuppressionRules: Array | null | undefined; + /** + * Whether to report "suppression" errors for Flow suppressions. If false, suppression errors + * are only emitted for ESLint suppressions + */ flowSuppressions: boolean; + /* * Ignore 'use no forget' annotations. Helpful during testing but should not be used in production. */ diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Suppression.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Suppression.ts index a0d06f96f0e52..45db4b2cc8db6 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Suppression.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Suppression.ts @@ -87,12 +87,18 @@ export function findProgramSuppressions( let enableComment: t.Comment | null = null; let source: SuppressionSource | null = null; - const rulePattern = `(${ruleNames.join('|')})`; - const disableNextLinePattern = new RegExp( - `eslint-disable-next-line ${rulePattern}`, - ); - const disablePattern = new RegExp(`eslint-disable ${rulePattern}`); - const enablePattern = new RegExp(`eslint-enable ${rulePattern}`); + let disableNextLinePattern: RegExp | null = null; + let disablePattern: RegExp | null = null; + let enablePattern: RegExp | null = null; + if (ruleNames.length !== 0) { + const rulePattern = `(${ruleNames.join('|')})`; + disableNextLinePattern = new RegExp( + `eslint-disable-next-line ${rulePattern}`, + ); + disablePattern = new RegExp(`eslint-disable ${rulePattern}`); + enablePattern = new RegExp(`eslint-enable ${rulePattern}`); + } + const flowSuppressionPattern = new RegExp( '\\$(FlowFixMe\\w*|FlowExpectedError|FlowIssue)\\[react\\-rule', ); @@ -108,6 +114,7 @@ export function findProgramSuppressions( * CommentLine within the block. */ disableComment == null && + disableNextLinePattern != null && disableNextLinePattern.test(comment.value) ) { disableComment = comment; @@ -125,12 +132,16 @@ export function findProgramSuppressions( source = 'Flow'; } - if (disablePattern.test(comment.value)) { + if (disablePattern != null && disablePattern.test(comment.value)) { disableComment = comment; source = 'Eslint'; } - if (enablePattern.test(comment.value) && source === 'Eslint') { + if ( + enablePattern != null && + enablePattern.test(comment.value) && + source === 'Eslint' + ) { enableComment = comment; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/empty-eslint-suppressions-config.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/empty-eslint-suppressions-config.expect.md new file mode 100644 index 0000000000000..eeb0ba6c96db9 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/empty-eslint-suppressions-config.expect.md @@ -0,0 +1,53 @@ + +## Input + +```javascript +// @eslintSuppressionRules:[] + +// The suppression here shouldn't cause compilation to get skipped +// Previously we had a bug where an empty list of suppressions would +// create a regexp that matched any suppression +function Component(props) { + 'use forget'; + // eslint-disable-next-line foo/not-react-related + return
{props.text}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{text: 'Hello'}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @eslintSuppressionRules:[] + +// The suppression here shouldn't cause compilation to get skipped +// Previously we had a bug where an empty list of suppressions would +// create a regexp that matched any suppression +function Component(props) { + "use forget"; + const $ = _c(2); + let t0; + if ($[0] !== props.text) { + t0 =
{props.text}
; + $[0] = props.text; + $[1] = t0; + } else { + t0 = $[1]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ text: "Hello" }], +}; + +``` + +### Eval output +(kind: ok)
Hello
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/empty-eslint-suppressions-config.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/empty-eslint-suppressions-config.js new file mode 100644 index 0000000000000..b81132d3b8269 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/empty-eslint-suppressions-config.js @@ -0,0 +1,15 @@ +// @eslintSuppressionRules:[] + +// The suppression here shouldn't cause compilation to get skipped +// Previously we had a bug where an empty list of suppressions would +// create a regexp that matched any suppression +function Component(props) { + 'use forget'; + // eslint-disable-next-line foo/not-react-related + return
{props.text}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{text: 'Hello'}], +}; From 051be780797c28b989597c749a00d2a66e23ad7b Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 28 Aug 2025 14:52:57 -0700 Subject: [PATCH 2/2] [compiler] Fix false positive memo validation Partial fix for #34262. Consider this example: ```js function useInputValue(input) { const object = React.useMemo(() => { const {value} = transform(input); return {value}; }, [input]); return object; } ``` React Compiler breaks this code into two reactive scopes: * One for `transform(input)` * One for `{value}` When we run ValidatePreserveExistingMemo, we see that the scope for `{value}` has the dependency `value`, whereas the original memoization had the dependency `input`, and throw an error that the dependencies didn't match. In other words, we're flagging the fact that memoized _better than the user_ as a problem. The more complete solution would be to validate that there is a subgraph of reactive scopes with a single input and output node, where the input node has the same dependencies as the original useMemo, and the output has the same outputs. That is true in this case, with the subgraph being the two consecutive scopes mentioned above. But that's complicated. As a shortcut, this PR checks for any dependencies that are defined after the start of the original useMemo. If we find one, we know that it's a case where we were able to memoize more precisely than the original, and we don't report an error on the dependency. We still check that the original _output_ value is able to be memoized, though. So if the scope of `object` were extended, eg with a call to `mutate(object)`, then we'd still correctly report an error that we couldn't preserve memoization. --- .../ValidatePreservedManualMemoization.ts | 14 +++ ...ken-as-dependency-later-mutation.expect.md | 59 ++++++++++ ...e-mistaken-as-dependency-later-mutation.js | 25 ++++ ...staken-as-dependency-mutated-dep.expect.md | 64 ++++++++++ ...alue-mistaken-as-dependency-mutated-dep.js | 36 ++++++ ...red-value-mistaken-as-dependency.expect.md | 109 ++++++++++++++++++ ...structured-value-mistaken-as-dependency.js | 31 +++++ 7 files changed, 338 insertions(+) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-later-mutation.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-later-mutation.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-mutated-dep.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-mutated-dep.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency.js 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 516ca232e9e4b..ce027467cc6e1 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts @@ -17,6 +17,7 @@ import { GeneratedSource, Identifier, IdentifierId, + InstructionId, InstructionValue, ManualMemoDependency, Place, @@ -109,6 +110,7 @@ type ManualMemoBlockState = { */ depsFromSource: Array | null; manualMemoId: number; + start: InstructionId; }; type VisitorState = { @@ -234,6 +236,8 @@ function validateInferredDep( validDepsInMemoBlock: Array, errorState: CompilerError, memoLocation: SourceLocation, + memoStartInstruction: InstructionId, + scopes: Set, ): void { let normalizedDep: ManualMemoDependency; const maybeNormalizedRoot = temporaries.get(dep.identifier.id); @@ -271,6 +275,13 @@ function validateInferredDep( return; } } + if ( + dep.identifier.mutableRange.start > memoStartInstruction && + !isUnmemoized(dep.identifier, scopes) + ) { + return; + } + let errorDiagnostic: CompareDependencyResult | null = null; for (const originalDep of validDepsInMemoBlock) { const compareResult = compareDeps(normalizedDep, originalDep); @@ -433,6 +444,8 @@ class Visitor extends ReactiveFunctionVisitor { state.manualMemoState.depsFromSource, state.errors, state.manualMemoState.loc, + state.manualMemoState.start, + this.scopes, ); } } @@ -508,6 +521,7 @@ class Visitor extends ReactiveFunctionVisitor { depsFromSource, manualMemoId: value.manualMemoId, reassignments: new Map(), + start: instruction.id, }; /** diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-later-mutation.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-later-mutation.expect.md new file mode 100644 index 0000000000000..44c6e685c4dcf --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-later-mutation.expect.md @@ -0,0 +1,59 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees + +/** + * Repro from https://github.com/facebook/react/issues/34262 + * + * The compiler memoizes more precisely than the original code, with two reactive scopes: + * - One for `transform(input)` with `input` as dep + * - One for `{value}` with `value` as dep + * + * When we validate preserving manual memoization we incorrectly reject this, because + * the original memoization had `object` depending on `input` but our scope depends on + * `value`. + * + * This fixture adds a later potential mutation, which extends the scope and should + * fail validation. This confirms that even though we allow the dependency to diverge, + * we still check that the output value is memoized. + */ +function useInputValue(input) { + const object = React.useMemo(() => { + const {value} = transform(input); + return {value}; + }, [input]); + mutate(object); + return object; +} + +``` + + +## Error + +``` +Found 1 error: + +Memoization: Compilation skipped because existing memoization could not be preserved + +React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This value was memoized in source but not in compilation output. + +error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-later-mutation.ts:19:17 + 17 | */ + 18 | function useInputValue(input) { +> 19 | const object = React.useMemo(() => { + | ^^^^^^^^^^^^^^^^^^^^^ +> 20 | const {value} = transform(input); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +> 21 | return {value}; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +> 22 | }, [input]); + | ^^^^^^^^^^^^^^ Could not preserve existing memoization + 23 | mutate(object); + 24 | return object; + 25 | } +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-later-mutation.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-later-mutation.js new file mode 100644 index 0000000000000..f07d00854cd85 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-later-mutation.js @@ -0,0 +1,25 @@ +// @validatePreserveExistingMemoizationGuarantees + +/** + * Repro from https://github.com/facebook/react/issues/34262 + * + * The compiler memoizes more precisely than the original code, with two reactive scopes: + * - One for `transform(input)` with `input` as dep + * - One for `{value}` with `value` as dep + * + * When we validate preserving manual memoization we incorrectly reject this, because + * the original memoization had `object` depending on `input` but our scope depends on + * `value`. + * + * This fixture adds a later potential mutation, which extends the scope and should + * fail validation. This confirms that even though we allow the dependency to diverge, + * we still check that the output value is memoized. + */ +function useInputValue(input) { + const object = React.useMemo(() => { + const {value} = transform(input); + return {value}; + }, [input]); + mutate(object); + return object; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-mutated-dep.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-mutated-dep.expect.md new file mode 100644 index 0000000000000..08a8523a39f38 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-mutated-dep.expect.md @@ -0,0 +1,64 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees + +import {identity, Stringify, useHook} from 'shared-runtime'; + +/** + * Repro from https://github.com/facebook/react/issues/34262 + * + * The compiler memoizes more precisely than the original code, with two reactive scopes: + * - One for `transform(input)` with `input` as dep + * - One for `{value}` with `value` as dep + * + * When we validate preserving manual memoization we incorrectly reject this, because + * the original memoization had `object` depending on `input` but our scope depends on + * `value`. + */ +function useInputValue(input) { + // Conflate the `identity(input, x)` call with something outside the useMemo, + // to try and break memoization of `value`. This gets correctly flagged since + // the dependency is being mutated + let x = {}; + useHook(); + const object = React.useMemo(() => { + const {value} = identity(input, x); + return {value}; + }, [input, x]); + return object; +} + +function Component() { + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + + +## Error + +``` +Found 1 error: + +Memoization: Compilation skipped because existing memoization could not be preserved + +React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This dependency may be mutated later, which could cause the value to change unexpectedly. + +error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-mutated-dep.ts:25:13 + 23 | const {value} = identity(input, x); + 24 | return {value}; +> 25 | }, [input, x]); + | ^ This dependency may be modified later + 26 | return object; + 27 | } + 28 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-mutated-dep.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-mutated-dep.js new file mode 100644 index 0000000000000..06d2945868dd1 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-mutated-dep.js @@ -0,0 +1,36 @@ +// @validatePreserveExistingMemoizationGuarantees + +import {identity, Stringify, useHook} from 'shared-runtime'; + +/** + * Repro from https://github.com/facebook/react/issues/34262 + * + * The compiler memoizes more precisely than the original code, with two reactive scopes: + * - One for `transform(input)` with `input` as dep + * - One for `{value}` with `value` as dep + * + * When we validate preserving manual memoization we incorrectly reject this, because + * the original memoization had `object` depending on `input` but our scope depends on + * `value`. + */ +function useInputValue(input) { + // Conflate the `identity(input, x)` call with something outside the useMemo, + // to try and break memoization of `value`. This gets correctly flagged since + // the dependency is being mutated + let x = {}; + useHook(); + const object = React.useMemo(() => { + const {value} = identity(input, x); + return {value}; + }, [input, x]); + return object; +} + +function Component() { + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency.expect.md new file mode 100644 index 0000000000000..eca36e2a7919d --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency.expect.md @@ -0,0 +1,109 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees + +import {identity, Stringify} from 'shared-runtime'; + +/** + * Repro from https://github.com/facebook/react/issues/34262 + * + * The compiler memoizes more precisely than the original code, with two reactive scopes: + * - One for `transform(input)` with `input` as dep + * - One for `{value}` with `value` as dep + * + * Previously ValidatePreservedManualMemoization rejected this input, because + * the original memoization had `object` depending on `input` but we split the scope per above, + * and the scope for the FinishMemoize instruction is the second scope which depends on `value` + */ +function useInputValue(input) { + const object = React.useMemo(() => { + const {value} = identity(input); + return {value}; + }, [input]); + return object; +} + +function Component() { + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees + +import { identity, Stringify } from "shared-runtime"; + +/** + * Repro from https://github.com/facebook/react/issues/34262 + * + * The compiler memoizes more precisely than the original code, with two reactive scopes: + * - One for `transform(input)` with `input` as dep + * - One for `{value}` with `value` as dep + * + * Previously ValidatePreservedManualMemoization rejected this input, because + * the original memoization had `object` depending on `input` but we split the scope per above, + * and the scope for the FinishMemoize instruction is the second scope which depends on `value` + */ +function useInputValue(input) { + const $ = _c(4); + let t0; + if ($[0] !== input) { + t0 = identity(input); + $[0] = input; + $[1] = t0; + } else { + t0 = $[1]; + } + const { value } = t0; + let t1; + if ($[2] !== value) { + t1 = { value }; + $[2] = value; + $[3] = t1; + } else { + t1 = $[3]; + } + const object = t1; + return object; +} + +function Component() { + const $ = _c(3); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = { value: 42 }; + $[0] = t0; + } else { + t0 = $[0]; + } + const t1 = useInputValue(t0); + let t2; + if ($[1] !== t1.value) { + t2 = ; + $[1] = t1.value; + $[2] = t2; + } else { + t2 = $[2]; + } + return t2; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + +### Eval output +(kind: ok)
{"value":42}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency.js new file mode 100644 index 0000000000000..a9533ce44bbde --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency.js @@ -0,0 +1,31 @@ +// @validatePreserveExistingMemoizationGuarantees + +import {identity, Stringify} from 'shared-runtime'; + +/** + * Repro from https://github.com/facebook/react/issues/34262 + * + * The compiler memoizes more precisely than the original code, with two reactive scopes: + * - One for `transform(input)` with `input` as dep + * - One for `{value}` with `value` as dep + * + * Previously ValidatePreservedManualMemoization rejected this input, because + * the original memoization had `object` depending on `input` but we split the scope per above, + * and the scope for the FinishMemoize instruction is the second scope which depends on `value` + */ +function useInputValue(input) { + const object = React.useMemo(() => { + const {value} = identity(input); + return {value}; + }, [input]); + return object; +} + +function Component() { + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +};