compiler: ValidateNoRefInRender detects writes of refs

Improves ValidateNoRefAccessInRender, detecting modifications of refs during render. Fixes #29161 ghstack-source-id: 99078b3cea5b2d9019dbf77ede9c2e4cd9fbfd27 Pull Request resolved: #29170
facebook · May 29, 2024 · e37f156 · e37f156
1 parent afb2c39
commit e37f156
Show file tree

Hide file tree

Showing 11 changed files with 132 additions and 18 deletions.
diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccesInRender.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccesInRender.ts
@@ -10,6 +10,7 @@ import {
   HIRFunction,
   IdentifierId,
   Place,
+  SourceLocation,
   isRefValueType,
   isUseRefType,
 } from "../HIR";
@@ -117,7 +118,12 @@ function validateNoRefAccessInRenderImpl(
         case "MethodCall": {
           if (!isEffectHook(instr.value.property.identifier)) {
             for (const operand of eachInstructionValueOperand(instr.value)) {
-              validateNoRefAccess(errors, refAccessingFunctions, operand);
+              validateNoRefAccess(
+                errors,
+                refAccessingFunctions,
+                operand,
+                operand.loc
+              );
             }
           }
           break;
@@ -138,15 +144,43 @@ function validateNoRefAccessInRenderImpl(
               });
             }
             for (const operand of eachInstructionValueOperand(instr.value)) {
-              validateNoRefAccess(errors, refAccessingFunctions, operand);
+              validateNoRefAccess(
+                errors,
+                refAccessingFunctions,
+                operand,
+                operand.loc
+              );
             }
           }
           break;
         }
         case "ObjectExpression":
         case "ArrayExpression": {
           for (const operand of eachInstructionValueOperand(instr.value)) {
-            validateNoRefAccess(errors, refAccessingFunctions, operand);
+            validateNoRefAccess(
+              errors,
+              refAccessingFunctions,
+              operand,
+              operand.loc
+            );
+          }
+          break;
+        }
+        case "PropertyDelete":
+        case "PropertyStore":
+        case "ComputedDelete":
+        case "ComputedStore": {
+          validateNoRefAccess(
+            errors,
+            refAccessingFunctions,
+            instr.value.object,
+            instr.loc
+          );
+          for (const operand of eachInstructionValueOperand(instr.value)) {
+            if (operand === instr.value.object) {
+              continue;
+            }
+            validateNoRefValueAccess(errors, refAccessingFunctions, operand);
           }
           break;
         }
