compiler: fix accidental propagation of function effects from StartMemoize/FinishMemoize #29154
Conversation
This file contains 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
…moize/FinishMemoize By default, React Compiler will skip compilation if it cannot preserve existing memoization. Ie, if the code has an existing `useMemo()` or `useCallback()` and the compiler cannot determine that it is safe to keep that memoization — or do even better — then we'll leave the code alone. The actual compilation doesn't use any hints from existing memo calls, this is purely to check and avoid regressing any specific memoization that developers may have already applied. However, we were accidentally reporting some false-positive _validation_ errors due to the StartMemoize and FinishMemoize instructions that we emit to track where the memoization was in the source code. This is now fixed. Fixes #29131 Fixes #29132 [ghstack-poisoned]
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
josephsavona
added a commit
that referenced
this pull request
May 17, 2024
…moize/FinishMemoize By default, React Compiler will skip compilation if it cannot preserve existing memoization. Ie, if the code has an existing `useMemo()` or `useCallback()` and the compiler cannot determine that it is safe to keep that memoization — or do even better — then we'll leave the code alone. The actual compilation doesn't use any hints from existing memo calls, this is purely to check and avoid regressing any specific memoization that developers may have already applied. However, we were accidentally reporting some false-positive _validation_ errors due to the StartMemoize and FinishMemoize instructions that we emit to track where the memoization was in the source code. This is now fixed. Fixes #29131 Fixes #29132 ghstack-source-id: 9f6b8dbc5074ccc96e6073cf11c4920b5375faf6 Pull Request resolved: #29154
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: 1d6eebf...8210d3b Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
mvitousek
approved these changes
May 18, 2024
There was a problem hiding this comment.
Tricky! If I understand it right, the issue is that we see a StartMemo instruction containing dependency values as triggering the effects of those dependencies to fire, similarly to if we'd passed them as args or propagated them somewhere else, is that right? But StartMemoize is a "virtual" instruction that doesn't correspond to any emitted code, so it shouldn't fire any such effects.
If we end up adding any further virtual instructions, will need to be careful not to repeat this issue!
josephsavona
added a commit
that referenced
this pull request
May 20, 2024
…moize/FinishMemoize By default, React Compiler will skip compilation if it cannot preserve existing memoization. Ie, if the code has an existing `useMemo()` or `useCallback()` and the compiler cannot determine that it is safe to keep that memoization — or do even better — then we'll leave the code alone. The actual compilation doesn't use any hints from existing memo calls, this is purely to check and avoid regressing any specific memoization that developers may have already applied. However, we were accidentally reporting some false-positive _validation_ errors due to the StartMemoize and FinishMemoize instructions that we emit to track where the memoization was in the source code. This is now fixed. Fixes #29131 Fixes #29132 ghstack-source-id: 9f6b8dbc5074ccc96e6073cf11c4920b5375faf6 Pull Request resolved: #29154
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.
Stack from ghstack (oldest at bottom):
By default, React Compiler will skip compilation if it cannot preserve existing memoization. Ie, if the code has an existing
useMemo()oruseCallback()and the compiler cannot determine that it is safe to keep that memoization — or do even better — then we'll leave the code alone. The actual compilation doesn't use any hints from existing memo calls, this is purely to check and avoid regressing any specific memoization that developers may have already applied.However, we were accidentally reporting some false-positive validation errors due to the StartMemoize and FinishMemoize instructions that we emit to track where the memoization was in the source code. This is now fixed.
Fixes #29131
Fixes #29132