Skip to content

Commit d724ba9

Browse files
committed
[compiler] Validate type configs for hooks/non-hooks
Alternative to #30868. The goal is to ensure that the types coming out of moduleTypeProvider are valid wrt to hook typing. If something is named like a hook, then it must be typed as a hook (or don't type it). ghstack-source-id: 3e8b5a0 Pull Request resolved: #30888
1 parent 3dfd5d9 commit d724ba9

11 files changed

+284
-60
lines changed

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

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ import {
3333
Type,
3434
ValidatedIdentifier,
3535
ValueKind,
36+
getHookKindForType,
3637
makeBlockId,
3738
makeIdentifierId,
3839
makeIdentifierName,
@@ -737,6 +738,8 @@ export class Environment {
737738
this.#globals,
738739
this.#shapes,
739740
moduleConfig,
741+
moduleName,
742+
loc,
740743
);
741744
} else {
742745
moduleType = null;
@@ -794,6 +797,21 @@ export class Environment {
794797
binding.imported,
795798
);
796799
if (importedType != null) {
800+
/*
801+
* Check that hook-like export names are hook types, and non-hook names are non-hook types.
802+
* The user-assigned alias isn't decidable by the type provider, so we ignore that for the check.
803+
* Thus we allow `import {fooNonHook as useFoo} from ...` because the name and type both say
804+
* that it's not a hook.
805+
*/
806+
const expectHook = isHookName(binding.imported);
807+
const isHook = getHookKindForType(this, importedType) != null;
808+
if (expectHook !== isHook) {
809+
CompilerError.throwInvalidConfig({
810+
reason: `Invalid type configuration for module`,
811+
description: `Expected type for \`import {${binding.imported}} from '${binding.module}'\` ${expectHook ? 'to be a hook' : 'not to be a hook'} based on the exported name`,
812+
loc,
813+
});
814+
}
797815
return importedType;
798816
}
799817
}
@@ -822,13 +840,30 @@ export class Environment {
822840
} else {
823841
const moduleType = this.#resolveModuleType(binding.module, loc);
824842
if (moduleType !== null) {
843+
let importedType: Type | null = null;
825844
if (binding.kind === 'ImportDefault') {
826845
const defaultType = this.getPropertyType(moduleType, 'default');
827846
if (defaultType !== null) {
828-
return defaultType;
847+
importedType = defaultType;
829848
}
830849
} else {
831-
return moduleType;
850+
importedType = moduleType;
851+
}
852+
if (importedType !== null) {
853+
/*
854+
* Check that the hook-like modules are defined as types, and non hook-like modules are not typed as hooks.
855+
* So `import Foo from 'useFoo'` is expected to be a hook based on the module name
856+
*/
857+
const expectHook = isHookName(binding.module);
858+
const isHook = getHookKindForType(this, importedType) != null;
859+
if (expectHook !== isHook) {
860+
CompilerError.throwInvalidConfig({
861+
reason: `Invalid type configuration for module`,
862+
description: `Expected type for \`import ... from '${binding.module}'\` ${expectHook ? 'to be a hook' : 'not to be a hook'} based on the module name`,
863+
loc,
864+
});
865+
}
866+
return importedType;
832867
}
833868
}
834869
return isHookName(binding.name) ? this.#getCustomHookType() : null;

compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts

Lines changed: 43 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@ import {
2828
import {BuiltInType, PolyType} from './Types';
2929
import {TypeConfig} from './TypeSchema';
3030
import {assertExhaustive} from '../Utils/utils';
31+
import {isHookName} from './Environment';
32+
import {CompilerError, SourceLocation} from '..';
3133

3234
/*
3335
* This file exports types and defaults for JavaScript global objects.
@@ -535,6 +537,8 @@ export function installTypeConfig(
535537
globals: GlobalRegistry,
536538
shapes: ShapeRegistry,
537539
typeConfig: TypeConfig,
540+
moduleName: string,
541+
loc: SourceLocation,
538542
): Global {
539543
switch (typeConfig.kind) {
540544
case 'type': {
@@ -567,7 +571,13 @@ export function installTypeConfig(
567571
positionalParams: typeConfig.positionalParams,
568572
restParam: typeConfig.restParam,
569573
calleeEffect: typeConfig.calleeEffect,
570-
returnType: installTypeConfig(globals, shapes, typeConfig.returnType),
574+
returnType: installTypeConfig(
575+
globals,
576+
shapes,
577+
typeConfig.returnType,
578+
moduleName,
579+
loc,
580+
),
571581
returnValueKind: typeConfig.returnValueKind,
572582
noAlias: typeConfig.noAlias === true,
573583
mutableOnlyIfOperandsAreMutable:
@@ -580,7 +590,13 @@ export function installTypeConfig(
580590
positionalParams: typeConfig.positionalParams ?? [],
581591
restParam: typeConfig.restParam ?? Effect.Freeze,
582592
calleeEffect: Effect.Read,
583-
returnType: installTypeConfig(globals, shapes, typeConfig.returnType),
593+
returnType: installTypeConfig(
594+
globals,
595+
shapes,
596+
typeConfig.returnType,
597+
moduleName,
598+
loc,
599+
),
584600
returnValueKind: typeConfig.returnValueKind ?? ValueKind.Frozen,
585601
noAlias: typeConfig.noAlias === true,
586602
});
@@ -589,10 +605,31 @@ export function installTypeConfig(
589605
return addObject(
590606
shapes,
591607
null,
592-
Object.entries(typeConfig.properties ?? {}).map(([key, value]) => [
593-
key,
594-
installTypeConfig(globals, shapes, value),
595-
]),
608+
Object.entries(typeConfig.properties ?? {}).map(([key, value]) => {
609+
const type = installTypeConfig(
610+
globals,
611+
shapes,
612+
value,
613+
moduleName,
614+
loc,
615+
);
616+
const expectHook = isHookName(key);
617+
let isHook = false;
618+
if (type.kind === 'Function' && type.shapeId !== null) {
619+
const functionType = shapes.get(type.shapeId);
620+
if (functionType?.functionType?.hookKind !== null) {
621+
isHook = true;
622+
}
623+
}
624+
if (expectHook !== isHook) {
625+
CompilerError.throwInvalidConfig({
626+
reason: `Invalid type configuration for module`,
627+
description: `Expected type for object property '${key}' from module '${moduleName}' ${expectHook ? 'to be a hook' : 'not to be a hook'} based on the property name`,
628+
loc,
629+
});
630+
}
631+
return [key, type];
632+
}),
596633
);
597634
}
598635
default: {
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
2+
## Input
3+
4+
```javascript
5+
import ReactCompilerTest from 'ReactCompilerTest';
6+
7+
function Component() {
8+
return ReactCompilerTest.useHookNotTypedAsHook();
9+
}
10+
11+
```
12+
13+
14+
## Error
15+
16+
```
17+
2 |
18+
3 | function Component() {
19+
> 4 | return ReactCompilerTest.useHookNotTypedAsHook();
20+
| ^^^^^^^^^^^^^^^^^ InvalidConfig: Invalid type configuration for module. Expected type for object property 'useHookNotTypedAsHook' from module 'ReactCompilerTest' to be a hook based on the property name (4:4)
21+
5 | }
22+
6 |
23+
```
24+
25+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
import ReactCompilerTest from 'ReactCompilerTest';
2+
3+
function Component() {
4+
return ReactCompilerTest.useHookNotTypedAsHook();
5+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
2+
## Input
3+
4+
```javascript
5+
import {useHookNotTypedAsHook} from 'ReactCompilerTest';
6+
7+
function Component() {
8+
return useHookNotTypedAsHook();
9+
}
10+
11+
```
12+
13+
14+
## Error
15+
16+
```
17+
2 |
18+
3 | function Component() {
19+
> 4 | return useHookNotTypedAsHook();
20+
| ^^^^^^^^^^^^^^^^^^^^^ InvalidConfig: Invalid type configuration for module. Expected type for object property 'useHookNotTypedAsHook' from module 'ReactCompilerTest' to be a hook based on the property name (4:4)
21+
5 | }
22+
6 |
23+
```
24+
25+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
import {useHookNotTypedAsHook} from 'ReactCompilerTest';
2+
3+
function Component() {
4+
return useHookNotTypedAsHook();
5+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
2+
## Input
3+
4+
```javascript
5+
import foo from 'useDefaultExportNotTypedAsHook';
6+
7+
function Component() {
8+
return <div>{foo()}</div>;
9+
}
10+
11+
```
12+
13+
14+
## Error
15+
16+
```
17+
2 |
18+
3 | function Component() {
19+
> 4 | return <div>{foo()}</div>;
20+
| ^^^ InvalidConfig: Invalid type configuration for module. Expected type for `import ... from 'useDefaultExportNotTypedAsHook'` to be a hook based on the module name (4:4)
21+
5 | }
22+
6 |
23+
```
24+
25+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
import foo from 'useDefaultExportNotTypedAsHook';
2+
3+
function Component() {
4+
return <div>{foo()}</div>;
5+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
2+
## Input
3+
4+
```javascript
5+
import {notAhookTypedAsHook} from 'ReactCompilerTest';
6+
7+
function Component() {
8+
return <div>{notAhookTypedAsHook()}</div>;
9+
}
10+
11+
```
12+
13+
14+
## Error
15+
16+
```
17+
2 |
18+
3 | function Component() {
19+
> 4 | return <div>{notAhookTypedAsHook()}</div>;
20+
| ^^^^^^^^^^^^^^^^^^^ InvalidConfig: Invalid type configuration for module. Expected type for object property 'useHookNotTypedAsHook' from module 'ReactCompilerTest' to be a hook based on the property name (4:4)
21+
5 | }
22+
6 |
23+
```
24+
25+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
import {notAhookTypedAsHook} from 'ReactCompilerTest';
2+
3+
function Component() {
4+
return <div>{notAhookTypedAsHook()}</div>;
5+
}

0 commit comments

Comments
 (0)