@@ -172,12 +206,12 @@ function validateNoRefAccessInRenderImpl(
 
 function validateNoRefValueAccess(
   errors: CompilerError,
-  unconditionalSetStateFunctions: Set<IdentifierId>,
+  refAccessingFunctions: Set<IdentifierId>,
   operand: Place
 ): void {
   if (
     isRefValueType(operand.identifier) ||
-    unconditionalSetStateFunctions.has(operand.identifier.id)
+    refAccessingFunctions.has(operand.identifier.id)
   ) {
     errors.push({
       severity: ErrorSeverity.InvalidReact,
@@ -192,20 +226,25 @@ function validateNoRefValueAccess(
 
 function validateNoRefAccess(
   errors: CompilerError,
-  unconditionalSetStateFunctions: Set<IdentifierId>,
-  operand: Place
+  refAccessingFunctions: Set<IdentifierId>,
+  operand: Place,
+  loc: SourceLocation
 ): void {
   if (
     isRefValueType(operand.identifier) ||
     isUseRefType(operand.identifier) ||
-    unconditionalSetStateFunctions.has(operand.identifier.id)
+    refAccessingFunctions.has(operand.identifier.id)
   ) {
     errors.push({
       severity: ErrorSeverity.InvalidReact,
       reason:
         "Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef)",
-      loc: operand.loc,
-      description: `Cannot access ref value at ${printPlace(operand)}`,
+      loc: loc,
+      description:
+        operand.identifier.name !== null &&
+        operand.identifier.name.kind === "named"
+          ? `Cannot access ref value \`${operand.identifier.name.value}\``
+          : null,
       suggestions: null,
     });
   }

diff --git a/...compiler/error.invalid-aliased-ref-in-callback-invoked-during-render-.expect.md b/...compiler/error.invalid-aliased-ref-in-callback-invoked-during-render-.expect.md
@@ -22,7 +22,7 @@ function Component(props) {
    7 |     return <Foo item={item} current={current} />;
    8 |   };
 >  9 |   return <Items>{props.items.map((item) => renderItem(item))}</Items>;
-     |                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at mutate? $64[13:15]:TObject<BuiltInFunction> (9:9)
+     |                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (9:9)
   10 | }
   11 |
 ```

diff --git a/...er/src/__tests__/fixtures/compiler/error.invalid-pass-ref-to-function.expect.md b/...er/src/__tests__/fixtures/compiler/error.invalid-pass-ref-to-function.expect.md
@@ -18,7 +18,7 @@ function Component(props) {
   2 | function Component(props) {
   3 |   const ref = useRef(null);
 > 4 |   const x = foo(ref);
-    |                 ^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at mutate? $21[6:8]:TObject<BuiltInUseRefId> (4:4)
+    |                 ^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (4:4)
   5 |   return x.current;
   6 | }
   7 |

diff --git a/...fixtures/compiler/error.invalid-ref-in-callback-invoked-during-render.expect.md b/...fixtures/compiler/error.invalid-ref-in-callback-invoked-during-render.expect.md
@@ -21,7 +21,7 @@ function Component(props) {
    6 |     return <Foo item={item} current={current} />;
    7 |   };
 >  8 |   return <Items>{props.items.map((item) => renderItem(item))}</Items>;
-     |                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at mutate? $60[14:16]:TObject<BuiltInFunction> (8:8)
+     |                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (8:8)
    9 | }
   10 |
 ```

diff --git a/...ests__/fixtures/compiler/error.invalid-set-and-read-ref-during-render.expect.md b/...ests__/fixtures/compiler/error.invalid-set-and-read-ref-during-render.expect.md
@@ -15,10 +15,13 @@ function Component(props) {
 ## Error
 
 ```
+  2 | function Component(props) {
   3 |   const ref = useRef(null);
-  4 |   ref.current = props.value;
-> 5 |   return ref.current;
-    |          ^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at freeze $24:TObject<BuiltInRefValue> (5:5)
+> 4 |   ref.current = props.value;
+    |   ^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (4:4)
+
+InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at freeze $24:TObject<BuiltInRefValue> (5:5)
+  5 |   return ref.current;
   6 | }
   7 |
 ```

diff --git a/...compiler/error.invalid-set-and-read-ref-nested-property-during-render.expect.md b/...compiler/error.invalid-set-and-read-ref-nested-property-during-render.expect.md
@@ -0,0 +1,29 @@
+
+## Input
+
+```javascript
+// @validateRefAccessDuringRender
+function Component(props) {
+  const ref = useRef({ inner: null });
+  ref.current.inner = props.value;
+  return ref.current.inner;
+}
+
+```
+
+
+## Error
+
+```
+  2 | function Component(props) {
+  3 |   const ref = useRef({ inner: null });
+> 4 |   ref.current.inner = props.value;
+    |   ^^^^^^^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (4:4)
+
+InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at freeze $30:TObject<BuiltInRefValue> (5:5)
+  5 |   return ref.current.inner;
+  6 | }
+  7 |
+```
+          
+      
diff --git a/...tests__/fixtures/compiler/error.invalid-set-and-read-ref-nested-property-during-render.js b/...tests__/fixtures/compiler/error.invalid-set-and-read-ref-nested-property-during-render.js
@@ -0,0 +1,6 @@
+// @validateRefAccessDuringRender
+function Component(props) {
+  const ref = useRef({ inner: null });
+  ref.current.inner = props.value;
+  return ref.current.inner;
+}
diff --git a/...ixtures/compiler/error.invalid-use-ref-added-to-dep-without-type-info.expect.md b/...ixtures/compiler/error.invalid-use-ref-added-to-dep-without-type-info.expect.md
@@ -25,7 +25,7 @@ function Foo({ a }) {
   3 |   const ref = useRef();
   4 |   // type information is lost here as we don't track types of fields
 > 5 |   const val = { ref };
-    |                 ^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at capture $29:TObject<BuiltInUseRefId> (5:5)
+    |                 ^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (5:5)
   6 |   // without type info, we don't know that val.ref.current is a ref value so we
   7 |   // *would* end up depending on val.ref.current
   8 |   // however, this is an instance of accessing a ref during render and is disallowed

diff --git a/...s__/fixtures/compiler/error.invalid-write-but-dont-read-ref-in-render.expect.md b/...s__/fixtures/compiler/error.invalid-write-but-dont-read-ref-in-render.expect.md
@@ -0,0 +1,29 @@
+
+## Input
+
+```javascript
+// @validateRefAccessDuringRender
+function useHook({ value }) {
+  const ref = useRef(null);
+  // Writing to a ref in render is against the rules:
+  ref.current = value;
+  // returning a ref is allowed, so this alone doesn't trigger an error:
+  return ref;
+}
+
+```
+
+
+## Error
+
+```
+  3 |   const ref = useRef(null);
+  4 |   // Writing to a ref in render is against the rules:
+> 5 |   ref.current = value;
+    |   ^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (5:5)
+  6 |   // returning a ref is allowed, so this alone doesn't trigger an error:
+  7 |   return ref;
+  8 | }
+```
+          
+      
diff --git a/...mpiler/src/__tests__/fixtures/compiler/error.invalid-write-but-dont-read-ref-in-render.js b/...mpiler/src/__tests__/fixtures/compiler/error.invalid-write-but-dont-read-ref-in-render.js
@@ -0,0 +1,8 @@
+// @validateRefAccessDuringRender
+function useHook({ value }) {
+  const ref = useRef(null);
+  // Writing to a ref in render is against the rules:
+  ref.current = value;
+  // returning a ref is allowed, so this alone doesn't trigger an error:
+  return ref;
+}
diff --git a/...c/__tests__/fixtures/compiler/error.validate-mutate-ref-arg-in-render.expect.md b/...c/__tests__/fixtures/compiler/error.validate-mutate-ref-arg-in-render.expect.md
@@ -23,7 +23,7 @@ export const FIXTURE_ENTRYPOINT = {
   1 | // @validateRefAccessDuringRender:true
   2 | function Foo(props, ref) {
 > 3 |   console.log(ref.current);
-    |               ^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at read $16:TObject<BuiltInRefValue> (3:3)
+    |               ^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (3:3)
   4 |   return <div>{props.bar}</div>;
   5 | }
   6 |