From 2827cbc594448a7bc2dd5e50fa013b040210372c Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Wed, 12 Jun 2024 15:51:01 -0400 Subject: [PATCH] [compiler][fixtures] Bug repros: codegen, alignScope, phis [ghstack-poisoned] --- .../bug-codegen-inline-iife.expect.md | 92 +++++++++++++++++++ .../compiler/bug-codegen-inline-iife.ts | 36 ++++++++ .../bug-phi-reference-effect.expect.md | 78 ++++++++++++++++ .../compiler/bug-phi-reference-effect.ts | 29 ++++++ ...o-align-scope-starts-within-cond.expect.md | 29 ++++++ ...ror.todo-align-scope-starts-within-cond.ts | 14 +++ ...gn-scopes-nested-block-structure.expect.md | 68 ++++++++++++++ ...odo-align-scopes-nested-block-structure.ts | 53 +++++++++++ .../packages/snap/src/SproutTodoFilter.ts | 2 + 9 files changed, 401 insertions(+) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-phi-reference-effect.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-phi-reference-effect.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scope-starts-within-cond.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scope-starts-within-cond.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scopes-nested-block-structure.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scopes-nested-block-structure.ts diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.expect.md new file mode 100644 index 00000000000..535018bf76b --- /dev/null +++ b/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( + 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: [], +}; + +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.ts new file mode 100644 index 00000000000..185bd89cb44 --- /dev/null +++ b/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( + print(1), + (function foo() { + print(2); + return 2; + })() + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useTest, + params: [], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-phi-reference-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-phi-reference-effect.expect.md new file mode 100644 index 00000000000..1ce503e4af4 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-phi-reference-effect.expect.md @@ -0,0 +1,78 @@ + +## Input + +```javascript +import { arrayPush } from "shared-runtime"; + +/** + * Evaluator error: + * Found differences in evaluator results + * Non-forget (expected): + * (kind: ok) [2] + * [2] + * Forget: + * (kind: ok) [2] + * [2,2] + */ +function Foo(cond) { + let x = null; + if (cond) { + x = []; + } else { + } + // Here, x = phi(x$null, x$[]) does not receive the correct ValueKind + arrayPush(x, 2); + + return x; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{ cond: true }], + sequentialRenders: [{ cond: true }, { cond: true }], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { arrayPush } from "shared-runtime"; + +/** + * Evaluator error: + * Found differences in evaluator results + * Non-forget (expected): + * (kind: ok) [2] + * [2] + * Forget: + * (kind: ok) [2] + * [2,2] + */ +function Foo(cond) { + const $ = _c(1); + let x = null; + if (cond) { + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = []; + $[0] = t0; + } else { + t0 = $[0]; + } + x = t0; + } + + arrayPush(x, 2); + return x; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{ cond: true }], + sequentialRenders: [{ cond: true }, { cond: true }], +}; + +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-phi-reference-effect.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-phi-reference-effect.ts new file mode 100644 index 00000000000..6660f059ca0 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-phi-reference-effect.ts @@ -0,0 +1,29 @@ +import { arrayPush } from "shared-runtime"; + +/** + * Evaluator error: + * Found differences in evaluator results + * Non-forget (expected): + * (kind: ok) [2] + * [2] + * Forget: + * (kind: ok) [2] + * [2,2] + */ +function Foo(cond) { + let x = null; + if (cond) { + x = []; + } else { + } + // Here, x = phi(x$null, x$[]) does not receive the correct ValueKind + arrayPush(x, 2); + + return x; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{ cond: true }], + sequentialRenders: [{ cond: true }, { cond: true }], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scope-starts-within-cond.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scope-starts-within-cond.expect.md new file mode 100644 index 00000000000..a3b498b1c37 --- /dev/null +++ b/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) +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scope-starts-within-cond.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scope-starts-within-cond.ts new file mode 100644 index 00000000000..555bb713898 --- /dev/null +++ b/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; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scopes-nested-block-structure.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scopes-nested-block-structure.expect.md new file mode 100644 index 00000000000..9d19f7fa219 --- /dev/null +++ b/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 │ + * │ │ + * └┬─────────────┘ + * │ bb1 + * ├─►┌───────┐ + * │ │s = {} ├────┐ + * │ └───────┘ │ + * │ bb2 │ + * └─►┌───────┐ │ + * │return;│ │ + * └───────┘ │ + * bb3 │ + * ┌──────────────┐◄┘ + * │test cond2 │ + * │ │ + * └┬─────────────┘ + * │ 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) +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scopes-nested-block-structure.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scopes-nested-block-structure.ts new file mode 100644 index 00000000000..8e99f0435b5 --- /dev/null +++ b/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 │ + * │ │ + * └┬─────────────┘ + * │ bb1 + * ├─►┌───────┐ + * │ │s = {} ├────┐ + * │ └───────┘ │ + * │ bb2 │ + * └─►┌───────┐ │ + * │return;│ │ + * └───────┘ │ + * bb3 │ + * ┌──────────────┐◄┘ + * │test cond2 │ + * │ │ + * └┬─────────────┘ + * │ 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; +} diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts index b36e8064a78..c2581e7e577 100644 --- a/compiler/packages/snap/src/SproutTodoFilter.ts +++ b/compiler/packages/snap/src/SproutTodoFilter.ts @@ -488,6 +488,8 @@ const skipFilter = new Set([ "bug-invalid-hoisting-functionexpr", "original-reactive-scopes-fork/bug-nonmutating-capture-in-unsplittable-memo-block", "original-reactive-scopes-fork/bug-hoisted-declaration-with-scope", + "bug-codegen-inline-iife", + "bug-phi-reference-effect", // 'react-compiler-runtime' not yet supported "flag-enable-emit-hook-guards",