Improve validateNoRefAccessInRender

Rewrites the validation to not rely on the mutable range of functions to 
determine whether they are called or not, since the range can be extended for 
other reasons (they happen to reference a mutable value that is mutated later, 
even though the function isn't called during render). 

Instead we use the same approach as validateNoSetStateInRender, explicitly 
tracking references to function expressions that access refs, and checking if 
those function expressions appear to be called. This can have false negatives, 
as with the setState validation, but catches lots of obviously incorrect code 
without false positives.

compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts

@@ -180,7 +180,7 @@ function* runWithEnvironment(
   }
 
   if (env.config.validateRefAccessDuringRender) {
-    validateNoRefAccessInRender(hir);
+    validateNoRefAccessInRender(hir).unwrap();
   }
 
   if (env.config.validateNoSetStateInRender) {

compiler/packages/babel-plugin-react-forget/src/Validation/ValidateNoRefAccesInRender.ts

@@ -5,135 +5,183 @@
  * LICENSE file in the root directory of this source tree.
  */
 
+import { CompilerError, ErrorSeverity } from "../CompilerError";
 import {
-  CompilerError,
-  CompilerErrorDetail,
-  ErrorSeverity,
-} from "../CompilerError";
-import { HIRFunction, Place, isRefValueType, isUseRefType } from "../HIR/HIR";
+  HIRFunction,
+  IdentifierId,
+  Place,
+  isRefValueType,
+  isUseRefType,
+} from "../HIR";
 import { printPlace } from "../HIR/PrintHIR";
 import {
   eachInstructionValueOperand,
   eachTerminalOperand,
 } from "../HIR/visitors";
+import { Err, Ok, Result } from "../Utils/Result";
 
-/*
- * Validates that ref values (the `current` property) are not accessed during render.
- * This validation is conservative and only rejects accesses of known ref values:
- *
- * ```javascript
- * // ERROR
- * const ref = useRef();
- * ref.current;
- *
- * const ref = useRef();
- * foo(ref); // may access .current
- *
- * // ALLOWED
- * const ref = useHookThatReturnsRef();
- * ref.current;
- * ```
- *
- * In the future we may reject more cases, based on either object names (`fooRef.current` is likely a ref)
- * or based on property name alone (`foo.current` might be a ref).
+/**
+ * Validates that a function does not access a ref value during render. This includes a partial check
+ * for ref values which are accessed indirectly via function expressions.
  */
-export function validateNoRefAccessInRender(fn: HIRFunction): void {
-  const error = new CompilerError();
+export function validateNoRefAccessInRender(
+  fn: HIRFunction
+): Result<void, CompilerError> {
+  const refAccessingFunctions: Set<IdentifierId> = new Set();
+  return validateNoRefAccessInRenderImpl(fn, refAccessingFunctions);
+}
 
+function validateNoRefAccessInRenderImpl(
+  fn: HIRFunction,
+  refAccessingFunctions: Set<IdentifierId>
+): Result<void, CompilerError> {
+  const errors = new CompilerError();
   for (const [, block] of fn.body.blocks) {
     for (const instr of block.instructions) {
       switch (instr.value.kind) {
-        case "PropertyLoad":
-        case "LoadLocal":
-        case "StoreLocal":
-        case "Destructure": {
-          /*
-           * These instructions are necessary for storing the results of a useRef into
-           * a variable and referencing them in functions. We can propagate type info
-           * for these instructions so they ensure we have a complete analysis.
-           */
+        case "JsxExpression":
+        case "JsxFragment": {
+          for (const operand of eachInstructionValueOperand(instr.value)) {
+            if (isRefValueType(operand.identifier)) {
+              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
+                )}`,
+                suggestions: null,
+              });
+            }
+          }
           break;
         }
-        case "JsxExpression": {
-          // It's okay to pass refs to JSX, but not ref *values*
-          for (const operand of eachInstructionValueOperand(instr.value)) {
-            validateNonRefValue(error, operand);
+        case "PropertyLoad": {
+          break;
+        }
+        case "LoadLocal": {
+          if (refAccessingFunctions.has(instr.value.place.identifier.id)) {
+            refAccessingFunctions.add(instr.lvalue.identifier.id);
+          }
+          break;
+        }
+        case "StoreLocal": {
+          if (refAccessingFunctions.has(instr.value.value.identifier.id)) {
+            refAccessingFunctions.add(instr.value.lvalue.place.identifier.id);
+            refAccessingFunctions.add(instr.lvalue.identifier.id);
           }
           break;
         }
         case "ObjectMethod":
         case "FunctionExpression": {
-          if (fn.env.config.validateRefAccessDuringRenderFunctionExpressions) {
+          if (
             /*
-             * functions are allowed to capture refs, so long as the function is not called
-             * during render. see AnalyzeFunctions for how we ensure that functions which
-             * capture refs get assigned a mutable range so we know here whether the function
-             * is called or not
+             * check if the function expression accesses a ref *or* some other
+             * function which accesses a ref
              */
-            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);
-              }
-            }
+            [...eachInstructionValueOperand(instr.value)].some(
+              (operand) =>
+                isRefValueType(operand.identifier) ||
+                refAccessingFunctions.has(operand.identifier.id)
+            ) ||
+            // check for cases where .current is accessed through an aliased ref
+            ([...eachInstructionValueOperand(instr.value)].some((operand) =>
+              isUseRefType(operand.identifier)
+            ) &&
+              validateNoRefAccessInRenderImpl(
+                instr.value.loweredFunc.func,
+                refAccessingFunctions
+              ).isErr())
+          ) {
+            // This function expression unconditionally accesses a ref
+            refAccessingFunctions.add(instr.lvalue.identifier.id);
           }
           break;
         }
-        case "CallExpression":
-        case "NewExpression": {
+        case "CallExpression": {
+          const callee = instr.value.callee;
+          // Report a more precise error when calling a local function that accesses a ref
+          if (refAccessingFunctions.has(callee.identifier.id)) {
+            errors.push({
+              severity: ErrorSeverity.InvalidReact,
+              reason:
+                "This function accesses a ref, which not be accessed during render. (https://react.dev/reference/react/useRef)",
+              loc: callee.loc,
+              description: `Function ${printPlace(callee)} accesses a ref`,
+              suggestions: null,
+            });
+          }
           for (const operand of eachInstructionValueOperand(instr.value)) {
-            validateNonRefValue(error, operand);
-            validateNonRefObject(error, operand);
+            validateNoRefAccess(errors, refAccessingFunctions, operand);
+          }
+          break;
+        }
+        case "ObjectExpression":
+        case "ArrayExpression":
+        case "MethodCall": {
+          for (const operand of eachInstructionValueOperand(instr.value)) {
+            validateNoRefAccess(errors, refAccessingFunctions, operand);
           }
           break;
         }
         default: {
           for (const operand of eachInstructionValueOperand(instr.value)) {
-            validateNonRefValue(error, operand);
-            validateNonRefObject(error, operand);
+            validateNoRefValueAccess(errors, refAccessingFunctions, operand);
           }
+          break;
         }
       }
     }
     for (const operand of eachTerminalOperand(block.terminal)) {
-      validateNonRefValue(error, operand);
+      validateNoRefValueAccess(errors, refAccessingFunctions, operand);
     }
   }
 
-  if (error.hasErrors()) {
-    throw error;
+  if (errors.hasErrors()) {
+    return Err(errors);
+  } else {
+    return Ok(undefined);
   }
 }
 
-// Check that the operand's type is not that of useRef().current (the ref's current value)
-function validateNonRefValue(error: CompilerError, operand: Place): void {
-  if (isRefValueType(operand.identifier)) {
-    error.pushErrorDetail(
-      new CompilerErrorDetail({
-        description: `Cannot access ref value at ${printPlace(operand)}`,
-        loc: typeof operand.loc !== "symbol" ? operand.loc : null,
-        reason:
-          "Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef)",
-        severity: ErrorSeverity.InvalidReact,
-        suggestions: null,
-      })
-    );
+function validateNoRefValueAccess(
+  errors: CompilerError,
+  unconditionalSetStateFunctions: Set<IdentifierId>,
+  operand: Place
+): void {
+  if (
+    isRefValueType(operand.identifier) ||
+    unconditionalSetStateFunctions.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)}`,
+      suggestions: null,
+    });
   }
 }
 
-// Check that the operand's type is not that of useRef() return value (the ref container)
-function validateNonRefObject(error: CompilerError, operand: Place): void {
-  if (isUseRefType(operand.identifier)) {
-    error.pushErrorDetail(
-      new CompilerErrorDetail({
-        description: `Cannot access ref object at ${printPlace(operand)}`,
-        loc: typeof operand.loc !== "symbol" ? operand.loc : null,
-        reason:
-          "Ref values may not be passed to functions because they could read the ref value (`current` property) during render. (https://react.dev/reference/react/useRef)",
-        severity: ErrorSeverity.InvalidReact,
-        suggestions: null,
-      })
-    );
+function validateNoRefAccess(
+  errors: CompilerError,
+  unconditionalSetStateFunctions: Set<IdentifierId>,
+  operand: Place
+): void {
+  if (
+    isRefValueType(operand.identifier) ||
+    isUseRefType(operand.identifier) ||
+    unconditionalSetStateFunctions.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)}`,
+      suggestions: null,
+    });
   }
 }

compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-aliased-ref-in-callback-invoked-during-render-.expect.md

@@ -0,0 +1,25 @@
+
+## Input
+
+```javascript
+// @validateRefAccessDuringRender @validateRefAccessDuringRenderFunctionExpressions
+function Component(props) {
+  const ref = useRef(null);
+  const renderItem = (item) => {
+    const aliasedRef = ref;
+    const current = aliasedRef.current;
+    return <Foo item={item} current={current} />;
+  };
+  return <Items>{props.items.map((item) => renderItem(item))}</Items>;
+}
+
+```
+
+
+## Error
+
+```
+[ReactForget] 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] (9:9)
+```
+          
+      

compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-aliased-ref-in-callback-invoked-during-render-.js

@@ -0,0 +1,10 @@
+// @validateRefAccessDuringRender @validateRefAccessDuringRenderFunctionExpressions
+function Component(props) {
+  const ref = useRef(null);
+  const renderItem = (item) => {
+    const aliasedRef = ref;
+    const current = aliasedRef.current;
+    return <Foo item={item} current={current} />;
+  };
+  return <Items>{props.items.map((item) => renderItem(item))}</Items>;
+}

compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-pass-ref-to-function.expect.md

@@ -15,7 +15,7 @@ function Component(props) {
 ## Error
 
 ```
-[ReactForget] InvalidReact: Ref values may not be passed to functions because they could read the ref value (`current` property) during render. (https://react.dev/reference/react/useRef). Cannot access ref object at mutate? $21[6:8]:TObject<BuiltInUseRefId> (4:4)
+[ReactForget] 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)
 ```
           
       

compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-ref-in-callback-invoked-during-render.expect.md

@@ -18,7 +18,7 @@ function Component(props) {
 ## Error
 
 ```
-[ReactForget] InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at capture $42[6:16]:TObject<BuiltInRefValue> (5:5)
+[ReactForget] 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] (8:8)
 ```
           
       

compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-set-and-read-ref-during-render.expect.md

@@ -15,8 +15,6 @@ function Component(props) {
 ## Error
 
 ```
-[ReactForget] InvalidReact: Ref values may not be passed to functions because they could read the ref value (`current` property) during render. (https://react.dev/reference/react/useRef). Cannot access ref object at store $21[7:9]:TObject<BuiltInUseRefId> (4:4)
-
 [ReactForget] 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)
 ```
           

compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-use-ref-added-to-dep-without-type-info.expect.md

@@ -22,7 +22,7 @@ function Foo({ a }) {
 ## Error
 
 ```
-[ReactForget] InvalidReact: Ref values may not be passed to functions because they could read the ref value (`current` property) during render. (https://react.dev/reference/react/useRef). Cannot access ref object at capture $29:TObject<BuiltInUseRefId> (5:5)
+[ReactForget] 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)
 ```