[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: fc389f36fb1ecdaca6fbe7217fde5ca05d6f4792 Pull Request resolved: #30888
facebook · Sep 5, 2024 · dfad575 · dfad575
1 parent 4c58fce
commit dfad575
Show file tree

Hide file tree

Showing 8 changed files with 209 additions and 54 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,
@@ -794,6 +795,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;
             }
           }
@@ -822,13 +838,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;

diff --git a/...ures/compiler/error.invalid-type-provider-hook-name-not-typed-as-hook.expect.md b/...ures/compiler/error.invalid-type-provider-hook-name-not-typed-as-hook.expect.md
@@ -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 `import {useHookNotTypedAsHook} from 'ReactCompilerTest'` to be a hook based on the exported name (4:4)
+  5 | }
+  6 |
+```
+          
+      
diff --git a/...rc/__tests__/fixtures/compiler/error.invalid-type-provider-hook-name-not-typed-as-hook.js b/...rc/__tests__/fixtures/compiler/error.invalid-type-provider-hook-name-not-typed-as-hook.js
@@ -0,0 +1,5 @@
+import {useHookNotTypedAsHook} from 'ReactCompilerTest';
+
+function Component() {
+  return useHookNotTypedAsHook();
+}
diff --git a/...compiler/error.invalid-type-provider-hooklike-module-default-not-hook.expect.md b/...compiler/error.invalid-type-provider-hooklike-module-default-not-hook.expect.md
@@ -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 |
+```
+          
+      
diff --git a/...tests__/fixtures/compiler/error.invalid-type-provider-hooklike-module-default-not-hook.js b/...tests__/fixtures/compiler/error.invalid-type-provider-hooklike-module-default-not-hook.js
@@ -0,0 +1,5 @@
+import foo from 'useDefaultExportNotTypedAsHook';
+
+function Component() {
+  return <div>{foo()}</div>;
+}
diff --git a/...tures/compiler/error.invalid-type-provider-nonhook-name-typed-as-hook.expect.md b/...tures/compiler/error.invalid-type-provider-nonhook-name-typed-as-hook.expect.md
@@ -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 `import {notAhookTypedAsHook} from 'ReactCompilerTest'` not to be a hook based on the exported name (4:4)
+  5 | }
+  6 |
+```
+          
+      
diff --git a/...src/__tests__/fixtures/compiler/error.invalid-type-provider-nonhook-name-typed-as-hook.js b/...src/__tests__/fixtures/compiler/error.invalid-type-provider-nonhook-name-typed-as-hook.js
@@ -0,0 +1,5 @@
+import {notAhookTypedAsHook} from 'ReactCompilerTest';
+
+function Component() {
+  return <div>{notAhookTypedAsHook()}</div>;
+}
diff --git a/compiler/packages/snap/src/sprout/shared-runtime-type-provider.ts b/compiler/packages/snap/src/sprout/shared-runtime-type-provider.ts
@@ -18,60 +18,92 @@ export function makeSharedRuntimeTypeProvider({
   return function sharedRuntimeTypeProvider(
     moduleName: string,
   ): TypeConfig | null {
-    if (moduleName !== 'shared-runtime') {
-      return null;
-    }
-    return {
-      kind: 'object',
-      properties: {
-        default: {
-          kind: 'function',
-          calleeEffect: EffectEnum.Read,
-          positionalParams: [],
-          restParam: EffectEnum.Read,
-          returnType: {kind: 'type', name: 'Primitive'},
-          returnValueKind: ValueKindEnum.Primitive,
-        },
-        graphql: {
-          kind: 'function',
-          calleeEffect: EffectEnum.Read,
-          positionalParams: [],
-          restParam: EffectEnum.Read,
-          returnType: {kind: 'type', name: 'Primitive'},
-          returnValueKind: ValueKindEnum.Primitive,
-        },
-        typedArrayPush: {
-          kind: 'function',
-          calleeEffect: EffectEnum.Read,
-          positionalParams: [EffectEnum.Store, EffectEnum.Capture],
-          restParam: EffectEnum.Capture,
-          returnType: {kind: 'type', name: 'Primitive'},
-          returnValueKind: ValueKindEnum.Primitive,
+    if (moduleName === 'shared-runtime') {
+      return {
+        kind: 'object',
+        properties: {
+          default: {
+            kind: 'function',
+            calleeEffect: EffectEnum.Read,
+            positionalParams: [],
+            restParam: EffectEnum.Read,
+            returnType: {kind: 'type', name: 'Primitive'},
+            returnValueKind: ValueKindEnum.Primitive,
+          },
+          graphql: {
+            kind: 'function',
+            calleeEffect: EffectEnum.Read,
+            positionalParams: [],
+            restParam: EffectEnum.Read,
+            returnType: {kind: 'type', name: 'Primitive'},
+            returnValueKind: ValueKindEnum.Primitive,
+          },
+          typedArrayPush: {
+            kind: 'function',
+            calleeEffect: EffectEnum.Read,
+            positionalParams: [EffectEnum.Store, EffectEnum.Capture],
+            restParam: EffectEnum.Capture,
+            returnType: {kind: 'type', name: 'Primitive'},
+            returnValueKind: ValueKindEnum.Primitive,
+          },
+          typedLog: {
+            kind: 'function',
+            calleeEffect: EffectEnum.Read,
+            positionalParams: [],
+            restParam: EffectEnum.Read,
+            returnType: {kind: 'type', name: 'Primitive'},
+            returnValueKind: ValueKindEnum.Primitive,
+          },
+          useFreeze: {
+            kind: 'hook',
+            returnType: {kind: 'type', name: 'Any'},
+          },
+          useFragment: {
+            kind: 'hook',
+            returnType: {kind: 'type', name: 'MixedReadonly'},
+            noAlias: true,
+          },
+          useNoAlias: {
+            kind: 'hook',
+            returnType: {kind: 'type', name: 'Any'},
+            returnValueKind: ValueKindEnum.Mutable,
+            noAlias: true,
+          },
         },
-        typedLog: {
-          kind: 'function',
-          calleeEffect: EffectEnum.Read,
-          positionalParams: [],
-          restParam: EffectEnum.Read,
-          returnType: {kind: 'type', name: 'Primitive'},
-          returnValueKind: ValueKindEnum.Primitive,
+      };
+    } else if (moduleName === 'ReactCompilerTest') {
+      /**
+       * Fake module used for testing validation that type providers return hook
+       * types for hook names and non-hook types for non-hook names
+       */
+      return {
+        kind: 'object',
+        properties: {
+          useHookNotTypedAsHook: {
+            kind: 'type',
+            name: 'Any',
+          },
+          notAhookTypedAsHook: {
+            kind: 'hook',
+            returnType: {kind: 'type', name: 'Any'},
+          },
         },
-        useFreeze: {
-          kind: 'hook',
-          returnType: {kind: 'type', name: 'Any'},
+      };
+    } else if (moduleName === 'useDefaultExportNotTypedAsHook') {
+      /**
+       * Fake module used for testing validation that type providers return hook
+       * types for hook names and non-hook types for non-hook names
+       */
+      return {
+        kind: 'object',
+        properties: {
+          default: {
+            kind: 'type',
+            name: 'Any',
+          },
         },
-        useFragment: {
-          kind: 'hook',
-          returnType: {kind: 'type', name: 'MixedReadonly'},
-          noAlias: true,
-        },
-        useNoAlias: {
-          kind: 'hook',
-          returnType: {kind: 'type', name: 'Any'},
-          returnValueKind: ValueKindEnum.Mutable,
-          noAlias: true,
-        },
-      },
-    };
+      };
+    }
+    return null;
   };
 }