Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[compiler] Validate type configs for hooks/non-hooks #30888

Merged
merged 7 commits into from
Sep 10, 2024
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) {
/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe factor this out into a helper?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm it's kind of tedious to get customize the error message if we extract to a helper, i'm gonna leave it

* 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can enforce that the isHookName of the exported name and the local alias are the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that makes sense, but we should do that everywhere regardless of whether we have a type provider for the module or not. Let's explore that in a follow-up.

* 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