From bd37fbe06acca8e11a00ea48752fb5e86146f42f Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Tue, 2 Jan 2024 15:31:57 -0800 Subject: [PATCH] [wip] Fix phi inference, expose InferMutableRange issue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit > Update: this is now passing all tests. The approach is likely wrong, and even if it's fine it needs some cleanup. Putting up for review as folks (esp @gsathya) have time. ## Background InferTypes was intended to infer types for phi identifiers, but by accident we ended up storing the inferred type on `phi.type` instead of `phi.id.type`, which is the type that usages of the phi will reference. Because of this, we weren't actually inferring types for several cases, for example if both if/else branches assign `x` to an array literal, we'd ideally like the corresponding phi id to be typed as a BuiltInArray: ```javascript let x; let y = { ... }; if (cond) { x = []; } else { x = []; } // x should be BuiltnArray here. We inferred that on Phi.type but the x here wouldn't get that type previously x.push(y); ``` ## Circular Types I started by removing the `Phi.type` property and updating inference to store the result of phi unification on `phi.id.type` — but this revealed other issues. First was this can create circular types when there are loops. The solution is to basically allow circular types _for phis only_, and when we detect them we remove the cycle. Basically whenever we have a situation where we have some type variable X, and a type Y that is a (nested) phi type one of whose transitive operands contains X, we remove X from the transitive type and attempt to collapse the phi type upwards if all of its remaining operands are the same: ``` X=Type(1) Y=Phi [ Type(2), Type(3) = Phi [ Type(1), // <-- cycle but we can prune this Type(2), Type(2), ] ] => X=Type(1) Y=Phi [ Type(2), Type(3) = Phi [ // all remaining operands are the same, we can prune this Type(2), Type(2), ] ] => X=Type(1) Y=Phi [ // all remaining operands are the same, we can prune this Type(2), Type(2), ] => X=Type(1) Y=Type(2) ``` We have to do this not just doing unify(), but also in `get()` since there are cases where we don't know yet which type variables we can remove from a phi. Without also doing the pruning in get, we get an infinite loop. ## Reactive Scope Alignment The above fixed the circular types, but exposed some new cases that can occur in terms of mutable ranges and ast structures: it wasn't possible before to have a Store on a phi node in practice, since that relied on type information which we didn't have for phis. The new validation that all instructions for a scope are part of that scope caught a couple issues, which were basically like this: ``` [1] Sequence ... [9] StoreLocal x@0[9:28] [10] ... ``` Note that scope 0 starts at instruction 9, but that instruction is not at the block scope level. The first instruction at the block scope level that is within the range of scope 0 is instruction 10, which is after the scope should have started! So I also had to update AlignScopesToBlockScopes to handle the case of logical, conditional, and sequence expressions: we sometime need to adjust a scope start earlier in case they contain instructions that should start a scope. --- .../AlignReactiveScopesToBlockScopes.ts | 40 ++++++++++++++++++ ...ze-value-block-value-conditional.expect.md | 42 +++++++++++++++++++ .../memoize-value-block-value-conditional.js | 10 +++++ ...-block-value-logical-no-sequence.expect.md | 42 +++++++++++++++++++ ...e-value-block-value-logical-no-sequence.js | 10 +++++ ...emoize-value-block-value-logical.expect.md | 42 +++++++++++++++++++ .../memoize-value-block-value-logical.js | 10 +++++ ...moize-value-block-value-sequence.expect.md | 42 +++++++++++++++++++ .../memoize-value-block-value-sequence.js | 10 +++++ ...ssa-renaming-ternary-destruction.expect.md | 10 ++++- .../compiler/ssa-renaming-ternary.expect.md | 10 ++++- ...onditional-ternary-with-mutation.expect.md | 15 +++++-- ...a-renaming-unconditional-ternary.expect.md | 10 ++++- 13 files changed, 284 insertions(+), 9 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-conditional.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-conditional.js create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-logical-no-sequence.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-logical-no-sequence.js create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-logical.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-logical.js create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-sequence.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-sequence.js diff --git a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/AlignReactiveScopesToBlockScopes.ts b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/AlignReactiveScopesToBlockScopes.ts index e5c18f44fd..851b464853 100644 --- a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/AlignReactiveScopesToBlockScopes.ts +++ b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/AlignReactiveScopesToBlockScopes.ts @@ -10,8 +10,10 @@ import { Place, ReactiveBlock, ReactiveFunction, + ReactiveInstruction, ReactiveScope, ScopeId, + makeInstructionId, } from "../HIR/HIR"; import { getPlaceScope } from "./BuildReactiveBlocks"; import { ReactiveFunctionVisitor, visitReactiveFunction } from "./visitors"; @@ -79,6 +81,40 @@ class Visitor extends ReactiveFunctionVisitor { state.visitScope(scope); } } + + override visitInstruction(instr: ReactiveInstruction, state: Context): void { + switch (instr.value.kind) { + case "SequenceExpression": + case "ConditionalExpression": + case "LogicalExpression": { + const prevScopeCount = state.currentScopes().length; + this.traverseInstruction(instr, state); + + /** + * These compound value types can have nested sequences of instructions + * with scopes that start "partway" through a block-level instruction. + * This would cause the start of the scope to not align with any block-level + * instruction and get skipped by the later BuildReactiveBlocks pass. + * + * Here we detect scopes created within compound instructions and align the + * start of these scopes to the outer instruction id to ensure the scopes + * aren't skipped. + */ + const scopes = state.currentScopes(); + for (let i = prevScopeCount; i < scopes.length; i++) { + const scope = scopes[i]; + scope.scope.range.start = makeInstructionId( + Math.min(instr.id, scope.scope.range.start) + ); + } + break; + } + default: { + this.traverseInstruction(instr, state); + } + } + } + override visitBlock(block: ReactiveBlock, state: Context): void { state.enter(() => { this.traverseBlock(block, state); @@ -108,6 +144,10 @@ class Context { */ #seenScopes: Set = new Set(); + currentScopes(): Array { + return this.#blockScopes.at(-1) ?? []; + } + enter(fn: () => void): void { this.#blockScopes.push([]); fn(); diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-conditional.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-conditional.expect.md new file mode 100644 index 0000000000..7c31cc569e --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-conditional.expect.md @@ -0,0 +1,42 @@ + +## Input + +```javascript +function Foo(props) { + let x; + true ? (x = []) : (x = {}); + return x; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{}], +}; + +``` + +## Code + +```javascript +import { unstable_useMemoCache as useMemoCache } from "react"; +function Foo(props) { + const $ = useMemoCache(1); + let x; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + true ? (x = []) : (x = {}); + $[0] = x; + } else { + x = $[0]; + } + return x; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{}], +}; + +``` + +### Eval output +(kind: ok) [] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-conditional.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-conditional.js new file mode 100644 index 0000000000..2b8b15eec4 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-conditional.js @@ -0,0 +1,10 @@ +function Foo(props) { + let x; + true ? (x = []) : (x = {}); + return x; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{}], +}; diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-logical-no-sequence.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-logical-no-sequence.expect.md new file mode 100644 index 0000000000..8dc2c32194 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-logical-no-sequence.expect.md @@ -0,0 +1,42 @@ + +## Input + +```javascript +function Foo(props) { + let x; + true && (x = []); + return x; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{}], +}; + +``` + +## Code + +```javascript +import { unstable_useMemoCache as useMemoCache } from "react"; +function Foo(props) { + const $ = useMemoCache(1); + let x; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + true && (x = []); + $[0] = x; + } else { + x = $[0]; + } + return x; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{}], +}; + +``` + +### Eval output +(kind: ok) [] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-logical-no-sequence.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-logical-no-sequence.js new file mode 100644 index 0000000000..643c62ed94 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-logical-no-sequence.js @@ -0,0 +1,10 @@ +function Foo(props) { + let x; + true && (x = []); + return x; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{}], +}; diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-logical.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-logical.expect.md new file mode 100644 index 0000000000..d84e4e4d13 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-logical.expect.md @@ -0,0 +1,42 @@ + +## Input + +```javascript +function Foo(props) { + let x; + true && ((x = []), null); + return x; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{}], +}; + +``` + +## Code + +```javascript +import { unstable_useMemoCache as useMemoCache } from "react"; +function Foo(props) { + const $ = useMemoCache(1); + let x; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + true && ((x = []), null); + $[0] = x; + } else { + x = $[0]; + } + return x; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{}], +}; + +``` + +### Eval output +(kind: ok) [] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-logical.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-logical.js new file mode 100644 index 0000000000..a55aac86ca --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-logical.js @@ -0,0 +1,10 @@ +function Foo(props) { + let x; + true && ((x = []), null); + return x; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{}], +}; diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-sequence.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-sequence.expect.md new file mode 100644 index 0000000000..0227db8ba9 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-sequence.expect.md @@ -0,0 +1,42 @@ + +## Input + +```javascript +function Foo(props) { + let x; + (x = []), null; + return x; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{}], +}; + +``` + +## Code + +```javascript +import { unstable_useMemoCache as useMemoCache } from "react"; +function Foo(props) { + const $ = useMemoCache(1); + let x; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + (x = []), null; + $[0] = x; + } else { + x = $[0]; + } + return x; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{}], +}; + +``` + +### Eval output +(kind: ok) [] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-sequence.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-sequence.js new file mode 100644 index 0000000000..5c731aabdf --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/memoize-value-block-value-sequence.js @@ -0,0 +1,10 @@ +function Foo(props) { + let x; + (x = []), null; + return x; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{}], +}; diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/ssa-renaming-ternary-destruction.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/ssa-renaming-ternary-destruction.expect.md index 3466e4a11c..99a375956d 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/ssa-renaming-ternary-destruction.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/ssa-renaming-ternary-destruction.expect.md @@ -22,7 +22,7 @@ export const FIXTURE_ENTRYPOINT = { ```javascript import { unstable_useMemoCache as useMemoCache } from "react"; function foo(props) { - const $ = useMemoCache(2); + const $ = useMemoCache(4); let x; if ($[0] !== props.bar) { x = []; @@ -32,7 +32,13 @@ function foo(props) { } else { x = $[1]; } - props.cond ? (([x] = [[]]), x.push(props.foo)) : null; + if ($[2] !== props) { + props.cond ? (([x] = [[]]), x.push(props.foo)) : null; + $[2] = props; + $[3] = x; + } else { + x = $[3]; + } return x; } diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/ssa-renaming-ternary.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/ssa-renaming-ternary.expect.md index fec0845332..16ef8591aa 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/ssa-renaming-ternary.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/ssa-renaming-ternary.expect.md @@ -22,7 +22,7 @@ export const FIXTURE_ENTRYPOINT = { ```javascript import { unstable_useMemoCache as useMemoCache } from "react"; function foo(props) { - const $ = useMemoCache(2); + const $ = useMemoCache(4); let x; if ($[0] !== props.bar) { x = []; @@ -32,7 +32,13 @@ function foo(props) { } else { x = $[1]; } - props.cond ? ((x = []), x.push(props.foo)) : null; + if ($[2] !== props) { + props.cond ? ((x = []), x.push(props.foo)) : null; + $[2] = props; + $[3] = x; + } else { + x = $[3]; + } return x; } diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/ssa-renaming-unconditional-ternary-with-mutation.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/ssa-renaming-unconditional-ternary-with-mutation.expect.md index 9777981965..1b76a89a8b 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/ssa-renaming-unconditional-ternary-with-mutation.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/ssa-renaming-unconditional-ternary-with-mutation.expect.md @@ -19,13 +19,22 @@ function foo(props) { ```javascript import { unstable_useMemoCache as useMemoCache } from "react"; function foo(props) { - const $ = useMemoCache(2); + const $ = useMemoCache(5); let x; if ($[0] !== props) { x = []; x.push(props.bar); - props.cond ? ((x = []), x.push(props.foo)) : ((x = []), x.push(props.bar)); - mut(x); + if ($[2] !== props || $[3] !== x) { + props.cond + ? ((x = []), x.push(props.foo)) + : ((x = []), x.push(props.bar)); + mut(x); + $[2] = props; + $[3] = x; + $[4] = x; + } else { + x = $[4]; + } $[0] = props; $[1] = x; } else { diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/ssa-renaming-unconditional-ternary.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/ssa-renaming-unconditional-ternary.expect.md index dc5f680bb3..699f00ee7b 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/ssa-renaming-unconditional-ternary.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/ssa-renaming-unconditional-ternary.expect.md @@ -24,7 +24,7 @@ export const FIXTURE_ENTRYPOINT = { ```javascript import { unstable_useMemoCache as useMemoCache } from "react"; function foo(props) { - const $ = useMemoCache(2); + const $ = useMemoCache(4); let x; if ($[0] !== props.bar) { x = []; @@ -34,7 +34,13 @@ function foo(props) { } else { x = $[1]; } - props.cond ? ((x = []), x.push(props.foo)) : ((x = []), x.push(props.bar)); + if ($[2] !== props) { + props.cond ? ((x = []), x.push(props.foo)) : ((x = []), x.push(props.bar)); + $[2] = props; + $[3] = x; + } else { + x = $[3]; + } return x; }