@samitouri / QOS-React / commits / 1b5ae0638e

Fix block scoping of declarations with early return

I addressed some of the cases that lead to this invariant but there were still more. In this case, we have scopes like this: ``` scope @1 declarations=[t$0] { let t$0 = ArrayExpression [] if (...) { return null; } } scope @2 deps=[t$0] declarations=[t$1] { let t$1 = Jsx children=[t$0] ... } ``` Because scope 1 has an early return, PropagateEarlyReturns wraps its contents in a label and converts the returns to breaks: ``` scope @1 declarations=[t$0] earlyReturn={t$2} { let t$2 bb0: { let t$0 = ArrayExpression [] if (...) { t$2 = null; break bb0; } } } scope @2 deps=[t$0] declarations=[t$1] { let t$1 = Jsx children=[t$0] ... } ``` But then MergeReactiveScopesThatInvalidateTogether smushes them together: ``` scope @1 declarations=[t$1] earlyReturn={t$2} { let t$2 bb0: { let t$0 = ArrayExpression [] // <--- Oops! We're inside a block now if (...) { t$2 = null; break bb0; } } let t$1 = Jsx children=[t$0] ... } ``` Note that the `t$0` binding is now created inside the labeled block, so it's no longer accessible to the Jsx instruction which follows the labeled block. This isn't an issue with promoting temporaries or propagating outputs, but a simple issue of the labeled block (used for early return) introducing a new block scope. The solution here is to simply reorder the passes so that we transform for early returns after other optimizations. This means the jsx element will basically move inside the labeled block, solving the scoping issue: ``` scope @1 declarations=[t$1] earlyReturn={t$2} { let t$2 bb0: { let t$0 = ArrayExpression [] // ok, same block scope as its use if (...) { t$2 = null; break bb0; } let t$1 = Jsx children=[t$0] // note this moved inside the labeled block } } ```

