-
Notifications
You must be signed in to change notification settings - Fork 50.3k
[compiler] Improve impurity/ref tracking #35298
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
josephsavona
wants to merge
1
commit into
main
Choose a base branch
from
pr35298
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
+1,057
−199
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
josephsavona
commented
Dec 5, 2025
josephsavona
commented
Dec 5, 2025
josephsavona
commented
Dec 5, 2025
compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts
Outdated
Show resolved
Hide resolved
josephsavona
commented
Dec 5, 2025
compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts
Show resolved
Hide resolved
josephsavona
commented
Dec 5, 2025
.../src/__tests__/fixtures/compiler/error.invalid-impure-functions-in-render-indirect.expect.md
Show resolved
Hide resolved
josephsavona
commented
Dec 5, 2025
...lugin-react-compiler/src/__tests__/fixtures/compiler/valid-use-impure-value-in-ref.expect.md
Outdated
Show resolved
Hide resolved
f0e4d14 to
2cd48b2
Compare
3ccb2f9 to
9899f74
Compare
note: This implements the idea discussed in reactwg/react#389 (comment) Currently we create `Impure` effects for impure functions like `Date.now()` or `Math.random()`, and then throw if the effect is reachable during render. However, impurity is a property of the resulting value: if the value isn't accessed during render then it's okay: maybe you're console-logging the time while debugging (fine), or storing the impure value into a ref and only accessing it in an effect or event handler (totally ok). This PR updates to validate that impure values are not transitively consumed during render, building on the `Render` effect. The validation currently uses an algorithm very similar to that of InferReactivePlaces - building a set of known impure values, starting with `Impure` effects as the sources and then flowing outward via data flow and mutations. If a transitively impure value reaches a `Render` effect, it's an error. We record both the source of the impure value and where it gets rendered to make it easier to understand and fix errors: ``` Error: Cannot access impure value during render Calling an impure function can produce unstable results that update unpredictably when the component happens to re-render. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#components-and-hooks-must-be-idempotent). error.invalid-impure-functions-in-render-via-render-helper.ts:10:25 8 | const array = makeArray(now); 9 | const hasDate = identity(array); > 10 | return <Bar hasDate={hasDate} />; | ^^^^^^^ Cannot access impure value during render 11 | }; 12 | return <Foo renderItem={renderItem} />; 13 | } error.invalid-impure-functions-in-render-via-render-helper.ts:6:14 4 | 5 | function Component() { > 6 | const now = Date.now(); | ^^^^^^^^^^ `Date.now` is an impure function. 7 | const renderItem = () => { 8 | const array = makeArray(now); 9 | const hasDate = identity(array); ``` Impure values are allowed to flow into refs, meaning that we now allow `useRef(Date.now())` or `useRef(localFunctionThatReturnsMathDotRandom())` which would have errored previously. The next PR reuses this improved impurity tracking to validate ref access in render as well.
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.
note: This implements the idea discussed in https://github.com/reactwg/react/discussions/389#discussioncomment-14252280
Currently we create
Impureeffects for impure functions likeDate.now()orMath.random(), and then throw if the effect is reachable during render. However, impurity is a property of the resulting value: if the value isn't accessed during render then it's okay: maybe you're console-logging the time while debugging (fine), or storing the impure value into a ref and only accessing it in an effect or event handler (totally ok).This PR updates to validate that impure values are not transitively consumed during render, building on the
Rendereffect. The validation currently uses an algorithm very similar to that of InferReactivePlaces - building a set of known impure values, starting withImpureeffects as the sources and then flowing outward via data flow and mutations. If a transitively impure value reaches aRendereffect, it's an error. We record both the source of the impure value and where it gets rendered to make it easier to understand and fix errors:Impure values are allowed to flow into refs, meaning that we now allow
useRef(Date.now())oruseRef(localFunctionThatReturnsMathDotRandom())which would have errored previously. The next PR reuses this improved impurity tracking to validate ref access in render as well.Stack created with Sapling. Best reviewed with ReviewStack.