From f335e7d4d97cabd934ee58acb44bbc1851c0a822 Mon Sep 17 00:00:00 2001
From: Mofei Zhang <mofei.zhang@utexas.edu>
Date: Mon, 27 Feb 2023 13:38:02 -0500
Subject: [PATCH] [rhir] Patch: ordering of overlapping input dependencies does
 not matter

---

Patch and simplify logic around merging overlapping reactive dependencies.

Added `reduce-reactive-unconditional-deps` test fixtures, which tries to cover
all cases of merging unconditional dependencies (to a minimal dependencies set).
Please let me know if I missed any
---
 .../PropagateScopeDependencies.ts             | 23 +++++-----
 ...ncond-deps-nonoverlap-descendant.expect.md | 44 +++++++++++++++++++
 ...ctive-uncond-deps-nonoverlap-descendant.js |  9 ++++
 ...ve-uncond-deps-nonoverlap-direct.expect.md | 40 +++++++++++++++++
 ...-reactive-uncond-deps-nonoverlap-direct.js |  8 ++++
 ...e-uncond-deps-overlap-descendant.expect.md | 40 +++++++++++++++++
 ...reactive-uncond-deps-overlap-descendant.js |  9 ++++
 ...ctive-uncond-deps-overlap-direct.expect.md | 40 +++++++++++++++++
 ...uce-reactive-uncond-deps-overlap-direct.js |  9 ++++
 ...ctive-uncond-deps-subpath-order1.expect.md | 40 +++++++++++++++++
 ...uce-reactive-uncond-deps-subpath-order1.js |  9 ++++
 ...ctive-uncond-deps-subpath-order2.expect.md | 40 +++++++++++++++++
 ...uce-reactive-uncond-deps-subpath-order2.js |  9 ++++
 ...ctive-uncond-deps-subpath-order3.expect.md | 40 +++++++++++++++++
 ...uce-reactive-uncond-deps-subpath-order3.js |  9 ++++
 15 files changed, 357 insertions(+), 12 deletions(-)
 create mode 100644 compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-nonoverlap-descendant.expect.md
 create mode 100644 compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-nonoverlap-descendant.js
 create mode 100644 compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-nonoverlap-direct.expect.md
 create mode 100644 compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-nonoverlap-direct.js
 create mode 100644 compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-overlap-descendant.expect.md
 create mode 100644 compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-overlap-descendant.js
 create mode 100644 compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-overlap-direct.expect.md
 create mode 100644 compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-overlap-direct.js
 create mode 100644 compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-subpath-order1.expect.md
 create mode 100644 compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-subpath-order1.js
 create mode 100644 compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-subpath-order2.expect.md
 create mode 100644 compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-subpath-order2.js
 create mode 100644 compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-subpath-order3.expect.md
 create mode 100644 compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-subpath-order3.js

