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 · c37ac41 · c37ac41
1 parent 0a8bea2
commit c37ac41
Show file tree

Hide file tree

Showing 8 changed files with 180 additions and 59 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
@@ -788,20 +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) {
+              // 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 '${binding.module}.${binding.imported}' to ${expectHook ? 'be a hook' : 'not be a hook'} based on its name`,
+                  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,
                 });
               }
@@ -817,7 +820,9 @@ export class Environment {
            * `import {useHook as foo} ...`
            * `import {foo as useHook} ...`
            */
-          return expectHook ? this.#getCustomHookType() : null;
+          return isHookName(binding.imported) || isHookName(binding.name)
+            ? this.#getCustomHookType()
+            : null;
         }
       }
       case 'ImportDefault':
@@ -829,7 +834,6 @@ 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;
@@ -842,18 +846,21 @@ export class Environment {
               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 '${binding.module}' (as '${binding.name}') to ${expectHook ? 'be a hook' : 'not be a hook'} based on its name`,
+                  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 expectHook ? this.#getCustomHookType() : null;
+          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,84 @@ 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') {
+      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') {
+      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;
   };
 }