@samitouri / QOS-React / commits / 4972718c26

[compiler] Fix: ref.current now correctly reactive (#31521)

We were previously filtering out `ref.current` dependencies in propagateScopeDependencies:checkValidDependency`. This is incorrect. Instead, we now always take a dependency on ref values (the outer box) as they may be reactive. Pruning is done in pruneNonReactiveDependencies. This PR includes a small patch to `collectReactiveIdentifier`. Prior to this, we conservatively assumed that pruned scopes always produced reactive declarations. This assumption fixed a bug with non-reactivity, but some of these declarations are `useRef` calls. Now we have special handling for this case ```js // This often produces a pruned scope React.useRef(1); ``` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31521). * #31202 * #31203 * #31201 * #31200 * __->__ #31521

mofeiZ committed Nov 15, 2024 at 13:05 UTC 4972718c264f8f92ea56b86fde6aab610e876717
11 files changed +272 -147
compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts
+10 -8
@@ -440,14 +440,6 @@ class Context {
440
441 // Checks if identifier is a valid dependency in the current scope
442 #checkValidDependency(maybeDependency: ReactiveScopeDependency): boolean {
443 - // ref.current access is not a valid dep
444 - if (
445 - isUseRefType(maybeDependency.identifier) &&
446 - maybeDependency.path.at(0)?.property === 'current'
447 - ) {
448 - return false;
449 - }
450 -
443 // ref value is not a valid dep
444 if (isRefValueType(maybeDependency.identifier)) {
445 return false;
@@ -549,6 +541,16 @@ class Context {
541 });
542 }
543
544 + // ref.current access is not a valid dep
545 + if (
546 + isUseRefType(maybeDependency.identifier) &&
547 + maybeDependency.path.at(0)?.property === 'current'
548 + ) {
549 + maybeDependency = {
550 + identifier: maybeDependency.identifier,
551 + path: [],
552 + };
553 + }
554 if (this.#checkValidDependency(maybeDependency)) {
555 this.#dependencies.value!.push(maybeDependency);
556 }
compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CollectReactiveIdentifiers.ts
+12 -2
@@ -12,6 +12,8 @@ import {
12 PrunedReactiveScopeBlock,
13 ReactiveFunction,
14 isPrimitiveType,
15 + isUseRefType,
16 + Identifier,
17 } from '../HIR/HIR';
18 import {ReactiveFunctionVisitor, visitReactiveFunction} from './visitors';
19
@@ -50,13 +52,21 @@ class Visitor extends ReactiveFunctionVisitor<Set<IdentifierId>> {
52 this.traversePrunedScope(scopeBlock, state);
53
54 for (const [id, decl] of scopeBlock.scope.declarations) {
53 - if (!isPrimitiveType(decl.identifier)) {
55 + if (
56 + !isPrimitiveType(decl.identifier) &&
57 + !isStableRefType(decl.identifier, state)
58 + ) {
59 state.add(id);
60 }
61 }
62 }
63 }
59 -
64 +function isStableRefType(
65 + identifier: Identifier,
66 + reactiveIdentifiers: Set<IdentifierId>,
67 +): boolean {
68 + return isUseRefType(identifier) && !reactiveIdentifiers.has(identifier.id);
69 +}
70 /*
71 * Computes a set of identifiers which are reactive, using the analysis previously performed
72 * in `InferReactivePlaces`.
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-nonreactive-ref.expect.md deleted
-89
@@ -1,89 +0,0 @@
1 -
2 -## Input
3 -
4 -```javascript
5 -import {useRef} from 'react';
6 -import {Stringify} from 'shared-runtime';
7 -
8 -/**
9 - * Bug: we're currently filtering out `ref.current` dependencies in
10 - * `propagateScopeDependencies:checkValidDependency`. This is incorrect.
11 - * Instead, we should always take a dependency on ref values (the outer box) as
12 - * they may be reactive. Pruning should be done in
13 - * `pruneNonReactiveDependencies`
14 - *
15 - * Found differences in evaluator results
16 - * Non-forget (expected):
17 - * (kind: ok)
18 - * <div>{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}</div>
19 - * <div>{"cb":{"kind":"Function","result":2},"shouldInvokeFns":true}</div>
20 - * Forget:
21 - * (kind: ok)
22 - * <div>{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}</div>
23 - * <div>{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}</div>
24 - */
25 -function Component({cond}) {
26 - const ref1 = useRef(1);
27 - const ref2 = useRef(2);
28 - const ref = cond ? ref1 : ref2;
29 - const cb = () => ref.current;
30 - return <Stringify cb={cb} shouldInvokeFns={true} />;
31 -}
32 -
33 -export const FIXTURE_ENTRYPOINT = {
34 - fn: Component,
35 - params: [{cond: true}],
36 - sequentialRenders: [{cond: true}, {cond: false}],
37 -};
38 -
39 -```
40 -
41 -## Code
42 -
43 -```javascript
44 -import { c as _c } from "react/compiler-runtime";
45 -import { useRef } from "react";
46 -import { Stringify } from "shared-runtime";
47 -
48 -/**
49 - * Bug: we're currently filtering out `ref.current` dependencies in
50 - * `propagateScopeDependencies:checkValidDependency`. This is incorrect.
51 - * Instead, we should always take a dependency on ref values (the outer box) as
52 - * they may be reactive. Pruning should be done in
53 - * `pruneNonReactiveDependencies`
54 - *
55 - * Found differences in evaluator results
56 - * Non-forget (expected):
57 - * (kind: ok)
58 - * <div>{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}</div>
59 - * <div>{"cb":{"kind":"Function","result":2},"shouldInvokeFns":true}</div>
60 - * Forget:
61 - * (kind: ok)
62 - * <div>{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}</div>
63 - * <div>{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}</div>
64 - */
65 -function Component(t0) {
66 - const $ = _c(1);
67 - const { cond } = t0;
68 - const ref1 = useRef(1);
69 - const ref2 = useRef(2);
70 - const ref = cond ? ref1 : ref2;
71 - let t1;
72 - if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
73 - const cb = () => ref.current;
74 - t1 = <Stringify cb={cb} shouldInvokeFns={true} />;
75 - $[0] = t1;
76 - } else {
77 - t1 = $[0];
78 - }
79 - return t1;
80 -}
81 -
82 -export const FIXTURE_ENTRYPOINT = {
83 - fn: Component,
84 - params: [{ cond: true }],
85 - sequentialRenders: [{ cond: true }, { cond: false }],
86 -};
87 -
88 -```
89 -
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-nonreactive-ref.tsx deleted
-33
@@ -1,33 +0,0 @@
1 -import {useRef} from 'react';
2 -import {Stringify} from 'shared-runtime';
3 -
4 -/**
5 - * Bug: we're currently filtering out `ref.current` dependencies in
6 - * `propagateScopeDependencies:checkValidDependency`. This is incorrect.
7 - * Instead, we should always take a dependency on ref values (the outer box) as
8 - * they may be reactive. Pruning should be done in
9 - * `pruneNonReactiveDependencies`
10 - *
11 - * Found differences in evaluator results
12 - * Non-forget (expected):
13 - * (kind: ok)
14 - * <div>{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}</div>
15 - * <div>{"cb":{"kind":"Function","result":2},"shouldInvokeFns":true}</div>
16 - * Forget:
17 - * (kind: ok)
18 - * <div>{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}</div>
19 - * <div>{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}</div>
20 - */
21 -function Component({cond}) {
22 - const ref1 = useRef(1);
23 - const ref2 = useRef(2);
24 - const ref = cond ? ref1 : ref2;
25 - const cb = () => ref.current;
26 - return <Stringify cb={cb} shouldInvokeFns={true} />;
27 -}
28 -
29 -export const FIXTURE_ENTRYPOINT = {
30 - fn: Component,
31 - params: [{cond: true}],
32 - sequentialRenders: [{cond: true}, {cond: false}],
33 -};
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-ref-param.expect.md new
+102
@@ -0,0 +1,102 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +import {useRef, forwardRef} from 'react';
6 +import {Stringify} from 'shared-runtime';
7 +
8 +/**
9 + * Fixture showing that Ref types may be reactive.
10 + * We should always take a dependency on ref values (the outer box) as
11 + * they may be reactive. Pruning should be done in
12 + * `pruneNonReactiveDependencies`
13 + */
14 +
15 +function Parent({cond}) {
16 + const ref1 = useRef(1);
17 + const ref2 = useRef(2);
18 + const ref = cond ? ref1 : ref2;
19 + return <Child ref={ref} />;
20 +}
21 +
22 +function ChildImpl(_props, ref) {
23 + const cb = () => ref.current;
24 + return <Stringify cb={cb} shouldInvokeFns={true} />;
25 +}
26 +
27 +const Child = forwardRef(ChildImpl);
28 +
29 +export const FIXTURE_ENTRYPOINT = {
30 + fn: Parent,
31 + params: [{cond: true}],
32 + sequentialRenders: [{cond: true}, {cond: false}],
33 +};
34 +
35 +```
36 +
37 +## Code
38 +
39 +```javascript
40 +import { c as _c } from "react/compiler-runtime";
41 +import { useRef, forwardRef } from "react";
42 +import { Stringify } from "shared-runtime";
43 +
44 +/**
45 + * Fixture showing that Ref types may be reactive.
46 + * We should always take a dependency on ref values (the outer box) as
47 + * they may be reactive. Pruning should be done in
48 + * `pruneNonReactiveDependencies`
49 + */
50 +
51 +function Parent(t0) {
52 + const $ = _c(2);
53 + const { cond } = t0;
54 + const ref1 = useRef(1);
55 + const ref2 = useRef(2);
56 + const ref = cond ? ref1 : ref2;
57 + let t1;
58 + if ($[0] !== ref) {
59 + t1 = <Child ref={ref} />;
60 + $[0] = ref;
61 + $[1] = t1;
62 + } else {
63 + t1 = $[1];
64 + }
65 + return t1;
66 +}
67 +
68 +function ChildImpl(_props, ref) {
69 + const $ = _c(4);
70 + let t0;
71 + if ($[0] !== ref) {
72 + t0 = () => ref.current;
73 + $[0] = ref;
74 + $[1] = t0;
75 + } else {
76 + t0 = $[1];
77 + }
78 + const cb = t0;
79 + let t1;
80 + if ($[2] !== cb) {
81 + t1 = <Stringify cb={cb} shouldInvokeFns={true} />;
82 + $[2] = cb;
83 + $[3] = t1;
84 + } else {
85 + t1 = $[3];
86 + }
87 + return t1;
88 +}
89 +
90 +const Child = forwardRef(ChildImpl);
91 +
92 +export const FIXTURE_ENTRYPOINT = {
93 + fn: Parent,
94 + params: [{ cond: true }],
95 + sequentialRenders: [{ cond: true }, { cond: false }],
96 +};
97 +
98 +```
99 +
100 +### Eval output
101 +(kind: ok) <div>{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}</div>
102 +<div>{"cb":{"kind":"Function","result":2},"shouldInvokeFns":true}</div>
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-ref-param.tsx new
+29
@@ -0,0 +1,29 @@
1 +import {useRef, forwardRef} from 'react';
2 +import {Stringify} from 'shared-runtime';
3 +
4 +/**
5 + * Fixture showing that Ref types may be reactive.
6 + * We should always take a dependency on ref values (the outer box) as
7 + * they may be reactive. Pruning should be done in
8 + * `pruneNonReactiveDependencies`
9 + */
10 +
11 +function Parent({cond}) {
12 + const ref1 = useRef(1);
13 + const ref2 = useRef(2);
14 + const ref = cond ? ref1 : ref2;
15 + return <Child ref={ref} />;
16 +}
17 +
18 +function ChildImpl(_props, ref) {
19 + const cb = () => ref.current;
20 + return <Stringify cb={cb} shouldInvokeFns={true} />;
21 +}
22 +
23 +const Child = forwardRef(ChildImpl);
24 +
25 +export const FIXTURE_ENTRYPOINT = {
26 + fn: Parent,
27 + params: [{cond: true}],
28 + sequentialRenders: [{cond: true}, {cond: false}],
29 +};
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-ref.expect.md new
+79
@@ -0,0 +1,79 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +import {useRef} from 'react';
6 +import {Stringify} from 'shared-runtime';
7 +
8 +/**
9 + * Fixture showing that Ref types may be reactive.
10 + * We should always take a dependency on ref values (the outer box) as
11 + * they may be reactive. Pruning should be done in
12 + * `pruneNonReactiveDependencies`
13 + */
14 +function Component({cond}) {
15 + const ref1 = useRef(1);
16 + const ref2 = useRef(2);
17 + const ref = cond ? ref1 : ref2;
18 + const cb = () => ref.current;
19 + return <Stringify cb={cb} shouldInvokeFns={true} />;
20 +}
21 +
22 +export const FIXTURE_ENTRYPOINT = {
23 + fn: Component,
24 + params: [{cond: true}],
25 + sequentialRenders: [{cond: true}, {cond: false}],
26 +};
27 +
28 +```
29 +
30 +## Code
31 +
32 +```javascript
33 +import { c as _c } from "react/compiler-runtime";
34 +import { useRef } from "react";
35 +import { Stringify } from "shared-runtime";
36 +
37 +/**
38 + * Fixture showing that Ref types may be reactive.
39 + * We should always take a dependency on ref values (the outer box) as
40 + * they may be reactive. Pruning should be done in
41 + * `pruneNonReactiveDependencies`
42 + */
43 +function Component(t0) {
44 + const $ = _c(4);
45 + const { cond } = t0;
46 + const ref1 = useRef(1);
47 + const ref2 = useRef(2);
48 + const ref = cond ? ref1 : ref2;
49 + let t1;
50 + if ($[0] !== ref) {
51 + t1 = () => ref.current;
52 + $[0] = ref;
53 + $[1] = t1;
54 + } else {
55 + t1 = $[1];
56 + }
57 + const cb = t1;
58 + let t2;
59 + if ($[2] !== cb) {
60 + t2 = <Stringify cb={cb} shouldInvokeFns={true} />;
61 + $[2] = cb;
62 + $[3] = t2;
63 + } else {
64 + t2 = $[3];
65 + }
66 + return t2;
67 +}
68 +
69 +export const FIXTURE_ENTRYPOINT = {
70 + fn: Component,
71 + params: [{ cond: true }],
72 + sequentialRenders: [{ cond: true }, { cond: false }],
73 +};
74 +
75 +```
76 +
77 +### Eval output
78 +(kind: ok) <div>{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}</div>
79 +<div>{"cb":{"kind":"Function","result":2},"shouldInvokeFns":true}</div>
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-ref.tsx new
+22
@@ -0,0 +1,22 @@
1 +import {useRef} from 'react';
2 +import {Stringify} from 'shared-runtime';
3 +
4 +/**
5 + * Fixture showing that Ref types may be reactive.
6 + * We should always take a dependency on ref values (the outer box) as
7 + * they may be reactive. Pruning should be done in
8 + * `pruneNonReactiveDependencies`
9 + */
10 +function Component({cond}) {
11 + const ref1 = useRef(1);
12 + const ref2 = useRef(2);
13 + const ref = cond ? ref1 : ref2;
14 + const cb = () => ref.current;
15 + return <Stringify cb={cb} shouldInvokeFns={true} />;
16 +}
17 +
18 +export const FIXTURE_ENTRYPOINT = {
19 + fn: Component,
20 + params: [{cond: true}],
21 + sequentialRenders: [{cond: true}, {cond: false}],
22 +};
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/ref-parameter-mutate-in-effect.expect.md
+17 -13
@@ -13,7 +13,7 @@ function Foo(props, ref) {
13
14 export const FIXTURE_ENTRYPOINT = {
15 fn: Foo,
16 - params: [{bar: 'foo'}, {ref: {cuurrent: 1}}],
16 + params: [{bar: 'foo'}, {ref: {current: 1}}],
17 isComponent: true,
18 };
19
@@ -26,35 +26,39 @@ import { c as _c } from "react/compiler-runtime";
26 import { useEffect } from "react";
27
28 function Foo(props, ref) {
29 - const $ = _c(4);
29 + const $ = _c(5);
30 let t0;
31 - let t1;
32 - if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
31 + if ($[0] !== ref) {
32 t0 = () => {
33 ref.current = 2;
34 };
35 + $[0] = ref;
36 + $[1] = t0;
37 + } else {
38 + t0 = $[1];
39 + }
40 + let t1;
41 + if ($[2] === Symbol.for("react.memo_cache_sentinel")) {
42 t1 = [];
37 - $[0] = t0;
38 - $[1] = t1;
43 + $[2] = t1;
44 } else {
40 - t0 = $[0];
41 - t1 = $[1];
45 + t1 = $[2];
46 }
47 useEffect(t0, t1);
48 let t2;
45 - if ($[2] !== props.bar) {
49 + if ($[3] !== props.bar) {
50 t2 = <div>{props.bar}</div>;
47 - $[2] = props.bar;
48 - $[3] = t2;
51 + $[3] = props.bar;
52 + $[4] = t2;
53 } else {
50 - t2 = $[3];
54 + t2 = $[4];
55 }
56 return t2;
57 }
58
59 export const FIXTURE_ENTRYPOINT = {
60 fn: Foo,
57 - params: [{ bar: "foo" }, { ref: { cuurrent: 1 } }],
61 + params: [{ bar: "foo" }, { ref: { current: 1 } }],
62 isComponent: true,
63 };
64
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/ref-parameter-mutate-in-effect.js
+1 -1
@@ -9,6 +9,6 @@ function Foo(props, ref) {
9
10 export const FIXTURE_ENTRYPOINT = {
11 fn: Foo,
12 - params: [{bar: 'foo'}, {ref: {cuurrent: 1}}],
12 + params: [{bar: 'foo'}, {ref: {current: 1}}],
13 isComponent: true,
14 };
compiler/packages/snap/src/SproutTodoFilter.ts
-1
@@ -484,7 +484,6 @@ const skipFilter = new Set([
484 'bug-aliased-capture-mutate',
485 'bug-functiondecl-hoisting',
486 'bug-try-catch-maybe-null-dependency',
487 - 'bug-nonreactive-ref',
487 'reduce-reactive-deps/bug-infer-function-cond-access-not-hoisted',
488 'bug-invalid-phi-as-dependency',
489 'reduce-reactive-deps/bug-merge-uncond-optional-chain-and-cond',