From 1d9c3927eaca445a91f14174608a659c78a58e29 Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Tue, 9 Sep 2025 14:09:57 -0700 Subject: [PATCH 1/4] [compiler] new tests for props derived Adds some new test cases for ValidateNoDerivedComputationsInEffects. --- ...ved-state-one-time-init-no-error.expect.md | 87 +++++++++++++++++++ .../derived-state-one-time-init-no-error.js | 21 +++++ ...-state-with-conditional-no-error.expect.md | 79 +++++++++++++++++ ...derived-state-with-conditional-no-error.js | 21 +++++ ...state-with-side-effects-no-error.expect.md | 74 ++++++++++++++++ ...erived-state-with-side-effects-no-error.js | 19 ++++ ...ug-derived-state-from-mixed-deps.expect.md | 49 +++++++++++ ...error.bug-derived-state-from-mixed-deps.js | 23 +++++ ...ed-state-from-props-destructured.expect.md | 43 +++++++++ ...d-derived-state-from-props-destructured.js | 17 ++++ ...rived-state-from-props-in-effect.expect.md | 43 +++++++++ ...alid-derived-state-from-props-in-effect.js | 17 ++++ ...rived-state-from-state-in-effect.expect.md | 51 +++++++++++ ...alid-derived-state-from-state-in-effect.js | 25 ++++++ ...erived-state-from-props-computed.expect.md | 72 +++++++++++++++ ...valid-derived-state-from-props-computed.js | 18 ++++ 16 files changed, 659 insertions(+) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-one-time-init-no-error.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-one-time-init-no-error.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-with-conditional-no-error.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-with-conditional-no-error.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-with-side-effects-no-error.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-with-side-effects-no-error.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.bug-derived-state-from-mixed-deps.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.bug-derived-state-from-mixed-deps.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-in-effect.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-in-effect.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-state-in-effect.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-state-in-effect.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/invalid-derived-state-from-props-computed.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/invalid-derived-state-from-props-computed.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-one-time-init-no-error.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-one-time-init-no-error.expect.md new file mode 100644 index 0000000000000..07a58aeef33ff --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-one-time-init-no-error.expect.md @@ -0,0 +1,87 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects +import {useEffect, useState} from 'react'; + +function Component({initialName}) { + const [name, setName] = useState(''); + + useEffect(() => { + setName(initialName); + }, []); + + return ( +
+ setName(e.target.value)} /> +
+ ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{initialName: 'John'}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects +import { useEffect, useState } from "react"; + +function Component(t0) { + const $ = _c(6); + const { initialName } = t0; + const [name, setName] = useState(""); + let t1; + if ($[0] !== initialName) { + t1 = () => { + setName(initialName); + }; + $[0] = initialName; + $[1] = t1; + } else { + t1 = $[1]; + } + let t2; + if ($[2] === Symbol.for("react.memo_cache_sentinel")) { + t2 = []; + $[2] = t2; + } else { + t2 = $[2]; + } + useEffect(t1, t2); + let t3; + if ($[3] === Symbol.for("react.memo_cache_sentinel")) { + t3 = (e) => setName(e.target.value); + $[3] = t3; + } else { + t3 = $[3]; + } + let t4; + if ($[4] !== name) { + t4 = ( +
+ +
+ ); + $[4] = name; + $[5] = t4; + } else { + t4 = $[5]; + } + return t4; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ initialName: "John" }], +}; + +``` + +### Eval output +(kind: ok)
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-one-time-init-no-error.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-one-time-init-no-error.js new file mode 100644 index 0000000000000..c6705378a5e68 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-one-time-init-no-error.js @@ -0,0 +1,21 @@ +// @validateNoDerivedComputationsInEffects +import {useEffect, useState} from 'react'; + +function Component({initialName}) { + const [name, setName] = useState(''); + + useEffect(() => { + setName(initialName); + }, []); + + return ( +
+ setName(e.target.value)} /> +
+ ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{initialName: 'John'}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-with-conditional-no-error.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-with-conditional-no-error.expect.md new file mode 100644 index 0000000000000..b7a1c85d52c40 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-with-conditional-no-error.expect.md @@ -0,0 +1,79 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects +import {useEffect, useState} from 'react'; + +function Component({value, enabled}) { + const [localValue, setLocalValue] = useState(''); + + useEffect(() => { + if (enabled) { + setLocalValue(value); + } else { + setLocalValue('disabled'); + } + }, [value, enabled]); + + return
{localValue}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{value: 'test', enabled: true}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects +import { useEffect, useState } from "react"; + +function Component(t0) { + const $ = _c(6); + const { value, enabled } = t0; + const [localValue, setLocalValue] = useState(""); + let t1; + let t2; + if ($[0] !== enabled || $[1] !== value) { + t1 = () => { + if (enabled) { + setLocalValue(value); + } else { + setLocalValue("disabled"); + } + }; + + t2 = [value, enabled]; + $[0] = enabled; + $[1] = value; + $[2] = t1; + $[3] = t2; + } else { + t1 = $[2]; + t2 = $[3]; + } + useEffect(t1, t2); + let t3; + if ($[4] !== localValue) { + t3 =
{localValue}
; + $[4] = localValue; + $[5] = t3; + } else { + t3 = $[5]; + } + return t3; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ value: "test", enabled: true }], +}; + +``` + +### Eval output +(kind: ok)
test
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-with-conditional-no-error.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-with-conditional-no-error.js new file mode 100644 index 0000000000000..79d83b89256ff --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-with-conditional-no-error.js @@ -0,0 +1,21 @@ +// @validateNoDerivedComputationsInEffects +import {useEffect, useState} from 'react'; + +function Component({value, enabled}) { + const [localValue, setLocalValue] = useState(''); + + useEffect(() => { + if (enabled) { + setLocalValue(value); + } else { + setLocalValue('disabled'); + } + }, [value, enabled]); + + return
{localValue}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{value: 'test', enabled: true}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-with-side-effects-no-error.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-with-side-effects-no-error.expect.md new file mode 100644 index 0000000000000..e0708dd1f7121 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-with-side-effects-no-error.expect.md @@ -0,0 +1,74 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects +import {useEffect, useState} from 'react'; + +function Component({value}) { + const [localValue, setLocalValue] = useState(''); + + useEffect(() => { + console.log('Value changed:', value); + setLocalValue(value); + document.title = `Value: ${value}`; + }, [value]); + + return
{localValue}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{value: 'test'}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects +import { useEffect, useState } from "react"; + +function Component(t0) { + const $ = _c(5); + const { value } = t0; + const [localValue, setLocalValue] = useState(""); + let t1; + let t2; + if ($[0] !== value) { + t1 = () => { + console.log("Value changed:", value); + setLocalValue(value); + document.title = `Value: ${value}`; + }; + t2 = [value]; + $[0] = value; + $[1] = t1; + $[2] = t2; + } else { + t1 = $[1]; + t2 = $[2]; + } + useEffect(t1, t2); + let t3; + if ($[3] !== localValue) { + t3 =
{localValue}
; + $[3] = localValue; + $[4] = t3; + } else { + t3 = $[4]; + } + return t3; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ value: "test" }], +}; + +``` + +### Eval output +(kind: ok)
test
+logs: ['Value changed:','test'] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-with-side-effects-no-error.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-with-side-effects-no-error.js new file mode 100644 index 0000000000000..b948dda6cb6aa --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-with-side-effects-no-error.js @@ -0,0 +1,19 @@ +// @validateNoDerivedComputationsInEffects +import {useEffect, useState} from 'react'; + +function Component({value}) { + const [localValue, setLocalValue] = useState(''); + + useEffect(() => { + console.log('Value changed:', value); + setLocalValue(value); + document.title = `Value: ${value}`; + }, [value]); + + return
{localValue}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{value: 'test'}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.bug-derived-state-from-mixed-deps.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.bug-derived-state-from-mixed-deps.expect.md new file mode 100644 index 0000000000000..54c95d68e32f5 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.bug-derived-state-from-mixed-deps.expect.md @@ -0,0 +1,49 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects +import {useEffect, useState} from 'react'; + +function Component({prefix}) { + const [name, setName] = useState(''); + const [displayName, setDisplayName] = useState(''); + + useEffect(() => { + setDisplayName(prefix + name); + }, [prefix, name]); + + return ( +
+ setName(e.target.value)} /> +
{displayName}
+
+ ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{prefix: 'Hello, '}], +}; + +``` + + +## Error + +``` +Found 1 error: + +Error: Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) + +error.derived-state-from-mixed-deps-no-error.ts:9:4 + 7 | + 8 | useEffect(() => { +> 9 | setDisplayName(prefix + name); + | ^^^^^^^^^^^^^^ Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) + 10 | }, [prefix, name]); + 11 | + 12 | return ( +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.bug-derived-state-from-mixed-deps.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.bug-derived-state-from-mixed-deps.js new file mode 100644 index 0000000000000..0004ab0ebfb47 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.bug-derived-state-from-mixed-deps.js @@ -0,0 +1,23 @@ +// @validateNoDerivedComputationsInEffects +import {useEffect, useState} from 'react'; + +function Component({prefix}) { + const [name, setName] = useState(''); + const [displayName, setDisplayName] = useState(''); + + useEffect(() => { + setDisplayName(prefix + name); + }, [prefix, name]); + + return ( +
+ setName(e.target.value)} /> +
{displayName}
+
+ ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{prefix: 'Hello, '}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.expect.md new file mode 100644 index 0000000000000..cb18bd12a34a6 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.expect.md @@ -0,0 +1,43 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects +import {useEffect, useState} from 'react'; + +function Component({user: {firstName, lastName}}) { + const [fullName, setFullName] = useState(''); + + useEffect(() => { + setFullName(firstName + ' ' + lastName); + }, [firstName, lastName]); + + return
{fullName}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{user: {firstName: 'John', lastName: 'Doe'}}], +}; + +``` + + +## Error + +``` +Found 1 error: + +Error: Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) + +error.invalid-derived-state-from-props-destructured.ts:8:4 + 6 | + 7 | useEffect(() => { +> 8 | setFullName(firstName + ' ' + lastName); + | ^^^^^^^^^^^ Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) + 9 | }, [firstName, lastName]); + 10 | + 11 | return
{fullName}
; +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.js new file mode 100644 index 0000000000000..130d31c11a248 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.js @@ -0,0 +1,17 @@ +// @validateNoDerivedComputationsInEffects +import {useEffect, useState} from 'react'; + +function Component({user: {firstName, lastName}}) { + const [fullName, setFullName] = useState(''); + + useEffect(() => { + setFullName(firstName + ' ' + lastName); + }, [firstName, lastName]); + + return
{fullName}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{user: {firstName: 'John', lastName: 'Doe'}}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-in-effect.expect.md new file mode 100644 index 0000000000000..15d94c39ad724 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-in-effect.expect.md @@ -0,0 +1,43 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects +import {useEffect, useState} from 'react'; + +function Component({firstName, lastName}) { + const [fullName, setFullName] = useState(''); + + useEffect(() => { + setFullName(firstName + ' ' + lastName); + }, [firstName, lastName]); + + return
{fullName}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{firstName: 'John', lastName: 'Doe'}], +}; + +``` + + +## Error + +``` +Found 1 error: + +Error: Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) + +error.invalid-derived-state-from-props-in-effect.ts:8:4 + 6 | + 7 | useEffect(() => { +> 8 | setFullName(firstName + ' ' + lastName); + | ^^^^^^^^^^^ Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) + 9 | }, [firstName, lastName]); + 10 | + 11 | return
{fullName}
; +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-in-effect.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-in-effect.js new file mode 100644 index 0000000000000..966f09ea89da0 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-in-effect.js @@ -0,0 +1,17 @@ +// @validateNoDerivedComputationsInEffects +import {useEffect, useState} from 'react'; + +function Component({firstName, lastName}) { + const [fullName, setFullName] = useState(''); + + useEffect(() => { + setFullName(firstName + ' ' + lastName); + }, [firstName, lastName]); + + return
{fullName}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{firstName: 'John', lastName: 'Doe'}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-state-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-state-in-effect.expect.md new file mode 100644 index 0000000000000..7466edb3c5c67 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-state-in-effect.expect.md @@ -0,0 +1,51 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects +import {useEffect, useState} from 'react'; + +function Component() { + const [firstName, setFirstName] = useState('John'); + const [lastName, setLastName] = useState('Doe'); + const [fullName, setFullName] = useState(''); + + useEffect(() => { + setFullName(firstName + ' ' + lastName); + }, [firstName, lastName]); + + return ( +
+ setFirstName(e.target.value)} /> + setLastName(e.target.value)} /> +
{fullName}
+
+ ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [], +}; + +``` + + +## Error + +``` +Found 1 error: + +Error: Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) + +error.invalid-derived-state-from-state-in-effect.ts:10:4 + 8 | + 9 | useEffect(() => { +> 10 | setFullName(firstName + ' ' + lastName); + | ^^^^^^^^^^^ Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) + 11 | }, [firstName, lastName]); + 12 | + 13 | return ( +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-state-in-effect.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-state-in-effect.js new file mode 100644 index 0000000000000..2b4f9f70669f5 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-state-in-effect.js @@ -0,0 +1,25 @@ +// @validateNoDerivedComputationsInEffects +import {useEffect, useState} from 'react'; + +function Component() { + const [firstName, setFirstName] = useState('John'); + const [lastName, setLastName] = useState('Doe'); + const [fullName, setFullName] = useState(''); + + useEffect(() => { + setFullName(firstName + ' ' + lastName); + }, [firstName, lastName]); + + return ( +
+ setFirstName(e.target.value)} /> + setLastName(e.target.value)} /> +
{fullName}
+
+ ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/invalid-derived-state-from-props-computed.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/invalid-derived-state-from-props-computed.expect.md new file mode 100644 index 0000000000000..3d0c4fe9c8950 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/invalid-derived-state-from-props-computed.expect.md @@ -0,0 +1,72 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects +import {useEffect, useState} from 'react'; + +function Component(props) { + const [displayValue, setDisplayValue] = useState(''); + + useEffect(() => { + const computed = props.prefix + props.value + props.suffix; + setDisplayValue(computed); + }, [props.prefix, props.value, props.suffix]); + + return
{displayValue}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{prefix: '[', value: 'test', suffix: ']'}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects +import { useEffect, useState } from "react"; + +function Component(props) { + const $ = _c(7); + const [displayValue, setDisplayValue] = useState(""); + let t0; + let t1; + if ($[0] !== props.prefix || $[1] !== props.suffix || $[2] !== props.value) { + t0 = () => { + const computed = props.prefix + props.value + props.suffix; + setDisplayValue(computed); + }; + t1 = [props.prefix, props.value, props.suffix]; + $[0] = props.prefix; + $[1] = props.suffix; + $[2] = props.value; + $[3] = t0; + $[4] = t1; + } else { + t0 = $[3]; + t1 = $[4]; + } + useEffect(t0, t1); + let t2; + if ($[5] !== displayValue) { + t2 =
{displayValue}
; + $[5] = displayValue; + $[6] = t2; + } else { + t2 = $[6]; + } + return t2; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ prefix: "[", value: "test", suffix: "]" }], +}; + +``` + +### Eval output +(kind: ok)
[test]
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/invalid-derived-state-from-props-computed.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/invalid-derived-state-from-props-computed.js new file mode 100644 index 0000000000000..0e726f86ab1ae --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/invalid-derived-state-from-props-computed.js @@ -0,0 +1,18 @@ +// @validateNoDerivedComputationsInEffects +import {useEffect, useState} from 'react'; + +function Component(props) { + const [displayValue, setDisplayValue] = useState(''); + + useEffect(() => { + const computed = props.prefix + props.value + props.suffix; + setDisplayValue(computed); + }, [props.prefix, props.value, props.suffix]); + + return
{displayValue}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{prefix: '[', value: 'test', suffix: ']'}], +}; From 7b38acca0b3efb6ae80ea778bba27819f8ab5b7f Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Tue, 9 Sep 2025 14:10:44 -0700 Subject: [PATCH 2/4] [compiler][wip] Extend ValidateNoDerivedComputationsInEffects for props derived effects This PR adds infra to disambiguate between two types of derived state in effects: 1. State derived from props 2. State derived from other state TODO: - [ ] Props tracking through destructuring and property access does not seem to be propagated correctly inside of Functions' instructions (or i might be misunderstanding how we track aliasing effects) - [ ] compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/invalid-derived-state-from-props-computed.js should be failing - [ ] Handle "mixed" case where deps flow from at least one prop AND state. Should probably have a different error reason, to aid with categorization --- .../ValidateNoDerivedComputationsInEffects.ts | 184 ++++++++++++++++-- ...id-derived-computation-in-effect.expect.md | 6 +- ...ug-derived-state-from-mixed-deps.expect.md | 8 +- ...ed-state-from-props-destructured.expect.md | 6 +- ...d-derived-state-from-props-destructured.js | 4 +- ...rived-state-from-props-in-effect.expect.md | 6 +- ...rived-state-from-state-in-effect.expect.md | 6 +- 7 files changed, 194 insertions(+), 26 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts index 8ec7542a9d989..d1fe8a7d5b0e2 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts @@ -13,14 +13,21 @@ import { FunctionExpression, HIRFunction, IdentifierId, + Place, isSetStateType, isUseEffectHookType, } from '../HIR'; +import {printInstruction, printPlace} from '../HIR/PrintHIR'; import { eachInstructionValueOperand, eachTerminalOperand, } from '../HIR/visitors'; +type SetStateCall = { + loc: SourceLocation; + propsSource: Place | null; // null means state-derived, non-null means props-derived +}; + /** * Validates that useEffect is not used for derived computations which could/should * be performed in render. @@ -48,12 +55,96 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { const candidateDependencies: Map = new Map(); const functions: Map = new Map(); const locals: Map = new Map(); + const derivedFromProps: Map = new Map(); const errors = new CompilerError(); + if (fn.fnType === 'Hook') { + for (const param of fn.params) { + if (param.kind === 'Identifier') { + derivedFromProps.set(param.identifier.id, param); + } + } + } else if (fn.fnType === 'Component') { + const props = fn.params[0]; + if (props != null && props.kind === 'Identifier') { + derivedFromProps.set(props.identifier.id, props); + } + } + for (const block of fn.body.blocks.values()) { for (const instr of block.instructions) { const {lvalue, value} = instr; + + // Track props derivation through instruction effects + if (instr.effects != null) { + for (const effect of instr.effects) { + switch (effect.kind) { + case 'Assign': + case 'Alias': + case 'MaybeAlias': + case 'Capture': { + const source = derivedFromProps.get(effect.from.identifier.id); + if (source != null) { + derivedFromProps.set(effect.into.identifier.id, source); + } + break; + } + } + } + } + + /** + * TODO: figure out why property access off of props does not create an Assign or Alias/Maybe + * Alias + * + * import {useEffect, useState} from 'react' + * + * function Component(props) { + * const [displayValue, setDisplayValue] = useState(''); + * + * useEffect(() => { + * const computed = props.prefix + props.value + props.suffix; + * ^^^^^^^^^^^^ ^^^^^^^^^^^ ^^^^^^^^^^^^ + * we want to track that these are from props + * setDisplayValue(computed); + * }, [props.prefix, props.value, props.suffix]); + * + * return
{displayValue}
; + * } + */ + if (value.kind === 'FunctionExpression') { + for (const [, block] of value.loweredFunc.func.body.blocks) { + for (const instr of block.instructions) { + if (instr.effects != null) { + console.group(printInstruction(instr)); + for (const effect of instr.effects) { + console.log(effect); + switch (effect.kind) { + case 'Assign': + case 'Alias': + case 'MaybeAlias': + case 'Capture': { + const source = derivedFromProps.get( + effect.from.identifier.id, + ); + if (source != null) { + derivedFromProps.set(effect.into.identifier.id, source); + } + break; + } + } + } + } + console.groupEnd(); + } + } + } + + for (const [, place] of derivedFromProps) { + console.log(printPlace(place)); + } + if (value.kind === 'LoadLocal') { locals.set(lvalue.identifier.id, value.place.identifier.id); } else if (value.kind === 'ArrayExpression') { @@ -97,6 +188,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { validateEffect( effectFunction.loweredFunc.func, dependencies, + derivedFromProps, errors, ); } @@ -112,6 +204,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { function validateEffect( effectFunction: HIRFunction, effectDeps: Array, + derivedFromProps: Map, errors: CompilerError, ): void { for (const operand of effectFunction.context) { @@ -119,16 +212,22 @@ function validateEffect( continue; } else if (effectDeps.find(dep => dep === operand.identifier.id) != null) { continue; + } else if (derivedFromProps.has(operand.identifier.id)) { + continue; } else { // Captured something other than the effect dep or setState + console.log('early return 1'); return; } } for (const dep of effectDeps) { + console.log({dep}); if ( effectFunction.context.find(operand => operand.identifier.id === dep) == - null + null || + derivedFromProps.has(dep) === false ) { + console.log('early return 2'); // effect dep wasn't actually used in the function return; } @@ -136,11 +235,18 @@ function validateEffect( const seenBlocks: Set = new Set(); const values: Map> = new Map(); + const effectDerivedFromProps: Map = new Map(); + for (const dep of effectDeps) { + console.log({dep}); values.set(dep, [dep]); + const propsSource = derivedFromProps.get(dep); + if (propsSource != null) { + effectDerivedFromProps.set(dep, propsSource); + } } - const setStateLocations: Array = []; + const setStateCalls: Array = []; for (const block of effectFunction.body.blocks.values()) { for (const pred of block.preds) { if (!seenBlocks.has(pred)) { @@ -150,6 +256,8 @@ function validateEffect( } for (const phi of block.phis) { const aggregateDeps: Set = new Set(); + let propsSource: Place | null = null; + for (const operand of phi.operands.values()) { const deps = values.get(operand.identifier.id); if (deps != null) { @@ -157,10 +265,18 @@ function validateEffect( aggregateDeps.add(dep); } } + const source = effectDerivedFromProps.get(operand.identifier.id); + if (source != null) { + propsSource = source; + } } + if (aggregateDeps.size !== 0) { values.set(phi.place.identifier.id, Array.from(aggregateDeps)); } + if (propsSource != null) { + effectDerivedFromProps.set(phi.place.identifier.id, propsSource); + } } for (const instr of block.instructions) { switch (instr.value.kind) { @@ -203,9 +319,16 @@ function validateEffect( ) { const deps = values.get(instr.value.args[0].identifier.id); if (deps != null && new Set(deps).size === effectDeps.length) { - setStateLocations.push(instr.value.callee.loc); + const propsSource = effectDerivedFromProps.get( + instr.value.args[0].identifier.id, + ); + + setStateCalls.push({ + loc: instr.value.callee.loc, + propsSource: propsSource ?? null, + }); } else { - // doesn't depend on any deps + // doesn't depend on all deps return; } } @@ -215,6 +338,26 @@ function validateEffect( return; } } + + // Track props derivation through instruction effects + if (instr.effects != null) { + for (const effect of instr.effects) { + switch (effect.kind) { + case 'Assign': + case 'Alias': + case 'MaybeAlias': + case 'Capture': { + const source = effectDerivedFromProps.get( + effect.from.identifier.id, + ); + if (source != null) { + effectDerivedFromProps.set(effect.into.identifier.id, source); + } + break; + } + } + } + } } for (const operand of eachTerminalOperand(block.terminal)) { if (values.has(operand.identifier.id)) { @@ -225,14 +368,29 @@ function validateEffect( seenBlocks.add(block.id); } - for (const loc of setStateLocations) { - errors.push({ - category: ErrorCategory.EffectDerivationsOfState, - reason: - 'Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)', - description: null, - loc, - suggestions: null, - }); + for (const call of setStateCalls) { + if (call.propsSource != null) { + const propName = call.propsSource.identifier.name?.value; + const propInfo = propName != null ? ` (from prop '${propName}')` : ''; + + errors.push({ + reason: `Consider lifting state up to the parent component to make this a controlled component. (https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes)`, + description: `You are using props${propInfo} to update local state in an effect.`, + severity: ErrorSeverity.InvalidReact, + loc: call.loc, + suggestions: null, + }); + } else { + errors.push({ + reason: + 'You may not need this effect. Values derived from state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)', + description: + 'This effect updates state based on other state values. ' + + 'Consider calculating this value directly during render', + severity: ErrorSeverity.InvalidReact, + loc: call.loc, + suggestions: null, + }); + } } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-derived-computation-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-derived-computation-in-effect.expect.md index d97a665ae698c..1d7e24b3efaff 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-derived-computation-in-effect.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-derived-computation-in-effect.expect.md @@ -24,13 +24,15 @@ function BadExample() { ``` Found 1 error: -Error: Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) +Error: You may not need this effect. Values derived from state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) + +This effect updates state based on other state values. Consider calculating this value directly during render. error.invalid-derived-computation-in-effect.ts:9:4 7 | const [fullName, setFullName] = useState(''); 8 | useEffect(() => { > 9 | setFullName(capitalize(firstName + ' ' + lastName)); - | ^^^^^^^^^^^ Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) + | ^^^^^^^^^^^ You may not need this effect. Values derived from state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) 10 | }, [firstName, lastName]); 11 | 12 | return
{fullName}
; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.bug-derived-state-from-mixed-deps.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.bug-derived-state-from-mixed-deps.expect.md index 54c95d68e32f5..8124f4b3f32d6 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.bug-derived-state-from-mixed-deps.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.bug-derived-state-from-mixed-deps.expect.md @@ -34,13 +34,15 @@ export const FIXTURE_ENTRYPOINT = { ``` Found 1 error: -Error: Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) +Error: You may not need this effect. Values derived from state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) -error.derived-state-from-mixed-deps-no-error.ts:9:4 +This effect updates state based on other state values. Consider calculating this value directly during render. + +error.bug-derived-state-from-mixed-deps.ts:9:4 7 | 8 | useEffect(() => { > 9 | setDisplayName(prefix + name); - | ^^^^^^^^^^^^^^ Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) + | ^^^^^^^^^^^^^^ You may not need this effect. Values derived from state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) 10 | }, [prefix, name]); 11 | 12 | return ( diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.expect.md index cb18bd12a34a6..26b8b7930b365 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.expect.md @@ -28,13 +28,15 @@ export const FIXTURE_ENTRYPOINT = { ``` Found 1 error: -Error: Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) +Error: You may not need this effect. Values derived from state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) + +This effect updates state based on other state values. Consider calculating this value directly during render. error.invalid-derived-state-from-props-destructured.ts:8:4 6 | 7 | useEffect(() => { > 8 | setFullName(firstName + ' ' + lastName); - | ^^^^^^^^^^^ Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) + | ^^^^^^^^^^^ You may not need this effect. Values derived from state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) 9 | }, [firstName, lastName]); 10 | 11 | return
{fullName}
; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.js index 130d31c11a248..966f09ea89da0 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.js @@ -1,7 +1,7 @@ // @validateNoDerivedComputationsInEffects import {useEffect, useState} from 'react'; -function Component({user: {firstName, lastName}}) { +function Component({firstName, lastName}) { const [fullName, setFullName] = useState(''); useEffect(() => { @@ -13,5 +13,5 @@ function Component({user: {firstName, lastName}}) { export const FIXTURE_ENTRYPOINT = { fn: Component, - params: [{user: {firstName: 'John', lastName: 'Doe'}}], + params: [{firstName: 'John', lastName: 'Doe'}], }; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-in-effect.expect.md index 15d94c39ad724..1f7ff8dc5d754 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-in-effect.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-in-effect.expect.md @@ -28,13 +28,15 @@ export const FIXTURE_ENTRYPOINT = { ``` Found 1 error: -Error: Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) +Error: You may not need this effect. Values derived from state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) + +This effect updates state based on other state values. Consider calculating this value directly during render. error.invalid-derived-state-from-props-in-effect.ts:8:4 6 | 7 | useEffect(() => { > 8 | setFullName(firstName + ' ' + lastName); - | ^^^^^^^^^^^ Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) + | ^^^^^^^^^^^ You may not need this effect. Values derived from state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) 9 | }, [firstName, lastName]); 10 | 11 | return
{fullName}
; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-state-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-state-in-effect.expect.md index 7466edb3c5c67..c5548c970b825 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-state-in-effect.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-state-in-effect.expect.md @@ -36,13 +36,15 @@ export const FIXTURE_ENTRYPOINT = { ``` Found 1 error: -Error: Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) +Error: You may not need this effect. Values derived from state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) + +This effect updates state based on other state values. Consider calculating this value directly during render. error.invalid-derived-state-from-state-in-effect.ts:10:4 8 | 9 | useEffect(() => { > 10 | setFullName(firstName + ' ' + lastName); - | ^^^^^^^^^^^ Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) + | ^^^^^^^^^^^ You may not need this effect. Values derived from state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) 11 | }, [firstName, lastName]); 12 | 13 | return ( From f807ce649209fde8fe48cffc6ded8e0a3f0cdb1c Mon Sep 17 00:00:00 2001 From: Jorge Cabiedes <57368278+jorge-cab@users.noreply.github.com> Date: Tue, 9 Sep 2025 14:11:19 -0700 Subject: [PATCH 3/4] [compiler] Basic solution for instruction based prop derivation validation --- .../ValidateNoDerivedComputationsInEffects.ts | 343 ++++++++++++------ 1 file changed, 228 insertions(+), 115 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts index d1fe8a7d5b0e2..70e9514e31bd2 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts @@ -5,7 +5,8 @@ * LICENSE file in the root directory of this source tree. */ -import {CompilerError, SourceLocation} from '..'; +import {TypeOf} from 'zod'; +import {CompilerError, Effect, ErrorSeverity, SourceLocation} from '..'; import {ErrorCategory} from '../CompilerError'; import { ArrayExpression, @@ -13,6 +14,7 @@ import { FunctionExpression, HIRFunction, IdentifierId, + InstructionValue, Place, isSetStateType, isUseEffectHookType, @@ -20,13 +22,74 @@ import { import {printInstruction, printPlace} from '../HIR/PrintHIR'; import { eachInstructionValueOperand, + eachInstructionOperand, eachTerminalOperand, + eachInstructionLValue, } from '../HIR/visitors'; +import {isMutable} from '../ReactiveScopes/InferReactiveScopeVariables'; +import {assertExhaustive} from '../Utils/utils'; type SetStateCall = { loc: SourceLocation; - propsSource: Place | null; // null means state-derived, non-null means props-derived + propsSources: Place[] | undefined; // undefined means state-derived, defined means props-derived }; +type TypeOfValue = 'ignored' | 'fromProps' | 'fromState' | 'fromPropsOrState'; + +type DerivationMetadata = { + identifierPlace: Place; + sources: Place[]; + typeOfValue: TypeOfValue; +}; + +function joinValue( + lvalueType: TypeOfValue, + valueType: TypeOfValue, +): TypeOfValue { + if (lvalueType === 'ignored') return valueType; + if (valueType === 'ignored') return lvalueType; + if (lvalueType === valueType) return lvalueType; + return 'fromPropsOrState'; +} + +function propagateDerivation( + dest: Place, + source: Place | undefined, + derivedFromProps: Map, +) { + if (source === undefined) { + return; + } + + if (source.identifier.name?.kind === 'promoted') { + derivedFromProps.set(dest.identifier.id, dest); + } else { + derivedFromProps.set(dest.identifier.id, source); + } +} + +function updateDerivationMetadata( + target: Place, + sources: DerivationMetadata[], + typeOfValue: TypeOfValue, + derivedTuple: Map, +): void { + let newValue: DerivationMetadata = { + identifierPlace: target, + sources: [], + typeOfValue: typeOfValue, + }; + + for (const source of sources) { + // If the identifier of the source is a promoted identifier, then + // we should set the source as the first named identifier. + if (source.identifierPlace.identifier.name?.kind === 'promoted') { + newValue.sources.push(target); + } else { + newValue.sources.push(...source.sources); + } + } + derivedTuple.set(target.identifier.id, newValue); +} /** * Validates that useEffect is not used for derived computations which could/should @@ -55,96 +118,138 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { const candidateDependencies: Map = new Map(); const functions: Map = new Map(); const locals: Map = new Map(); - const derivedFromProps: Map = new Map(); + + // MY take on this + const valueToType: Map = new Map(); + const valueToSourceProps: Map> = new Map(); + const valueToSourceStates: Map> = new Map(); + const valueToSources: Map> = new Map(); + + // Sources are still probably not correct + const derivedTuple: Map = new Map(); const errors = new CompilerError(); if (fn.fnType === 'Hook') { for (const param of fn.params) { if (param.kind === 'Identifier') { - derivedFromProps.set(param.identifier.id, param); + derivedTuple.set(param.identifier.id, { + identifierPlace: param, + sources: [param], + typeOfValue: 'fromProps', + }); } } } else if (fn.fnType === 'Component') { const props = fn.params[0]; if (props != null && props.kind === 'Identifier') { - derivedFromProps.set(props.identifier.id, props); + derivedTuple.set(props.identifier.id, { + identifierPlace: props, + sources: [props], + typeOfValue: 'fromProps', + }); } } for (const block of fn.body.blocks.values()) { + for (const phi of block.phis) { + for (const operand of phi.operands.values()) { + const source = derivedTuple.get(operand.identifier.id); + if (source !== undefined && source.typeOfValue === 'fromProps') { + if ( + source.identifierPlace.identifier.name === null || + source.identifierPlace.identifier.name?.kind === 'promoted' + ) { + derivedTuple.set(phi.place.identifier.id, { + identifierPlace: phi.place, + sources: [phi.place], + typeOfValue: 'fromProps', + }); + } else { + derivedTuple.set(phi.place.identifier.id, { + identifierPlace: phi.place, + sources: source.sources, + typeOfValue: 'fromProps', + }); + } + } + } + } + for (const instr of block.instructions) { const {lvalue, value} = instr; - // Track props derivation through instruction effects - if (instr.effects != null) { - for (const effect of instr.effects) { - switch (effect.kind) { - case 'Assign': - case 'Alias': - case 'MaybeAlias': - case 'Capture': { - const source = derivedFromProps.get(effect.from.identifier.id); - if (source != null) { - derivedFromProps.set(effect.into.identifier.id, source); - } - break; - } - } + // This needs to be repeated "recursively" on FunctionExpressions + // HERE >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> + // DERIVATION LOGIC----------------------------------------------------- + console.log('instr', printInstruction(instr)); + console.log('instr', instr); + // console.log('instr lValue', instr.lvalue); + + let typeOfValue: TypeOfValue = 'ignored'; + + // TODO: Add handling for state derived props + let sources: DerivationMetadata[] = []; + for (const operand of eachInstructionValueOperand(value)) { + const opSource = derivedTuple.get(operand.identifier.id); + if (opSource === undefined) { + continue; } + + typeOfValue = joinValue(typeOfValue, opSource.typeOfValue); + sources.push(opSource); } - /** - * TODO: figure out why property access off of props does not create an Assign or Alias/Maybe - * Alias - * - * import {useEffect, useState} from 'react' - * - * function Component(props) { - * const [displayValue, setDisplayValue] = useState(''); - * - * useEffect(() => { - * const computed = props.prefix + props.value + props.suffix; - * ^^^^^^^^^^^^ ^^^^^^^^^^^ ^^^^^^^^^^^^ - * we want to track that these are from props - * setDisplayValue(computed); - * }, [props.prefix, props.value, props.suffix]); - * - * return
{displayValue}
; - * } - */ - if (value.kind === 'FunctionExpression') { - for (const [, block] of value.loweredFunc.func.body.blocks) { - for (const instr of block.instructions) { - if (instr.effects != null) { - console.group(printInstruction(instr)); - for (const effect of instr.effects) { - console.log(effect); - switch (effect.kind) { - case 'Assign': - case 'Alias': - case 'MaybeAlias': - case 'Capture': { - const source = derivedFromProps.get( - effect.from.identifier.id, - ); - if (source != null) { - derivedFromProps.set(effect.into.identifier.id, source); - } - break; - } - } + // TODO: Add handling for state derived props + if (typeOfValue !== 'ignored') { + for (const lvalue of eachInstructionLValue(instr)) { + updateDerivationMetadata(lvalue, sources, typeOfValue, derivedTuple); + } + + for (const operand of eachInstructionValueOperand(value)) { + switch (operand.effect) { + case Effect.Capture: + case Effect.Store: + case Effect.ConditionallyMutate: + case Effect.ConditionallyMutateIterator: + case Effect.Mutate: { + if (isMutable(instr, operand)) { + updateDerivationMetadata( + operand, + sources, + typeOfValue, + derivedTuple, + ); } + break; + } + case Effect.Freeze: + case Effect.Read: { + // no-op + break; + } + case Effect.Unknown: { + CompilerError.invariant(false, { + reason: 'Unexpected unknown effect', + description: null, + loc: operand.loc, + suggestions: null, + }); + } + default: { + assertExhaustive( + operand.effect, + `Unexpected effect kind \`${operand.effect}\``, + ); } - console.groupEnd(); } } } + console.log('derivedTuple', derivedTuple); + // HERE >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> - for (const [, place] of derivedFromProps) { - console.log(printPlace(place)); - } - + // console.log('derivedTuple', derivedTuple); + // DERIVATION LOGIC----------------------------------------------------- if (value.kind === 'LoadLocal') { locals.set(lvalue.identifier.id, value.place.identifier.id); } else if (value.kind === 'ArrayExpression') { @@ -157,6 +262,8 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { ) { const callee = value.kind === 'CallExpression' ? value.callee : value.property; + + // This is a useEffect hook if ( isUseEffectHookType(callee.identifier) && value.args.length === 2 && @@ -188,7 +295,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { validateEffect( effectFunction.loweredFunc.func, dependencies, - derivedFromProps, + derivedTuple, errors, ); } @@ -204,7 +311,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { function validateEffect( effectFunction: HIRFunction, effectDeps: Array, - derivedFromProps: Map, + derivedTuple: Map, errors: CompilerError, ): void { for (const operand of effectFunction.context) { @@ -212,7 +319,7 @@ function validateEffect( continue; } else if (effectDeps.find(dep => dep === operand.identifier.id) != null) { continue; - } else if (derivedFromProps.has(operand.identifier.id)) { + } else if (derivedTuple.has(operand.identifier.id)) { continue; } else { // Captured something other than the effect dep or setState @@ -220,29 +327,36 @@ function validateEffect( return; } } + + // This might be wrong gotta double check + let hasInvalidDep = false; for (const dep of effectDeps) { - console.log({dep}); + const depMetadata = derivedTuple.get(dep); if ( - effectFunction.context.find(operand => operand.identifier.id === dep) == + effectFunction.context.find(operand => operand.identifier.id === dep) != null || - derivedFromProps.has(dep) === false + (depMetadata !== undefined && depMetadata.typeOfValue !== 'ignored') ) { - console.log('early return 2'); - // effect dep wasn't actually used in the function - return; + hasInvalidDep = true; } } + if (!hasInvalidDep) { + console.log('early return 2'); + // effect dep wasn't actually used in the function + return; + } + const seenBlocks: Set = new Set(); + // This variable is suspicious maybe we don't need it? const values: Map> = new Map(); - const effectDerivedFromProps: Map = new Map(); + const effectInvalidlyDerived: Map = new Map(); for (const dep of effectDeps) { - console.log({dep}); values.set(dep, [dep]); - const propsSource = derivedFromProps.get(dep); - if (propsSource != null) { - effectDerivedFromProps.set(dep, propsSource); + const depMetadata = derivedTuple.get(dep); + if (depMetadata !== undefined) { + effectInvalidlyDerived.set(dep, depMetadata.sources); } } @@ -254,9 +368,11 @@ function validateEffect( return; } } + + // TODO: This might need editing for (const phi of block.phis) { const aggregateDeps: Set = new Set(); - let propsSource: Place | null = null; + let propsSources: Place[] | null = null; for (const operand of phi.operands.values()) { const deps = values.get(operand.identifier.id); @@ -265,19 +381,20 @@ function validateEffect( aggregateDeps.add(dep); } } - const source = effectDerivedFromProps.get(operand.identifier.id); - if (source != null) { - propsSource = source; + const sources = effectInvalidlyDerived.get(operand.identifier.id); + if (sources != null) { + propsSources = sources; } } if (aggregateDeps.size !== 0) { values.set(phi.place.identifier.id, Array.from(aggregateDeps)); } - if (propsSource != null) { - effectDerivedFromProps.set(phi.place.identifier.id, propsSource); + if (propsSources != null) { + effectInvalidlyDerived.set(phi.place.identifier.id, propsSources); } } + for (const instr of block.instructions) { switch (instr.value.kind) { case 'Primitive': @@ -299,7 +416,7 @@ function validateEffect( case 'CallExpression': case 'MethodCall': { const aggregateDeps: Set = new Set(); - for (const operand of eachInstructionValueOperand(instr.value)) { + for (const operand of eachInstructionOperand(instr)) { const deps = values.get(operand.identifier.id); if (deps != null) { for (const dep of deps) { @@ -318,60 +435,56 @@ function validateEffect( instr.value.args[0].kind === 'Identifier' ) { const deps = values.get(instr.value.args[0].identifier.id); + console.log('deps', deps); if (deps != null && new Set(deps).size === effectDeps.length) { - const propsSource = effectDerivedFromProps.get( + // console.log('setState arg', instr.value.args[0].identifier.id); + // console.log('effectInvalidlyDerived', effectInvalidlyDerived); + // console.log('derivedTuple', derivedTuple); + const propSources = derivedTuple.get( instr.value.args[0].identifier.id, ); - setStateCalls.push({ - loc: instr.value.callee.loc, - propsSource: propsSource ?? null, - }); + console.log('Final reference', propSources); + if (propSources !== undefined) { + setStateCalls.push({ + loc: instr.value.callee.loc, + propsSources: propSources.sources, + }); + } else { + setStateCalls.push({ + loc: instr.value.callee.loc, + propsSources: undefined, + }); + } } else { // doesn't depend on all deps + console.log('early return 3'); return; } } break; } default: { + console.log('early return 4'); return; } } - - // Track props derivation through instruction effects - if (instr.effects != null) { - for (const effect of instr.effects) { - switch (effect.kind) { - case 'Assign': - case 'Alias': - case 'MaybeAlias': - case 'Capture': { - const source = effectDerivedFromProps.get( - effect.from.identifier.id, - ); - if (source != null) { - effectDerivedFromProps.set(effect.into.identifier.id, source); - } - break; - } - } - } - } } for (const operand of eachTerminalOperand(block.terminal)) { if (values.has(operand.identifier.id)) { - // return; } } seenBlocks.add(block.id); } + console.log('setStateCalls', setStateCalls); for (const call of setStateCalls) { - if (call.propsSource != null) { - const propName = call.propsSource.identifier.name?.value; - const propInfo = propName != null ? ` (from prop '${propName}')` : ''; + if (call.propsSources != null) { + const propNames = call.propsSources + .map(place => place.identifier.name?.value) + .join(', '); + const propInfo = propNames != null ? ` (from props '${propNames}')` : ''; errors.push({ reason: `Consider lifting state up to the parent component to make this a controlled component. (https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes)`, From 5cf71b322d01850d3bac313f87012890b29b6410 Mon Sep 17 00:00:00 2001 From: Jorge Cabiedes <57368278+jorge-cab@users.noreply.github.com> Date: Tue, 9 Sep 2025 14:11:19 -0700 Subject: [PATCH 4/4] [compiler] Validation for values derived from props in useEffect ready --- .../ValidateNoDerivedComputationsInEffects.ts | 444 ++++++++++-------- 1 file changed, 248 insertions(+), 196 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts index 70e9514e31bd2..6ec5479a5ed81 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts @@ -5,40 +5,55 @@ * LICENSE file in the root directory of this source tree. */ -import {TypeOf} from 'zod'; +import {effect} from 'zod'; import {CompilerError, Effect, ErrorSeverity, SourceLocation} from '..'; import {ErrorCategory} from '../CompilerError'; import { ArrayExpression, + BasicBlock, BlockId, + Identifier, FunctionExpression, HIRFunction, IdentifierId, - InstructionValue, + Instruction, Place, isSetStateType, isUseEffectHookType, + isUseStateType, + IdentifierName, + GeneratedSource, } from '../HIR'; -import {printInstruction, printPlace} from '../HIR/PrintHIR'; +import {printInstruction} from '../HIR/PrintHIR'; import { - eachInstructionValueOperand, eachInstructionOperand, eachTerminalOperand, eachInstructionLValue, + eachPatternOperand, } from '../HIR/visitors'; import {isMutable} from '../ReactiveScopes/InferReactiveScopeVariables'; import {assertExhaustive} from '../Utils/utils'; type SetStateCall = { loc: SourceLocation; - propsSources: Place[] | undefined; // undefined means state-derived, defined means props-derived + invalidDeps: Map | undefined; + setStateId: IdentifierId; }; type TypeOfValue = 'ignored' | 'fromProps' | 'fromState' | 'fromPropsOrState'; type DerivationMetadata = { + typeOfValue: TypeOfValue; + // TODO: Rename to place identifierPlace: Place; sources: Place[]; - typeOfValue: TypeOfValue; +}; + +// TODO: This needs refining +type ErrorMetadata = { + errorType: 'HoistState' | 'CalculateInRender'; + propInfo: string | undefined; + loc: SourceLocation; + setStateId: IdentifierId; }; function joinValue( @@ -51,22 +66,6 @@ function joinValue( return 'fromPropsOrState'; } -function propagateDerivation( - dest: Place, - source: Place | undefined, - derivedFromProps: Map, -) { - if (source === undefined) { - return; - } - - if (source.identifier.name?.kind === 'promoted') { - derivedFromProps.set(dest.identifier.id, dest); - } else { - derivedFromProps.set(dest.identifier.id, source); - } -} - function updateDerivationMetadata( target: Place, sources: DerivationMetadata[], @@ -81,7 +80,7 @@ function updateDerivationMetadata( for (const source of sources) { // If the identifier of the source is a promoted identifier, then - // we should set the source as the first named identifier. + // we should set the target as the source. if (source.identifierPlace.identifier.name?.kind === 'promoted') { newValue.sources.push(target); } else { @@ -91,6 +90,133 @@ function updateDerivationMetadata( derivedTuple.set(target.identifier.id, newValue); } +function parseInstr( + instr: Instruction, + derivedTuple: Map, + setStateCalls: Map, +) { + // console.log(printInstruction(instr)); + // console.log(instr); + let typeOfValue: TypeOfValue = 'ignored'; + + // If the instruction is destructuring a useState hook call + if ( + instr.value.kind === 'Destructure' && + instr.value.lvalue.pattern.kind === 'ArrayPattern' && + isUseStateType(instr.value.value.identifier) + ) { + const value = instr.value.lvalue.pattern.items[0]; + if (value.kind === 'Identifier') { + derivedTuple.set(value.identifier.id, { + identifierPlace: value, + sources: [value], + typeOfValue: 'fromState', + }); + } + } + + // If the instruction is calling a setState + if ( + instr.value.kind === 'CallExpression' && + isSetStateType(instr.value.callee.identifier) && + instr.value.args.length === 1 && + instr.value.args[0].kind === 'Identifier' && + instr.value.callee.loc !== GeneratedSource && + instr.value.callee.loc.identifierName !== undefined && + instr.value.callee.loc.identifierName !== null + ) { + setStateCalls.set( + instr.value.callee.loc.identifierName, + instr.value.callee, + ); + } + + let sources: DerivationMetadata[] = []; + for (const operand of eachInstructionOperand(instr)) { + const opSource = derivedTuple.get(operand.identifier.id); + if (opSource === undefined) { + continue; + } + + typeOfValue = joinValue(typeOfValue, opSource.typeOfValue); + sources.push(opSource); + } + + if (typeOfValue !== 'ignored') { + for (const lvalue of eachInstructionLValue(instr)) { + updateDerivationMetadata(lvalue, sources, typeOfValue, derivedTuple); + } + + for (const operand of eachInstructionOperand(instr)) { + switch (operand.effect) { + case Effect.Capture: + case Effect.Store: + case Effect.ConditionallyMutate: + case Effect.ConditionallyMutateIterator: + case Effect.Mutate: { + if (isMutable(instr, operand)) { + updateDerivationMetadata( + operand, + sources, + typeOfValue, + derivedTuple, + ); + } + break; + } + case Effect.Freeze: + case Effect.Read: { + // no-op + break; + } + case Effect.Unknown: { + CompilerError.invariant(false, { + reason: 'Unexpected unknown effect', + description: null, + loc: operand.loc, + suggestions: null, + }); + } + default: { + assertExhaustive( + operand.effect, + `Unexpected effect kind \`${operand.effect}\``, + ); + } + } + } + } +} + +function parseBlockPhi( + block: BasicBlock, + derivedTuple: Map, +) { + for (const phi of block.phis) { + for (const operand of phi.operands.values()) { + const source = derivedTuple.get(operand.identifier.id); + if (source !== undefined && source.typeOfValue === 'fromProps') { + if ( + source.identifierPlace.identifier.name === null || + source.identifierPlace.identifier.name?.kind === 'promoted' + ) { + derivedTuple.set(phi.place.identifier.id, { + identifierPlace: phi.place, + sources: [phi.place], + typeOfValue: 'fromProps', + }); + } else { + derivedTuple.set(phi.place.identifier.id, { + identifierPlace: phi.place, + sources: source.sources, + typeOfValue: 'fromProps', + }); + } + } + } + } +} + /** * Validates that useEffect is not used for derived computations which could/should * be performed in render. @@ -118,17 +244,15 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { const candidateDependencies: Map = new Map(); const functions: Map = new Map(); const locals: Map = new Map(); + const derivedTuple: Map = new Map(); - // MY take on this - const valueToType: Map = new Map(); - const valueToSourceProps: Map> = new Map(); - const valueToSourceStates: Map> = new Map(); - const valueToSources: Map> = new Map(); + // Investigating + const effectSetStates: Map = new Map(); + const setStateCalls: Map = new Map(); - // Sources are still probably not correct - const derivedTuple: Map = new Map(); + // let shouldCalculateInRender: boolean = true; - const errors = new CompilerError(); + const errors: ErrorMetadata[] = []; if (fn.fnType === 'Hook') { for (const param of fn.params) { @@ -152,104 +276,26 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { } for (const block of fn.body.blocks.values()) { - for (const phi of block.phis) { - for (const operand of phi.operands.values()) { - const source = derivedTuple.get(operand.identifier.id); - if (source !== undefined && source.typeOfValue === 'fromProps') { - if ( - source.identifierPlace.identifier.name === null || - source.identifierPlace.identifier.name?.kind === 'promoted' - ) { - derivedTuple.set(phi.place.identifier.id, { - identifierPlace: phi.place, - sources: [phi.place], - typeOfValue: 'fromProps', - }); - } else { - derivedTuple.set(phi.place.identifier.id, { - identifierPlace: phi.place, - sources: source.sources, - typeOfValue: 'fromProps', - }); - } - } - } - } + parseBlockPhi(block, derivedTuple); for (const instr of block.instructions) { const {lvalue, value} = instr; - // This needs to be repeated "recursively" on FunctionExpressions - // HERE >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> - // DERIVATION LOGIC----------------------------------------------------- - console.log('instr', printInstruction(instr)); - console.log('instr', instr); - // console.log('instr lValue', instr.lvalue); - - let typeOfValue: TypeOfValue = 'ignored'; - - // TODO: Add handling for state derived props - let sources: DerivationMetadata[] = []; - for (const operand of eachInstructionValueOperand(value)) { - const opSource = derivedTuple.get(operand.identifier.id); - if (opSource === undefined) { - continue; - } - - typeOfValue = joinValue(typeOfValue, opSource.typeOfValue); - sources.push(opSource); - } - - // TODO: Add handling for state derived props - if (typeOfValue !== 'ignored') { - for (const lvalue of eachInstructionLValue(instr)) { - updateDerivationMetadata(lvalue, sources, typeOfValue, derivedTuple); - } + parseInstr(instr, derivedTuple, setStateCalls); - for (const operand of eachInstructionValueOperand(value)) { - switch (operand.effect) { - case Effect.Capture: - case Effect.Store: - case Effect.ConditionallyMutate: - case Effect.ConditionallyMutateIterator: - case Effect.Mutate: { - if (isMutable(instr, operand)) { - updateDerivationMetadata( - operand, - sources, - typeOfValue, - derivedTuple, - ); - } - break; - } - case Effect.Freeze: - case Effect.Read: { - // no-op - break; - } - case Effect.Unknown: { - CompilerError.invariant(false, { - reason: 'Unexpected unknown effect', - description: null, - loc: operand.loc, - suggestions: null, - }); - } - default: { - assertExhaustive( - operand.effect, - `Unexpected effect kind \`${operand.effect}\``, - ); - } + /* + * Special case for function expressions, we need to parse nested instructions + * TODO: Can there be more recursive levels? + */ + if (value.kind === 'FunctionExpression') { + for (const [, block] of value.loweredFunc.func.body.blocks) { + for (const instr of block.instructions) { + parseInstr(instr, derivedTuple, setStateCalls); } } } - console.log('derivedTuple', derivedTuple); - // HERE >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> - // console.log('derivedTuple', derivedTuple); - // DERIVATION LOGIC----------------------------------------------------- + // Maybe this should run for every instruction being parsed if (value.kind === 'LoadLocal') { locals.set(lvalue.identifier.id, value.place.identifier.id); } else if (value.kind === 'ArrayExpression') { @@ -263,7 +309,6 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { const callee = value.kind === 'CallExpression' ? value.callee : value.property; - // This is a useEffect hook if ( isUseEffectHookType(callee.identifier) && value.args.length === 2 && @@ -296,6 +341,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { effectFunction.loweredFunc.func, dependencies, derivedTuple, + effectSetStates, errors, ); } @@ -303,8 +349,21 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { } } } - if (errors.hasAnyErrors()) { - throw errors; + + console.log('setStateCalls: ', setStateCalls); + console.log('effectSetStates: ', effectSetStates); + const throwableErrors = new CompilerError(); + for (const error of errors) { + throwableErrors.push({ + reason: `You may not need an effect. Values derived from state should be calculated in render, not in an effect. `, + description: `You are using a value derived from props${error.propInfo} to update local state in an effect.`, + severity: ErrorSeverity.InvalidReact, + loc: error.loc, + }); + } + + if (throwableErrors.hasAnyErrors()) { + throw throwableErrors; } } @@ -312,8 +371,13 @@ function validateEffect( effectFunction: HIRFunction, effectDeps: Array, derivedTuple: Map, - errors: CompilerError, + effectSetStates: Map, + errors: ErrorMetadata[], ): void { + /* + * TODO: This makes it so we only capture single line useEffects. + * We should be able to capture multiline as well + */ for (const operand of effectFunction.context) { if (isSetStateType(operand.identifier)) { continue; @@ -323,7 +387,6 @@ function validateEffect( continue; } else { // Captured something other than the effect dep or setState - console.log('early return 1'); return; } } @@ -350,17 +413,18 @@ function validateEffect( const seenBlocks: Set = new Set(); // This variable is suspicious maybe we don't need it? const values: Map> = new Map(); - const effectInvalidlyDerived: Map = new Map(); + const effectInvalidlyDerived: Map = + new Map(); for (const dep of effectDeps) { values.set(dep, [dep]); const depMetadata = derivedTuple.get(dep); if (depMetadata !== undefined) { - effectInvalidlyDerived.set(dep, depMetadata.sources); + effectInvalidlyDerived.set(dep, depMetadata); } } - const setStateCalls: Array = []; + const setStateCallsInEffect: Array = []; for (const block of effectFunction.body.blocks.values()) { for (const pred of block.preds) { if (!seenBlocks.has(pred)) { @@ -369,33 +433,23 @@ function validateEffect( } } - // TODO: This might need editing - for (const phi of block.phis) { - const aggregateDeps: Set = new Set(); - let propsSources: Place[] | null = null; - - for (const operand of phi.operands.values()) { - const deps = values.get(operand.identifier.id); - if (deps != null) { - for (const dep of deps) { - aggregateDeps.add(dep); - } - } - const sources = effectInvalidlyDerived.get(operand.identifier.id); - if (sources != null) { - propsSources = sources; - } - } - - if (aggregateDeps.size !== 0) { - values.set(phi.place.identifier.id, Array.from(aggregateDeps)); - } - if (propsSources != null) { - effectInvalidlyDerived.set(phi.place.identifier.id, propsSources); - } - } + parseBlockPhi(block, effectInvalidlyDerived); for (const instr of block.instructions) { + if ( + instr.value.kind === 'CallExpression' && + isSetStateType(instr.value.callee.identifier) && + instr.value.args.length === 1 && + instr.value.args[0].kind === 'Identifier' && + instr.value.callee.loc !== GeneratedSource && + instr.value.callee.loc.identifierName !== undefined && + instr.value.callee.loc.identifierName !== null + ) { + effectSetStates.set( + instr.value.callee.loc.identifierName, + instr.value.callee, + ); + } switch (instr.value.kind) { case 'Primitive': case 'JSXText': @@ -434,32 +488,24 @@ function validateEffect( instr.value.args.length === 1 && instr.value.args[0].kind === 'Identifier' ) { - const deps = values.get(instr.value.args[0].identifier.id); - console.log('deps', deps); - if (deps != null && new Set(deps).size === effectDeps.length) { - // console.log('setState arg', instr.value.args[0].identifier.id); - // console.log('effectInvalidlyDerived', effectInvalidlyDerived); - // console.log('derivedTuple', derivedTuple); - const propSources = derivedTuple.get( - instr.value.args[0].identifier.id, - ); - - console.log('Final reference', propSources); - if (propSources !== undefined) { - setStateCalls.push({ - loc: instr.value.callee.loc, - propsSources: propSources.sources, - }); - } else { - setStateCalls.push({ - loc: instr.value.callee.loc, - propsSources: undefined, - }); - } + const propSources = derivedTuple.get( + instr.value.args[0].identifier.id, + ); + + if (propSources !== undefined) { + setStateCallsInEffect.push({ + loc: instr.value.callee.loc, + setStateId: instr.value.callee.identifier.id, + invalidDeps: new Map([ + [instr.value.args[0].identifier, propSources.sources], + ]), + }); } else { - // doesn't depend on all deps - console.log('early return 3'); - return; + setStateCallsInEffect.push({ + loc: instr.value.callee.loc, + setStateId: instr.value.callee.identifier.id, + invalidDeps: undefined, + }); } } break; @@ -470,6 +516,7 @@ function validateEffect( } } } + for (const operand of eachTerminalOperand(block.terminal)) { if (values.has(operand.identifier.id)) { return; @@ -478,31 +525,36 @@ function validateEffect( seenBlocks.add(block.id); } - console.log('setStateCalls', setStateCalls); - for (const call of setStateCalls) { - if (call.propsSources != null) { - const propNames = call.propsSources - .map(place => place.identifier.name?.value) - .join(', '); - const propInfo = propNames != null ? ` (from props '${propNames}')` : ''; + // need to track if the setState call has been used elsewhere + // if it is then the solution should be to lift the state up to the parent component + // if not the solution should be to calculate the value in rende + // + // If the same setState is used both inside and outside the effect + + for (const call of setStateCallsInEffect) { + if (call.invalidDeps != null) { + let propNames = ''; + for (const [, places] of call.invalidDeps.entries()) { + const placeNames = places + .map(place => place.identifier.name?.value) + .join(', '); + propNames += `[${placeNames}], `; + } + propNames = propNames.slice(0, -2); + const propInfo = propNames ? ` (from props '${propNames}')` : ''; errors.push({ - reason: `Consider lifting state up to the parent component to make this a controlled component. (https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes)`, - description: `You are using props${propInfo} to update local state in an effect.`, - severity: ErrorSeverity.InvalidReact, + errorType: 'HoistState', + propInfo: propInfo, loc: call.loc, - suggestions: null, + setStateId: call.setStateId, }); } else { errors.push({ - reason: - 'You may not need this effect. Values derived from state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)', - description: - 'This effect updates state based on other state values. ' + - 'Consider calculating this value directly during render', - severity: ErrorSeverity.InvalidReact, + errorType: 'CalculateInRender', + propInfo: undefined, loc: call.loc, - suggestions: null, + setStateId: call.setStateId, }); } }