[compiler] Fix false positive memo validation #34276
Open
+338
−0
Conversation
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
052f2c9 to
0f632e5
Compare
josephsavona
commented
Aug 23, 2025
| @@ -271,6 +275,13 @@ function validateInferredDep( | |||
| return; | |||
| } | |||
| } | |||
| if ( | |||
| dep.identifier.mutableRange.start > memoStartInstruction && | |||
| !isUnmemoized(dep.identifier, scopes) | |||
There was a problem hiding this comment.
The isUnmemoized() check here is for sanity. I can't construct an example where it wouldn't be memoized, since the dep in this case would be constructed within the useMemo block. The only way to force such a dep to not memoize is if it somehow mixes up with a scope from outside the memo block, in which it would take on a dep that is mutated later which fails a separate validation.
There was a problem hiding this comment.
added a fixture demonstrating that case just to be safe
Partial fix for #34262. Consider this example: ```js function useInputValue(input) { const object = React.useMemo(() => { const {value} = transform(input); return {value}; }, [input]); return object; } ``` React Compiler breaks this code into two reactive scopes: * One for `transform(input)` * One for `{value}` When we run ValidatePreserveExistingMemo, we see that the scope for `{value}` has the dependency `value`, whereas the original memoization had the dependency `input`, and throw an error that the dependencies didn't match. In other words, we're flagging the fact that memoized _better than the user_ as a problem. The more complete solution would be to validate that there is a subgraph of reactive scopes with a single input and output node, where the input node has the same dependencies as the original useMemo, and the output has the same outputs. That is true in this case, with the subgraph being the two consecutive scopes mentioned above. But that's complicated. As a shortcut, this PR checks for any dependencies that are defined after the start of the original useMemo. If we find one, we know that it's a case where we were able to memoize more precisely than the original, and we don't report an error on the dependency. We still check that the original _output_ value is able to be memoized, though. So if the scope of `object` were extended, eg with a call to `mutate(object)`, then we'd still correctly report an error that we couldn't preserve memoization.
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.
Partial fix for #34262. Consider this example:
React Compiler breaks this code into two reactive scopes:
transform(input){value}When we run ValidatePreserveExistingMemo, we see that the scope for
{value}has the dependencyvalue, whereas the original memoization had the dependencyinput, and throw an error that the dependencies didn't match.In other words, we're flagging the fact that memoized better than the user as a problem. The more complete solution would be to validate that there is a subgraph of reactive scopes with a single input and output node, where the input node has the same dependencies as the original useMemo, and the output has the same outputs. That is true in this case, with the subgraph being the two consecutive scopes mentioned above.
But that's complicated. As a shortcut, this PR checks for any dependencies that are defined after the start of the original useMemo. If we find one, we know that it's a case where we were able to memoize more precisely than the original, and we don't report an error on the dependency. We still check that the original output value is able to be memoized, though. So if the scope of
objectwere extended, eg with a call tomutate(object), then we'd still correctly report an error that we couldn't preserve memoization.