[RFC] Improve validation against ref access in render

The goal of this PR is to improve ValidateNoRefAccessInRender to find function expressions which a) access refs and b) may be called during render. Currently we always allow ref access in any function expression, but that's obviously optimistic. For the approach, the observation is that we already have a system that tells us whether a function may get called — mutable range inference. So long as we consider a function "mutable", we'll infer a range for it, but the problem is that we don't currently view functions which depend on refs to be mutable. So here I'm doing ~~sort of~~ a hack to force function deps on refs to be treated as Effect.Capture. This is enough for the function to be considered mutable, for a mutable range to be assigned, and for us to detect that during ref validation. I don't love the hack, i'm open to other ideas!
2025-11-01 09:12:30 +00:00 · 2023-06-05 14:08:26 -04:00
parent b160ee8522
commit ad32811b64
11 changed files with 64 additions and 11 deletions
@@ -95,9 +95,6 @@ export function* run(
  inferTypes(hir);
  yield log({ kind: "hir", name: "InferTypes", value: hir });

-  if (env.validateRefAccessDuringRender) {
-    validateNoRefAccessInRender(hir);
-  }
  if (env.validateHooksUsage) {
    validateHooksUsage(hir);
    const conditionalHooksResult = validateUnconditionalHooks(hir).unwrap();
@@ -128,6 +125,10 @@ export function* run(
  inferMutableRanges(hir);
  yield log({ kind: "hir", name: "InferMutableRanges", value: hir });

+  if (env.validateRefAccessDuringRender) {
+    validateNoRefAccessInRender(hir);
+  }
+
  leaveSSA(hir);
  yield log({ kind: "hir", name: "LeaveSSA", value: hir });

@@ -53,6 +53,13 @@ export function validateNoRefAccessInRender(fn: HIRFunction): void {
          // For now we assume *all* function expressions are safe, eventually we can
          // be more precise and disallow ref access in functions that may be called
          // during render
+          const mutableRange = instr.lvalue.identifier.mutableRange;
+          if (mutableRange.end > mutableRange.start + 1) {
+            for (const operand of eachInstructionValueOperand(instr.value)) {
+              validateNonRefValue(error, operand);
+              validateNonRefObject(error, operand);
+            }
+          }
          break;
        }
        case "CallExpression":
@@ -11,6 +11,8 @@ import {
  FunctionExpression,
  HIRFunction,
  Identifier,
+  isRefValueType,
+  isUseRefType,
  mergeConsecutiveBlocks,
  Place,
  ReactiveScopeDependency,
@@ -123,6 +125,13 @@ function infer(

    if (name !== null && mutations.has(name)) {
      dep.effect = Effect.Capture;
+    } else if (isUseRefType(dep.identifier) || isRefValueType(dep.identifier)) {
+      // TODO: this is a hack to ensure we treat functions which reference refs
+      // as having a capture and therefore being considered mutable. this ensures
+      // the function gets a mutable range which accounts for anywhere that it
+      // could be called, and allows us to help ensure it isn't called during
+      // render
+      dep.effect = Effect.Capture;
    }
  }

@@ -15,7 +15,7 @@ function Component(props) {
 ## Error

 ```
-[ReactForget] InvalidInput: Ref values (the `current` property) may not be accessed during render. Cannot access ref value at <unknown> $23:TObject<BuiltInRefValue> (5:5)
+[ReactForget] InvalidInput: Ref values (the `current` property) may not be accessed during render. Cannot access ref value at freeze $23:TObject<BuiltInRefValue> (5:5)
 ```
          
      
@@ -14,7 +14,7 @@ function Component(props) {
 ## Error

 ```
-[ReactForget] InvalidInput: Ref values may not be passed to functions because they could read the ref value (`current` property) during render. Cannot access ref object at <unknown> $22:TObject<BuiltInUseRefId> (3:3)
+[ReactForget] InvalidInput: Ref values may not be passed to functions because they could read the ref value (`current` property) during render. Cannot access ref object at mutate $22[6:8]:TObject<BuiltInUseRefId> (3:3)
 ```
          
      
@@ -14,9 +14,9 @@ function Component(props) {
 ## Error

 ```
-[ReactForget] InvalidInput: Ref values may not be passed to functions because they could read the ref value (`current` property) during render. Cannot access ref object at <unknown> $22:TObject<BuiltInUseRefId> (3:3)
+[ReactForget] InvalidInput: Ref values may not be passed to functions because they could read the ref value (`current` property) during render. Cannot access ref object at store $22[7:9]:TObject<BuiltInUseRefId> (3:3)

-[ReactForget] InvalidInput: Ref values (the `current` property) may not be accessed during render. Cannot access ref value at <unknown> $25:TObject<BuiltInRefValue> (4:4)
+[ReactForget] InvalidInput: Ref values (the `current` property) may not be accessed during render. Cannot access ref value at freeze $25:TObject<BuiltInRefValue> (4:4)
 ```
          
      
@@ -21,7 +21,7 @@ function Foo({ a }) {
 ## Error

 ```
-[ReactForget] InvalidInput: Ref values may not be passed to functions because they could read the ref value (`current` property) during render. Cannot access ref object at <unknown> $30:TObject<BuiltInUseRefId> (4:4)
+[ReactForget] InvalidInput: Ref values may not be passed to functions because they could read the ref value (`current` property) during render. Cannot access ref object at capture $30:TObject<BuiltInUseRefId> (4:4)
 ```
          
      
@@ -5,7 +5,8 @@
 function Component(props) {
  const ref = useRef(null);
  const onChange = (e) => {
-    ref.current = e.target.value;
+    const newValue = e.target.value ?? ref.current;
+    ref.current = newValue;
  };
  useEffect(() => {
    console.log(ref.current);
@@ -25,7 +26,8 @@ function Component(props) {
  let t0;
  if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
    t0 = (e) => {
-      ref.current = e.target.value;
+      const newValue = e.target.value ?? ref.current;
+      ref.current = newValue;
    };
    $[0] = t0;
  } else {
@@ -1,7 +1,8 @@
 function Component(props) {
  const ref = useRef(null);
  const onChange = (e) => {
-    ref.current = e.target.value;
+    const newValue = e.target.value ?? ref.current;
+    ref.current = newValue;
  };
  useEffect(() => {
    console.log(ref.current);
@@ -0,0 +1,24 @@
+
+## Input
+
+```javascript
+// @debug
+function Component(props) {
+  const ref = useRef(null);
+  const renderItem = (item) => {
+    const current = ref.current;
+    return <Foo item={item} current={current} />;
+  };
+  return <Items>{props.items.map((item) => renderItem(item))}</Items>;
+}
+
+```
+
+
+## Error
+
+```
+[ReactForget] InvalidInput: Ref values (the `current` property) may not be accessed during render. Cannot access ref value at capture $43[6:16]:TObject<BuiltInRefValue> (5:5)
+```
+          
+      
@@ -0,0 +1,9 @@
+// @debug
+function Component(props) {
+  const ref = useRef(null);
+  const renderItem = (item) => {
+    const current = ref.current;
+    return <Foo item={item} current={current} />;
+  };
+  return <Items>{props.items.map((item) => renderItem(item))}</Items>;
+}