[compiler] More complete validation against locals being reassigned after render #30540
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
…fter render Summary: This diff extends the existing work on validating against locals being reassigned after render, by propagating the reassignment "effect" into the lvalues of instructions when the rvalue operands include values known to cause reassignments. In particular, this "closes the loop" for function definitions and function calls: a function that returns a function that reassigns will be considered to also perform reassignments, but previous to this we didn't consider the result of a `Call` of a function that reassigns to itself be a value that reassigns. This causes a number of new bailouts in test cases, all of which appear to me to be legit. [ghstack-poisoned]
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
mvitousek
added a commit
that referenced
this pull request
Jul 30, 2024
…fter render Summary: This diff extends the existing work on validating against locals being reassigned after render, by propagating the reassignment "effect" into the lvalues of instructions when the rvalue operands include values known to cause reassignments. In particular, this "closes the loop" for function definitions and function calls: a function that returns a function that reassigns will be considered to also perform reassignments, but previous to this we didn't consider the result of a `Call` of a function that reassigns to itself be a value that reassigns. This causes a number of new bailouts in test cases, all of which appear to me to be legit. ghstack-source-id: b3fe541 Pull Request resolved: #30540
Comment on lines
41
to
45
| | ^ InvalidReact: Reassigning a variable after render has completed can cause inconsistent behavior on subsequent renders. Consider using state instead. Variable `y` cannot be reassigned after render (14:14) | ||
| 15 | return <span key={item.id}>{item.text}</span>; | ||
| 16 | })} | ||
| 17 | {mutate(y)} | ||
| ``` |
There was a problem hiding this comment.
hmmm this case is legit since we know x is an array and that the .map is executed synchronously. we have an existing notion of functions where the arguments don't escape (noAlias) which is set for Array.prototype.map.
I think we could check that here and allow reassigning functions in the case where noAlias is set on the callee/method. But also...reassigning in a map like this is pretty weird and it's probably fine to disallow it.
There was a problem hiding this comment.
Good point, and even if it's sketchy might as well be permissive when we already have the tools to do so safely. Updated!
…eassigned after render" Summary: This diff extends the existing work on validating against locals being reassigned after render, by propagating the reassignment "effect" into the lvalues of instructions when the rvalue operands include values known to cause reassignments. In particular, this "closes the loop" for function definitions and function calls: a function that returns a function that reassigns will be considered to also perform reassignments, but previous to this we didn't consider the result of a `Call` of a function that reassigns to itself be a value that reassigns. This causes a number of new bailouts in test cases, all of which appear to me to be legit. [ghstack-poisoned]
mvitousek
added a commit
that referenced
this pull request
Jul 31, 2024
…fter render Summary: This diff extends the existing work on validating against locals being reassigned after render, by propagating the reassignment "effect" into the lvalues of instructions when the rvalue operands include values known to cause reassignments. In particular, this "closes the loop" for function definitions and function calls: a function that returns a function that reassigns will be considered to also perform reassignments, but previous to this we didn't consider the result of a `Call` of a function that reassigns to itself be a value that reassigns. This causes a number of new bailouts in test cases, all of which appear to me to be legit. ghstack-source-id: 770bf02 Pull Request resolved: #30540
mvitousek
added a commit
that referenced
this pull request
Jul 31, 2024
…fter render Summary: This diff extends the existing work on validating against locals being reassigned after render, by propagating the reassignment "effect" into the lvalues of instructions when the rvalue operands include values known to cause reassignments. In particular, this "closes the loop" for function definitions and function calls: a function that returns a function that reassigns will be considered to also perform reassignments, but previous to this we didn't consider the result of a `Call` of a function that reassigns to itself be a value that reassigns. This causes a number of new bailouts in test cases, all of which appear to me to be legit. ghstack-source-id: 770bf02 Pull Request resolved: #30540
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):
Summary:
This diff extends the existing work on validating against locals being reassigned after render, by propagating the reassignment "effect" into the lvalues of instructions when the rvalue operands include values known to cause reassignments. In particular, this "closes the loop" for function definitions and function calls: a function that returns a function that reassigns will be considered to also perform reassignments, but previous to this we didn't consider the result of a
Callof a function that reassigns to itself be a value that reassigns.This causes a number of new bailouts in test cases, all of which appear to me to be legit.