Skip to content

Commit

Permalink
Update on "[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-poisoned]
  • Loading branch information
josephsavona committed Sep 5, 2024
1 parent c26ca80 commit 9df57a0
Show file tree
Hide file tree
Showing 6 changed files with 77 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -738,6 +738,8 @@ export class Environment {
this.#globals,
this.#shapes,
moduleConfig,
moduleName,
loc,
);
} else {
moduleType = 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
Expand Up @@ -17,7 +17,7 @@ function Component() {
2 |
3 | function Component() {
> 4 | return useHookNotTypedAsHook();
| ^^^^^^^^^^^^^^^^^^^^^ InvalidConfig: Invalid type configuration for module. Expected type for `import {useHookNotTypedAsHook} from 'ReactCompilerTest'` to be a hook based on the exported name (4:4)
| ^^^^^^^^^^^^^^^^^^^^^ 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 |
```
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ function Component() {
2 |
3 | function Component() {
> 4 | return <div>{notAhookTypedAsHook()}</div>;
| ^^^^^^^^^^^^^^^^^^^ InvalidConfig: Invalid type configuration for module. Expected type for `import {notAhookTypedAsHook} from 'ReactCompilerTest'` not to be a hook based on the exported name (4:4)
| ^^^^^^^^^^^^^^^^^^^ 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 |
```
Expand Down

0 comments on commit 9df57a0

Please sign in to comment.