-
Notifications
You must be signed in to change notification settings - Fork 50k
[compiler][poc] 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
base: main
Are you sure you want to change the base?
Conversation
| // Render the arguments | ||
| { | ||
| kind: 'Render', | ||
| place: '@rest', | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to add the Render effect
| ], | ||
| }); | ||
|
|
||
| const EffectHookAliasing: AliasingSignatureConfig = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extracting from useEffect so we can reuse for insertion/layout effects too
compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts
Outdated
Show resolved
Hide resolved
| if (prop.kind === 'JsxAttribute' && /^on[A-Z]/.test(prop.name)) { | ||
| continue; | ||
| } | ||
| effects.push({ | ||
| kind: 'Render', | ||
| place: prop.kind === 'JsxAttribute' ? prop.place : prop.argument, | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider all props except obvious event handlers as having a render effect, but InferMutationAliasingRanges is smarter about how it applies render effects to functions
.../src/__tests__/fixtures/compiler/error.invalid-impure-functions-in-render-indirect.expect.md
Outdated
Show resolved
Hide resolved
| const f = () => Math.random(); | ||
| const ref = useRef(f()); | ||
| return <div ref={ref} nonRef={nonRef} state={state} setState={setState} />; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs cleanup but yay, allowing using an impure value in a ref
d7beb75 to
6ab3ca1
Compare
We currently model impure functions like `Date.now()` with an `Impure` effect, but the way this works is that the effect is considered an error if it's called at all. But we really want to reflect the fact that the _value_ is impure, and is okay to be used outside of render. For example, `useRef(Date.now())` should be fine.
So this PR changes the Impure effect to describe that an impure value flows `into: Place`. We flow impurity through the data flow graph during InferMutationAliasingRanges, including propagating this to the function's effects. So if a function expression returns a transitively impure value, that's expressed in our inferred signature for that function. Calling it propagates the impurity via our effect system.
We stop this propagation when reaching a ref, allowing `useRef(Date.now())` or `useRef(localFunctionThatReturnsDateNow())`.
This lets us also model accessing a ref as an impure value - we just emit an `Impure` event for PropertyLoad/ComputedLoad off of a ref, and the above tracking kicks in.
A final piece is that we can also use our existing machinery to disallow writing to global values during render to handle writing to refs - except that we need to allow the lazy init pattern. So it probably makes sense to keep the ref validation pass, but scope it back to just handling writes and not reads.
This definitely needs more polish, the error messages would ideally be something like:
```
Error: cannot call impure function in render
Blah blah description of why this is bad...
foo.js:0:0
<Foo x={x} />
^ This value reads from an impure function
foo.js:0:0
x = Date.now();
^^^^^^^^ The value derives from Date.now which is impure
```
Ie show you both the local site (where impurity flows into render) and the ultiamte source of the impure value.
We currently model impure functions like
Date.now()with anImpureeffect, but the way this works is that the effect is considered an error if it's called at all. But we really want to reflect the fact that the value is impure, and is okay to be used outside of render. For example,useRef(Date.now())should be fine.So this PR changes the Impure effect to describe that an impure value flows
into: Place. We flow impurity through the data flow graph during InferMutationAliasingRanges, including propagating this to the function's effects. So if a function expression returns a transitively impure value, that's expressed in our inferred signature for that function. Calling it propagates the impurity via our effect system.We stop this propagation when reaching a ref, allowing
useRef(Date.now())oruseRef(localFunctionThatReturnsDateNow()).This lets us also model accessing a ref as an impure value - we just emit an
Impureevent for PropertyLoad/ComputedLoad off of a ref, and the above tracking kicks in.A final piece is that we can also use our existing machinery to disallow writing to global values during render to handle writing to refs - except that we need to allow the lazy init pattern. So it probably makes sense to keep the ref validation pass, but scope it back to just handling writes and not reads.
This definitely needs more polish, the error messages would ideally be something like:
Ie show you both the local site (where impurity flows into render) and the ultiamte source of the impure value.