Update on "[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-poisoned]
facebook · Sep 5, 2024 · 0a8bea2 · 0a8bea2
1 parent 7a6efb2
commit 0a8bea2
Show file tree

Hide file tree

Showing 2 changed files with 32 additions and 72 deletions.
diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts
@@ -33,6 +33,7 @@ import {
   Type,
   ValidatedIdentifier,
   ValueKind,
+  getHookKindForType,
   makeBlockId,
   makeIdentifierId,
   makeIdentifierName,
@@ -734,11 +735,9 @@ export class Environment {
         }
         const moduleConfig = parsedModuleConfig.data;
         moduleType = installTypeConfig(
-          moduleName,
           this.#globals,
           this.#shapes,
           moduleConfig,
-          moduleName,
         );
       } else {
         moduleType = null;
@@ -789,13 +788,23 @@ export class Environment {
             (isHookName(binding.imported) ? this.#getCustomHookType() : null)
           );
         } else {
+          const expectHook =
+            isHookName(binding.imported) || isHookName(binding.name);
           const moduleType = this.#resolveModuleType(binding.module, loc);
           if (moduleType !== null) {
             const importedType = this.getPropertyType(
               moduleType,
               binding.imported,
             );
             if (importedType != null) {
+              const isHook = getHookKindForType(this, importedType) != null;
+              if (expectHook !== isHook) {
+                CompilerError.throwInvalidConfig({
+                  reason: `Invalid type configuration for module`,
+                  description: `Expected type for '${binding.module}.${binding.imported}' to ${expectHook ? 'be a hook' : 'not be a hook'} based on its name`,
+                  loc,
+                });
+              }
               return importedType;
             }
           }
@@ -808,9 +817,7 @@ export class Environment {
            * `import {useHook as foo} ...`
            * `import {foo as useHook} ...`
            */
-          return isHookName(binding.imported) || isHookName(binding.name)
-            ? this.#getCustomHookType()
-            : null;
+          return expectHook ? this.#getCustomHookType() : null;
         }
       }
       case 'ImportDefault':
@@ -822,18 +829,31 @@ export class Environment {
             (isHookName(binding.name) ? this.#getCustomHookType() : null)
           );
         } else {
+          const expectHook = isHookName(binding.name);
           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) {
+              const isHook = getHookKindForType(this, importedType) != null;
+              if (expectHook !== isHook) {
+                CompilerError.throwInvalidConfig({
+                  reason: `Invalid type configuration for module`,
+                  description: `Expected type for '${binding.module}' (as '${binding.name}') to ${expectHook ? 'be a hook' : 'not be a hook'} based on its name`,
+                  loc,
+                });
+              }
+              return importedType;
             }
           }
-          return isHookName(binding.name) ? this.#getCustomHookType() : null;
+          return expectHook ? this.#getCustomHookType() : null;
         }
       }
     }

diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts
@@ -5,11 +5,10 @@
  * LICENSE file in the root directory of this source tree.
  */
 
-import {Effect, GeneratedSource, ValueKind, ValueReason} from './HIR';
+import {Effect, ValueKind, ValueReason} from './HIR';
 import {
   BUILTIN_SHAPES,
   BuiltInArrayId,
-  BuiltInJsxId,
   BuiltInMixedReadonlyId,
   BuiltInUseActionStateId,
   BuiltInUseContextHookId,
@@ -29,8 +28,6 @@ import {
 import {BuiltInType, PolyType} from './Types';
 import {TypeConfig} from './TypeSchema';
 import {assertExhaustive} from '../Utils/utils';
-import {isHookName} from './Environment';
-import {CompilerError} from '..';
 
 /*
  * This file exports types and defaults for JavaScript global objects.
@@ -535,50 +532,6 @@ DEFAULT_GLOBALS.set(
 );
 
 export function installTypeConfig(
-  moduleName: string,
-  globals: GlobalRegistry,
-  shapes: ShapeRegistry,
-  typeConfig: TypeConfig,
-  moduleOrPropertyName: string | null,
-): Global {
-  const type = convertTypeConfig(moduleName, globals, shapes, typeConfig);
-  if (moduleOrPropertyName !== null) {
-    if (isHookName(moduleOrPropertyName)) {
-      // Named like a hook => must be typed as a hook
-      if (type.kind !== 'Function' || type.shapeId == null) {
-        CompilerError.throwInvalidConfig({
-          reason: `Invalid moduleTypeProvider result for module '${moduleName}', expected type for '${moduleOrPropertyName}' to be a hook based on its name, but the type was not a hook`,
-          loc: GeneratedSource,
-        });
-      }
-      const functionType = shapes.get(type.shapeId);
-      if (functionType == null || functionType.functionType?.hookKind == null) {
-        CompilerError.throwInvalidConfig({
-          reason: `Invalid moduleTypeProvider result for module '${moduleName}', expected type for '${moduleOrPropertyName}' to be a hook based on its name, but the type was not a hook`,
-          loc: GeneratedSource,
-        });
-      }
-    } else {
-      // Not named like a hook => must not be a hook
-      if (type.kind === 'Function' && type.shapeId != null) {
-        const functionType = shapes.get(type.shapeId);
-        if (
-          functionType != null &&
-          functionType.functionType?.hookKind != null
-        ) {
-          CompilerError.throwInvalidConfig({
-            reason: `Invalid moduleTypeProvider result for module '${moduleName}', expected type for '${moduleOrPropertyName}' not to be a hook, but it was typed as a hook`,
-            loc: GeneratedSource,
-          });
-        }
-      }
-    }
-  }
-  return type;
-}
-
-function convertTypeConfig(
-  moduleName: string,
   globals: GlobalRegistry,
   shapes: ShapeRegistry,
   typeConfig: TypeConfig,
@@ -601,9 +554,6 @@ function convertTypeConfig(
         case 'Any': {
           return {kind: 'Poly'};
         }
-        case 'JSX': {
-          return {kind: 'Object', shapeId: BuiltInJsxId};
-        }
         default: {
           assertExhaustive(
             typeConfig.name,
@@ -617,12 +567,7 @@ function convertTypeConfig(
         positionalParams: typeConfig.positionalParams,
         restParam: typeConfig.restParam,
         calleeEffect: typeConfig.calleeEffect,
-        returnType: convertTypeConfig(
-          moduleName,
-          globals,
-          shapes,
-          typeConfig.returnType,
-        ),
+        returnType: installTypeConfig(globals, shapes, typeConfig.returnType),
         returnValueKind: typeConfig.returnValueKind,
         noAlias: typeConfig.noAlias === true,
         mutableOnlyIfOperandsAreMutable:
@@ -635,12 +580,7 @@ function convertTypeConfig(
         positionalParams: typeConfig.positionalParams ?? [],
         restParam: typeConfig.restParam ?? Effect.Freeze,
         calleeEffect: Effect.Read,
-        returnType: convertTypeConfig(
-          moduleName,
-          globals,
-          shapes,
-          typeConfig.returnType,
-        ),
+        returnType: installTypeConfig(globals, shapes, typeConfig.returnType),
         returnValueKind: typeConfig.returnValueKind ?? ValueKind.Frozen,
         noAlias: typeConfig.noAlias === true,
       });
@@ -651,7 +591,7 @@ function convertTypeConfig(
         null,
         Object.entries(typeConfig.properties ?? {}).map(([key, value]) => [
           key,
-          installTypeConfig(moduleName, globals, shapes, value, key),
+          installTypeConfig(globals, shapes, value),
         ]),
       );
     }