Skip to content

[compiler] Prevent local state source variables from depending on other state #35044

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
merged 1 commit into from
Nov 10, 2025
Merged

[compiler] Prevent local state source variables from depending on other state #35044

merged 1 commit into from
Nov 10, 2025
+85 −2

Conversation

@jorge-cab
Copy link
Contributor

@jorge-cab jorge-cab commented Nov 4, 2025 •
edited
Loading

Summary:
When a local state is created sometimes it uses a prop or even other local state for its initial value.

This value is only relevant on first render so we shouldn't consider it part of our data flow

Test Plan:
Added tests

This was referenced Nov 4, 2025
Merged
Merged
Merged
Merged
Merged
@meta-cla meta-cla bot added the CLA Signed label Nov 4, 2025
mofeiZ
mofeiZ reviewed Nov 6, 2025
View reviewed changes
...compiler/effect-derived-computations/usestate-derived-from-prop-no-show-in-data-flow-tree.js
@@ -0,0 +1,18 @@
// @validateNoDerivedComputationsInEffects_exp
Copy link
Contributor

@mofeiZ mofeiZ Nov 6, 2025

Choose a reason for hiding this comment

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

@loggerTestOnly might be missing

mofeiZ
mofeiZ approved these changes Nov 6, 2025
View reviewed changes
Copy link
Contributor

@mofeiZ mofeiZ left a comment

Choose a reason for hiding this comment

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

shipit!

jorge-cab added a commit that referenced this pull request Nov 10, 2025
@jorge-cab
[compiler] Prevent overriding a derivationEntry on effect mutation an…
01fb328 
…d instead update typeOfValue and fix infinite loops (#34967)

Summary:
With this we are now comparing a snapshot of the derivationCache with
the new changes every time we are done recording the derivations
happening in the HIR.

We have to do this after recording everything since we still do some
mutations on the cache when recording mutations.



Test Plan:
Test the following in playground:
```
// @validateNoDerivedComputationsInEffects_exp

function Component({ value }) {
  const [checked, setChecked] = useState('');

  useEffect(() => {
    setChecked(value === '' ? [] : value.split(','));
  }, [value]);

  return (
    <div>{checked}</div>
  )
}
```

This no longer causes an infinite loop.

Added a test case in the next PR in the stack

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34967).
* #35044
* #35020
* #34973
* #34972
* #34995
* __->__ #34967
jorge-cab added a commit that referenced this pull request Nov 10, 2025
@jorge-cab
[compiler] Fix false negatives and add data flow tree to compiler err…
6347c6d 
…or for `no-deriving-state-in-effects` (#34995)

Summary:
Revamped the derivationCache graph.

This fixes a bunch of bugs where sometimes we fail to track from which
props/state we derived values from.

Also, it is more intuitive and allows us to easily implement a Data Flow
Tree.

We can print this tree which gives insight on how the data is derived
and should facilitate error resolution in complicated components

Test Plan:
Added a test case where we were failing to track derivations. Also
updated the test cases with the new error containing the data flow tree

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34995).
* #35044
* #35020
* #34973
* #34972
* __->__ #34995
* #34967
github-actions bot pushed a commit that referenced this pull request Nov 10, 2025
@jorge-cab
[compiler] Prevent overriding a derivationEntry on effect mutation an…
af0a333 
…d instead update typeOfValue and fix infinite loops (#34967)

Summary:
With this we are now comparing a snapshot of the derivationCache with
the new changes every time we are done recording the derivations
happening in the HIR.

We have to do this after recording everything since we still do some
mutations on the cache when recording mutations.

Test Plan:
Test the following in playground:
```
// @validateNoDerivedComputationsInEffects_exp

function Component({ value }) {
  const [checked, setChecked] = useState('');

  useEffect(() => {
    setChecked(value === '' ? [] : value.split(','));
  }, [value]);

  return (
    <div>{checked}</div>
  )
}
```

This no longer causes an infinite loop.

Added a test case in the next PR in the stack

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34967).
* #35044
* #35020
* #34973
* #34972
* #34995
* __->__ #34967

DiffTrain build for [01fb328](01fb328)
github-actions bot pushed a commit that referenced this pull request Nov 10, 2025
@jorge-cab
[compiler] Prevent overriding a derivationEntry on effect mutation an…
66d8807 
…d instead update typeOfValue and fix infinite loops (#34967)

Summary:
With this we are now comparing a snapshot of the derivationCache with
the new changes every time we are done recording the derivations
happening in the HIR.

We have to do this after recording everything since we still do some
mutations on the cache when recording mutations.

Test Plan:
Test the following in playground:
```
// @validateNoDerivedComputationsInEffects_exp

function Component({ value }) {
  const [checked, setChecked] = useState('');

  useEffect(() => {
    setChecked(value === '' ? [] : value.split(','));
  }, [value]);

  return (
    <div>{checked}</div>
  )
}
```

This no longer causes an infinite loop.

Added a test case in the next PR in the stack

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34967).
* #35044
* #35020
* #34973
* #34972
* #34995
* __->__ #34967

