compiler: Allow mutating refs in callbacks passed to JSX #29733
Merged
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
[ghstack-poisoned]
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
josephsavona
added a commit
that referenced
this pull request
Jun 3, 2024
ghstack-source-id: c534d4943f1433f79afdad7ed2d926589cfdea31 Pull Request resolved: #29733
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: c4b433f...2cfa72f 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
|
Fixes false positives where we currently disallow mutations of refs from callbacks passed to JSX, if the ref is also passed to jsx. We consider these to be mutations of "frozen" values, but refs are explicitly allowed to have interior mutability. The fix is to add a new RefMutation variant to FunctionEffect, which we emit whenever a ref is mutated. We disallow this effect type from within a function body, but allow it within function expressions that are passed to useEffect and friends as well as to jsx. [ghstack-poisoned]
josephsavona
added a commit
that referenced
this pull request
Jun 3, 2024
Fixes false positives where we currently disallow mutations of refs from callbacks passed to JSX, if the ref is also passed to jsx. We consider these to be mutations of "frozen" values, but refs are explicitly allowed to have interior mutability. The fix is to add a new RefMutation variant to FunctionEffect, which we emit whenever a ref is mutated. We disallow this effect type from within a function body, but allow it within function expressions that are passed to useEffect and friends as well as to jsx. ghstack-source-id: 057e505c16be50d16fc1dc12c1e5ed754ace87ed Pull Request resolved: #29733
josephsavona
commented
Jun 3, 2024
Comment on lines
528
to
532
| if ( | ||
| isRefValueType(place.identifier) || | ||
| isUseRefType(place.identifier) | ||
| ) { | ||
| if (this.#env.config.validateRefAccessDuringRender) { |
There was a problem hiding this comment.
Previously refs would have fallen through to the case where the valueKind is mutable, and we would record it as a mutation. Now we always treat refs specially, and either ignore them (default) or record a RefMutation error when ref validation is on.
Ref validation still has a bunch of false positives that we have to fix before we can enable it more broadly. However, the fix here avoids the false negatives even when the validation is off.
Fixes false positives where we currently disallow mutations of refs from callbacks passed to JSX, if the ref is also passed to jsx. We consider these to be mutations of "frozen" values, but refs are explicitly allowed to have interior mutability. The fix is to add a new RefMutation variant to FunctionEffect, which we emit whenever a ref is mutated. We disallow this effect type from within a function body, but allow it within function expressions that are passed to useEffect and friends as well as to jsx. Fixes #29741 Fixes #29703 [ghstack-poisoned]
josephsavona
added a commit
that referenced
this pull request
Jun 7, 2024
Fixes false positives where we currently disallow mutations of refs from callbacks passed to JSX, if the ref is also passed to jsx. We consider these to be mutations of "frozen" values, but refs are explicitly allowed to have interior mutability. The fix is to always allow (at leat within InferReferenceEffects) for refs to be mutated. This means we completely rely on ValidateNoRefAccessInRender to validate ref access and stop reporting false positives. ghstack-source-id: 1a30609f5f7831086077d10dac46ab70687f47e0 Pull Request resolved: #29733
Fixes false positives where we currently disallow mutations of refs from callbacks passed to JSX, if the ref is also passed to jsx. We consider these to be mutations of "frozen" values, but refs are explicitly allowed to have interior mutability. The fix is to always allow (at leat within InferReferenceEffects) for refs to be mutated. This means we completely rely on ValidateNoRefAccessInRender to validate ref access and stop reporting false positives. [ghstack-poisoned]
josephsavona
added a commit
that referenced
this pull request
Jun 7, 2024
Fixes false positives where we currently disallow mutations of refs from callbacks passed to JSX, if the ref is also passed to jsx. We consider these to be mutations of "frozen" values, but refs are explicitly allowed to have interior mutability. The fix is to always allow (at leat within InferReferenceEffects) for refs to be mutated. This means we completely rely on ValidateNoRefAccessInRender to validate ref access and stop reporting false positives. ghstack-source-id: 1a30609f5f7831086077d10dac46ab70687f47e0 Pull Request resolved: #29733
josephsavona
changed the title
|
Heads up that i updated this to remove the RefMutation variant of FunctionEffect, we can keep this simple by just ignoring ref mutations completely in InferReferenceEffects. |
josephsavona
added a commit
that referenced
this pull request
Jun 7, 2024
Fixes false positives where we currently disallow mutations of refs from callbacks passed to JSX, if the ref is also passed to jsx. We consider these to be mutations of "frozen" values, but refs are explicitly allowed to have interior mutability. The fix is to always allow (at leat within InferReferenceEffects) for refs to be mutated. This means we completely rely on ValidateNoRefAccessInRender to validate ref access and stop reporting false positives. ghstack-source-id: 1a30609f5f7831086077d10dac46ab70687f47e0 Pull Request resolved: #29733
This was referenced Jun 7, 2024
github-actions bot
pushed a commit
that referenced
this pull request
Jun 7, 2024
Fixes false positives where we currently disallow mutations of refs from callbacks passed to JSX, if the ref is also passed to jsx. We consider these to be mutations of "frozen" values, but refs are explicitly allowed to have interior mutability. The fix is to always allow (at leat within InferReferenceEffects) for refs to be mutated. This means we completely rely on ValidateNoRefAccessInRender to validate ref access and stop reporting false positives. ghstack-source-id: 1a30609f5f7831086077d10dac46ab70687f47e0 Pull Request resolved: #29733 DiffTrain build for commit 827cbea.
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):
Fixes false positives where we currently disallow mutations of refs from callbacks passed to JSX, if the ref is also passed to jsx. We consider these to be mutations of "frozen" values, but refs are explicitly allowed to have interior mutability. The fix is to always allow (at leat within InferReferenceEffects) for refs to be mutated. This means we completely rely on ValidateNoRefAccessInRender to validate ref access and stop reporting false positives.