From 404f627c2c26a2ffcb17459fe9eef5468c7a2701 Mon Sep 17 00:00:00 2001
From: Joe Savona <joesavona@fb.com>
Date: Tue, 2 May 2023 16:49:54 -0700
Subject: [PATCH] Improve conditional dependency tracking for optional member
 expr inside optional call

When we traverse an OptionalExpression in PropagateScopeDependencies, we
previously considered the entire value to be optional. With the changes in this
stack to more accurately model OptionalMemberExpression, the `object` portion of
an OptionalMemberExpression is now evaluated within the OptionalExpression. This
PR refines the handling of OptionalExpression accordingly, so that we only treat
the optional portion as conditional.
---
 .../PropagateScopeDependencies.ts             | 24 +++++++++++--
 ...properties-inside-optional-chain.expect.md | 30 ++++++++++++++++
 ...tional-properties-inside-optional-chain.js |  3 ++
 ...ncies-optional-member-expression.expect.md | 35 +++++++++++++++++++
 ...dependencies-optional-member-expression.js |  6 ++++
 5 files changed, 95 insertions(+), 3 deletions(-)
 create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/reactive-dependencies-non-optional-properties-inside-optional-chain.expect.md
 create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/reactive-dependencies-non-optional-properties-inside-optional-chain.js
 create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/reduce-reactive-dependencies-optional-member-expression.expect.md
 create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/reduce-reactive-dependencies-optional-member-expression.js

diff --git a/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts b/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts
index 5c27e56e1e..a00f9f927f 100644
--- a/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts
+++ b/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts
@@ -5,6 +5,7 @@
  * LICENSE file in the root directory of this source tree.
  */
 
+import { CompilerError } from "../CompilerError";
 import {
   Identifier,
   IdentifierId,
@@ -451,9 +452,26 @@ class PropagationVisitor extends ReactiveFunctionVisitor<Context> {
   ): void {
     switch (value.kind) {
       case "OptionalExpression": {
-        context.enterConditional(() => {
-          this.visitReactiveValue(context, id, value.value);
-        });
+        const inner = value.value;
+        // OptionalExpression value is a SequenceExpression where the instructions
+        // represent the code prior to the `?` and the final value represents the
+        // conditional code that follows.
+        if (inner.kind === "SequenceExpression") {
+          // Instructions are the unconditionally executed portion before the `?`
+          for (const instr of inner.instructions) {
+            this.visitInstruction(instr, context);
+          }
+          // The final value is the conditional portion following the `?`
+          context.enterConditional(() => {
+            this.visitReactiveValue(context, id, inner.value);
+          });
+        } else {
+          CompilerError.invariant(
+            "Expected OptionalExpression value to be a SequenceExpression",
+            value.loc,
+            `Found a '${value.kind}'`
+          );
+        }
         break;
       }
       case "LogicalExpression": {
diff --git a/compiler/forget/src/__tests__/fixtures/compiler/reactive-dependencies-non-optional-properties-inside-optional-chain.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/reactive-dependencies-non-optional-properties-inside-optional-chain.expect.md
new file mode 100644
index 0000000000..1fa0d3b170
--- /dev/null
+++ b/compiler/forget/src/__tests__/fixtures/compiler/reactive-dependencies-non-optional-properties-inside-optional-chain.expect.md
@@ -0,0 +1,30 @@
+
+## Input
+
+```javascript
+function Component(props) {
+  return props.post.feedback.comments?.edges?.map(render);
+}
+
+```
+
+## Code
+
+```javascript
+import { unstable_useMemoCache as useMemoCache } from "react";
+function Component(props) {
+  const $ = useMemoCache(2);
+  const c_0 = $[0] !== props.post.feedback.comments;
+  let t0;
+  if (c_0) {
+    t0 = props.post.feedback.comments?.edges?.map(render);
+    $[0] = props.post.feedback.comments;
+    $[1] = t0;
+  } else {
+    t0 = $[1];
+  }
+  return t0;
+}
+
+```
+      
\ No newline at end of file
diff --git a/compiler/forget/src/__tests__/fixtures/compiler/reactive-dependencies-non-optional-properties-inside-optional-chain.js b/compiler/forget/src/__tests__/fixtures/compiler/reactive-dependencies-non-optional-properties-inside-optional-chain.js
new file mode 100644
index 0000000000..e8e0da392e
--- /dev/null
+++ b/compiler/forget/src/__tests__/fixtures/compiler/reactive-dependencies-non-optional-properties-inside-optional-chain.js
@@ -0,0 +1,3 @@
+function Component(props) {
+  return props.post.feedback.comments?.edges?.map(render);
+}
diff --git a/compiler/forget/src/__tests__/fixtures/compiler/reduce-reactive-dependencies-optional-member-expression.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/reduce-reactive-dependencies-optional-member-expression.expect.md
new file mode 100644
index 0000000000..5c827f6154
--- /dev/null
+++ b/compiler/forget/src/__tests__/fixtures/compiler/reduce-reactive-dependencies-optional-member-expression.expect.md
@@ -0,0 +1,35 @@
+
+## Input
+
+```javascript
+function Component(props) {
+  const x = [];
+  x.push(props.items?.length);
+  x.push(props.items?.edges?.map?.(render)?.filter?.(Boolean) ?? []);
+  return x;
+}
+
+```
+
+## Code
+
+```javascript
+import { unstable_useMemoCache as useMemoCache } from "react";
+function Component(props) {
+  const $ = useMemoCache(2);
+  const c_0 = $[0] !== props.items;
+  let x;
+  if (c_0) {
+    x = [];
+    x.push(props.items?.length);
+    x.push(props.items?.edges?.map?.(render)?.filter?.(Boolean) ?? []);
+    $[0] = props.items;
+    $[1] = x;
+  } else {
+    x = $[1];
+  }
+  return x;
+}
+
+```
+      
\ No newline at end of file
diff --git a/compiler/forget/src/__tests__/fixtures/compiler/reduce-reactive-dependencies-optional-member-expression.js b/compiler/forget/src/__tests__/fixtures/compiler/reduce-reactive-dependencies-optional-member-expression.js
new file mode 100644
index 0000000000..a87587ced7
--- /dev/null
+++ b/compiler/forget/src/__tests__/fixtures/compiler/reduce-reactive-dependencies-optional-member-expression.js
@@ -0,0 +1,6 @@
+function Component(props) {
+  const x = [];
+  x.push(props.items?.length);
+  x.push(props.items?.edges?.map?.(render)?.filter?.(Boolean) ?? []);
+  return x;
+}