Skip to content

Commit

Permalink
[compiler] Validate type configs for hooks/non-hooks
Browse files Browse the repository at this point in the history
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: 3e8b5a0a7010d0c484bbb417fb258e76bf4e32bc
Pull Request resolved: #30888
  • Loading branch information
josephsavona committed Sep 10, 2024
1 parent 3dfd5d9 commit d724ba9
Show file tree
Hide file tree
Showing 11 changed files with 284 additions and 60 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
Type,
ValidatedIdentifier,
ValueKind,
getHookKindForType,
makeBlockId,
makeIdentifierId,
makeIdentifierName,
Expand Down Expand Up @@ -737,6 +738,8 @@ export class Environment {
this.#globals,
this.#shapes,
moduleConfig,
moduleName,
loc,
);
} else {
moduleType = null;
Expand Down Expand Up @@ -794,6 +797,21 @@ export class Environment {
binding.imported,
);
if (importedType != null) {
/*
* Check that hook-like export names are hook types, and non-hook names are non-hook types.
* The user-assigned alias isn't decidable by the type provider, so we ignore that for the check.
* Thus we allow `import {fooNonHook as useFoo} from ...` because the name and type both say
* that it's not a hook.
*/
const expectHook = isHookName(binding.imported);
const isHook = getHookKindForType(this, importedType) != null;
if (expectHook !== isHook) {
CompilerError.throwInvalidConfig({
reason: `Invalid type configuration for module`,
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`,
loc,
});
}
return importedType;
}
}
Expand Down Expand Up @@ -822,13 +840,30 @@ export class Environment {
} else {
const moduleType = this.#resolveModuleType(binding.module, loc);
if (moduleType !== null) {
let importedType: Type | null = null;
if (binding.kind === 'ImportDefault') {
const defaultType = this.getPropertyType(moduleType, 'default');
if (defaultType !== null) {
return defaultType;
importedType = defaultType;
}
} else {
return moduleType;
importedType = moduleType;
}
if (importedType !== null) {
/*
* Check that the hook-like modules are defined as types, and non hook-like modules are not typed as hooks.
* So `import Foo from 'useFoo'` is expected to be a hook based on the module name
*/
const expectHook = isHookName(binding.module);
const isHook = getHookKindForType(this, importedType) != null;
if (expectHook !== isHook) {
CompilerError.throwInvalidConfig({
reason: `Invalid type configuration for module`,
description: `Expected type for \`import ... from '${binding.module}'\` ${expectHook ? 'to be a hook' : 'not to be a hook'} based on the module name`,
loc,
});
}
return importedType;
}
}
return isHookName(binding.name) ? this.#getCustomHookType() : null;
Expand Down
49 changes: 43 additions & 6 deletions compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ import {
import {BuiltInType, PolyType} from './Types';
import {TypeConfig} from './TypeSchema';
import {assertExhaustive} from '../Utils/utils';
import {isHookName} from './Environment';
import {CompilerError, SourceLocation} from '..';

/*
* This file exports types and defaults for JavaScript global objects.
Expand Down Expand Up @@ -535,6 +537,8 @@ export function installTypeConfig(
globals: GlobalRegistry,
shapes: ShapeRegistry,
typeConfig: TypeConfig,
moduleName: string,
loc: SourceLocation,
): Global {
switch (typeConfig.kind) {
case 'type': {
Expand Down Expand Up @@ -567,7 +571,13 @@ export function installTypeConfig(
positionalParams: typeConfig.positionalParams,
restParam: typeConfig.restParam,
calleeEffect: typeConfig.calleeEffect,
returnType: installTypeConfig(globals, shapes, typeConfig.returnType),
returnType: installTypeConfig(
globals,
shapes,
typeConfig.returnType,
moduleName,
loc,
),
returnValueKind: typeConfig.returnValueKind,
noAlias: typeConfig.noAlias === true,
mutableOnlyIfOperandsAreMutable:
Expand All @@ -580,7 +590,13 @@ export function installTypeConfig(
positionalParams: typeConfig.positionalParams ?? [],
restParam: typeConfig.restParam ?? Effect.Freeze,
calleeEffect: Effect.Read,
returnType: installTypeConfig(globals, shapes, typeConfig.returnType),
returnType: installTypeConfig(
globals,
shapes,
typeConfig.returnType,
moduleName,
loc,
),
returnValueKind: typeConfig.returnValueKind ?? ValueKind.Frozen,
noAlias: typeConfig.noAlias === true,
});
Expand All @@ -589,10 +605,31 @@ export function installTypeConfig(
return addObject(
shapes,
null,
Object.entries(typeConfig.properties ?? {}).map(([key, value]) => [
key,
installTypeConfig(globals, shapes, value),
]),
Object.entries(typeConfig.properties ?? {}).map(([key, value]) => {
const type = installTypeConfig(
globals,
shapes,
value,
moduleName,
loc,
);
const expectHook = isHookName(key);
let isHook = false;
if (type.kind === 'Function' && type.shapeId !== null) {
const functionType = shapes.get(type.shapeId);
if (functionType?.functionType?.hookKind !== null) {
isHook = true;
}
}
if (expectHook !== isHook) {
CompilerError.throwInvalidConfig({
reason: `Invalid type configuration for module`,
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`,
loc,
});
}
return [key, type];
}),
);
}
default: {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@

## Input

```javascript
import ReactCompilerTest from 'ReactCompilerTest';

function Component() {
return ReactCompilerTest.useHookNotTypedAsHook();
}

```


## Error

```
2 |
3 | function Component() {
> 4 | return ReactCompilerTest.useHookNotTypedAsHook();
| ^^^^^^^^^^^^^^^^^ 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)
5 | }
6 |
```
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import ReactCompilerTest from 'ReactCompilerTest';

function Component() {
return ReactCompilerTest.useHookNotTypedAsHook();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@

## Input

```javascript
import {useHookNotTypedAsHook} from 'ReactCompilerTest';

function Component() {
return useHookNotTypedAsHook();
}

```


## Error

```
2 |
3 | function Component() {
> 4 | return useHookNotTypedAsHook();
| ^^^^^^^^^^^^^^^^^^^^^ 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)
5 | }
6 |
```
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import {useHookNotTypedAsHook} from 'ReactCompilerTest';

function Component() {
return useHookNotTypedAsHook();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@

## Input

```javascript
import foo from 'useDefaultExportNotTypedAsHook';

function Component() {
return <div>{foo()}</div>;
}

```


## Error

```
2 |
3 | function Component() {
> 4 | return <div>{foo()}</div>;
| ^^^ InvalidConfig: Invalid type configuration for module. Expected type for `import ... from 'useDefaultExportNotTypedAsHook'` to be a hook based on the module name (4:4)
5 | }
6 |
```
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import foo from 'useDefaultExportNotTypedAsHook';

function Component() {
return <div>{foo()}</div>;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@

## Input

```javascript
import {notAhookTypedAsHook} from 'ReactCompilerTest';

function Component() {
return <div>{notAhookTypedAsHook()}</div>;
}

```


## Error

```
2 |
3 | function Component() {
> 4 | return <div>{notAhookTypedAsHook()}</div>;
| ^^^^^^^^^^^^^^^^^^^ 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)
5 | }
6 |
```
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import {notAhookTypedAsHook} from 'ReactCompilerTest';

function Component() {
return <div>{notAhookTypedAsHook()}</div>;
}
Loading

0 comments on commit d724ba9

Please sign in to comment.