[compiler][fixtures] test repros: codegen, alignScope, phis

ghstack-source-id: d30fd3f
Pull Request resolved: #29878

The AlignReactiveScope bug should be simplest to fix, but it's also caught by an invariant assertion. I think a fix could be either keeping track of "active" block-fallthrough pairs (`retainWhere(pair => pair.range.end > current.instr[0].id)`) or following the approach in `assertValidBlockNesting`.
I'm tempted to pull the value-block aligning logic out into its own pass (using the current `node` tree traversal), then align to non-value blocks with the `assertValidBlockNesting` approach. Happy to hear feedback on this though!

The other two are likely bigger issues, as they're not caught by static invariants.

Update:
- removed bug-phi-reference-effect as it's been patched by @josephsavona
- added bug-array-concat-should-capture

Author: mofeiZ

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-array-concat-should-capture.expect.md

@@ -0,0 +1,72 @@
+
+## Input
+
+```javascript
+import { mutate } from "shared-runtime";
+
+/**
+ * Fixture showing why `concat` needs to capture both the callee and rest args.
+ * Here, observe that arr1's values are captured into arr2.
+ *  - Later mutations of arr2 may write to values within arr1.
+ *  - Observe that it's technically valid to separately memoize the array arr1
+ *    itself.
+ */
+function Foo({ inputNum }) {
+  const arr1: Array<number | object> = [{ a: 1 }, {}];
+  const arr2 = arr1.concat([1, inputNum]);
+  mutate(arr2[0]);
+  return arr2;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Foo,
+  params: [{ inputNum: 2 }],
+  sequentialRenders: [{ inputNum: 2 }, { inputNum: 3 }],
+};
+
+```
+
+## Code
+
+```javascript
+import { c as _c } from "react/compiler-runtime";
+import { mutate } from "shared-runtime";
+
+/**
+ * Fixture showing why `concat` needs to capture both the callee and rest args.
+ * Here, observe that arr1's values are captured into arr2.
+ *  - Later mutations of arr2 may write to values within arr1.
+ *  - Observe that it's technically valid to separately memoize the array arr1
+ *    itself.
+ */
+function Foo(t0) {
+  const $ = _c(3);
+  const { inputNum } = t0;
+  let t1;
+  if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
+    t1 = [{ a: 1 }, {}];
+    $[0] = t1;
+  } else {
+    t1 = $[0];
+  }
+  const arr1 = t1;
+  let arr2;
+  if ($[1] !== inputNum) {
+    arr2 = arr1.concat([1, inputNum]);
+    mutate(arr2[0]);
+    $[1] = inputNum;
+    $[2] = arr2;
+  } else {
+    arr2 = $[2];
+  }
+  return arr2;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Foo,
+  params: [{ inputNum: 2 }],
+  sequentialRenders: [{ inputNum: 2 }, { inputNum: 3 }],
+};
+
+```
+      

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-array-concat-should-capture.ts

@@ -0,0 +1,21 @@
+import { mutate } from "shared-runtime";
+
+/**
+ * Fixture showing why `concat` needs to capture both the callee and rest args.
+ * Here, observe that arr1's values are captured into arr2.
+ *  - Later mutations of arr2 may write to values within arr1.
+ *  - Observe that it's technically valid to separately memoize the array arr1
+ *    itself.
+ */
+function Foo({ inputNum }) {
+  const arr1: Array<number | object> = [{ a: 1 }, {}];
+  const arr2 = arr1.concat([1, inputNum]);
+  mutate(arr2[0]);
+  return arr2;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Foo,
+  params: [{ inputNum: 2 }],
+  sequentialRenders: [{ inputNum: 2 }, { inputNum: 3 }],
+};

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.expect.md

