@samitouri / QOS-React-1 / commits / 935180c7e0

compiler: only resolve globals and react imports

Updates Environment#getGlobalDeclaration() to only resolve "globals" if they are a true global or an import from react/react-dom. We still keep the logic to resolve hook-like names as custom hooks. Notably, this means that a local `Array` reference won't get confused with our Array global declaration, a local `useState` (or import from something other than React) won't get confused as `React.useState()`, etc. I tried to write a proper fixture test to test that we react to changes to a custom setState setter function, but I think there may be an issue with snap and how it handles re-renders from effects. I think the tests are good here but open to feedback if we want to go down the rabbit hole of figuring out a proper snap test for this. ghstack-source-id: 5e9a8f6e0d23659c72a9d041e8d394b83d6e526d Pull Request resolved: https://github.com/facebook/react/pull/29190

Joe Savona committed May 24, 2024 at 06:59 UTC 935180c7e060e4d6e7868cef8f2e7c1b77cf8f7f
16 files changed +536 -56
compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts
+62 -14
@@ -11,7 +11,6 @@ import { fromZodError } from "zod-validation-error";
11 import { CompilerError } from "../CompilerError";
12 import { Logger } from "../Entrypoint";
13 import { Err, Ok, Result } from "../Utils/Result";
14 -import { log } from "../Utils/logger";
14 import {
15 DEFAULT_GLOBALS,
16 DEFAULT_SHAPES,
@@ -320,6 +319,8 @@ const EnvironmentConfigSchema = z.object({
319 */
320 throwUnknownException__testonly: z.boolean().default(false),
321
322 + enableSharedRuntime__testonly: z.boolean().default(false),
323 +
324 /**
325 * Enables deps of a function epxression to be treated as conditional. This
326 * makes sure we don't load a dep when it's a property (to check if it has
@@ -513,31 +514,78 @@ export class Environment {
514 }
515
516 getGlobalDeclaration(binding: NonLocalBinding): Global | null {
516 - const name = binding.name;
517 - let resolvedName = name;
518 -
517 if (this.config.hookPattern != null) {
520 - const match = new RegExp(this.config.hookPattern).exec(name);
518 + const match = new RegExp(this.config.hookPattern).exec(binding.name);
519 if (
520 match != null &&
521 typeof match[1] === "string" &&
522 isHookName(match[1])
523 ) {
526 - resolvedName = match[1];
524 + const resolvedName = match[1];
525 + return this.#globals.get(resolvedName) ?? this.#getCustomHookType();
526 }
527 }
528
530 - let resolvedGlobal: Global | null = this.#globals.get(resolvedName) ?? null;
531 - if (resolvedGlobal === null) {
532 - // Hack, since we don't track module level declarations and imports
533 - if (isHookName(resolvedName)) {
534 - return this.#getCustomHookType();
535 - } else {
536 - log(() => `Undefined global \`${name}\``);
529 + switch (binding.kind) {
530 + case "ModuleLocal": {
531 + // don't resolve module locals
532 + return isHookName(binding.name) ? this.#getCustomHookType() : null;
533 + }
534 + case "Global": {
535 + return (
536 + this.#globals.get(binding.name) ??
537 + (isHookName(binding.name) ? this.#getCustomHookType() : null)
538 + );
539 + }
540 + case "ImportSpecifier": {
541 + if (this.#isKnownReactModule(binding.module)) {
542 + /**
543 + * For `import {imported as name} from "..."` form, we use the `imported`
544 + * name rather than the local alias. Because we don't have definitions for
545 + * every React builtin hook yet, we also check to see if the imported name
546 + * is hook-like (whereas the fall-through below is checking if the aliased
547 + * name is hook-like)
548 + */
549 + return (
550 + this.#globals.get(binding.imported) ??
551 + (isHookName(binding.imported) ? this.#getCustomHookType() : null)
552 + );
553 + } else {
554 + /**
555 + * For modules we don't own, we look at whether the original name or import alias
556 + * are hook-like. Both of the following are likely hooks so we would return a hook
557 + * type for both:
558 + *
559 + * `import {useHook as foo} ...`
560 + * `import {foo as useHook} ...`
561 + */
562 + return isHookName(binding.imported) || isHookName(binding.name)
563 + ? this.#getCustomHookType()
564 + : null;
565 + }
566 + }
567 + case "ImportDefault":
568 + case "ImportNamespace": {
569 + if (this.#isKnownReactModule(binding.module)) {
570 + // only resolve imports to modules we know about
571 + return (
572 + this.#globals.get(binding.name) ??
573 + (isHookName(binding.name) ? this.#getCustomHookType() : null)
574 + );
575 + } else {
576 + return isHookName(binding.name) ? this.#getCustomHookType() : null;
577 + }
578 }
579 }
580 + }
581
540 - return resolvedGlobal;
582 + #isKnownReactModule(moduleName: string): boolean {
583 + return (
584 + moduleName.toLowerCase() === "react" ||
585 + moduleName.toLowerCase() === "react-dom" ||
586 + (this.config.enableSharedRuntime__testonly &&
587 + moduleName === "shared-runtime")
588 + );
589 }
590
591 getPropertyType(
compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts
+32 -1
@@ -588,7 +588,38 @@ export function printInstructionValue(instrValue: ReactiveValue): string {
588 break;
589 }
590 case "LoadGlobal": {
591 - value = `LoadGlobal ${instrValue.binding.name}`;
591 + switch (instrValue.binding.kind) {
592 + case "Global": {
593 + value = `LoadGlobal(global) ${instrValue.binding.name}`;
594 + break;
595 + }
596 + case "ModuleLocal": {
597 + value = `LoadGlobal(module) ${instrValue.binding.name}`;
598 + break;
599 + }
600 + case "ImportDefault": {
601 + value = `LoadGlobal import ${instrValue.binding.name} from '${instrValue.binding.module}'`;
602 + break;
603 + }
604 + case "ImportNamespace": {
605 + value = `LoadGlobal import * as ${instrValue.binding.name} from '${instrValue.binding.module}'`;
606 + break;
607 + }
608 + case "ImportSpecifier": {
609 + if (instrValue.binding.imported !== instrValue.binding.name) {
610 + value = `LoadGlobal import { ${instrValue.binding.imported} as ${instrValue.binding.name} } from '${instrValue.binding.module}'`;
611 + } else {
612 + value = `LoadGlobal import { ${instrValue.binding.name} } from '${instrValue.binding.module}'`;
613 + }
614 + break;
615 + }
616 + default: {
617 + assertExhaustive(
618 + instrValue.binding,
619 + `Unexpected binding kind \`${(instrValue.binding as any).kind}\``
620 + );
621 + }
622 + }
623 break;
624 }
625 case "StoreGlobal": {
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/destructuring-mixed-scope-and-local-variables-with-default.expect.md
+48 -41
@@ -56,71 +56,78 @@ function useFragment(_arg1, _arg2) {
56 }
57
58 function Component(props) {
59 - const $ = _c(14);
60 - const post = useFragment(graphql`...`, props.post);
59 + const $ = _c(15);
60 + let t0;
61 + if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
62 + t0 = graphql`...`;
63 + $[0] = t0;
64 + } else {
65 + t0 = $[0];
66 + }
67 + const post = useFragment(t0, props.post);
68 let media;
69 let allUrls;
70 let onClick;
64 - if ($[0] !== post) {
71 + if ($[1] !== post) {
72 allUrls = [];
73
67 - const { media: t0, comments: t1, urls: t2 } = post;
68 - media = t0 === undefined ? null : t0;
69 - let t3;
70 - if ($[4] !== t1) {
71 - t3 = t1 === undefined ? [] : t1;
72 - $[4] = t1;
73 - $[5] = t3;
74 - } else {
75 - t3 = $[5];
76 - }
77 - const comments = t3;
74 + const { media: t1, comments: t2, urls: t3 } = post;
75 + media = t1 === undefined ? null : t1;
76 let t4;
79 - if ($[6] !== t2) {
77 + if ($[5] !== t2) {
78 t4 = t2 === undefined ? [] : t2;
81 - $[6] = t2;
82 - $[7] = t4;
79 + $[5] = t2;
80 + $[6] = t4;
81 } else {
84 - t4 = $[7];
82 + t4 = $[6];
83 }
86 - const urls = t4;
84 + const comments = t4;
85 let t5;
88 - if ($[8] !== comments.length) {
89 - t5 = (e) => {
86 + if ($[7] !== t3) {
87 + t5 = t3 === undefined ? [] : t3;
88 + $[7] = t3;
89 + $[8] = t5;
90 + } else {
91 + t5 = $[8];
92 + }
93 + const urls = t5;
94 + let t6;
95 + if ($[9] !== comments.length) {
96 + t6 = (e) => {
97 if (!comments.length) {
98 return;
99 }
100
101 console.log(comments.length);
102 };
96 - $[8] = comments.length;
97 - $[9] = t5;
103 + $[9] = comments.length;
104 + $[10] = t6;
105 } else {
99 - t5 = $[9];
106 + t6 = $[10];
107 }
101 - onClick = t5;
108 + onClick = t6;
109
110 allUrls.push(...urls);
104 - $[0] = post;
105 - $[1] = media;
106 - $[2] = allUrls;
107 - $[3] = onClick;
111 + $[1] = post;
112 + $[2] = media;
113 + $[3] = allUrls;
114 + $[4] = onClick;
115 } else {
109 - media = $[1];
110 - allUrls = $[2];
111 - onClick = $[3];
116 + media = $[2];
117 + allUrls = $[3];
118 + onClick = $[4];
119 }
113 - let t0;
114 - if ($[10] !== media || $[11] !== allUrls || $[12] !== onClick) {
115 - t0 = <Stringify media={media} allUrls={allUrls} onClick={onClick} />;
116 - $[10] = media;
117 - $[11] = allUrls;
118 - $[12] = onClick;
119 - $[13] = t0;
120 + let t1;
121 + if ($[11] !== media || $[12] !== allUrls || $[13] !== onClick) {
122 + t1 = <Stringify media={media} allUrls={allUrls} onClick={onClick} />;
123 + $[11] = media;
124 + $[12] = allUrls;
125 + $[13] = onClick;
126 + $[14] = t1;
127 } else {
121 - t0 = $[13];
128 + t1 = $[14];
129 }
123 - return t0;
130 + return t1;
131 }
132
133 export const FIXTURE_ENTRYPOINT = {
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-conditional-call-aliased-hook-import.expect.md new
+30
@@ -0,0 +1,30 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +import { useFragment as readFragment } from "shared-runtime";
6 +
7 +function Component(props) {
8 + let data;
9 + if (props.cond) {
10 + data = readFragment();
11 + }
12 + return data;
13 +}
14 +
15 +```
16 +
17 +
18 +## Error
19 +
20 +```
21 + 4 | let data;
22 + 5 | if (props.cond) {
23 +> 6 | data = readFragment();
24 + | ^^^^^^^^^^^^ InvalidReact: Hooks must always be called in a consistent order, and may not be called conditionally. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning) (6:6)
25 + 7 | }
26 + 8 | return data;
27 + 9 | }
28 +```
29 +
30 +
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-conditional-call-aliased-hook-import.js new
+9
@@ -0,0 +1,9 @@
1 +import { useFragment as readFragment } from "shared-runtime";
2 +
3 +function Component(props) {
4 + let data;
5 + if (props.cond) {
6 + data = readFragment();
7 + }
8 + return data;
9 +}
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-conditional-call-aliased-react-hook.expect.md new
+30
@@ -0,0 +1,30 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +import { useState as state } from "react";
6 +
7 +function Component(props) {
8 + let s;
9 + if (props.cond) {
10 + [s] = state();
11 + }
12 + return s;
13 +}
14 +
15 +```
16 +
17 +
18 +## Error
19 +
20 +```
21 + 4 | let s;
22 + 5 | if (props.cond) {
23 +> 6 | [s] = state();
24 + | ^^^^^ InvalidReact: Hooks must always be called in a consistent order, and may not be called conditionally. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning) (6:6)
25 + 7 | }
26 + 8 | return s;
27 + 9 | }
28 +```
29 +
30 +
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-conditional-call-aliased-react-hook.js new
+9
@@ -0,0 +1,9 @@
1 +import { useState as state } from "react";
2 +
3 +function Component(props) {
4 + let s;
5 + if (props.cond) {
6 + [s] = state();
7 + }
8 + return s;
9 +}
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-conditional-call-non-hook-imported-as-hook.expect.md new
+30
@@ -0,0 +1,30 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +import { makeArray as useArray } from "other";
6 +
7 +function Component(props) {
8 + let data;
9 + if (props.cond) {
10 + data = useArray();
11 + }
12 + return data;
13 +}
14 +
15 +```
16 +
17 +
18 +## Error
19 +
20 +```
21 + 4 | let data;
22 + 5 | if (props.cond) {
23 +> 6 | data = useArray();
24 + | ^^^^^^^^ InvalidReact: Hooks must always be called in a consistent order, and may not be called conditionally. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning) (6:6)
25 + 7 | }
26 + 8 | return data;
27 + 9 | }
28 +```
29 +
30 +
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-conditional-call-non-hook-imported-as-hook.js new
+9
@@ -0,0 +1,9 @@
1 +import { makeArray as useArray } from "other";
2 +
3 +function Component(props) {
4 + let data;
5 + if (props.cond) {
6 + data = useArray();
7 + }
8 + return data;
9 +}
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/globals-dont-resolve-local-useState.expect.md new
+78
@@ -0,0 +1,78 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +import { useState as _useState, useCallback, useEffect } from "react";
6 +import { ValidateMemoization } from "shared-runtime";
7 +
8 +function useState(value) {
9 + const [state, setState] = _useState(value);
10 + return [state, setState];
11 +}
12 +
13 +function Component() {
14 + const [state, setState] = useState("hello");
15 +
16 + return <div onClick={() => setState("goodbye")}>{state}</div>;
17 +}
18 +
19 +export const FIXTURE_ENTRYPOINT = {
20 + fn: Component,
21 + params: [{}],
22 +};
23 +
24 +```
25 +
26 +## Code
27 +
28 +```javascript
29 +import { c as _c } from "react/compiler-runtime";
30 +import { useState as _useState, useCallback, useEffect } from "react";
31 +import { ValidateMemoization } from "shared-runtime";
32 +
33 +function useState(value) {
34 + const $ = _c(2);
35 + const [state, setState] = _useState(value);
36 + let t0;
37 + if ($[0] !== state) {
38 + t0 = [state, setState];
39 + $[0] = state;
40 + $[1] = t0;
41 + } else {
42 + t0 = $[1];
43 + }
44 + return t0;
45 +}
46 +
47 +function Component() {
48 + const $ = _c(5);
49 + const [state, setState] = useState("hello");
50 + let t0;
51 + if ($[0] !== setState) {
52 + t0 = () => setState("goodbye");
53 + $[0] = setState;
54 + $[1] = t0;
55 + } else {
56 + t0 = $[1];
57 + }
58 + let t1;
59 + if ($[2] !== t0 || $[3] !== state) {
60 + t1 = <div onClick={t0}>{state}</div>;
61 + $[2] = t0;
62 + $[3] = state;
63 + $[4] = t1;
64 + } else {
65 + t1 = $[4];
66 + }
67 + return t1;
68 +}
69 +
70 +export const FIXTURE_ENTRYPOINT = {
71 + fn: Component,
72 + params: [{}],
73 +};
74 +
75 +```
76 +
77 +### Eval output
78 +(kind: ok) <div>hello</div>
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/globals-dont-resolve-local-useState.js new
+18
@@ -0,0 +1,18 @@
1 +import { useState as _useState, useCallback, useEffect } from "react";
2 +import { ValidateMemoization } from "shared-runtime";
3 +
4 +function useState(value) {
5 + const [state, setState] = _useState(value);
6 + return [state, setState];
7 +}
8 +
9 +function Component() {
10 + const [state, setState] = useState("hello");
11 +
12 + return <div onClick={() => setState("goodbye")}>{state}</div>;
13 +}
14 +
15 +export const FIXTURE_ENTRYPOINT = {
16 + fn: Component,
17 + params: [{}],
18 +};
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/resolve-react-hooks-based-on-import-name.expect.md new
+79
@@ -0,0 +1,79 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +import { useState as useReactState } from "react";
6 +
7 +function Component() {
8 + const [state, setState] = useReactState(0);
9 +
10 + const onClick = () => {
11 + setState((s) => s + 1);
12 + };
13 +
14 + return (
15 + <>
16 + Count {state}
17 + <button onClick={onClick}>Increment</button>
18 + </>
19 + );
20 +}
21 +
22 +export const FIXTURE_ENTRYPOINT = {
23 + fn: Component,
24 + params: [{}],
25 +};
26 +
27 +```
28 +
29 +## Code
30 +
31 +```javascript
32 +import { c as _c } from "react/compiler-runtime";
33 +import { useState as useReactState } from "react";
34 +
35 +function Component() {
36 + const $ = _c(4);
37 + const [state, setState] = useReactState(0);
38 + let t0;
39 + if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
40 + t0 = () => {
41 + setState((s) => s + 1);
42 + };
43 + $[0] = t0;
44 + } else {
45 + t0 = $[0];
46 + }
47 + const onClick = t0;
48 + let t1;
49 + if ($[1] === Symbol.for("react.memo_cache_sentinel")) {
50 + t1 = <button onClick={onClick}>Increment</button>;
51 + $[1] = t1;
52 + } else {
53 + t1 = $[1];
54 + }
55 + let t2;
56 + if ($[2] !== state) {
57 + t2 = (
58 + <>
59 + Count {state}
60 + {t1}
61 + </>
62 + );
63 + $[2] = state;
64 + $[3] = t2;
65 + } else {
66 + t2 = $[3];
67 + }
68 + return t2;
69 +}
70 +
71 +export const FIXTURE_ENTRYPOINT = {
72 + fn: Component,
73 + params: [{}],
74 +};
75 +
76 +```
77 +
78 +### Eval output
79 +(kind: ok) Count 0<button>Increment</button>
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/resolve-react-hooks-based-on-import-name.js new
+21
@@ -0,0 +1,21 @@
1 +import { useState as useReactState } from "react";
2 +
3 +function Component() {
4 + const [state, setState] = useReactState(0);
5 +
6 + const onClick = () => {
7 + setState((s) => s + 1);
8 + };
9 +
10 + return (
11 + <>
12 + Count {state}
13 + <button onClick={onClick}>Increment</button>
14 + </>
15 + );
16 +}
17 +
18 +export const FIXTURE_ENTRYPOINT = {
19 + fn: Component,
20 + params: [{}],
21 +};
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect-snap-test.expect.md new
+65
@@ -0,0 +1,65 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +import { useEffect, useState } from "react";
6 +
7 +function Component() {
8 + const [state, setState] = useState("hello");
9 + useEffect(() => {
10 + setState("goodbye");
11 + }, []);
12 +
13 + return <div>{state}</div>;
14 +}
15 +
16 +export const FIXTURE_ENTRYPOINT = {
17 + fn: Component,
18 + params: [{}],
19 +};
20 +
21 +```
22 +
23 +## Code
24 +
25 +```javascript
26 +import { c as _c } from "react/compiler-runtime";
27 +import { useEffect, useState } from "react";
28 +
29 +function Component() {
30 + const $ = _c(4);
31 + const [state, setState] = useState("hello");
32 + let t0;
33 + let t1;
34 + if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
35 + t0 = () => {
36 + setState("goodbye");
37 + };
38 + t1 = [];
39 + $[0] = t0;
40 + $[1] = t1;
41 + } else {
42 + t0 = $[0];
43 + t1 = $[1];
44 + }
45 + useEffect(t0, t1);
46 + let t2;
47 + if ($[2] !== state) {
48 + t2 = <div>{state}</div>;
49 + $[2] = state;
50 + $[3] = t2;
51 + } else {
52 + t2 = $[3];
53 + }
54 + return t2;
55 +}
56 +
57 +export const FIXTURE_ENTRYPOINT = {
58 + fn: Component,
59 + params: [{}],
60 +};
61 +
62 +```
63 +
64 +### Eval output
65 +(kind: ok) <div>goodbye</div>
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect-snap-test.js new
+15
@@ -0,0 +1,15 @@
1 +import { useEffect, useState } from "react";
2 +
3 +function Component() {
4 + const [state, setState] = useState("hello");
5 + useEffect(() => {
6 + setState("goodbye");
7 + }, []);
8 +
9 + return <div>{state}</div>;
10 +}
11 +
12 +export const FIXTURE_ENTRYPOINT = {
13 + fn: Component,
14 + params: [{}],
15 +};
compiler/packages/snap/src/compiler.ts
+1
@@ -170,6 +170,7 @@ function makePluginOptions(
170 enableEmitInstrumentForget,
171 enableEmitHookGuards,
172 assertValidMutableRanges: true,
173 + enableSharedRuntime__testonly: true,
174 hookPattern,
175 validatePreserveExistingMemoizationGuarantees,
176 },