-
Notifications
You must be signed in to change notification settings - Fork 49.9k
[compiler] Fix false positive hook return mutation error #34424
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
Conversation
|
Fixes #34418 |
| effect.kind === 'MaybeAlias' || | ||
| initialized.has(effect.into.identifier.id), | ||
| { | ||
| reason: `Expected destination value to already be initialized within this instruction for Alias effect`, |
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.
minor nit, should we update the reason or move into description?
| reason: `Expected destination value to already be initialized within this instruction for Alias effect`, | |
| reason: `Expected destination value to already be initialized within this instruction for ${effect.kind} effect`, |
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.
Yeah fair. I’m less worried about aggregating invariants since they shouldn’t happen but for consistency, yes!
| place: alias, | ||
| transitive, | ||
| direction: 'backwards', | ||
| kind, |
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.
maybe obvious, but do we need to downgrade as well when traversing backwards?
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.
Yup! We already do, it’s a bit further down where we iterate the node.maybeAliases
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.
This was fun. We previously added the MaybeAlias effect in #33984 in order to describe the semantic that an unknown function call _may_ alias its return value in its result, but that we don't know this for sure. We record mutations through MaybeAlias edges when walking backward in the data flow graph, but downgrade them to conditional mutations. See the original PR for full context. That change was sufficient for the original case like ```js const frozen = useContext(); useEffect(() => { frozen.method().property = true; }, [...]); ``` But it wasn't sufficient for cases where the aliasing occured between operands: ```js const dispatch = useDispatch(); <div onClick={(e) => { dispatch(...e.target.value) e.target.value = ...; }} /> ``` Here we would record a `Capture dispatch <- e.target` effect. Then during processing of the `event.target.value = ...` assignment we'd eventually _forward_ from `event` to `dispatch` (along a MaybeAlias edge). But in #33984 I missed that this forward walk also has to downgrade to conditional. In addition to that change, we also have to be a bit more precise about which set of effects we create for alias/capture/maybe-alias. The new logic is a bit clearer, I think: * If the value is frozen, it's an ImmutableCapture edge * If the values are mutable, it's a Capture * If it's a context->context, context->mutable, or mutable->context, count it as MaybeAlias.
8913020 to
8219b34
Compare
This was fun. We previously added the MaybeAlias effect in #33984 in order to describe the semantic that an unknown function call _may_ alias its return value in its result, but that we don't know this for sure. We record mutations through MaybeAlias edges when walking backward in the data flow graph, but downgrade them to conditional mutations. See the original PR for full context. That change was sufficient for the original case like ```js const frozen = useContext(); useEffect(() => { frozen.method().property = true; }, [...]); ``` But it wasn't sufficient for cases where the aliasing occured between operands: ```js const dispatch = useDispatch(); <div onClick={(e) => { dispatch(...e.target.value) e.target.value = ...; }} /> ``` Here we would record a `Capture dispatch <- e.target` effect. Then during processing of the `event.target.value = ...` assignment we'd eventually _forward_ from `event` to `dispatch` (along a MaybeAlias edge). But in #33984 I missed that this forward walk also has to downgrade to conditional. In addition to that change, we also have to be a bit more precise about which set of effects we create for alias/capture/maybe-alias. The new logic is a bit clearer, I think: * If the value is frozen, it's an ImmutableCapture edge * If the values are mutable, it's a Capture * If it's a context->context, context->mutable, or mutable->context, count it as MaybeAlias. DiffTrain build for [acada30](acada30)
This was fun. We previously added the MaybeAlias effect in #33984 in order to describe the semantic that an unknown function call _may_ alias its return value in its result, but that we don't know this for sure. We record mutations through MaybeAlias edges when walking backward in the data flow graph, but downgrade them to conditional mutations. See the original PR for full context. That change was sufficient for the original case like ```js const frozen = useContext(); useEffect(() => { frozen.method().property = true; }, [...]); ``` But it wasn't sufficient for cases where the aliasing occured between operands: ```js const dispatch = useDispatch(); <div onClick={(e) => { dispatch(...e.target.value) e.target.value = ...; }} /> ``` Here we would record a `Capture dispatch <- e.target` effect. Then during processing of the `event.target.value = ...` assignment we'd eventually _forward_ from `event` to `dispatch` (along a MaybeAlias edge). But in #33984 I missed that this forward walk also has to downgrade to conditional. In addition to that change, we also have to be a bit more precise about which set of effects we create for alias/capture/maybe-alias. The new logic is a bit clearer, I think: * If the value is frozen, it's an ImmutableCapture edge * If the values are mutable, it's a Capture * If it's a context->context, context->mutable, or mutable->context, count it as MaybeAlias. DiffTrain build for [acada30](acada30)
Two small QoL improvements inspired by feedback:
* `if (ref.current === undefined) { ref.current = ... }` is now allowed.
* `if (!ref.current) { ref.current = ... }` is still disallowed, but we
emit an extra hint suggesting the `if (!ref.current == null)` pattern.
I was on the fence about the latter. We got feedback asking to allow `if
(!ref.current)` but if your ref stores a boolean value then this would
allow reading the ref in render. The unary form is also less precise in
general due to sketchy truthiness conversions. I figured a hint is a
good compromise.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34449).
* __->__ #34449
* #34424
Two small QoL improvements inspired by feedback:
* `if (ref.current === undefined) { ref.current = ... }` is now allowed.
* `if (!ref.current) { ref.current = ... }` is still disallowed, but we
emit an extra hint suggesting the `if (!ref.current == null)` pattern.
I was on the fence about the latter. We got feedback asking to allow `if
(!ref.current)` but if your ref stores a boolean value then this would
allow reading the ref in render. The unary form is also less precise in
general due to sketchy truthiness conversions. I figured a hint is a
good compromise.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34449).
* __->__ #34449
* #34424
DiffTrain build for [bd9e6e0](bd9e6e0)
Two small QoL improvements inspired by feedback:
* `if (ref.current === undefined) { ref.current = ... }` is now allowed.
* `if (!ref.current) { ref.current = ... }` is still disallowed, but we
emit an extra hint suggesting the `if (!ref.current == null)` pattern.
I was on the fence about the latter. We got feedback asking to allow `if
(!ref.current)` but if your ref stores a boolean value then this would
allow reading the ref in render. The unary form is also less precise in
general due to sketchy truthiness conversions. I figured a hint is a
good compromise.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34449).
* __->__ #34449
* #34424
DiffTrain build for [bd9e6e0](bd9e6e0)
) Two small QoL improvements inspired by feedback: * `if (ref.current === undefined) { ref.current = ... }` is now allowed. * `if (!ref.current) { ref.current = ... }` is still disallowed, but we emit an extra hint suggesting the `if (!ref.current == null)` pattern. I was on the fence about the latter. We got feedback asking to allow `if (!ref.current)` but if your ref stores a boolean value then this would allow reading the ref in render. The unary form is also less precise in general due to sketchy truthiness conversions. I figured a hint is a good compromise. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34449). * __->__ facebook#34449 * facebook#34424 DiffTrain build for [bd9e6e0](facebook@bd9e6e0)
) Two small QoL improvements inspired by feedback: * `if (ref.current === undefined) { ref.current = ... }` is now allowed. * `if (!ref.current) { ref.current = ... }` is still disallowed, but we emit an extra hint suggesting the `if (!ref.current == null)` pattern. I was on the fence about the latter. We got feedback asking to allow `if (!ref.current)` but if your ref stores a boolean value then this would allow reading the ref in render. The unary form is also less precise in general due to sketchy truthiness conversions. I figured a hint is a good compromise. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34449). * __->__ facebook#34449 * facebook#34424 DiffTrain build for [bd9e6e0](facebook@bd9e6e0)
This was fun. We previously added the MaybeAlias effect in #33984 in order to describe the semantic that an unknown function call may alias its return value in its result, but that we don't know this for sure. We record mutations through MaybeAlias edges when walking backward in the data flow graph, but downgrade them to conditional mutations. See the original PR for full context.
That change was sufficient for the original case like
But it wasn't sufficient for cases where the aliasing occured between operands:
Here we would record a
Capture dispatch <- e.targeteffect. Then during processing of theevent.target.value = ...assignment we'd eventually forward fromeventtodispatch(along a MaybeAlias edge). But in #33984 I missed that this forward walk also has to downgrade to conditional.In addition to that change, we also have to be a bit more precise about which set of effects we create for alias/capture/maybe-alias. The new logic is a bit clearer, I think: