[compiler] Distingush optional/extraneous deps#35204
Merged
josephsavona merged 1 commit intomainfrom Nov 25, 2025
Merged
Conversation
github-actions
bot
added
the
React Core Team
josephsavona
force-pushed
the
pr35204
branch
2 times, most recently
from
842865e to
6bb6f89
Compare
In ValidateExhaustiveDependencies, I previously changed to allow extraneous dependencies as long as they were non-reactive. Here we make that more precise, and distinguish between values that are definitely referenced in the memo function but optional as dependencies vs values that are not even referenced in the memo function. The latter now error as extraneous even if they're non-reactive. This also turned up a case where constant-folded primitives could show up as false positives of the latter category, so now we track manual deps which quality for constant folding and don't error on them.
josephsavona
force-pushed
the
pr35204
branch
from
6bb6f89 to
87af9cd
Compare
josephsavona
commented
Nov 25, 2025
| if ($[0] === Symbol.for("react.memo_cache_sentinel")) { | ||
| Component = Stringify; | ||
|
|
||
| Component; |
Member
Author
There was a problem hiding this comment.
looks like an existing bug where DCE isn't cleaning this up
mofeiZ
reviewed
Nov 25, 2025
Comment on lines
+244
to
+249
| const isOptionalDependency = | ||
| !reactive.has(inferredDependency.identifier.id) && | ||
| (isStableType(inferredDependency.identifier) || | ||
| isPrimitiveType(inferredDependency.identifier)); | ||
| if (hasMatchingManualDependency || isOptionalDependency) { | ||
| continue; |
Contributor
There was a problem hiding this comment.
Nice, this switches us back to special casing non-reactive deps allowed by exhaustive-deps rule.
This code should now bail out (right)?
hook useHook() {
const obj = {};
return useMemo(() => obj, []);
}
Member
Author
There was a problem hiding this comment.
tested in a follow-up
mofeiZ
reviewed
Nov 25, 2025
Comment on lines
+267
to
+270
| /* | ||
| * Constant primitives can get constant-folded, which means we won't | ||
| * see a LoadLocal for the value within the memo function. | ||
| */ |
Contributor
There was a problem hiding this comment.
makes sense, thanks for the explanation
mofeiZ
reviewed
Nov 25, 2025
| const x = useMemo(() => { | ||
| return [state]; | ||
| // error: `setState` is a stable type, but not actually referenced | ||
| }, [state, setState]); |
mofeiZ
approved these changes
Nov 25, 2025
Contributor
mofeiZ
left a comment
mofeiZ
left a comment
There was a problem hiding this comment.
Makes sense! Could we add something like this as a fixture?
// error.fixture
hook useHook() {
const obj = {};
return useMemo(() => obj, []);
}
// error.fixture2
hook useHook() {
// here, obj goes unmemoized
const obj = {};
useHook();
obj.x = 4;
return useMemo(() => obj, []);
}
This was referenced Dec 2, 2025
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
2 participants
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.
In ValidateExhaustiveDependencies, I previously changed to allow extraneous dependencies as long as they were non-reactive. Here we make that more precise, and distinguish between values that are definitely referenced in the memo function but optional as dependencies vs values that are not even referenced in the memo function. The latter now error as extraneous even if they're non-reactive. This also turned up a case where constant-folded primitives could show up as false positives of the latter category, so now we track manual deps which quality for constant folding and don't error on them.
Stack created with Sapling. Best reviewed with ReviewStack.