@@ -0,0 +1,92 @@
+
+## Input
+
+```javascript
+import { makeArray, print } from "shared-runtime";
+
+/**
+ * Exposes bug involving iife inlining + codegen.
+ * We currently inline iifes to labeled blocks (not value-blocks).
+ *
+ * Here, print(1) and the evaluation of makeArray(...) get the same scope
+ * as the compiler infers that the makeArray call may mutate its arguments.
+ * Since print(1) does not get its own scope (and is thus not a declaration
+ * or dependency), it does not get promoted.
+ * As a result, print(1) gets reordered across the labeled-block instructions
+ * to be inlined at the makeArray callsite.
+ *
+ * Current evaluator results:
+ *   Found differences in evaluator results
+ *   Non-forget (expected):
+ *   (kind: ok) [null,2]
+ *   logs: [1,2]
+ *   Forget:
+ *   (kind: ok) [null,2]
+ *   logs: [2,1]
+ */
+function useTest() {
+  return makeArray<number | void>(
+    print(1),
+    (function foo() {
+      print(2);
+      return 2;
+    })()
+  );
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: useTest,
+  params: [],
+};
+
+```
+
+## Code
+
+```javascript
+import { c as _c } from "react/compiler-runtime";
+import { makeArray, print } from "shared-runtime";
+
+/**
+ * Exposes bug involving iife inlining + codegen.
+ * We currently inline iifes to labeled blocks (not value-blocks).
+ *
+ * Here, print(1) and the evaluation of makeArray(...) get the same scope
+ * as the compiler infers that the makeArray call may mutate its arguments.
+ * Since print(1) does not get its own scope (and is thus not a declaration
+ * or dependency), it does not get promoted.
+ * As a result, print(1) gets reordered across the labeled-block instructions
+ * to be inlined at the makeArray callsite.
+ *
+ * Current evaluator results:
+ *   Found differences in evaluator results
+ *   Non-forget (expected):
+ *   (kind: ok) [null,2]
+ *   logs: [1,2]
+ *   Forget:
+ *   (kind: ok) [null,2]
+ *   logs: [2,1]
+ */
+function useTest() {
+  const $ = _c(1);
+  let t0;
+  if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
+    let t1;
+
+    print(2);
+    t1 = 2;
+    t0 = makeArray(print(1), t1);
+    $[0] = t0;
+  } else {
+    t0 = $[0];
+  }
+  return t0;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: useTest,
+  params: [],
+};
+
+```
+      

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.ts

@@ -0,0 +1,36 @@
+import { makeArray, print } from "shared-runtime";
+
+/**
+ * Exposes bug involving iife inlining + codegen.
+ * We currently inline iifes to labeled blocks (not value-blocks).
+ *
+ * Here, print(1) and the evaluation of makeArray(...) get the same scope
+ * as the compiler infers that the makeArray call may mutate its arguments.
+ * Since print(1) does not get its own scope (and is thus not a declaration
+ * or dependency), it does not get promoted.
+ * As a result, print(1) gets reordered across the labeled-block instructions
+ * to be inlined at the makeArray callsite.
+ *
+ * Current evaluator results:
+ *   Found differences in evaluator results
+ *   Non-forget (expected):
+ *   (kind: ok) [null,2]
+ *   logs: [1,2]
+ *   Forget:
+ *   (kind: ok) [null,2]
+ *   logs: [2,1]
+ */
+function useTest() {
+  return makeArray<number | void>(
+    print(1),
+    (function foo() {
+      print(2);
+      return 2;
+    })()
+  );
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: useTest,
+  params: [],
+};

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scope-starts-within-cond.expect.md

@@ -0,0 +1,29 @@
+
+## Input
+
+```javascript
+/**
+ * Similar fixture to `error.todo-align-scopes-nested-block-structure`, but
+ * a simpler case.
+ */
+function useFoo(cond) {
+  let s = null;
+  if (cond) {
+    s = {};
+  } else {
+    return null;
+  }
+  mutate(s);
+  return s;
+}
+
+```
+
+
+## Error
+
+```
+Invariant: Invalid nesting in program blocks or scopes. Items overlap but are not nested: 4:10(5:13)
+```
+          
+      

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scope-starts-within-cond.ts

@@ -0,0 +1,14 @@
+/**
+ * Similar fixture to `error.todo-align-scopes-nested-block-structure`, but
+ * a simpler case.
+ */
+function useFoo(cond) {
+  let s = null;
+  if (cond) {
+    s = {};
+  } else {
+    return null;
+  }
+  mutate(s);
+  return s;
+}

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scopes-nested-block-structure.expect.md

@@ -0,0 +1,68 @@
+
+## Input
+
+```javascript
+/**
+ * Fixture showing that it's not sufficient to only align direct scoped
+ * accesses of a block-fallthrough pair.
+ * Below is a simplified view of HIR blocks in this fixture.
+ * Note that here, s is mutated in both bb1 and bb4. However, neither
+ * bb1 nor bb4 have terminal fallthroughs or are fallthroughs themselves.
+ *
+ * This means that we need to recursively visit all scopes accessed between
+ * a block and its fallthrough and extend the range of those scopes which overlap
+ * with an active block/fallthrough pair,
+ *
+ *  bb0
+ *  ┌──────────────┐
+ *  │let s = null  │
+ *  │test cond1    │
+ *  │ <fallthr=bb3>│
+ *  └┬─────────────┘
+ *   │  bb1
+ *   ├─►┌───────┐
+ *   │  │s = {} ├────┐
+ *   │  └───────┘    │
+ *   │  bb2          │
+ *   └─►┌───────┐    │
+ *      │return;│    │
+ *      └───────┘    │
+ *  bb3              │
+ *  ┌──────────────┐◄┘
+ *  │test cond2    │
+ *  │ <fallthr=bb5>│
+ *  └┬─────────────┘
+ *   │  bb4
+ *   ├─►┌─────────┐
+ *   │  │mutate(s)├─┐
+ *   ▼  └─────────┘ │
+ *  bb5             │
+ *  ┌───────────┐   │
+ *  │return s;  │◄──┘
+ *  └───────────┘
+ */
+function useFoo(cond1, cond2) {
+  let s = null;
+  if (cond1) {
+    s = {};
+  } else {
+    return null;
+  }
+
+  if (cond2) {
+    mutate(s);
+  }
+
+  return s;
+}
+
+```
+
+
+## Error
+
+```
+Invariant: Invalid nesting in program blocks or scopes. Items overlap but are not nested: 4:10(5:15)
+```
+          
+      

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scopes-nested-block-structure.ts

@@ -0,0 +1,53 @@
+/**
+ * Fixture showing that it's not sufficient to only align direct scoped
+ * accesses of a block-fallthrough pair.
+ * Below is a simplified view of HIR blocks in this fixture.
+ * Note that here, s is mutated in both bb1 and bb4. However, neither
+ * bb1 nor bb4 have terminal fallthroughs or are fallthroughs themselves.
+ *
+ * This means that we need to recursively visit all scopes accessed between
+ * a block and its fallthrough and extend the range of those scopes which overlap
+ * with an active block/fallthrough pair,
+ *
+ *  bb0
+ *  ┌──────────────┐
+ *  │let s = null  │
+ *  │test cond1    │
+ *  │ <fallthr=bb3>│
+ *  └┬─────────────┘
+ *   │  bb1
+ *   ├─►┌───────┐
+ *   │  │s = {} ├────┐
+ *   │  └───────┘    │
+ *   │  bb2          │
+ *   └─►┌───────┐    │
+ *      │return;│    │
+ *      └───────┘    │
+ *  bb3              │
+ *  ┌──────────────┐◄┘
+ *  │test cond2    │
+ *  │ <fallthr=bb5>│
+ *  └┬─────────────┘
+ *   │  bb4
+ *   ├─►┌─────────┐
+ *   │  │mutate(s)├─┐
+ *   ▼  └─────────┘ │
+ *  bb5             │
+ *  ┌───────────┐   │
+ *  │return s;  │◄──┘
+ *  └───────────┘
+ */
+function useFoo(cond1, cond2) {
+  let s = null;
+  if (cond1) {
+    s = {};
+  } else {
+    return null;
+  }
+
+  if (cond2) {
+    mutate(s);
+  }
+
+  return s;
+}