Joe Savona committed Mar 13, 2024 at 21:29 UTC 1b5ae0638ee36b7f68a89641f6808c1afc9b217e
7 files changed +134 -60
compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts
+7 -7
@@ -307,13 +307,6 @@ function* runWithEnvironment(
307 value: reactiveFunction,
308 });
309
310 - propagateEarlyReturns(reactiveFunction);
311 - yield log({
312 - kind: "reactive",
313 - name: "PropagateEarlyReturns",
314 - value: reactiveFunction,
315 - });
316 -
310 pruneUnusedScopes(reactiveFunction);
311 yield log({
312 kind: "reactive",
@@ -335,6 +328,13 @@ function* runWithEnvironment(
328 value: reactiveFunction,
329 });
330
331 + propagateEarlyReturns(reactiveFunction);
332 + yield log({
333 + kind: "reactive",
334 + name: "PropagateEarlyReturns",
335 + value: reactiveFunction,
336 + });
337 +
338 promoteUsedTemporaries(reactiveFunction);
339 yield log({
340 kind: "reactive",
compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/CodegenReactiveFunction.ts
+15 -7
@@ -582,22 +582,30 @@ function codegenReactiveScope(
582 }
583 statements.push(memoStatement);
584
585 - if (scope.earlyReturnValue !== null) {
585 + const earlyReturnValue = scope.earlyReturnValue;
586 + if (earlyReturnValue !== null) {
587 + CompilerError.invariant(
588 + earlyReturnValue.value.name !== null &&
589 + earlyReturnValue.value.name.kind === "named",
590 + {
591 + reason: `Expected early return value to be promoted to a named variable`,
592 + loc: earlyReturnValue.loc,
593 + description: null,
594 + suggestions: null,
595 + }
596 + );
597 + const name: ValidIdentifierName = earlyReturnValue.value.name.value;
598 statements.push(
599 t.ifStatement(
600 t.binaryExpression(
601 "!==",
590 - t.identifier(scope.earlyReturnValue.value.name!.value),
602 + t.identifier(name),
603 t.callExpression(
604 t.memberExpression(t.identifier("Symbol"), t.identifier("for")),
605 [t.stringLiteral(EARLY_RETURN_SENTINEL)]
606 )
607 ),
596 - t.blockStatement([
597 - t.returnStatement(
598 - t.identifier(scope.earlyReturnValue.value.name!.value)
599 - ),
600 - ])
608 + t.blockStatement([t.returnStatement(t.identifier(name))])
609 )
610 );
611 }
compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/PrintReactiveFunction.ts
+3 -1
@@ -66,7 +66,9 @@ export function printReactiveScopeSummary(scope: ReactiveScope): string {
66 );
67 if (scope.earlyReturnValue !== null) {
68 items.push(
69 - `earlyReturn={id: ${scope.earlyReturnValue.value}, label: ${scope.earlyReturnValue.label}}`
69 + `earlyReturn={id: ${printIdentifier(
70 + scope.earlyReturnValue.value
71 + )}, label: ${scope.earlyReturnValue.label}}}`
72 );
73 }
74 return items.join(" ");
compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/PruneUnusedScopes.ts
+19 -5
@@ -9,6 +9,7 @@ import {
9 ReactiveFunction,
10 ReactiveScopeBlock,
11 ReactiveStatement,
12 + ReactiveTerminalStatement,
13 } from "../HIR/HIR";
14 import {
15 ReactiveFunctionTransform,
@@ -18,17 +19,30 @@ import {
19
20 // Converts scopes without outputs into regular blocks.
21 export function pruneUnusedScopes(fn: ReactiveFunction): void {
21 - visitReactiveFunction(fn, new Transform(), undefined);
22 + visitReactiveFunction(fn, new Transform(), {
23 + hasReturnStatement: false,
24 + } as State);
25 }
26
24 -class Transform extends ReactiveFunctionTransform<void> {
27 +type State = {
28 + hasReturnStatement: boolean;
29 +};
30 +
31 +class Transform extends ReactiveFunctionTransform<State> {
32 + override visitTerminal(stmt: ReactiveTerminalStatement, state: State): void {
33 + this.traverseTerminal(stmt, state);
34 + if (stmt.terminal.kind === "return") {
35 + state.hasReturnStatement = true;
36 + }
37 + }
38 override transformScope(
39 scopeBlock: ReactiveScopeBlock,
27 - state: void
40 + _state: State
41 ): Transformed<ReactiveStatement> {
29 - this.visitScope(scopeBlock, state);
42 + const scopeState: State = { hasReturnStatement: false };
43 + this.visitScope(scopeBlock, scopeState);
44 if (
31 - scopeBlock.scope.earlyReturnValue === null &&
45 + !scopeState.hasReturnStatement &&
46 scopeBlock.scope.reassignments.size === 0 &&
47 (scopeBlock.scope.declarations.size === 0 ||
48 /*
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-repro-no-value-for-temporary-reactive-scope-with-early-return.expect.md deleted
-40
@@ -1,40 +0,0 @@
1 -
2 -## Input
3 -
4 -```javascript
5 -// @flow @enableAssumeHooksFollowRulesOfReact @enableTransitivelyFreezeFunctionExpressions
6 -import { identity, makeObject_Primitives } from "shared-runtime";
7 -
8 -function Component(props) {
9 - const object = makeObject_Primitives();
10 - const cond = makeObject_Primitives();
11 - if (!cond) {
12 - return null;
13 - }
14 -
15 - return (
16 - <div className="foo">
17 - {fbt(
18 - "Lorum ipsum" + fbt.param("thing", object.b) + " blah blah blah",
19 - "More text"
20 - )}
21 - </div>
22 - );
23 -}
24 -
25 -```
26 -
27 -
28 -## Error
29 -
30 -```
31 - 10 |
32 - 11 | return (
33 -> 12 | <div className="foo">
34 - | ^^^^^ [ReactForget] Invariant: [Codegen] No value found for temporary. Value for 'read $40:TPrimitive' was not set in the codegen context (12:12)
35 - 13 | {fbt(
36 - 14 | "Lorum ipsum" + fbt.param("thing", object.b) + " blah blah blah",
37 - 15 | "More text"
38 -```
39 -
40 -
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/repro-no-value-for-temporary-reactive-scope-with-early-return.expect.md new
+84
@@ -0,0 +1,84 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +// @flow @enableAssumeHooksFollowRulesOfReact @enableTransitivelyFreezeFunctionExpressions
6 +import { identity, makeObject_Primitives } from "shared-runtime";
7 +import fbt from "fbt";
8 +
9 +function Component(props) {
10 + const object = makeObject_Primitives();
11 + const cond = makeObject_Primitives();
12 + if (!cond) {
13 + return null;
14 + }
15 +
16 + return (
17 + <div className="foo">
18 + {fbt(
19 + "Lorum ipsum" + fbt.param("thing", object.b) + " blah blah blah",
20 + "More text"
21 + )}
22 + </div>
23 + );
24 +}
25 +
26 +export const FIXTURE_ENTRYPOINT = {
27 + fn: Component,
28 + params: [{}],
29 +};
30 +
31 +```
32 +
33 +## Code
34 +
35 +```javascript
36 +import { unstable_useMemoCache as useMemoCache } from "react";
37 +import { identity, makeObject_Primitives } from "shared-runtime";
38 +import fbt from "fbt";
39 +
40 +function Component(props) {
41 + const $ = useMemoCache(2);
42 + let t0;
43 + let t1;
44 + if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
45 + t1 = Symbol.for("react.early_return_sentinel");
46 + bb7: {
47 + const object = makeObject_Primitives();
48 + const cond = makeObject_Primitives();
49 + if (!cond) {
50 + t1 = null;
51 + break bb7;
52 + }
53 +
54 + t0 = (
55 + <div className="foo">
56 + {fbt._(
57 + "Lorum ipsum{thing} blah blah blah",
58 + [fbt._param("thing", object.b)],
59 + { hk: "lwmuH" }
60 + )}
61 + </div>
62 + );
63 + }
64 + $[0] = t0;
65 + $[1] = t1;
66 + } else {
67 + t0 = $[0];
68 + t1 = $[1];
69 + }
70 + if (t1 !== Symbol.for("react.early_return_sentinel")) {
71 + return t1;
72 + }
73 + return t0;
74 +}
75 +
76 +export const FIXTURE_ENTRYPOINT = {
77 + fn: Component,
78 + params: [{}],
79 +};
80 +
81 +```
82 +
83 +### Eval output
84 +(kind: ok) <div class="foo">Lorum ipsumvalue1 blah blah blah</div>
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/repro-no-value-for-temporary-reactive-scope-with-early-return.js renamed
+6
@@ -1,5 +1,6 @@
1 // @flow @enableAssumeHooksFollowRulesOfReact @enableTransitivelyFreezeFunctionExpressions
2 import { identity, makeObject_Primitives } from "shared-runtime";
3 +import fbt from "fbt";
4
5 function Component(props) {
6 const object = makeObject_Primitives();
@@ -17,3 +18,8 @@ function Component(props) {
18 </div>
19 );
20 }
21 +
22 +export const FIXTURE_ENTRYPOINT = {
23 + fn: Component,
24 + params: [{}],
25 +};