[compiler] fix: preserve-manual-memoization validation should not require stable (non-reactive) values in deps#36397
Open
sleitor wants to merge 1 commit intofacebook:mainfrom
Open
[compiler] fix: preserve-manual-memoization validation should not require stable (non-reactive) values in deps#36397sleitor wants to merge 1 commit intofacebook:mainfrom
sleitor wants to merge 1 commit 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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