Skip to content

Conversation

@jorge-cab
Copy link
Contributor

@jorge-cab jorge-cab commented Oct 24, 2025

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


Stack created with Sapling. Best reviewed with ReviewStack.

context.effectSetStateCache.set(operand.loc.identifierName, [
operand,
]);
if (isSetStateType(operand.identifier)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we should be able to remove this isSetStateType (as discussed)
this will keep the logic cleaner (we rely on just dataflow instead of a hybrid of data-flow and typing)

context.setStateCache.get(operand.loc.identifierName)!.push(operand);
} else {
context.setStateCache.set(operand.loc.identifierName, [operand]);
if (isSetStateType(operand.identifier) && isFirstPass) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should be able to remove this as well

Comment on lines 312 to 315
if (isSetStateType(operand.identifier)) {
if (instr.value.kind === 'LoadLocal') {
loads.set(operand.identifier.id, instr.value.place.identifier.id);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would move line 312 to line 315:

// invariant: everything in the loads sidemap is a 'valid' setState

if (instr is LoadLocal) {
  // check if there is an entry for `instr.value.place...` in the `loads` sidemap
  // if there is, insert the loadlocal lvalue into `loads` as well
} else {
  // is this a possible "source" of setStates
  if (ifSetStateType(...)) {
    // this is a root setState
  }
}

@jorge-cab jorge-cab force-pushed the pr34973 branch 3 times, most recently from 557b4f1 to f84fda3 Compare November 4, 2025 18:33
jorge-cab added a commit that referenced this pull request Nov 10, 2025
…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
…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
…he error instead of throwing

Summary:
TSIA

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

Test Plan:
updated snap tests
…entifier names

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
github-actions bot pushed a commit that referenced this pull request Nov 10, 2025
…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
…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
…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
…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 jorge-cab merged commit f76c361 into main Nov 10, 2025
23 of 29 checks passed
github-actions bot pushed a commit that referenced this pull request Nov 10, 2025
…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
…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)
manNomi pushed a commit to manNomi/react that referenced this pull request Nov 15, 2025
…d instead update typeOfValue and fix infinite loops (facebook#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).
* facebook#35044
* facebook#35020
* facebook#34973
* facebook#34972
* facebook#34995
* __->__ facebook#34967
manNomi pushed a commit to manNomi/react that referenced this pull request Nov 15, 2025
…or for `no-deriving-state-in-effects` (facebook#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).
* facebook#35044
* facebook#35020
* facebook#34973
* facebook#34972
* __->__ facebook#34995
* facebook#34967
manNomi pushed a commit to manNomi/react that referenced this pull request Nov 15, 2025
…he error instead of throwing (facebook#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).
* facebook#35044
* facebook#35020
* facebook#34973
* __->__ facebook#34972
manNomi pushed a commit to manNomi/react that referenced this pull request Nov 15, 2025
…entifier names (facebook#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).
* facebook#35044
* facebook#35020
* __->__ facebook#34973
* facebook#34972
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants