@samitouri / QOS-React-1 / commits / 69733c5ad8

[hoisting][patch] use Babel identifier apis in BuildHIR hoisting logic

--- Three functional changes: - Instead of visiting all identifier references, explicitly traverse only function decls/exprs. This avoids bugs like accidentally hoisting inline references ```js // input const x = identity(y); const y = 2; // lowered HIR before this PR (simplified) [0] DeclareContext HoistedConst y$0 [1] LoadContext y$0 [2] StoreLocal Const x$5 = identity([1]) ``` - Rely on `isReferencedIdentifier()` instead of manually checking member properties / assignments, which is error prone ```js // added fixture hoisting-repro-variable-used-in-assignment const callbk = () => { // before this PR, we skip hoisting x because it's part of a declaration const copy = x; return copy; }; const x = 2; return callbk(); ``` - Visit lvalues after rvalues. This allows for recursive self-references (e.g. factorial) From the Babel side, this change relies heavily on babel's scope binding resolution logic. My understanding is: - Babel guarantees node objects are uniqued (`node1 === node2` <--> node1 and node2 are the same node in the ast) - Each binding has exactly one `bindingIdentifier` (`binding.identifier`, `getBindingIdentifier`, etc) which is identifier node @ its declaration site ```js // x is a binding identifier const x = 2; // foo is a binding identifier function foo() { } // param is a binding identifier (param) => {...} // this bar is a binding identifier let bar; // but not this bar bar = 2; ```

Mofei Zhang committed Jan 31, 2024 at 10:59 UTC 69733c5ad8cfb66204c53a71eb9f2545ba937a73
14 files changed +366 -108
compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts
+52 -43
@@ -5,7 +5,7 @@
5 * LICENSE file in the root directory of this source tree.
6 */
7
8 -import { Binding, NodePath, Scope } from "@babel/traverse";
8 +import { NodePath, Scope } from "@babel/traverse";
9 import * as t from "@babel/types";
10 import { Expression } from "@babel/types";
11 import invariant from "invariant";
@@ -326,21 +326,11 @@ function lowerStatement(
326 case "BlockStatement": {
327 const stmt = stmtPath as NodePath<t.BlockStatement>;
328 const statements = stmt.get("body");
329 - const hoistableBindings: Set<Binding> = new Set();
330 -
331 - const recordDeclaration = (lval: NodePath<t.LVal>): void => {
332 - // TODO: support other kinds of declarations that might need to be hoisted
333 - switch (lval.type) {
334 - case "Identifier": {
335 - const lv = lval as NodePath<t.Identifier>;
336 - const binding = stmt.scope.getBinding(lv.node.name);
337 - if (binding != null) {
338 - hoistableBindings.delete(binding);
339 - }
340 - break;
341 - }
342 - }
343 - };
329 + /**
330 + * Hoistable identifier bindings defined for this precise block
331 + * scope (excluding bindings from parent or child block scopes).
332 + */
333 + const hoistableIdentifiers: Set<t.Identifier> = new Set();
334
335 for (const [, binding] of Object.entries(stmt.scope.bindings)) {
336 // TODO: support other kinds of bindings
@@ -349,50 +339,60 @@ function lowerStatement(
339 binding.path.isVariableDeclarator() &&
340 binding.path.get("id").isIdentifier()
341 ) {
352 - hoistableBindings.add(binding);
342 + hoistableIdentifiers.add(binding.identifier);
343 }
344 }
345 }
346
347 for (const s of statements) {
358 - const hoistableIdentifiers = new Set<NodePath<t.Identifier>>();
359 - /*
360 - * After visiting the declaration, hoisting is no longer required
361 - * TODO: support other kinds of declarations
362 - */
363 - if (s.isVariableDeclaration()) {
364 - for (const decl of s.get("declarations")) {
365 - recordDeclaration(decl.get("id"));
366 - }
367 - }
368 -
348 + const willHoist = new Set<NodePath<t.Identifier>>();
349 /*
350 * If we see a hoistable identifier before its declaration, it should be hoisted just
371 - * before the statement that references it
351 + * before the statement that references it.
352 + * Identifier can only be hoisted if the reference occurs within an inner function
353 */
354 + let fnDepth = s.isFunctionDeclaration() ? 1 : 0;
355 + const withFunctionContext = {
356 + enter: (): void => {
357 + fnDepth++;
358 + },
359 + exit: (): void => {
360 + fnDepth--;
361 + },
362 + };
363 s.traverse({
364 + FunctionExpression: withFunctionContext,
365 + FunctionDeclaration: withFunctionContext,
366 + ArrowFunctionExpression: withFunctionContext,
367 + ObjectMethod: withFunctionContext,
368 Identifier(id: NodePath<t.Identifier>) {
375 - if (!id.isReferencedIdentifier()) {
369 + if (!id.isReferencedIdentifier() || fnDepth === 0) {
370 return;
371 }
378 - const binding = id.scope.getBinding(id.node.name);
379 - if (binding != null && hoistableBindings.has(binding)) {
380 - if (
381 - id.parentPath.isVariableDeclarator() ||
382 - // don't hoist MemberExpr `property`s, only their `object`
383 - (id.parentPath.isMemberExpression() &&
384 - id.parentPath.get("property") === id &&
385 - id.parentPath.node.computed === false)
386 - ) {
387 - return;
388 - }
389 - hoistableIdentifiers.add(id);
372 + const bindingIdentifier = id.scope.getBindingIdentifier(
373 + id.node.name
374 + );
375 + if (
376 + bindingIdentifier != null &&
377 + hoistableIdentifiers.has(bindingIdentifier)
378 + ) {
379 + willHoist.add(id);
380 + }
381 + },
382 + });
383 + /*
384 + * After visiting the declaration, hoisting is no longer required
385 + */
386 + s.traverse({
387 + Identifier(path: NodePath<t.Identifier>) {
388 + if (hoistableIdentifiers.has(path.node)) {
389 + hoistableIdentifiers.delete(path.node);
390 }
391 },
392 });
393
394 // Hoist declarations that need it to the earliest point where they are needed
395 - for (const id of hoistableIdentifiers) {
395 + for (const id of willHoist) {
396 const binding = stmt.scope.getBinding(id.node.name);
397 CompilerError.invariant(binding != null, {
398 reason: "Expected to find binding for hoisted identifier",
@@ -413,6 +413,15 @@ function lowerStatement(
413 loc: id.parentPath.node.loc ?? GeneratedSource,
414 });
415 continue;
416 + } else if (!binding.path.get("id").isIdentifier()) {
417 + builder.errors.push({
418 + severity: ErrorSeverity.Todo,
419 + reason: "Unsupported variable declaration type for hoisting",
420 + description: `${binding.path.get("id").type}`,
421 + suggestions: null,
422 + loc: id.parentPath.node.loc ?? GeneratedSource,
423 + });
424 + continue;
425 }
426 const identifier = builder.resolveIdentifier(id)!;
427 const place: Place = {
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.bug-hoisting2.expect.md deleted
-24
@@ -1,24 +0,0 @@
1 -
2 -## Input
3 -
4 -```javascript
5 -function Component() {
6 - useRunOnceDuringRender(() => {
7 - const handler = () => {
8 - return () => {
9 - detachHandler(handler);
10 - };
11 - };
12 - });
13 -}
14 -
15 -```
16 -
17 -
18 -## Error
19 -
20 -```
21 -[ReactForget] Todo: [hoisting] EnterSSA: Expected identifier to be defined before being used. Identifier handler$1 is undefined (3:7)
22 -```
23 -
24 -
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.bug-hoisting2.js deleted
-9
@@ -1,9 +0,0 @@
1 -function Component() {
2 - useRunOnceDuringRender(() => {
3 - const handler = () => {
4 - return () => {
5 - detachHandler(handler);
6 - };
7 - };
8 - });
9 -}
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.dont-hoist-inline-reference.expect.md new
+25
@@ -0,0 +1,25 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +import { identity } from "shared-runtime";
6 +function useInvalid() {
7 + const x = identity(x);
8 + return x;
9 +}
10 +
11 +export const FIXTURE_ENTRYPOINT = {
12 + fn: useInvalid,
13 + params: [],
14 +};
15 +
16 +```
17 +
18 +
19 +## Error
20 +
21 +```
22 +[ReactForget] Todo: [hoisting] EnterSSA: Expected identifier to be defined before being used. Identifier x$1 is undefined (3:3)
23 +```
24 +
25 +
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.dont-hoist-inline-reference.js new
+10
@@ -0,0 +1,10 @@
1 +import { identity } from "shared-runtime";
2 +function useInvalid() {
3 + const x = identity(x);
4 + return x;
5 +}
6 +
7 +export const FIXTURE_ENTRYPOINT = {
8 + fn: useInvalid,
9 + params: [],
10 +};
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-recursive-references-inner-functions.expect.md deleted
-31
@@ -1,31 +0,0 @@
1 -
2 -## Input
3 -
4 -```javascript
5 -function Foo({ value }: { value: number }) {
6 - const factorial = (x: number) => {
7 - if (x <= 1) {
8 - return 1;
9 - } else {
10 - return x * factorial(x - 1);
11 - }
12 - };
13 -
14 - return factorial(value);
15 -}
16 -
17 -export const FIXTURE_ENTRYPONT = {
18 - fn: Foo,
19 - params: [{ value: 3 }],
20 -};
21 -
22 -```
23 -
24 -
25 -## Error
26 -
27 -```
28 -[ReactForget] Todo: [hoisting] EnterSSA: Expected identifier to be defined before being used. Identifier factorial$3 is undefined (2:8)
29 -```
30 -
31 -
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/hoisting-object-method.expect.md new
+57
@@ -0,0 +1,57 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +function hoisting() {
6 + const x = {
7 + foo() {
8 + return bar();
9 + },
10 + };
11 + const bar = () => {
12 + return 1;
13 + };
14 +
15 + return x.foo(); // OK: bar's value is only accessed outside of its TDZ
16 +}
17 +
18 +export const FIXTURE_ENTRYPOINT = {
19 + fn: hoisting,
20 + params: [],
21 +};
22 +
23 +```
24 +
25 +## Code
26 +
27 +```javascript
28 +import { unstable_useMemoCache as useMemoCache } from "react";
29 +function hoisting() {
30 + const $ = useMemoCache(1);
31 + let t0;
32 + if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
33 + const x = {
34 + foo() {
35 + return bar();
36 + },
37 + };
38 +
39 + const bar = () => 1;
40 +
41 + t0 = x.foo();
42 + $[0] = t0;
43 + } else {
44 + t0 = $[0];
45 + }
46 + return t0;
47 +}
48 +
49 +export const FIXTURE_ENTRYPOINT = {
50 + fn: hoisting,
51 + params: [],
52 +};
53 +
54 +```
55 +
56 +### Eval output
57 +(kind: ok) 1
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/hoisting-object-method.js new
+17
@@ -0,0 +1,17 @@
1 +function hoisting() {
2 + const x = {
3 + foo() {
4 + return bar();
5 + },
6 + };
7 + const bar = () => {
8 + return 1;
9 + };
10 +
11 + return x.foo(); // OK: bar's value is only accessed outside of its TDZ
12 +}
13 +
14 +export const FIXTURE_ENTRYPOINT = {
15 + fn: hoisting,
16 + params: [],
17 +};
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/hoisting-recursive-call-within-lambda.expect.md new
+65
@@ -0,0 +1,65 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +function Foo({}) {
6 + const outer = (val) => {
7 + const fact = (x) => {
8 + if (x <= 0) {
9 + return 1;
10 + }
11 + return x * fact(x - 1);
12 + };
13 + return fact(val);
14 + };
15 + return outer(3);
16 +}
17 +
18 +export const FIXTURE_ENTRYPOINT = {
19 + fn: Foo,
20 + params: [{}],
21 +};
22 +
23 +```
24 +
25 +## Code
26 +
27 +```javascript
28 +import { unstable_useMemoCache as useMemoCache } from "react";
29 +function Foo(t32) {
30 + const $ = useMemoCache(2);
31 + let t0;
32 + if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
33 + t0 = (val) => {
34 + const fact = (x) => {
35 + if (x <= 0) {
36 + return 1;
37 + }
38 + return x * fact(x - 1);
39 + };
40 + return fact(val);
41 + };
42 + $[0] = t0;
43 + } else {
44 + t0 = $[0];
45 + }
46 + const outer = t0;
47 + let t1;
48 + if ($[1] === Symbol.for("react.memo_cache_sentinel")) {
49 + t1 = outer(3);
50 + $[1] = t1;
51 + } else {
52 + t1 = $[1];
53 + }
54 + return t1;
55 +}
56 +
57 +export const FIXTURE_ENTRYPOINT = {
58 + fn: Foo,
59 + params: [{}],
60 +};
61 +
62 +```
63 +
64 +### Eval output
65 +(kind: ok) 6
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/hoisting-recursive-call-within-lambda.js new
+17
@@ -0,0 +1,17 @@
1 +function Foo({}) {
2 + const outer = (val) => {
3 + const fact = (x) => {
4 + if (x <= 0) {
5 + return 1;
6 + }
7 + return x * fact(x - 1);
8 + };
9 + return fact(val);
10 + };
11 + return outer(3);
12 +}
13 +
14 +export const FIXTURE_ENTRYPOINT = {
15 + fn: Foo,
16 + params: [{}],
17 +};
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/hoisting-recursive-call.expect.md new
+58
@@ -0,0 +1,58 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +function Foo({ value }: { value: number }) {
6 + const factorial = (x: number) => {
7 + if (x <= 1) {
8 + return 1;
9 + } else {
10 + return x * factorial(x - 1);
11 + }
12 + };
13 +
14 + return factorial(value);
15 +}
16 +
17 +export const FIXTURE_ENTRYPOINT = {
18 + fn: Foo,
19 + params: [{ value: 3 }],
20 +};
21 +
22 +```
23 +
24 +## Code
25 +
26 +```javascript
27 +import { unstable_useMemoCache as useMemoCache } from "react";
28 +function Foo(t25) {
29 + const $ = useMemoCache(2);
30 + const { value } = t25;
31 + let t0;
32 + if ($[0] !== value) {
33 + const factorial = (x) => {
34 + if (x <= 1) {
35 + return 1;
36 + } else {
37 + return x * factorial(x - 1);
38 + }
39 + };
40 +
41 + t0 = factorial(value);
42 + $[0] = value;
43 + $[1] = t0;
44 + } else {
45 + t0 = $[1];
46 + }
47 + return t0;
48 +}
49 +
50 +export const FIXTURE_ENTRYPOINT = {
51 + fn: Foo,
52 + params: [{ value: 3 }],
53 +};
54 +
55 +```
56 +
57 +### Eval output
58 +(kind: ok) 6
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/hoisting-recursive-call.ts renamed
+1 -1
@@ -10,7 +10,7 @@ function Foo({ value }: { value: number }) {
10 return factorial(value);
11 }
12
13 -export const FIXTURE_ENTRYPONT = {
13 +export const FIXTURE_ENTRYPOINT = {
14 fn: Foo,
15 params: [{ value: 3 }],
16 };
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/hoisting-repro-variable-used-in-assignment.expect.md new
+51
@@ -0,0 +1,51 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +function get2() {
6 + const callbk = () => {
7 + const copy = x;
8 + return copy;
9 + };
10 + const x = 2;
11 + return callbk();
12 +}
13 +
14 +export const FIXTURE_ENTRYPOINT = {
15 + fn: get2,
16 + params: [],
17 +};
18 +
19 +```
20 +
21 +## Code
22 +
23 +```javascript
24 +import { unstable_useMemoCache as useMemoCache } from "react";
25 +function get2() {
26 + const $ = useMemoCache(1);
27 + let t0;
28 + if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
29 + const callbk = () => {
30 + const copy = x;
31 + return copy;
32 + };
33 +
34 + const x = 2;
35 + t0 = callbk();
36 + $[0] = t0;
37 + } else {
38 + t0 = $[0];
39 + }
40 + return t0;
41 +}
42 +
43 +export const FIXTURE_ENTRYPOINT = {
44 + fn: get2,
45 + params: [],
46 +};
47 +
48 +```
49 +
50 +### Eval output
51 +(kind: ok) 2
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/hoisting-repro-variable-used-in-assignment.js new
+13
@@ -0,0 +1,13 @@
1 +function get2() {
2 + const callbk = () => {
3 + const copy = x;
4 + return copy;
5 + };
6 + const x = 2;
7 + return callbk();
8 +}
9 +
10 +export const FIXTURE_ENTRYPOINT = {
11 + fn: get2,
12 + params: [],
13 +};