From 1fdbcfe162c0719d8156f40f3e71a8fc005f9b7b Mon Sep 17 00:00:00 2001
From: Mofei Zhang <mofei.zhang@utexas.edu>
Date: Tue, 28 Feb 2023 16:36:01 -0500
Subject: [PATCH] [rhir] Add dependencies produced by active (incomplete)
 scopes

---

> If this operand is used in a scope, has a dynamic value, and was defined
before this scope, then its a dependency of the scope.

> (from current comments in PropagateScopeDependencies::visitDependency)

A reactive scope can take a dependency from a definition produced by an
incomplete parent scope. Our tests previously did not cover this, since most
object types aliased together and remained mutable throughout a ReactiveScope.

e.g. our tests did not have

```

scope @0 (deps=...,  declarations=[x, y]) {

x = {};

// define a reactive, immutable value that is not aliased to become mutable

const immutableVal = ...;

scope @1 (deps=immutableVal, declarations=[y]) {

y = read(immutableVal)

}

mutateX(x, ...);

}

```

We should not add a dependency if it is produced in exactly the same scope as
the one it is used. It is safe (and correct) to depend on values produced by a
parent scope.

---

Note that we still should check for whether a defining scope is active to
determine whether it should be added as a output of that scope
([src](https://github.com/facebook/react-forget/blob/b608ab20d57229b528deeffa19f1ee08a4bad37a/forget/src/ReactiveScopes/PropagateScopeDependencies.ts#L469-L478)).

Access of an identifier produced by a parent scope (i.e. adding a variable
defined by a scope's parent as its own dependency) does not require adding that
identifier to the parent's `declarations`, since that identifier is already
valid to access via identifier binding rules.
---
 .../PropagateScopeDependencies.ts             |  2 +-
 .../hir/allocating-primitive-as-dep.expect.md | 31 +++++++++++--------
 .../fixtures/hir/primitive-as-dep.expect.md   | 31 +++++++++++--------
 3 files changed, 37 insertions(+), 27 deletions(-)

diff --git a/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts b/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts
index 01f61a88fe..c5a7970793 100644
--- a/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts
+++ b/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts
@@ -505,7 +505,7 @@ class Context {
       currentDeclaration !== undefined &&
       currentDeclaration.id < currentScope.range.start &&
       (currentDeclaration.scope == null ||
-        !this.#isScopeActive(currentDeclaration.scope))
+        currentDeclaration.scope !== currentScope)
     ) {
       // Check if there is an existing dependency that describes this operand
       // We do not try to join/reduce dependencies here due to missing info
diff --git a/compiler/forget/src/__tests__/fixtures/hir/allocating-primitive-as-dep.expect.md b/compiler/forget/src/__tests__/fixtures/hir/allocating-primitive-as-dep.expect.md
index d50063894e..e6031161e4 100644
--- a/compiler/forget/src/__tests__/fixtures/hir/allocating-primitive-as-dep.expect.md
+++ b/compiler/forget/src/__tests__/fixtures/hir/allocating-primitive-as-dep.expect.md
@@ -53,7 +53,7 @@ function AllocatingPrimitiveAsDep(props) {
 }
 
 function PrimitiveAsDepNested(props) {
-  const $ = React.unstable_useMemoCache(8);
+  const $ = React.unstable_useMemoCache(10);
   const c_0 = $[0] !== props.b;
   const c_1 = $[1] !== props.a;
   let x;
@@ -69,12 +69,15 @@ function PrimitiveAsDepNested(props) {
     } else {
       t0 = $[4];
     }
+    const t1 = t0 + 1;
+    const c_5 = $[5] !== t1;
     let y;
-    if ($[5] === Symbol.for("react.memo_cache_sentinel")) {
-      y = foo(t0 + 1);
-      $[5] = y;
+    if (c_5) {
+      y = foo(t1);
+      $[5] = t1;
+      $[6] = y;
     } else {
-      y = $[5];
+      y = $[6];
     }
     mutate(x, props.a);
     $[0] = props.b;
@@ -83,16 +86,18 @@ function PrimitiveAsDepNested(props) {
   } else {
     x = $[2];
   }
-  const c_6 = $[6] !== x;
-  let t1;
-  if (c_6) {
-    t1 = [x, y];
-    $[6] = x;
-    $[7] = t1;
+  const c_7 = $[7] !== x;
+  const c_8 = $[8] !== y;
+  let t2;
+  if (c_7 || c_8) {
+    t2 = [x, y];
+    $[7] = x;
+    $[8] = y;
+    $[9] = t2;
   } else {
-    t1 = $[7];
+    t2 = $[9];
   }
-  return t1;
+  return t2;
 }
 
 ```
diff --git a/compiler/forget/src/__tests__/fixtures/hir/primitive-as-dep.expect.md b/compiler/forget/src/__tests__/fixtures/hir/primitive-as-dep.expect.md
index f06413e71b..d7066d71a9 100644
--- a/compiler/forget/src/__tests__/fixtures/hir/primitive-as-dep.expect.md
+++ b/compiler/forget/src/__tests__/fixtures/hir/primitive-as-dep.expect.md
@@ -46,19 +46,22 @@ function PrimitiveAsDep(props) {
 }
 
 function PrimitiveAsDepNested(props) {
-  const $ = React.unstable_useMemoCache(6);
+  const $ = React.unstable_useMemoCache(8);
   const c_0 = $[0] !== props.b;
   const c_1 = $[1] !== props.a;
   let x;
   if (c_0 || c_1) {
     x = {};
     mutate(x);
+    const t0 = props.b + 1;
+    const c_3 = $[3] !== t0;
     let y;
-    if ($[3] === Symbol.for("react.memo_cache_sentinel")) {
-      y = foo(props.b + 1);
-      $[3] = y;
+    if (c_3) {
+      y = foo(t0);
+      $[3] = t0;
+      $[4] = y;
     } else {
-      y = $[3];
+      y = $[4];
     }
     mutate(x, props.a);
     $[0] = props.b;
@@ -67,16 +70,18 @@ function PrimitiveAsDepNested(props) {
   } else {
     x = $[2];
   }
-  const c_4 = $[4] !== x;
-  let t0;
-  if (c_4) {
-    t0 = [x, y];
-    $[4] = x;
-    $[5] = t0;
+  const c_5 = $[5] !== x;
+  const c_6 = $[6] !== y;
+  let t1;
+  if (c_5 || c_6) {
+    t1 = [x, y];
+    $[5] = x;
+    $[6] = y;
+    $[7] = t1;
   } else {
-    t0 = $[5];
+    t1 = $[7];
   }
-  return t0;
+  return t1;
 }
 
 ```