[compiler] fix: preserve-manual-memoization validation should not require stable (non-reactive) values in deps#36397
[compiler] fix: preserve-manual-memoization validation should not require stable (non-reactive) values in deps#36397sleitor wants to merge 2 commits intofacebook:mainfrom
Conversation
…uire stable (non-reactive) values in deps State setters (from useState), dispatch functions (from useReducer), startTransition, and other stable values guaranteed not to change identity across renders should not be required in manual dependency arrays. The Rules of React and the exhaustive-deps lint rule explicitly exempt stable values from dependency lists. However, ValidatePreservedManualMemoization's validateInferredDep was checking ALL inferred dependencies against the user-provided source deps list, including stable non-reactive values. This caused the compiler to skip optimizing components that correctly omitted stable values like state setters from their useCallback/useMemo dependency arrays. Fix: skip the validateInferredDep check when dep.reactive is false AND the identifier is a stable type (isStableType). We gate on !dep.reactive to preserve the existing behavior for edge cases like `const ref = cond ? ref1 : ref2` where a stable-typed identifier happens to be reactive because its value depends on a reactive condition. Fixes: facebook#36384
|
Thank you for contributing this PR. As the reporter of the bug, please allow me to share my understanding of this fix in relation to the reported bug. I apologize if any of the below is incorrect, if so please feel free to correct any inaccuracy. The current version of the compiler (without this PR applied) currently does not generally infer state setters etc. as dependencies, except in the very narrow edge case demonstrated in #36384, which requires multiple specific unrelated lines of code to trigger the compiler error. So the bug reported in #36384 is that the state setter is being inferred as a dependency in that narrow edge case, even though it should not. This PR does not address the fact the state setter is being inferred to be a dependency, instead it simply ignores any inferred dependency which happens to be a state setter (or other stable value). I.e. it is dealing with this bug after the fact rather than addressing its root cause. Note that the test case added with this PR does NOT fail in the current version of the compiler (without this PR applied), as can be seen here, meaning that it doesn't test what is being fixed by this PR. Ideally the test case would fail in the current version of the compiler but pass with this PR applied. |
|
@laug Thank you for the careful analysis — you're completely right, and I apologize for submitting an incomplete regression test. My added test ( What the fix does: When What's missing: An error fixture that replicates the narrow edge case from issue #36384 — where the compiler actually does infer I'll work on creating that fixture. The issue's playground shows the exact triggering pattern involves a custom hook returning a state setter + specific surrounding code. If you or the original reporter can share the minimal reproduction that I can convert directly into an I'll update the PR shortly with a proper regression test. |
|
The issue's playground is already the smallest repro I was able to come up with, so please use that |
|
@laug Thank you for clarifying! I'll use the playground directly as the basis for the regression fixture — that's the most faithful way to capture the exact code pattern that triggers the narrow edge case. Will add a proper error fixture shortly. |
The previous test (useCallback-state-setter-stable-dep.ts) did not actually trigger the bug — it compiled successfully even without the fix because the simple pattern doesn't cause the compiler to include state setters in inferred scope deps. This adds a fixture using the exact pattern from the issue playground (JSON.parse of an external value as initial state + useCallback with empty deps that uses a state setter). This pattern DOES trigger the compiler to incorrectly infer setImages as a required dependency, causing it to skip optimization — the bug this PR fixes. Without the fix in the previous commit, this fixture would produce: Compilation Skipped: Existing memoization could not be preserved. The inferred dependency was `setImages`, but the source dependencies were [].
|
@laug Done — added a proper regression fixture based on the exact playground code: This uses the specific pattern from the issue playground (external
With the fix applied, it compiles successfully. The previous test ( |
|
Thanks. As state setter functions should not be inferred as dependencies in the first place, are you able to investigate and fix the code such that even in that test case, the setImages state setter function does not get inferred by the compiler as a dependency? |
Summary
validatePreservedManualMemoization(run whenvalidatePreserveExistingMemoizationGuaranteesis enabled) was incorrectly requiring state setters, dispatch functions, and other stable values to appear in the manual dependency array ofuseCallback/useMemocalls.The Rules of React explicitly state that stable values (state setters returned by
useState, dispatch returned byuseReducer,startTransition, refs, etc.) do not need to be listed in dependency arrays — they are guaranteed to never change identity across renders. Theeslint-plugin-react-hooks/exhaustive-depsrule already implements this exemption.Root cause
ValidatePreservedManualMemoization.validateInferredDepchecked ALL inferred scope dependencies against the user-provided source deps list, with no exemption for stable values. When the compiler inferred that a state setter likesetImageswas a dependency of the memoized scope, it would produce:This caused the compiler to bail out on components that correctly omitted state setters from their
useCallback/useMemodeps arrays.Fix
Add an early return in
validateInferredDepwhen!dep.reactive && isStableType(dep.identifier):!dep.reactiveguard ensures we still validate reactive identifiers that happen to have a stable type — e.g.const ref = cond ? ref1 : ref2(where the identity changes reactively).isStableTypecoversSetState,SetActionState, dispatch,useRef,startTransition, andsetOptimistic.This mirrors the logic in
ValidateExhaustiveDependencies.isOptionalDependencywhich already applies the same exemption.How did you test this change?
preserve-memo-validation/useCallback-state-setter-stable-depwhich exercises a component usingsetProcessedandsetCount(state setters) insideuseCallbackwith an empty deps array.preserve-memo-validation/error.preserve-use-memo-ref-missing-reactive(reactive conditional ref still required in deps) continues to fail as expected.Fixes #36384