Skip to content

Commit 5cec297

Browse files
josephsavonajosephsavona
committed
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: 5e9a8f6 Pull Request resolved: #29190
1 parent 4a3e365 commit 5cec297

16 files changed

+536
-56
lines changed

‎compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts

Lines changed: 62 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import { fromZodError } from "zod-validation-error";
1111
import { CompilerError } from "../CompilerError";
1212
import { Logger } from "../Entrypoint";
1313
import { Err, Ok, Result } from "../Utils/Result";
14-
import { log } from "../Utils/logger";
1514
import {
1615
DEFAULT_GLOBALS,
1716
DEFAULT_SHAPES,
@@ -320,6 +319,8 @@ const EnvironmentConfigSchema = z.object({
320319
*/
321320
throwUnknownException__testonly: z.boolean().default(false),
322321

322+
enableSharedRuntime__testonly: z.boolean().default(false),
323+
323324
/**
324325
* Enables deps of a function epxression to be treated as conditional. This
325326
* 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 {
513514
}
514515

515516
getGlobalDeclaration(binding: NonLocalBinding): Global | null {
516-
const name = binding.name;
517-
let resolvedName = name;
518-
519517
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);
521519
if (
522520
match != null &&
523521
typeof match[1] === "string" &&
524522
isHookName(match[1])
525523
) {
526-
resolvedName = match[1];
524+
const resolvedName = match[1];
525+
return this.#globals.get(resolvedName) ?? this.#getCustomHookType();
527526
}
528527
}
529528

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+
}
537578
}
538579
}
580+
}
539581

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+
);
541589
}
542590

543591
getPropertyType(

‎compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -588,7 +588,38 @@ export function printInstructionValue(instrValue: ReactiveValue): string {
588588
break;
589589
}
590590
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+
}
592623
break;
593624
}
594625
case "StoreGlobal": {

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/destructuring-mixed-scope-and-local-variables-with-default.expect.md

Lines changed: 48 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -56,71 +56,78 @@ function useFragment(_arg1, _arg2) {
5656
}
5757

5858
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);
6168
let media;
6269
let allUrls;
6370
let onClick;
64-
if ($[0] !== post) {
71+
if ($[1] !== post) {
6572
allUrls = [];
6673

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;
7876
let t4;
79-
if ($[6] !== t2) {
77+
if ($[5] !== t2) {
8078
t4 = t2 === undefined ? [] : t2;
81-
$[6] = t2;
82-
$[7] = t4;
79+
$[5] = t2;
80+
$[6] = t4;
8381
} else {
84-
t4 = $[7];
82+
t4 = $[6];
8583
}
86-
const urls = t4;
84+
const comments = t4;
8785
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) => {
9097
if (!comments.length) {
9198
return;
9299
}
93100

94101
console.log(comments.length);
95102
};
96-
$[8] = comments.length;
97-
$[9] = t5;
103+
$[9] = comments.length;
104+
$[10] = t6;
98105
} else {
99-
t5 = $[9];
106+
t6 = $[10];
100107
}
101-
onClick = t5;
108+
onClick = t6;
102109

103110
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;
108115
} else {
109-
media = $[1];
110-
allUrls = $[2];
111-
onClick = $[3];
116+
media = $[2];
117+
allUrls = $[3];
118+
onClick = $[4];
112119
}
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;
120127
} else {
121-
t0 = $[13];
128+
t1 = $[14];
122129
}
123-
return t0;
130+
return t1;
124131
}
125132

126133
export const FIXTURE_ENTRYPOINT = {

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-conditional-call-aliased-hook-import.expect.md

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -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+

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-conditional-call-aliased-hook-import.js

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -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+

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-conditional-call-aliased-react-hook.js

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -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+

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-conditional-call-non-hook-imported-as-hook.js

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -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+
}

0 commit comments

Comments
 (0)