Skip to content

[compiler] More flexible/helpful lazy ref initialization #34449

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.

Sign up for GitHub

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

Jump to bottom
Merged
josephsavona merged 2 commits into from
Sep 10, 2025
Merged

[compiler] More flexible/helpful lazy ref initialization #34449

josephsavona merged 2 commits into from
Sep 10, 2025

Conversation

@josephsavona
Copy link
Member

@josephsavona josephsavona commented Sep 10, 2025 •
edited
Loading

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.

Stack created with Sapling. Best reviewed with ReviewStack.

@josephsavona
[compiler] Fix false positive hook return mutation error
8219b34 
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.
@meta-cla meta-cla bot added the CLA Signed label Sep 10, 2025
@github-actions github-actions bot added the React Core Team Opened by a member of the React Core Team label Sep 10, 2025
@josephsavona
[compiler] More flexible/helpful lazy ref initialization
5eb09a7 
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.
mvitousek
mvitousek approved these changes Sep 10, 2025
View reviewed changes
Copy link
Contributor

@mvitousek mvitousek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely like erroring with a suggestion as the right solution here!

@josephsavona josephsavona merged commit bd9e6e0 into main Sep 10, 2025
21 checks passed
github-actions bot pushed a commit that referenced this pull request Sep 10, 2025
@josephsavona
[compiler] More flexible/helpful lazy ref initialization (#34449)
6e1b4fd 
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)
github-actions bot pushed a commit that referenced this pull request Sep 10, 2025
@josephsavona
[compiler] More flexible/helpful lazy ref initialization (#34449)
fed0666 
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)
bpinto
bpinto reviewed Sep 10, 2025
View reviewed changes
...-react-compiler/src/__tests__/fixtures/compiler/allow-ref-initialization-undefined.expect.md

component C() {
const r = useRef(null);
if (r.current == undefined) {
Copy link

@bpinto bpinto Sep 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it correct to compare with an undefined value when useRef was initialized with null?

Copy link
Member Author

@josephsavona josephsavona Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, but it’s valid if you initialized w undefined or no initializer argument. We could try to be more precise and validate that you’re comparing exactly against the initializer value, but this feels close enough.

@nextjs-bot nextjs-bot mentioned this pull request Sep 11, 2025
Merged
github-actions bot pushed a commit to code/lib-react that referenced this pull request Sep 13, 2025
@pull
[compiler] More flexible/helpful lazy ref initialization (facebook#34449
53cb662 
)

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)
github-actions bot pushed a commit to code/lib-react that referenced this pull request Sep 13, 2025
@pull
[compiler] More flexible/helpful lazy ref initialization (facebook#34449
eac8015 
)

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 referenced Sep 17, 2025
Closed
Closed
@celicoo celicoo mentioned this pull request Sep 23, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mvitousek mvitousek mvitousek approved these changes

@poteto poteto Awaiting requested review from poteto

+1 more reviewer

@bpinto bpinto bpinto left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CLA Signed React Core Team Opened by a member of the React Core Team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@josephsavona @bpinto @mvitousek