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/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/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'}], +}; 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: [{}], +};