diff --git a/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts b/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts
index 556ace5e92..830a0d283d 100644
--- a/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts
+++ b/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts
@@ -189,24 +189,17 @@ class Context {
       (currentDeclaration.scope == null ||
         !this.#isScopeActive(currentDeclaration.scope))
     ) {
+      // Below logic ensures that `operand` is either added to `this.#dependencies`
+      // directly, or is covered by an existing dependency.
+
       // Check if there is an existing dependency that describes this operand
       for (const dep of this.#dependencies) {
         // not the same identifier
         if (dep.place.identifier.id !== maybeDependency.place.identifier.id) {
           continue;
         }
-        const depPath = dep.path;
-        // existing dep covers all paths
-        if (depPath === null) {
-          return;
-        }
-        const operandPath = maybeDependency.path;
-        // existing dep is for a path, this operand covers all paths so swap them
-        if (operandPath === null) {
-          this.#dependencies.delete(dep);
-          this.#dependencies.add(maybeDependency);
-          return;
-        }
+        const depPath = dep.path ?? [];
+        const operandPath = maybeDependency.path ?? [];
         // both the operand and dep have paths, determine if the existing path
         // is a subset of the new path
         let commonPathIndex = 0;
@@ -218,7 +211,13 @@ class Context {
           commonPathIndex++;
         }
         if (commonPathIndex === depPath.length) {
+          // existing dep is a subpath of the operand, so we don't need to
+          // add the operand
           return;
+        } else if (commonPathIndex === operandPath.length) {
+          // operand is a subpath of the existing path, delete the existing
+          // path
+          this.#dependencies.delete(dep);
         }
       }
       this.#dependencies.add(maybeDependency);
diff --git a/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-nonoverlap-descendant.expect.md b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-nonoverlap-descendant.expect.md
new file mode 100644
index 0000000000..6759d8c732
--- /dev/null
+++ b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-nonoverlap-descendant.expect.md
@@ -0,0 +1,44 @@
+
+## Input
+
+```javascript
+// Test that we can track non-overlapping dependencies separately.
+// (not needed for correctness but for dependency granularity)
+function TestNonOverlappingDescendantTracked(props) {
+  let x = {};
+  x.a = props.a.x.y;
+  x.b = props.b;
+  x.c = props.a.c.x.y.z;
+  return x;
+}
+
+```
+
+## Code
+
+```javascript
+// Test that we can track non-overlapping dependencies separately.
+// (not needed for correctness but for dependency granularity)
+function TestNonOverlappingDescendantTracked(props) {
+  const $ = React.unstable_useMemoCache(4);
+  const c_0 = $[0] !== props.a.x.y;
+  const c_1 = $[1] !== props.b;
+  const c_2 = $[2] !== props.a.c.x.y.z;
+  let x;
+  if (c_0 || c_1 || c_2) {
+    x = {};
+    x.a = props.a.x.y;
+    x.b = props.b;
+    x.c = props.a.c.x.y.z;
+    $[0] = props.a.x.y;
+    $[1] = props.b;
+    $[2] = props.a.c.x.y.z;
+    $[3] = x;
+  } else {
+    x = $[3];
+  }
+  return x;
+}
+
+```
+      
\ No newline at end of file
diff --git a/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-nonoverlap-descendant.js b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-nonoverlap-descendant.js
new file mode 100644
index 0000000000..65557df725
--- /dev/null
+++ b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-nonoverlap-descendant.js
@@ -0,0 +1,9 @@
+// Test that we can track non-overlapping dependencies separately.
+// (not needed for correctness but for dependency granularity)
+function TestNonOverlappingDescendantTracked(props) {
+  let x = {};
+  x.a = props.a.x.y;
+  x.b = props.b;
+  x.c = props.a.c.x.y.z;
+  return x;
+}
diff --git a/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-nonoverlap-direct.expect.md b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-nonoverlap-direct.expect.md
new file mode 100644
index 0000000000..0336312c4a
--- /dev/null
+++ b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-nonoverlap-direct.expect.md
@@ -0,0 +1,40 @@
+
+## Input
+
+```javascript
+// Test that we can track non-overlapping dependencies separately.
+// (not needed for correctness but for dependency granularity)
+function TestNonOverlappingTracked(props) {
+  let x = {};
+  x.b = props.a.b;
+  x.c = props.a.c;
+  return x;
+}
+
+```
+
+## Code
+
+```javascript
+// Test that we can track non-overlapping dependencies separately.
+// (not needed for correctness but for dependency granularity)
+function TestNonOverlappingTracked(props) {
+  const $ = React.unstable_useMemoCache(3);
+  const c_0 = $[0] !== props.a.b;
+  const c_1 = $[1] !== props.a.c;
+  let x;
+  if (c_0 || c_1) {
+    x = {};
+    x.b = props.a.b;
+    x.c = props.a.c;
+    $[0] = props.a.b;
+    $[1] = props.a.c;
+    $[2] = x;
+  } else {
+    x = $[2];
+  }
+  return x;
+}
+
+```
+      
\ No newline at end of file
diff --git a/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-nonoverlap-direct.js b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-nonoverlap-direct.js
new file mode 100644
index 0000000000..9a4bfabeb8
--- /dev/null
+++ b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-nonoverlap-direct.js
@@ -0,0 +1,8 @@
+// Test that we can track non-overlapping dependencies separately.
+// (not needed for correctness but for dependency granularity)
+function TestNonOverlappingTracked(props) {
+  let x = {};
+  x.b = props.a.b;
+  x.c = props.a.c;
+  return x;
+}
diff --git a/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-overlap-descendant.expect.md b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-overlap-descendant.expect.md
new file mode 100644
index 0000000000..75cb92c532
--- /dev/null
+++ b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-overlap-descendant.expect.md
@@ -0,0 +1,40 @@
+
+## Input
+
+```javascript
+// Test that we correctly track a subpath if the subpath itself is accessed as
+// a dependency
+function TestOverlappingDescendantTracked(props) {
+  let x = {};
+  x.b = props.a.b.c;
+  x.c = props.a.b.c.x.y;
+  x.a = props.a;
+  return x;
+}
+
+```
+
+## Code
+
+```javascript
+// Test that we correctly track a subpath if the subpath itself is accessed as
+// a dependency
+function TestOverlappingDescendantTracked(props) {
+  const $ = React.unstable_useMemoCache(2);
+  const c_0 = $[0] !== props.a;
+  let x;
+  if (c_0) {
+    x = {};
+    x.b = props.a.b.c;
+    x.c = props.a.b.c.x.y;
+    x.a = props.a;
+    $[0] = props.a;
+    $[1] = x;
+  } else {
+    x = $[1];
+  }
+  return x;
+}
+
+```
+      
\ No newline at end of file
diff --git a/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-overlap-descendant.js b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-overlap-descendant.js
new file mode 100644
index 0000000000..0ebc163e7c
--- /dev/null
+++ b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-overlap-descendant.js
@@ -0,0 +1,9 @@
+// Test that we correctly track a subpath if the subpath itself is accessed as
+// a dependency
+function TestOverlappingDescendantTracked(props) {
+  let x = {};
+  x.b = props.a.b.c;
+  x.c = props.a.b.c.x.y;
+  x.a = props.a;
+  return x;
+}
diff --git a/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-overlap-direct.expect.md b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-overlap-direct.expect.md
new file mode 100644
index 0000000000..84da7c119e
--- /dev/null
+++ b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-overlap-direct.expect.md
@@ -0,0 +1,40 @@
+
+## Input
+
+```javascript
+// Test that we correctly track a subpath if the subpath itself is accessed as
+// a dependency
+function TestOverlappingTracked(props) {
+  let x = {};
+  x.b = props.a.b;
+  x.c = props.a.c;
+  x.a = props.a;
+  return x;
+}
+
+```
+
+## Code
+
+```javascript
+// Test that we correctly track a subpath if the subpath itself is accessed as
+// a dependency
+function TestOverlappingTracked(props) {
+  const $ = React.unstable_useMemoCache(2);
+  const c_0 = $[0] !== props.a;
+  let x;
+  if (c_0) {
+    x = {};
+    x.b = props.a.b;
+    x.c = props.a.c;
+    x.a = props.a;
+    $[0] = props.a;
+    $[1] = x;
+  } else {
+    x = $[1];
+  }
+  return x;
+}
+
+```
+      
\ No newline at end of file
diff --git a/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-overlap-direct.js b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-overlap-direct.js
new file mode 100644
index 0000000000..9dc0b31043
--- /dev/null
+++ b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-overlap-direct.js
@@ -0,0 +1,9 @@
+// Test that we correctly track a subpath if the subpath itself is accessed as
+// a dependency
+function TestOverlappingTracked(props) {
+  let x = {};
+  x.b = props.a.b;
+  x.c = props.a.c;
+  x.a = props.a;
+  return x;
+}
diff --git a/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-subpath-order1.expect.md b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-subpath-order1.expect.md
new file mode 100644
index 0000000000..964fe4f067
--- /dev/null
+++ b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-subpath-order1.expect.md
@@ -0,0 +1,40 @@
+
+## Input
+
+```javascript
+// Determine that we only need to track p.a here
+// Ordering of access should not matter
+function TestDepsSubpathOrder1(props) {
+  let x = {};
+  x.b = props.a.b;
+  x.a = props.a;
+  x.c = props.a.b.c;
+  return x;
+}
+
+```
+
+## Code
+
+```javascript
+// Determine that we only need to track p.a here
+// Ordering of access should not matter
+function TestDepsSubpathOrder1(props) {
+  const $ = React.unstable_useMemoCache(2);
+  const c_0 = $[0] !== props.a;
+  let x;
+  if (c_0) {
+    x = {};
+    x.b = props.a.b;
+    x.a = props.a;
+    x.c = props.a.b.c;
+    $[0] = props.a;
+    $[1] = x;
+  } else {
+    x = $[1];
+  }
+  return x;
+}
+
+```
+      
\ No newline at end of file
diff --git a/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-subpath-order1.js b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-subpath-order1.js
new file mode 100644
index 0000000000..ccf4afec5d
--- /dev/null
+++ b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-subpath-order1.js
@@ -0,0 +1,9 @@
+// Determine that we only need to track p.a here
+// Ordering of access should not matter
+function TestDepsSubpathOrder1(props) {
+  let x = {};
+  x.b = props.a.b;
+  x.a = props.a;
+  x.c = props.a.b.c;
+  return x;
+}
diff --git a/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-subpath-order2.expect.md b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-subpath-order2.expect.md
new file mode 100644
index 0000000000..ec0e124308
--- /dev/null
+++ b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-subpath-order2.expect.md
@@ -0,0 +1,40 @@
+
+## Input
+
+```javascript
+// Determine that we only need to track p.a here
+// Ordering of access should not matter
+function TestDepsSubpathOrder2(props) {
+  let x = {};
+  x.a = props.a;
+  x.b = props.a.b;
+  x.c = props.a.b.c;
+  return x;
+}
+
+```
+
+## Code
+
+```javascript
+// Determine that we only need to track p.a here
+// Ordering of access should not matter
+function TestDepsSubpathOrder2(props) {
+  const $ = React.unstable_useMemoCache(2);
+  const c_0 = $[0] !== props.a;
+  let x;
+  if (c_0) {
+    x = {};
+    x.a = props.a;
+    x.b = props.a.b;
+    x.c = props.a.b.c;
+    $[0] = props.a;
+    $[1] = x;
+  } else {
+    x = $[1];
+  }
+  return x;
+}
+
+```
+      
\ No newline at end of file
diff --git a/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-subpath-order2.js b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-subpath-order2.js
new file mode 100644
index 0000000000..1b078ea8ea
--- /dev/null
+++ b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-subpath-order2.js
@@ -0,0 +1,9 @@
+// Determine that we only need to track p.a here
+// Ordering of access should not matter
+function TestDepsSubpathOrder2(props) {
+  let x = {};
+  x.a = props.a;
+  x.b = props.a.b;
+  x.c = props.a.b.c;
+  return x;
+}
diff --git a/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-subpath-order3.expect.md b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-subpath-order3.expect.md
new file mode 100644
index 0000000000..4b5e191a35
--- /dev/null
+++ b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-subpath-order3.expect.md
@@ -0,0 +1,40 @@
+
+## Input
+
+```javascript
+// Determine that we only need to track p.a here
+// Ordering of access should not matter
+function TestDepsSubpathOrder3(props) {
+  let x = {};
+  x.c = props.a.b.c;
+  x.a = props.a;
+  x.b = props.a.b;
+  return x;
+}
+
+```
+
+## Code
+
+```javascript
+// Determine that we only need to track p.a here
+// Ordering of access should not matter
+function TestDepsSubpathOrder3(props) {
+  const $ = React.unstable_useMemoCache(2);
+  const c_0 = $[0] !== props.a;
+  let x;
+  if (c_0) {
+    x = {};
+    x.c = props.a.b.c;
+    x.a = props.a;
+    x.b = props.a.b;
+    $[0] = props.a;
+    $[1] = x;
+  } else {
+    x = $[1];
+  }
+  return x;
+}
+
+```
+      
\ No newline at end of file
diff --git a/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-subpath-order3.js b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-subpath-order3.js
new file mode 100644
index 0000000000..5d72ff2deb
--- /dev/null
+++ b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-uncond-deps-subpath-order3.js
@@ -0,0 +1,9 @@
+// Determine that we only need to track p.a here
+// Ordering of access should not matter
+function TestDepsSubpathOrder3(props) {
+  let x = {};
+  x.c = props.a.b.c;
+  x.a = props.a;
+  x.b = props.a.b;
+  return x;
+}