diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts index bbec25a57cc0c..7fd44c29dccf6 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts @@ -440,14 +440,6 @@ class Context { // Checks if identifier is a valid dependency in the current scope #checkValidDependency(maybeDependency: ReactiveScopeDependency): boolean { - // ref.current access is not a valid dep - if ( - isUseRefType(maybeDependency.identifier) && - maybeDependency.path.at(0)?.property === 'current' - ) { - return false; - } - // ref value is not a valid dep if (isRefValueType(maybeDependency.identifier)) { return false; @@ -549,6 +541,16 @@ class Context { }); } + // ref.current access is not a valid dep + if ( + isUseRefType(maybeDependency.identifier) && + maybeDependency.path.at(0)?.property === 'current' + ) { + maybeDependency = { + identifier: maybeDependency.identifier, + path: [], + }; + } if (this.#checkValidDependency(maybeDependency)) { this.#dependencies.value!.push(maybeDependency); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CollectReactiveIdentifiers.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CollectReactiveIdentifiers.ts index 610e93965fc1a..385123400581f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CollectReactiveIdentifiers.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CollectReactiveIdentifiers.ts @@ -12,6 +12,8 @@ import { PrunedReactiveScopeBlock, ReactiveFunction, isPrimitiveType, + isUseRefType, + Identifier, } from '../HIR/HIR'; import {ReactiveFunctionVisitor, visitReactiveFunction} from './visitors'; @@ -50,13 +52,21 @@ class Visitor extends ReactiveFunctionVisitor> { this.traversePrunedScope(scopeBlock, state); for (const [id, decl] of scopeBlock.scope.declarations) { - if (!isPrimitiveType(decl.identifier)) { + if ( + !isPrimitiveType(decl.identifier) && + !isStableRefType(decl.identifier, state) + ) { state.add(id); } } } } - +function isStableRefType( + identifier: Identifier, + reactiveIdentifiers: Set, +): boolean { + return isUseRefType(identifier) && !reactiveIdentifiers.has(identifier.id); +} /* * Computes a set of identifiers which are reactive, using the analysis previously performed * in `InferReactivePlaces`. diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-nonreactive-ref.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-nonreactive-ref.expect.md deleted file mode 100644 index f79df15d52492..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-nonreactive-ref.expect.md +++ /dev/null @@ -1,89 +0,0 @@ - -## Input - -```javascript -import {useRef} from 'react'; -import {Stringify} from 'shared-runtime'; - -/** - * Bug: we're currently filtering out `ref.current` dependencies in - * `propagateScopeDependencies:checkValidDependency`. This is incorrect. - * Instead, we should always take a dependency on ref values (the outer box) as - * they may be reactive. Pruning should be done in - * `pruneNonReactiveDependencies` - * - * Found differences in evaluator results - * Non-forget (expected): - * (kind: ok) - *
{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}
- *
{"cb":{"kind":"Function","result":2},"shouldInvokeFns":true}
- * Forget: - * (kind: ok) - *
{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}
- *
{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}
- */ -function Component({cond}) { - const ref1 = useRef(1); - const ref2 = useRef(2); - const ref = cond ? ref1 : ref2; - const cb = () => ref.current; - return ; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{cond: true}], - sequentialRenders: [{cond: true}, {cond: false}], -}; - -``` - -## Code - -```javascript -import { c as _c } from "react/compiler-runtime"; -import { useRef } from "react"; -import { Stringify } from "shared-runtime"; - -/** - * Bug: we're currently filtering out `ref.current` dependencies in - * `propagateScopeDependencies:checkValidDependency`. This is incorrect. - * Instead, we should always take a dependency on ref values (the outer box) as - * they may be reactive. Pruning should be done in - * `pruneNonReactiveDependencies` - * - * Found differences in evaluator results - * Non-forget (expected): - * (kind: ok) - *
{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}
- *
{"cb":{"kind":"Function","result":2},"shouldInvokeFns":true}
- * Forget: - * (kind: ok) - *
{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}
- *
{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}
- */ -function Component(t0) { - const $ = _c(1); - const { cond } = t0; - const ref1 = useRef(1); - const ref2 = useRef(2); - const ref = cond ? ref1 : ref2; - let t1; - if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - const cb = () => ref.current; - t1 = ; - $[0] = t1; - } else { - t1 = $[0]; - } - return t1; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{ cond: true }], - sequentialRenders: [{ cond: true }, { cond: false }], -}; - -``` - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-nonreactive-ref.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-nonreactive-ref.tsx deleted file mode 100644 index a0115f2df3cfb..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-nonreactive-ref.tsx +++ /dev/null @@ -1,33 +0,0 @@ -import {useRef} from 'react'; -import {Stringify} from 'shared-runtime'; - -/** - * Bug: we're currently filtering out `ref.current` dependencies in - * `propagateScopeDependencies:checkValidDependency`. This is incorrect. - * Instead, we should always take a dependency on ref values (the outer box) as - * they may be reactive. Pruning should be done in - * `pruneNonReactiveDependencies` - * - * Found differences in evaluator results - * Non-forget (expected): - * (kind: ok) - *
{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}
- *
{"cb":{"kind":"Function","result":2},"shouldInvokeFns":true}
- * Forget: - * (kind: ok) - *
{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}
- *
{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}
- */ -function Component({cond}) { - const ref1 = useRef(1); - const ref2 = useRef(2); - const ref = cond ? ref1 : ref2; - const cb = () => ref.current; - return ; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{cond: true}], - sequentialRenders: [{cond: true}, {cond: false}], -}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-ref-param.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-ref-param.expect.md new file mode 100644 index 0000000000000..c8922ab0c46fa --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-ref-param.expect.md @@ -0,0 +1,102 @@ + +## Input + +```javascript +import {useRef, forwardRef} from 'react'; +import {Stringify} from 'shared-runtime'; + +/** + * Fixture showing that Ref types may be reactive. + * We should always take a dependency on ref values (the outer box) as + * they may be reactive. Pruning should be done in + * `pruneNonReactiveDependencies` + */ + +function Parent({cond}) { + const ref1 = useRef(1); + const ref2 = useRef(2); + const ref = cond ? ref1 : ref2; + return ; +} + +function ChildImpl(_props, ref) { + const cb = () => ref.current; + return ; +} + +const Child = forwardRef(ChildImpl); + +export const FIXTURE_ENTRYPOINT = { + fn: Parent, + params: [{cond: true}], + sequentialRenders: [{cond: true}, {cond: false}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { useRef, forwardRef } from "react"; +import { Stringify } from "shared-runtime"; + +/** + * Fixture showing that Ref types may be reactive. + * We should always take a dependency on ref values (the outer box) as + * they may be reactive. Pruning should be done in + * `pruneNonReactiveDependencies` + */ + +function Parent(t0) { + const $ = _c(2); + const { cond } = t0; + const ref1 = useRef(1); + const ref2 = useRef(2); + const ref = cond ? ref1 : ref2; + let t1; + if ($[0] !== ref) { + t1 = ; + $[0] = ref; + $[1] = t1; + } else { + t1 = $[1]; + } + return t1; +} + +function ChildImpl(_props, ref) { + const $ = _c(4); + let t0; + if ($[0] !== ref) { + t0 = () => ref.current; + $[0] = ref; + $[1] = t0; + } else { + t0 = $[1]; + } + const cb = t0; + let t1; + if ($[2] !== cb) { + t1 = ; + $[2] = cb; + $[3] = t1; + } else { + t1 = $[3]; + } + return t1; +} + +const Child = forwardRef(ChildImpl); + +export const FIXTURE_ENTRYPOINT = { + fn: Parent, + params: [{ cond: true }], + sequentialRenders: [{ cond: true }, { cond: false }], +}; + +``` + +### Eval output +(kind: ok)
{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}
+
{"cb":{"kind":"Function","result":2},"shouldInvokeFns":true}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-ref-param.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-ref-param.tsx new file mode 100644 index 0000000000000..c3eea7a24379f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-ref-param.tsx @@ -0,0 +1,29 @@ +import {useRef, forwardRef} from 'react'; +import {Stringify} from 'shared-runtime'; + +/** + * Fixture showing that Ref types may be reactive. + * We should always take a dependency on ref values (the outer box) as + * they may be reactive. Pruning should be done in + * `pruneNonReactiveDependencies` + */ + +function Parent({cond}) { + const ref1 = useRef(1); + const ref2 = useRef(2); + const ref = cond ? ref1 : ref2; + return ; +} + +function ChildImpl(_props, ref) { + const cb = () => ref.current; + return ; +} + +const Child = forwardRef(ChildImpl); + +export const FIXTURE_ENTRYPOINT = { + fn: Parent, + params: [{cond: true}], + sequentialRenders: [{cond: true}, {cond: false}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-ref.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-ref.expect.md new file mode 100644 index 0000000000000..c3876c22924fe --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-ref.expect.md @@ -0,0 +1,79 @@ + +## Input + +```javascript +import {useRef} from 'react'; +import {Stringify} from 'shared-runtime'; + +/** + * Fixture showing that Ref types may be reactive. + * We should always take a dependency on ref values (the outer box) as + * they may be reactive. Pruning should be done in + * `pruneNonReactiveDependencies` + */ +function Component({cond}) { + const ref1 = useRef(1); + const ref2 = useRef(2); + const ref = cond ? ref1 : ref2; + const cb = () => ref.current; + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{cond: true}], + sequentialRenders: [{cond: true}, {cond: false}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { useRef } from "react"; +import { Stringify } from "shared-runtime"; + +/** + * Fixture showing that Ref types may be reactive. + * We should always take a dependency on ref values (the outer box) as + * they may be reactive. Pruning should be done in + * `pruneNonReactiveDependencies` + */ +function Component(t0) { + const $ = _c(4); + const { cond } = t0; + const ref1 = useRef(1); + const ref2 = useRef(2); + const ref = cond ? ref1 : ref2; + let t1; + if ($[0] !== ref) { + t1 = () => ref.current; + $[0] = ref; + $[1] = t1; + } else { + t1 = $[1]; + } + const cb = t1; + let t2; + if ($[2] !== cb) { + t2 = ; + $[2] = cb; + $[3] = t2; + } else { + t2 = $[3]; + } + return t2; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ cond: true }], + sequentialRenders: [{ cond: true }, { cond: false }], +}; + +``` + +### Eval output +(kind: ok)
{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}
+
{"cb":{"kind":"Function","result":2},"shouldInvokeFns":true}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-ref.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-ref.tsx new file mode 100644 index 0000000000000..db95c5c204a3a --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-ref.tsx @@ -0,0 +1,22 @@ +import {useRef} from 'react'; +import {Stringify} from 'shared-runtime'; + +/** + * Fixture showing that Ref types may be reactive. + * We should always take a dependency on ref values (the outer box) as + * they may be reactive. Pruning should be done in + * `pruneNonReactiveDependencies` + */ +function Component({cond}) { + const ref1 = useRef(1); + const ref2 = useRef(2); + const ref = cond ? ref1 : ref2; + const cb = () => ref.current; + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{cond: true}], + sequentialRenders: [{cond: true}, {cond: false}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/ref-parameter-mutate-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/ref-parameter-mutate-in-effect.expect.md index 8b5a2eb1a0b35..7a10cfe48e2e4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/ref-parameter-mutate-in-effect.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/ref-parameter-mutate-in-effect.expect.md @@ -13,7 +13,7 @@ function Foo(props, ref) { export const FIXTURE_ENTRYPOINT = { fn: Foo, - params: [{bar: 'foo'}, {ref: {cuurrent: 1}}], + params: [{bar: 'foo'}, {ref: {current: 1}}], isComponent: true, }; @@ -26,35 +26,39 @@ import { c as _c } from "react/compiler-runtime"; import { useEffect } from "react"; function Foo(props, ref) { - const $ = _c(4); + const $ = _c(5); let t0; - let t1; - if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + if ($[0] !== ref) { t0 = () => { ref.current = 2; }; + $[0] = ref; + $[1] = t0; + } else { + t0 = $[1]; + } + let t1; + if ($[2] === Symbol.for("react.memo_cache_sentinel")) { t1 = []; - $[0] = t0; - $[1] = t1; + $[2] = t1; } else { - t0 = $[0]; - t1 = $[1]; + t1 = $[2]; } useEffect(t0, t1); let t2; - if ($[2] !== props.bar) { + if ($[3] !== props.bar) { t2 =
{props.bar}
; - $[2] = props.bar; - $[3] = t2; + $[3] = props.bar; + $[4] = t2; } else { - t2 = $[3]; + t2 = $[4]; } return t2; } export const FIXTURE_ENTRYPOINT = { fn: Foo, - params: [{ bar: "foo" }, { ref: { cuurrent: 1 } }], + params: [{ bar: "foo" }, { ref: { current: 1 } }], isComponent: true, }; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/ref-parameter-mutate-in-effect.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/ref-parameter-mutate-in-effect.js index c60141e8d5d81..ffd9eae08d66f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/ref-parameter-mutate-in-effect.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/ref-parameter-mutate-in-effect.js @@ -9,6 +9,6 @@ function Foo(props, ref) { export const FIXTURE_ENTRYPOINT = { fn: Foo, - params: [{bar: 'foo'}, {ref: {cuurrent: 1}}], + params: [{bar: 'foo'}, {ref: {current: 1}}], isComponent: true, }; diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts index 6c136f6310928..1971fe0d33161 100644 --- a/compiler/packages/snap/src/SproutTodoFilter.ts +++ b/compiler/packages/snap/src/SproutTodoFilter.ts @@ -484,7 +484,6 @@ const skipFilter = new Set([ 'bug-aliased-capture-mutate', 'bug-functiondecl-hoisting', 'bug-try-catch-maybe-null-dependency', - 'bug-nonreactive-ref', 'reduce-reactive-deps/bug-infer-function-cond-access-not-hoisted', 'bug-invalid-phi-as-dependency', 'reduce-reactive-deps/bug-merge-uncond-optional-chain-and-cond',