DiffTrain build for [01fb328](01fb328)
github-actions bot pushed a commit that referenced this pull request Nov 10, 2025
@jorge-cab
[compiler] Fix false negatives and add data flow tree to compiler err…
46c72f4 
…or for `no-deriving-state-in-effects` (#34995)

Summary:
Revamped the derivationCache graph.

This fixes a bunch of bugs where sometimes we fail to track from which
props/state we derived values from.

Also, it is more intuitive and allows us to easily implement a Data Flow
Tree.

We can print this tree which gives insight on how the data is derived
and should facilitate error resolution in complicated components

Test Plan:
Added a test case where we were failing to track derivations. Also
updated the test cases with the new error containing the data flow tree

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34995).
* #35044
* #35020
* #34973
* #34972
* __->__ #34995
* #34967

DiffTrain build for [6347c6d](6347c6d)
jorge-cab added a commit that referenced this pull request Nov 10, 2025
@jorge-cab
[compiler] Update ValidateNoDerivedComputationsInEffects_exp to log t…
7296120 
…he error instead of throwing (#34972)

Summary:
TSIA

Simple change to log errors in Pipeline.ts instead of throwing in the
validation

Test Plan:
updated snap tests

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34972).
* #35044
* #35020
* #34973
* __->__ #34972
jorge-cab added a commit that referenced this pull request Nov 10, 2025
@jorge-cab
[compiler] Switch to track setStates by aliasing and id instead of id…
f76c361 
…entifier names (#34973)

Summary:
This makes the setState usage logic much more robust. We no longer rely
on identifierName.

Now we track when a setState is loaded into a new promoted identifier
variable and track this in a map `setStateLoaded` map.

For other types of instructions we consider the setState to be being
used. In this case we record its usage into the `setStateUsages` map.



Test Plan:
We expect no changes in behavior for the current tests

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34973).
* #35044
* #35020
* __->__ #34973
* #34972
@jorge-cab jorge-cab force-pushed the pr35044 branch 2 times, most recently from ddaf65e to 65c3356 Compare November 10, 2025 20:20
github-actions bot pushed a commit that referenced this pull request Nov 10, 2025
@jorge-cab
[compiler] Update ValidateNoDerivedComputationsInEffects_exp to log t…
8d2f7ad 
…he error instead of throwing (#34972)

Summary:
TSIA

Simple change to log errors in Pipeline.ts instead of throwing in the
validation

Test Plan:
updated snap tests

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34972).
* #35044
* #35020
* #34973
* __->__ #34972

DiffTrain build for [7296120](7296120)
github-actions bot pushed a commit that referenced this pull request Nov 10, 2025
@jorge-cab
[compiler] Update ValidateNoDerivedComputationsInEffects_exp to log t…
e99d2f2 
…he error instead of throwing (#34972)

Summary:
TSIA

Simple change to log errors in Pipeline.ts instead of throwing in the
validation

Test Plan:
updated snap tests

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34972).
* #35044
* #35020
* #34973
* __->__ #34972

DiffTrain build for [7296120](7296120)
jorge-cab added a commit that referenced this pull request Nov 10, 2025
@jorge-cab
[compiler] Don't validate when effect cleanup function depends on eff…
92ac4e8 
…ect localized setState state derived values (#35020)

Summary:
If we are using a clean up function in an effect and that clean up
function depends on a value that is used to set the state we are
validating for we shouldn't throw an error since it is a valid use case
for an effect.

Test Plan:
added test

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35020).
* #35044
* __->__ #35020
@jorge-cab
[compiler] Prevent local state source variables from depending on oth…
b4921db 
…er state

Summary:
When a local state is created sometimes it uses a `prop` or even other local state for its initial value.

This value is only relevant on first render so we shouldn't consider it part of our data flow

Test Plan:
Added tests
@jorge-cab jorge-cab merged commit 100fc4a into main Nov 10, 2025
3 of 4 checks passed
github-actions bot pushed a commit that referenced this pull request Nov 10, 2025
@jorge-cab
[compiler] Don't validate when effect cleanup function depends on eff…
d4e81f0 
…ect localized setState state derived values (#35020)

Summary:
If we are using a clean up function in an effect and that clean up
function depends on a value that is used to set the state we are
validating for we shouldn't throw an error since it is a valid use case
for an effect.

Test Plan:
added test

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35020).
* #35044
* __->__ #35020

DiffTrain build for [92ac4e8](92ac4e8)
github-actions bot pushed a commit that referenced this pull request Nov 10, 2025
@jorge-cab
[compiler] Don't validate when effect cleanup function depends on eff…
ea53d17 
…ect localized setState state derived values (#35020)

Summary:
If we are using a clean up function in an effect and that clean up
function depends on a value that is used to set the state we are
validating for we shouldn't throw an error since it is a valid use case
for an effect.

Test Plan:
added test

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35020).
* #35044
* __->__ #35020

DiffTrain build for [92ac4e8](92ac4e8)
github-actions bot pushed a commit that referenced this pull request Nov 10, 2025
@jorge-cab
[compiler] Prevent local state source variables from depending on oth…
0c602bb 
…er state (#35044)

Summary:
When a local state is created sometimes it uses a `prop` or even other
local state for its initial value.

This value is only relevant on first render so we shouldn't consider it
part of our data flow

Test Plan:
Added tests

DiffTrain build for [100fc4a](100fc4a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mofeiZ mofeiZ mofeiZ approved these changes

Assignees

No one assigned

Labels

CLA Signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jorge-cab @mofeiZ