diff --git a/compiler/.claude/settings.local.json b/compiler/.claude/settings.local.json new file mode 100644 index 00000000000..9bbd18802e4 --- /dev/null +++ b/compiler/.claude/settings.local.json @@ -0,0 +1,9 @@ +{ + "permissions": { + "allow": [ + "Bash(node scripts/enable-feature-flag.js:*)" + ], + "deny": [], + "ask": [] + } +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts b/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts index 352d31b2f89..c7c8d6a161e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts @@ -1067,7 +1067,15 @@ function getRuleForCategoryImpl(category: ErrorCategory): LintRule { name: 'memo-dependencies', description: 'Validates that useMemo() and useCallback() specify comprehensive dependencies without extraneous values. See [`useMemo()` docs](https://react.dev/reference/react/useMemo) for more information.', - preset: LintRulePreset.RecommendedLatest, + /** + * TODO: the "MemoDependencies" rule largely reimplements the "exhaustive-deps" non-compiler rule, + * allowing the compiler to ensure it does not regress change behavior due to different dependencies. + * We previously relied on the source having ESLint suppressions for any exhaustive-deps violations, + * but it's more reliable to verify it within the compiler. + * + * Long-term we should de-duplicate these implementations. + */ + preset: LintRulePreset.Off, }; } case ErrorCategory.IncompatibleLibrary: { diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts index 3d97d4ddf04..1b76dfb43e1 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -303,9 +303,11 @@ function runWithEnvironment( inferReactivePlaces(hir); log({kind: 'hir', name: 'InferReactivePlaces', value: hir}); - if (env.config.validateExhaustiveMemoizationDependencies) { - // NOTE: this relies on reactivity inference running first - validateExhaustiveDependencies(hir).unwrap(); + if (env.enableValidations) { + if (env.config.validateExhaustiveMemoizationDependencies) { + // NOTE: this relies on reactivity inference running first + validateExhaustiveDependencies(hir).unwrap(); + } } rewriteInstructionKindsBasedOnReassignment(hir); diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts index 7d12e054373..5ac580abbf0 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts @@ -400,7 +400,15 @@ export function compileProgram( */ const suppressions = findProgramSuppressions( pass.comments, - pass.opts.eslintSuppressionRules ?? DEFAULT_ESLINT_SUPPRESSIONS, + /* + * If the compiler is validating hooks rules and exhaustive memo dependencies, we don't need to check + * for React ESLint suppressions + */ + pass.opts.environment.validateExhaustiveMemoizationDependencies && + pass.opts.environment.validateHooksUsage + ? null + : (pass.opts.eslintSuppressionRules ?? DEFAULT_ESLINT_SUPPRESSIONS), + // Always bail on Flow suppressions pass.opts.flowSuppressions, ); 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 24a9bccf426..e0f845f269b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Suppression.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Suppression.ts @@ -78,7 +78,7 @@ export function filterSuppressionsThatAffectFunction( export function findProgramSuppressions( programComments: Array, - ruleNames: Array, + ruleNames: Array | null, flowSuppressions: boolean, ): Array { const suppressionRanges: Array = []; @@ -89,7 +89,7 @@ export function findProgramSuppressions( let disableNextLinePattern: RegExp | null = null; let disablePattern: RegExp | null = null; let enablePattern: RegExp | null = null; - if (ruleNames.length !== 0) { + if (ruleNames != null && ruleNames.length !== 0) { const rulePattern = `(${ruleNames.join('|')})`; disableNextLinePattern = new RegExp( `eslint-disable-next-line ${rulePattern}`, diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts index 313f773e294..b3a68fc0134 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts @@ -24,6 +24,7 @@ import { InstructionKind, isStableType, isSubPath, + isSubPathIgnoringOptionals, isUseRefType, LoadGlobal, ManualMemoDependency, @@ -42,20 +43,37 @@ import {retainWhere} from '../Utils/utils'; const DEBUG = false; /** - * Validates that existing manual memoization had exhaustive dependencies. - * Memoization with missing or extra reactive dependencies is invalid React - * and compilation can change behavior, causing a value to be computed more - * or less times. + * Validates that existing manual memoization is exhaustive and does not + * have extraneous dependencies. The goal of the validation is to ensure + * that auto-memoization will not substantially change the behavior of + * the program: + * - If the manual dependencies were non-exhaustive (missing important deps) + * then auto-memoization will include those dependencies, and cause the + * value to update *more* frequently. + * - If the manual dependencies had extraneous deps, then auto memoization + * will remove them and cause the value to update *less* frequently. * - * TODOs: - * - Handle cases of mixed optional and non-optional versions of the same path, - * eg referecing both x.y.z and x.y?.z in the same memo block. we should collapse - * this into a single canonical dep that we look for in the manual deps. see the - * existing exhaustive deps rule for implementation. - * - Handle cases where the user deps were not simple identifiers + property chains. - * We try to detect this in ValidateUseMemo but we miss some cases. The problem - * is that invalid forms can be value blocks or function calls that don't get - * removed by DCE, leaving a structure like: + * We consider a value V as missing if ALL of the following conditions are met: + * - V is reactive + * - There is no manual dependency path P such that whenever V would change, + * P would also change. If V is `x.y.z`, this means there must be some + * path P that is either `x.y.z`, `x.y`, or `x`. Note that we assume no + * interior mutability, such that a shorter path "covers" changes to longer + * more precise paths. + * + * We consider a value V extraneous if either of the folowing are true: + * - V is a reactive local that is unreferenced + * - V is a global that is unreferenced + * + * In other words, we allow extraneous non-reactive values since we know they cannot + * impact how often the memoization would run. + * + * ## TODO: Invalid, Complex Deps + * + * Handle cases where the user deps were not simple identifiers + property chains. + * We try to detect this in ValidateUseMemo but we miss some cases. The problem + * is that invalid forms can be value blocks or function calls that don't get + * removed by DCE, leaving a structure like: * * StartMemoize * t0 = @@ -208,31 +226,9 @@ export function validateExhaustiveDependencies( reason: 'Unexpected function dependency', loc: value.loc, }); - /** - * Dependencies technically only need to include reactive values. However, - * reactivity inference for general values is subtle since it involves all - * of our complex control and data flow analysis. To keep results more - * stable and predictable to developers, we intentionally stay closer to - * the rules of the classic exhaustive-deps rule. Values should be included - * as dependencies if either of the following is true: - * - They're reactive - * - They're non-reactive and not a known-stable value type. - * - * Thus `const ref: Ref = cond ? ref1 : ref2` has to be a dependency - * (assuming `cond` is reactive) since it's reactive despite being a ref. - * - * Similarly, `const x = [1,2,3]` has to be a dependency since even - * though it's non reactive, it's not a known stable type. - * - * TODO: consider reimplementing a simpler form of reactivity inference. - * Ideally we'd consider `const ref: Ref = cond ? ref1 : ref2` as a required - * dependency even if our data/control flow tells us that `cond` is non-reactive. - * It's simpler for developers to reason about based on a more structural/AST - * driven approach. - */ - const isRequiredDependency = - reactive.has(inferredDependency.identifier.id) || - !isStableType(inferredDependency.identifier); + const isRequiredDependency = reactive.has( + inferredDependency.identifier.id, + ); let hasMatchingManualDependency = false; for (const manualDependency of manualDependencies) { if ( @@ -240,7 +236,10 @@ export function validateExhaustiveDependencies( manualDependency.root.value.identifier.id === inferredDependency.identifier.id && (areEqualPaths(manualDependency.path, inferredDependency.path) || - isSubPath(manualDependency.path, inferredDependency.path)) + isSubPathIgnoringOptionals( + manualDependency.path, + inferredDependency.path, + )) ) { hasMatchingManualDependency = true; matched.add(manualDependency); @@ -261,6 +260,15 @@ export function validateExhaustiveDependencies( extra.push(dep); } + /** + * Per docblock, we only consider dependencies as extraneous if + * they are unused globals or reactive locals. Notably, this allows + * non-reactive locals. + */ + retainWhere(extra, dep => { + return dep.root.kind === 'Global' || dep.root.value.reactive; + }); + if (missing.length !== 0 || extra.length !== 0) { let suggestions: Array | null = null; if (startMemo.depsLoc != null && typeof startMemo.depsLoc !== 'symbol') { @@ -276,7 +284,7 @@ export function validateExhaustiveDependencies( if (missing.length !== 0) { const diagnostic = CompilerDiagnostic.create({ category: ErrorCategory.MemoDependencies, - reason: 'Found non-exhaustive dependencies', + reason: 'Found missing memoization dependencies', description: 'Missing dependencies can cause a value not to update when those inputs change, ' + 'resulting in stale UI', @@ -301,7 +309,7 @@ export function validateExhaustiveDependencies( reason: 'Found unnecessary memoization dependencies', description: 'Unnecessary dependencies can cause a value to update more often than necessary, ' + - 'which can cause effects to run more than expected', + 'causing performance regressions and effects to fire more often than expected', }); diagnostic.withDetails({ kind: 'error', diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/compile-files-with-exhaustive-deps-violation-in-effects.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/compile-files-with-exhaustive-deps-violation-in-effects.expect.md new file mode 100644 index 00000000000..e8e18395eca --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/compile-files-with-exhaustive-deps-violation-in-effects.expect.md @@ -0,0 +1,91 @@ + +## Input + +```javascript +// @validateExhaustiveMemoizationDependencies + +import {useMemo} from 'react'; +import {ValidateMemoization} from 'shared-runtime'; + +function Component({x}) { + useEffect( + () => { + console.log(x); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, + [ + /* intentionally missing deps */ + ] + ); + + const memo = useMemo(() => { + return [x]; + }, [x]); + + return ; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateExhaustiveMemoizationDependencies + +import { useMemo } from "react"; +import { ValidateMemoization } from "shared-runtime"; + +function Component(t0) { + const $ = _c(10); + const { x } = t0; + let t1; + if ($[0] !== x) { + t1 = () => { + console.log(x); + }; + $[0] = x; + $[1] = t1; + } else { + t1 = $[1]; + } + let t2; + if ($[2] === Symbol.for("react.memo_cache_sentinel")) { + t2 = []; + $[2] = t2; + } else { + t2 = $[2]; + } + useEffect(t1, t2); + let t3; + if ($[3] !== x) { + t3 = [x]; + $[3] = x; + $[4] = t3; + } else { + t3 = $[4]; + } + const memo = t3; + let t4; + if ($[5] !== x) { + t4 = [x]; + $[5] = x; + $[6] = t4; + } else { + t4 = $[6]; + } + let t5; + if ($[7] !== memo || $[8] !== t4) { + t5 = ; + $[7] = memo; + $[8] = t4; + $[9] = t5; + } else { + t5 = $[9]; + } + return t5; +} + +``` + +### 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/compile-files-with-exhaustive-deps-violation-in-effects.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/compile-files-with-exhaustive-deps-violation-in-effects.js new file mode 100644 index 00000000000..64817e701f0 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/compile-files-with-exhaustive-deps-violation-in-effects.js @@ -0,0 +1,22 @@ +// @validateExhaustiveMemoizationDependencies + +import {useMemo} from 'react'; +import {ValidateMemoization} from 'shared-runtime'; + +function Component({x}) { + useEffect( + () => { + console.log(x); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, + [ + /* intentionally missing deps */ + ] + ); + + const memo = useMemo(() => { + return [x]; + }, [x]); + + return ; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps.expect.md index 5152f4121a2..ed317be118e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps.expect.md @@ -9,23 +9,29 @@ import {Stringify} from 'shared-runtime'; function Component({x, y, z}) { const a = useMemo(() => { return x?.y.z?.a; + // error: too precise }, [x?.y.z?.a.b]); const b = useMemo(() => { return x.y.z?.a; + // ok, not our job to type check nullability }, [x.y.z.a]); const c = useMemo(() => { return x?.y.z.a?.b; + // error: too precise }, [x?.y.z.a?.b.z]); const d = useMemo(() => { return x?.y?.[(console.log(y), z?.b)]; + // ok }, [x?.y, y, z?.b]); const e = useMemo(() => { const e = []; e.push(x); return e; + // ok }, [x]); const f = useMemo(() => { return []; + // error: unnecessary }, [x, y.z, z?.y?.a, UNUSED_GLOBAL]); const ref1 = useRef(null); const ref2 = useRef(null); @@ -34,6 +40,7 @@ function Component({x, y, z}) { return () => { return ref.current; }; + // error: ref is a stable type but reactive }, []); return ; } @@ -44,9 +51,9 @@ function Component({x, y, z}) { ## Error ``` -Found 5 errors: +Found 4 errors: -Error: Found non-exhaustive dependencies +Error: Found missing memoization dependencies Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI. @@ -55,61 +62,48 @@ error.invalid-exhaustive-deps.ts:7:11 6 | const a = useMemo(() => { > 7 | return x?.y.z?.a; | ^^^^^^^^^ Missing dependency `x?.y.z?.a` - 8 | }, [x?.y.z?.a.b]); - 9 | const b = useMemo(() => { - 10 | return x.y.z?.a; + 8 | // error: too precise + 9 | }, [x?.y.z?.a.b]); + 10 | const b = useMemo(() => { -Error: Found non-exhaustive dependencies +Error: Found missing memoization dependencies Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI. -error.invalid-exhaustive-deps.ts:10:11 - 8 | }, [x?.y.z?.a.b]); - 9 | const b = useMemo(() => { -> 10 | return x.y.z?.a; - | ^^^^^^^^ Missing dependency `x.y.z?.a` - 11 | }, [x.y.z.a]); - 12 | const c = useMemo(() => { - 13 | return x?.y.z.a?.b; - -Error: Found non-exhaustive dependencies - -Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI. - -error.invalid-exhaustive-deps.ts:13:11 - 11 | }, [x.y.z.a]); - 12 | const c = useMemo(() => { -> 13 | return x?.y.z.a?.b; +error.invalid-exhaustive-deps.ts:15:11 + 13 | }, [x.y.z.a]); + 14 | const c = useMemo(() => { +> 15 | return x?.y.z.a?.b; | ^^^^^^^^^^^ Missing dependency `x?.y.z.a?.b` - 14 | }, [x?.y.z.a?.b.z]); - 15 | const d = useMemo(() => { - 16 | return x?.y?.[(console.log(y), z?.b)]; + 16 | // error: too precise + 17 | }, [x?.y.z.a?.b.z]); + 18 | const d = useMemo(() => { Error: Found unnecessary memoization dependencies -Unnecessary dependencies can cause a value to update more often than necessary, which can cause effects to run more than expected. +Unnecessary dependencies can cause a value to update more often than necessary, causing performance regressions and effects to fire more often than expected. -error.invalid-exhaustive-deps.ts:25:5 - 23 | const f = useMemo(() => { - 24 | return []; -> 25 | }, [x, y.z, z?.y?.a, UNUSED_GLOBAL]); +error.invalid-exhaustive-deps.ts:31:5 + 29 | return []; + 30 | // error: unnecessary +> 31 | }, [x, y.z, z?.y?.a, UNUSED_GLOBAL]); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Unnecessary dependencies `x`, `y.z`, `z?.y?.a`, `UNUSED_GLOBAL` - 26 | const ref1 = useRef(null); - 27 | const ref2 = useRef(null); - 28 | const ref = z ? ref1 : ref2; + 32 | const ref1 = useRef(null); + 33 | const ref2 = useRef(null); + 34 | const ref = z ? ref1 : ref2; -Error: Found non-exhaustive dependencies +Error: Found missing memoization dependencies Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI. -error.invalid-exhaustive-deps.ts:31:13 - 29 | const cb = useMemo(() => { - 30 | return () => { -> 31 | return ref.current; +error.invalid-exhaustive-deps.ts:37:13 + 35 | const cb = useMemo(() => { + 36 | return () => { +> 37 | return ref.current; | ^^^ Missing dependency `ref`. Refs, setState functions, and other "stable" values generally do not need to be added as dependencies, but this variable may change over time to point to different values - 32 | }; - 33 | }, []); - 34 | return ; + 38 | }; + 39 | // error: ref is a stable type but reactive + 40 | }, []); ``` \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps.js index 53c189cc428..c0f8d28837a 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps.js @@ -5,23 +5,29 @@ import {Stringify} from 'shared-runtime'; function Component({x, y, z}) { const a = useMemo(() => { return x?.y.z?.a; + // error: too precise }, [x?.y.z?.a.b]); const b = useMemo(() => { return x.y.z?.a; + // ok, not our job to type check nullability }, [x.y.z.a]); const c = useMemo(() => { return x?.y.z.a?.b; + // error: too precise }, [x?.y.z.a?.b.z]); const d = useMemo(() => { return x?.y?.[(console.log(y), z?.b)]; + // ok }, [x?.y, y, z?.b]); const e = useMemo(() => { const e = []; e.push(x); return e; + // ok }, [x]); const f = useMemo(() => { return []; + // error: unnecessary }, [x, y.z, z?.y?.a, UNUSED_GLOBAL]); const ref1 = useRef(null); const ref2 = useRef(null); @@ -30,6 +36,7 @@ function Component({x, y, z}) { return () => { return ref.current; }; + // error: ref is a stable type but reactive }, []); return ; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps-allow-nonreactive-stable-types-as-extra-deps.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps-allow-nonreactive-stable-types-as-extra-deps.expect.md new file mode 100644 index 00000000000..5b6319ed7cf --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps-allow-nonreactive-stable-types-as-extra-deps.expect.md @@ -0,0 +1,100 @@ + +## Input + +```javascript +// @validateExhaustiveMemoizationDependencies +import { + useCallback, + useTransition, + useState, + useOptimistic, + useActionState, + useRef, + useReducer, +} from 'react'; + +function useFoo() { + const [s, setState] = useState(); + const ref = useRef(null); + const [t, startTransition] = useTransition(); + const [u, addOptimistic] = useOptimistic(); + const [v, dispatch] = useReducer(() => {}, null); + const [isPending, dispatchAction] = useActionState(() => {}, null); + + return useCallback(() => { + dispatch(); + startTransition(() => {}); + addOptimistic(); + setState(null); + dispatchAction(); + ref.current = true; + }, [ + // intentionally adding unnecessary deps on nonreactive stable values + // to check that they're allowed + dispatch, + startTransition, + addOptimistic, + setState, + dispatchAction, + ref, + ]); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateExhaustiveMemoizationDependencies +import { + useCallback, + useTransition, + useState, + useOptimistic, + useActionState, + useRef, + useReducer, +} from "react"; + +function useFoo() { + const $ = _c(1); + const [, setState] = useState(); + const ref = useRef(null); + const [, startTransition] = useTransition(); + const [, addOptimistic] = useOptimistic(); + const [, dispatch] = useReducer(_temp, null); + const [, dispatchAction] = useActionState(_temp2, null); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = () => { + dispatch(); + startTransition(_temp3); + addOptimistic(); + setState(null); + dispatchAction(); + ref.current = true; + }; + $[0] = t0; + } else { + t0 = $[0]; + } + return t0; +} +function _temp3() {} +function _temp2() {} +function _temp() {} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [], +}; + +``` + +### Eval output +(kind: ok) "[[ function params=0 ]]" \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps-allow-nonreactive-stable-types-as-extra-deps.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps-allow-nonreactive-stable-types-as-extra-deps.js new file mode 100644 index 00000000000..71000afcc42 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps-allow-nonreactive-stable-types-as-extra-deps.js @@ -0,0 +1,42 @@ +// @validateExhaustiveMemoizationDependencies +import { + useCallback, + useTransition, + useState, + useOptimistic, + useActionState, + useRef, + useReducer, +} from 'react'; + +function useFoo() { + const [s, setState] = useState(); + const ref = useRef(null); + const [t, startTransition] = useTransition(); + const [u, addOptimistic] = useOptimistic(); + const [v, dispatch] = useReducer(() => {}, null); + const [isPending, dispatchAction] = useActionState(() => {}, null); + + return useCallback(() => { + dispatch(); + startTransition(() => {}); + addOptimistic(); + setState(null); + dispatchAction(); + ref.current = true; + }, [ + // intentionally adding unnecessary deps on nonreactive stable values + // to check that they're allowed + dispatch, + startTransition, + addOptimistic, + setState, + dispatchAction, + ref, + ]); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps.expect.md index 655d6bff415..70d8a3abbf6 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps.expect.md @@ -3,7 +3,7 @@ ```javascript // @validateExhaustiveMemoizationDependencies -import {useMemo} from 'react'; +import {useCallback, useMemo} from 'react'; import {makeObject_Primitives, Stringify} from 'shared-runtime'; function useHook1(x) { @@ -47,6 +47,16 @@ function useHook6(x) { }, [x]); } +function useHook7(x) { + const [state, setState] = useState(true); + const f = () => { + setState(x => !x); + }; + return useCallback(() => { + f(); + }, [f]); +} + function Component({x, y, z}) { const a = useHook1(x); const b = useHook2(x); @@ -54,7 +64,8 @@ function Component({x, y, z}) { const d = useHook4(x, y, z); const e = useHook5(x); const f = useHook6(x); - return ; + const g = useHook7(x); + return ; } ``` @@ -63,7 +74,7 @@ function Component({x, y, z}) { ```javascript import { c as _c } from "react/compiler-runtime"; // @validateExhaustiveMemoizationDependencies -import { useMemo } from "react"; +import { useCallback, useMemo } from "react"; import { makeObject_Primitives, Stringify } from "shared-runtime"; function useHook1(x) { @@ -121,8 +132,36 @@ function useHook6(x) { return f; } +function useHook7(x) { + const $ = _c(2); + const [, setState] = useState(true); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = () => { + setState(_temp); + }; + $[0] = t0; + } else { + t0 = $[0]; + } + const f = t0; + let t1; + if ($[1] === Symbol.for("react.memo_cache_sentinel")) { + t1 = () => { + f(); + }; + $[1] = t1; + } else { + t1 = $[1]; + } + return t1; +} +function _temp(x_0) { + return !x_0; +} + function Component(t0) { - const $ = _c(7); + const $ = _c(8); const { x, y, z } = t0; const a = useHook1(x); const b = useHook2(x); @@ -130,6 +169,7 @@ function Component(t0) { const d = useHook4(x, y, z); const e = useHook5(x); const f = useHook6(x); + const g = useHook7(x); let t1; if ( $[0] !== a || @@ -137,18 +177,20 @@ function Component(t0) { $[2] !== c || $[3] !== d || $[4] !== e || - $[5] !== f + $[5] !== f || + $[6] !== g ) { - t1 = ; + t1 = ; $[0] = a; $[1] = b; $[2] = c; $[3] = d; $[4] = e; $[5] = f; - $[6] = t1; + $[6] = g; + $[7] = t1; } else { - t1 = $[6]; + t1 = $[7]; } return t1; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps.js index d4c833c7d29..38e730b0d21 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps.js @@ -1,5 +1,5 @@ // @validateExhaustiveMemoizationDependencies -import {useMemo} from 'react'; +import {useCallback, useMemo} from 'react'; import {makeObject_Primitives, Stringify} from 'shared-runtime'; function useHook1(x) { @@ -43,6 +43,16 @@ function useHook6(x) { }, [x]); } +function useHook7(x) { + const [state, setState] = useState(true); + const f = () => { + setState(x => !x); + }; + return useCallback(() => { + f(); + }, [f]); +} + function Component({x, y, z}) { const a = useHook1(x); const b = useHook2(x); @@ -50,5 +60,6 @@ function Component({x, y, z}) { const d = useHook4(x, y, z); const e = useHook5(x); const f = useHook6(x); - return ; + const g = useHook7(x); + return ; } diff --git a/compiler/packages/snap/src/runner.ts b/compiler/packages/snap/src/runner.ts index 92a0a0f82ee..478a32d426c 100644 --- a/compiler/packages/snap/src/runner.ts +++ b/compiler/packages/snap/src/runner.ts @@ -53,8 +53,10 @@ const opts: RunnerOptions = yargs .default('worker-threads', true) .boolean('watch') .describe('watch', 'Run compiler in watch mode, re-running after changes') + .alias('w', 'watch') .default('watch', false) .boolean('update') + .alias('u', 'update') .describe('update', 'Update fixtures') .default('update', false) .boolean('filter') diff --git a/compiler/scripts/enable-feature-flag.js b/compiler/scripts/enable-feature-flag.js new file mode 100755 index 00000000000..f51c5b48417 --- /dev/null +++ b/compiler/scripts/enable-feature-flag.js @@ -0,0 +1,347 @@ +#!/usr/bin/env node +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +'use strict'; + +const fs = require('fs'); +const path = require('path'); +const {execSync} = require('child_process'); +const yargs = require('yargs/yargs'); +const {hideBin} = require('yargs/helpers'); + +// Constants +const COMPILER_ROOT = path.resolve(__dirname, '..'); +const ENVIRONMENT_TS_PATH = path.join( + COMPILER_ROOT, + 'packages/babel-plugin-react-compiler/src/HIR/Environment.ts' +); +const FIXTURES_PATH = path.join( + COMPILER_ROOT, + 'packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler' +); +const FIXTURE_EXTENSIONS = ['.js', '.jsx', '.ts', '.tsx']; + +/** + * Parse command line arguments + */ +function parseArgs() { + const argv = yargs(hideBin(process.argv)) + .usage('Usage: $0 ') + .command('$0 ', 'Enable a feature flag by default', yargs => { + yargs.positional('flag-name', { + describe: 'Name of the feature flag to enable', + type: 'string', + }); + }) + .example( + '$0 validateExhaustiveMemoizationDependencies', + 'Enable the validateExhaustiveMemoizationDependencies flag' + ) + .help('h') + .alias('h', 'help') + .strict() + .parseSync(); + + return argv['flag-name']; +} + +/** + * Enable a feature flag in Environment.ts by changing default(false) to default(true) + */ +function enableFlagInEnvironment(flagName) { + console.log(`\nEnabling flag "${flagName}" in Environment.ts...`); + + const content = fs.readFileSync(ENVIRONMENT_TS_PATH, 'utf8'); + + // Check if the flag exists with default(false) + const flagPatternFalse = new RegExp( + `(${escapeRegex(flagName)}:\\s*z\\.boolean\\(\\)\\.default\\()false(\\))`, + 'g' + ); + + if (!flagPatternFalse.test(content)) { + // Check if flag exists at all + const flagExistsPattern = new RegExp( + `${escapeRegex(flagName)}:\\s*z\\.boolean\\(\\)`, + 'g' + ); + if (flagExistsPattern.test(content)) { + // Check if it's already true + const flagPatternTrue = new RegExp( + `${escapeRegex(flagName)}:\\s*z\\.boolean\\(\\)\\.default\\(true\\)`, + 'g' + ); + if (flagPatternTrue.test(content)) { + console.error(`Error: Flag "${flagName}" already has default(true)`); + process.exit(1); + } + console.error( + `Error: Flag "${flagName}" exists but doesn't have default(false)` + ); + process.exit(1); + } + console.error(`Error: Flag "${flagName}" not found in Environment.ts`); + process.exit(1); + } + + // Perform the replacement + const newContent = content.replace(flagPatternFalse, '$1true$2'); + + // Verify the replacement worked + if (content === newContent) { + console.error(`Error: Failed to replace flag "${flagName}"`); + process.exit(1); + } + + fs.writeFileSync(ENVIRONMENT_TS_PATH, newContent, 'utf8'); + console.log(`Successfully enabled "${flagName}" in Environment.ts`); +} + +/** + * Helper to escape regex special characters + */ +function escapeRegex(string) { + return string.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} + +/** + * Run yarn snap and capture output + */ +function runTests() { + console.log('\nRunning test suite (yarn snap)...'); + + try { + const output = execSync('yarn snap', { + cwd: COMPILER_ROOT, + encoding: 'utf8', + stdio: 'pipe', + maxBuffer: 10 * 1024 * 1024, // 10MB buffer + }); + return {success: true, output}; + } catch (error) { + // yarn snap exits with code 1 when tests fail, which throws an error + return {success: false, output: error.stdout || error.message}; + } +} + +/** + * Parse failing test names from test output + */ +function parseFailingTests(output) { + const failingTests = []; + + // Look for lines that contain "FAIL:" followed by the test name + // Format: "FAIL: test-name" or with ANSI codes + const lines = output.split('\n'); + for (const line of lines) { + // Remove ANSI codes for easier parsing + const cleanLine = line.replace(/\x1b\[[0-9;]*m/g, ''); + + // Match "FAIL: test-name" + const match = cleanLine.match(/^FAIL:\s*(.+)$/); + if (match) { + failingTests.push(match[1].trim()); + } + } + + return failingTests; +} + +/** + * Find the fixture file for a given test name + */ +function findFixtureFile(testName) { + const basePath = path.join(FIXTURES_PATH, testName); + + for (const ext of FIXTURE_EXTENSIONS) { + const filePath = basePath + ext; + if (fs.existsSync(filePath)) { + return filePath; + } + } + + return null; +} + +/** + * Add pragma to disable the feature flag in a fixture file + */ +function addPragmaToFixture(filePath, flagName) { + const content = fs.readFileSync(filePath, 'utf8'); + const lines = content.split('\n'); + + if (lines.length === 0) { + console.warn(`Warning: Empty file ${filePath}`); + return false; + } + + const firstLine = lines[0]; + const pragma = `@${flagName}:false`; + + // Check if pragma already exists + if (firstLine.includes(pragma)) { + return false; // Already has the pragma + } + + // Check if first line is a single-line comment + if (firstLine.trim().startsWith('//')) { + // Append pragma to existing comment + lines[0] = firstLine + ' ' + pragma; + } else if (firstLine.trim().startsWith('/*')) { + // Multi-line comment - insert new line before it + lines.unshift('// ' + pragma); + } else { + // No comment - insert new comment as first line + lines.unshift('// ' + pragma); + } + + fs.writeFileSync(filePath, lines.join('\n'), 'utf8'); + return true; +} + +/** + * Update snapshot files + */ +function updateSnapshots() { + console.log('\nUpdating snapshots (yarn snap -u)...'); + + try { + execSync('yarn snap -u', { + cwd: COMPILER_ROOT, + encoding: 'utf8', + stdio: 'pipe', + maxBuffer: 10 * 1024 * 1024, + }); + console.log('Snapshots updated successfully'); + return true; + } catch (error) { + console.error('Error updating snapshots:', error.message); + return false; + } +} + +/** + * Verify all tests pass + */ +function verifyAllTestsPass() { + console.log('\nRunning final verification (yarn snap)...'); + + const {success, output} = runTests(); + + // Parse summary line: "N Tests, N Passed, N Failed" + const summaryMatch = output.match( + /(\d+)\s+Tests,\s+(\d+)\s+Passed,\s+(\d+)\s+Failed/ + ); + + if (summaryMatch) { + const [, total, passed, failed] = summaryMatch; + console.log( + `\nTest Results: ${total} Tests, ${passed} Passed, ${failed} Failed` + ); + + if (failed === '0') { + console.log('All tests passed!'); + return true; + } else { + console.error(`${failed} tests still failing`); + const failingTests = parseFailingTests(output); + if (failingTests.length > 0) { + console.error('\nFailing tests:'); + failingTests.forEach(test => console.error(` - ${test}`)); + } + return false; + } + } + + return success; +} + +/** + * Main function + */ +async function main() { + const flagName = parseArgs(); + + console.log(`\nEnabling flag: '${flagName}'`); + + try { + // Step 1: Enable flag in Environment.ts + enableFlagInEnvironment(flagName); + + // Step 2: Run tests to find failures + const {output} = runTests(); + const failingTests = parseFailingTests(output); + + console.log(`\nFound ${failingTests.length} failing tests`); + + if (failingTests.length === 0) { + console.log('No failing tests! Feature flag enabled successfully.'); + process.exit(0); + } + + // Step 3: Add pragma to each failing fixture + console.log(`\nAdding '@${flagName}:false' pragma to failing fixtures...`); + + const notFound = []; + let notFoundCount = 0; + + for (const testName of failingTests) { + const fixturePath = findFixtureFile(testName); + + if (!fixturePath) { + console.warn(`Could not find fixture file for: ${testName}`); + notFound.push(fixturePath); + continue; + } + + const updated = addPragmaToFixture(fixturePath, flagName); + if (updated) { + updatedCount++; + console.log(` Updated: ${testName}`); + } + } + + console.log( + `\nSummary: Updated ${updatedCount} fixtures, ${notFoundCount} not found` + ); + + if (notFoundCount.length !== 0) { + console.error( + '\nFailed to update snapshots, could not find:\n' + notFound.join('\n') + ); + process.exit(1); + } + + // Step 4: Update snapshots + if (!updateSnapshots()) { + console.error('\nFailed to update snapshots'); + process.exit(1); + } + + // Step 5: Verify all tests pass + if (!verifyAllTestsPass()) { + console.error('\nVerification failed: Some tests are still failing'); + process.exit(1); + } + + console.log('\nSuccess! Feature flag enabled and all tests passing.'); + console.log(`\nSummary:`); + console.log(` - Enabled "${flagName}" in Environment.ts`); + console.log(` - Updated ${updatedCount} fixture files with pragma`); + console.log(` - All tests passing`); + + process.exit(0); + } catch (error) { + console.error('\nFatal error:', error.message); + console.error(error.stack); + process.exit(1); + } +} + +// Run the script +main();