diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-array-concat-should-capture.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-array-concat-should-capture.expect.md new file mode 100644 index 000000000000..41271ffaf093 --- /dev/null +++ b/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 = [{ 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 }], +}; + +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-array-concat-should-capture.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-array-concat-should-capture.ts new file mode 100644 index 000000000000..f9b5ed619c23 --- /dev/null +++ b/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 = [{ 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 }], +}; 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 000000000000..535018bf76b3 --- /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 000000000000..185bd89cb44f --- /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/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 000000000000..a3b498b1c373 --- /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 000000000000..555bb713898a --- /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 000000000000..9d19f7fa2198 --- /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 000000000000..8e99f0435b56 --- /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 e193fcb753b9..52834a2d1a14 100644 --- a/compiler/packages/snap/src/SproutTodoFilter.ts +++ b/compiler/packages/snap/src/SproutTodoFilter.ts @@ -487,6 +487,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-array-concat-should-capture", // 'react-compiler-runtime' not yet supported "flag-enable-emit-hook-guards", diff --git a/compiler/packages/snap/src/sprout/shared-runtime.ts b/compiler/packages/snap/src/sprout/shared-runtime.ts index 94fba22c0497..7d4687218d0e 100644 --- a/compiler/packages/snap/src/sprout/shared-runtime.ts +++ b/compiler/packages/snap/src/sprout/shared-runtime.ts @@ -36,7 +36,7 @@ export const CONST_NUMBER2 = 2; export const CONST_TRUE = true; export const CONST_FALSE = false; -export function initFbt() { +export function initFbt(): void { const viewerContext: IntlViewerContext = { GENDER: IntlVariations.GENDER_UNKNOWN, locale: "en_US", @@ -52,7 +52,7 @@ export function initFbt() { export function mutate(arg: any): void { // don't mutate primitive - if (typeof arg === null || typeof arg !== "object") { + if (arg == null || typeof arg !== "object") { return; } @@ -80,7 +80,7 @@ export function mutateAndReturnNewValue(arg: T): string { export function setProperty(arg: any, property: any): void { // don't mutate primitive - if (typeof arg === null || typeof arg !== "object") { + if (arg == null || typeof arg !== "object") { return arg; } @@ -123,7 +123,7 @@ export function calculateExpensiveNumber(x: number): number { /** * Functions that do not mutate their parameters */ -export function shallowCopy(obj: Object): object { +export function shallowCopy(obj: object): object { return Object.assign({}, obj); } @@ -139,9 +139,11 @@ export function addOne(value: number): number { return value + 1; } -// Alias console.log, as it is defined as a global and may have -// different compiler handling than unknown functions -export function print(...args: Array) { +/* + * Alias console.log, as it is defined as a global and may have + * different compiler handling than unknown functions + */ +export function print(...args: Array): void { console.log(...args); } @@ -153,7 +155,7 @@ export function throwErrorWithMessage(message: string): never { throw new Error(message); } -export function throwInput(x: Object): never { +export function throwInput(x: object): never { throw x; } @@ -167,12 +169,12 @@ export function logValue(value: T): void { console.log(value); } -export function useHook(): Object { +export function useHook(): object { return makeObject_Primitives(); } const noAliasObject = Object.freeze({}); -export function useNoAlias(...args: Array): object { +export function useNoAlias(..._args: Array): object { return noAliasObject; } @@ -183,7 +185,7 @@ export function useIdentity(arg: T): T { export function invoke, ReturnType>( fn: (...input: T) => ReturnType, ...params: T -) { +): ReturnType { return fn(...params); } @@ -191,7 +193,7 @@ export function conditionalInvoke, ReturnType>( shouldInvoke: boolean, fn: (...input: T) => ReturnType, ...params: T -) { +): ReturnType | null { if (shouldInvoke) { return fn(...params); } else { @@ -205,21 +207,25 @@ export function conditionalInvoke, ReturnType>( export function Text(props: { value: string; children?: Array; -}) { +}): React.ReactElement { return React.createElement("div", null, props.value, props.children); } -export function StaticText1(props: { children?: Array }) { +export function StaticText1(props: { + children?: Array; +}): React.ReactElement { return React.createElement("div", null, "StaticText1", props.children); } -export function StaticText2(props: { children?: Array }) { +export function StaticText2(props: { + children?: Array; +}): React.ReactElement { return React.createElement("div", null, "StaticText2", props.children); } export function RenderPropAsChild(props: { items: Array<() => React.ReactNode>; -}) { +}): React.ReactElement { return React.createElement( "div", null, @@ -242,7 +248,7 @@ export function ValidateMemoization({ }: { inputs: Array; output: any; -}) { +}): React.ReactElement { "use no forget"; const [previousInputs, setPreviousInputs] = React.useState(inputs); const [previousOutput, setPreviousOutput] = React.useState(output); @@ -273,7 +279,7 @@ export function createHookWrapper( } // helper functions -export function toJSON(value: any, invokeFns: boolean = false) { +export function toJSON(value: any, invokeFns: boolean = false): string { const seen = new Map(); return JSON.stringify(value, (_key: string, val: any) => { @@ -319,7 +325,7 @@ export const ObjectWithHooks = { }, }; -export function useFragment(...args: Array): Object { +export function useFragment(..._args: Array): object { return { a: [1, 2, 3], b: { c: { d: 4 } },