From 23b6f20e68b78f9f03709bd84449c0416de1e856 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 5 Sep 2024 14:32:43 -0700 Subject: [PATCH 1/7] [compiler] Validate type configs for hooks/non-hooks [ghstack-poisoned] --- .../src/HIR/Environment.ts | 2 + .../src/HIR/Globals.ts | 68 +++++++++++++++++-- .../src/HIR/TypeSchema.ts | 4 +- 3 files changed, 69 insertions(+), 5 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 index 2b007f9659b99..888708e05d7f9 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -734,9 +734,11 @@ export class Environment { } const moduleConfig = parsedModuleConfig.data; moduleType = installTypeConfig( + moduleName, this.#globals, this.#shapes, moduleConfig, + moduleName, ); } else { moduleType = 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 index 2812394300ad5..d4c4227fad3ef 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts @@ -5,10 +5,11 @@ * LICENSE file in the root directory of this source tree. */ -import {Effect, ValueKind, ValueReason} from './HIR'; +import {Effect, GeneratedSource, ValueKind, ValueReason} from './HIR'; import { BUILTIN_SHAPES, BuiltInArrayId, + BuiltInJsxId, BuiltInMixedReadonlyId, BuiltInUseActionStateId, BuiltInUseContextHookId, @@ -28,6 +29,8 @@ 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. @@ -532,6 +535,50 @@ 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, @@ -554,6 +601,9 @@ export function installTypeConfig( case 'Any': { return {kind: 'Poly'}; } + case 'JSX': { + return {kind: 'Object', shapeId: BuiltInJsxId}; + } default: { assertExhaustive( typeConfig.name, @@ -567,7 +617,12 @@ export function installTypeConfig( positionalParams: typeConfig.positionalParams, restParam: typeConfig.restParam, calleeEffect: typeConfig.calleeEffect, - returnType: installTypeConfig(globals, shapes, typeConfig.returnType), + returnType: convertTypeConfig( + moduleName, + globals, + shapes, + typeConfig.returnType, + ), returnValueKind: typeConfig.returnValueKind, noAlias: typeConfig.noAlias === true, mutableOnlyIfOperandsAreMutable: @@ -580,7 +635,12 @@ export function installTypeConfig( positionalParams: typeConfig.positionalParams ?? [], restParam: typeConfig.restParam ?? Effect.Freeze, calleeEffect: Effect.Read, - returnType: installTypeConfig(globals, shapes, typeConfig.returnType), + returnType: convertTypeConfig( + moduleName, + globals, + shapes, + typeConfig.returnType, + ), returnValueKind: typeConfig.returnValueKind ?? ValueKind.Frozen, noAlias: typeConfig.noAlias === true, }); @@ -591,7 +651,7 @@ export function installTypeConfig( null, Object.entries(typeConfig.properties ?? {}).map(([key, value]) => [ key, - installTypeConfig(globals, shapes, value), + installTypeConfig(moduleName, globals, shapes, value, key), ]), ); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/TypeSchema.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/TypeSchema.ts index 362328db72155..c187389239dc6 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/TypeSchema.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/TypeSchema.ts @@ -74,13 +74,15 @@ export type BuiltInTypeConfig = | 'Ref' | 'Array' | 'Primitive' - | 'MixedReadonly'; + | 'MixedReadonly' + | 'JSX'; export const BuiltInTypeSchema: z.ZodType = z.union([ z.literal('Any'), z.literal('Ref'), z.literal('Array'), z.literal('Primitive'), z.literal('MixedReadonly'), + z.literal('JSX'), ]); export type TypeReferenceConfig = { From 6e4115a6ffa6a05499b217a304c1a766db79e92d Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 5 Sep 2024 14:36:21 -0700 Subject: [PATCH 2/7] 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] From 7a6efb2eed3386389079526dad9408ec0d991be5 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 5 Sep 2024 14:36:54 -0700 Subject: [PATCH 3/7] 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] --- .../babel-plugin-react-compiler/src/HIR/TypeSchema.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/TypeSchema.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/TypeSchema.ts index c187389239dc6..362328db72155 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/TypeSchema.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/TypeSchema.ts @@ -74,15 +74,13 @@ export type BuiltInTypeConfig = | 'Ref' | 'Array' | 'Primitive' - | 'MixedReadonly' - | 'JSX'; + | 'MixedReadonly'; export const BuiltInTypeSchema: z.ZodType = z.union([ z.literal('Any'), z.literal('Ref'), z.literal('Array'), z.literal('Primitive'), z.literal('MixedReadonly'), - z.literal('JSX'), ]); export type TypeReferenceConfig = { From 0a8bea2b12276e3b4da8deaeecf2f3ebba42a5b3 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 5 Sep 2024 14:46:17 -0700 Subject: [PATCH 4/7] 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] --- .../src/HIR/Environment.ts | 36 +++++++--- .../src/HIR/Globals.ts | 68 ++----------------- 2 files changed, 32 insertions(+), 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 index 888708e05d7f9..b747d34fc34a5 100644 --- 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,6 +788,8 @@ 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( @@ -796,6 +797,14 @@ export class Environment { 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 index d4c4227fad3ef..2812394300ad5 100644 --- 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), ]), ); } From c37ac41e82fcc8809c1edeea382370efa9f2896e Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 5 Sep 2024 15:10:18 -0700 Subject: [PATCH 5/7] 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] --- .../src/HIR/Environment.ts | 21 ++- ...ider-hook-name-not-typed-as-hook.expect.md | 25 ++++ ...pe-provider-hook-name-not-typed-as-hook.js | 5 + ...hooklike-module-default-not-hook.expect.md | 25 ++++ ...ovider-hooklike-module-default-not-hook.js | 5 + ...vider-nonhook-name-typed-as-hook.expect.md | 25 ++++ ...ype-provider-nonhook-name-typed-as-hook.js | 5 + .../sprout/shared-runtime-type-provider.ts | 128 +++++++++++------- 8 files changed, 180 insertions(+), 59 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-hook-name-not-typed-as-hook.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-hook-name-not-typed-as-hook.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-hooklike-module-default-not-hook.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-hooklike-module-default-not-hook.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-nonhook-name-typed-as-hook.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-nonhook-name-typed-as-hook.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts index b747d34fc34a5..af692a3837cd9 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -788,8 +788,6 @@ 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( @@ -797,11 +795,16 @@ 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 '${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/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-hook-name-not-typed-as-hook.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-hook-name-not-typed-as-hook.expect.md new file mode 100644 index 0000000000000..2765a0fc7cf11 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/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 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-hook-name-not-typed-as-hook.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-hook-name-not-typed-as-hook.js new file mode 100644 index 0000000000000..d4ae58c5d9501 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__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/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-hooklike-module-default-not-hook.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-hooklike-module-default-not-hook.expect.md new file mode 100644 index 0000000000000..99944b5813387 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/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
{foo()}
; +} + +``` + + +## Error + +``` + 2 | + 3 | function Component() { +> 4 | return
{foo()}
; + | ^^^ 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 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-hooklike-module-default-not-hook.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-hooklike-module-default-not-hook.js new file mode 100644 index 0000000000000..75d040fde079f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-hooklike-module-default-not-hook.js @@ -0,0 +1,5 @@ +import foo from 'useDefaultExportNotTypedAsHook'; + +function Component() { + return
{foo()}
; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-nonhook-name-typed-as-hook.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-nonhook-name-typed-as-hook.expect.md new file mode 100644 index 0000000000000..7eac1b30a5a75 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/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
{notAhookTypedAsHook()}
; +} + +``` + + +## Error + +``` + 2 | + 3 | function Component() { +> 4 | return
{notAhookTypedAsHook()}
; + | ^^^^^^^^^^^^^^^^^^^ 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 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-nonhook-name-typed-as-hook.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-nonhook-name-typed-as-hook.js new file mode 100644 index 0000000000000..3763bed79c6bb --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/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
{notAhookTypedAsHook()}
; +} diff --git a/compiler/packages/snap/src/sprout/shared-runtime-type-provider.ts b/compiler/packages/snap/src/sprout/shared-runtime-type-provider.ts index 10aa87c32b39e..22226dfc80ac7 100644 --- 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; }; } From c26ca805a4ff84595660df93b7c7d3c548671193 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 5 Sep 2024 15:12:05 -0700 Subject: [PATCH 6/7] 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] --- .../src/HIR/Environment.ts | 16 ++++++++++------ .../src/sprout/shared-runtime-type-provider.ts | 8 ++++++++ 2 files changed, 18 insertions(+), 6 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 index af692a3837cd9..643e1f42dc82c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -795,10 +795,12 @@ 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. + /* + * 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) { @@ -846,8 +848,10 @@ 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 + /* + * 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) { diff --git a/compiler/packages/snap/src/sprout/shared-runtime-type-provider.ts b/compiler/packages/snap/src/sprout/shared-runtime-type-provider.ts index 22226dfc80ac7..4c1d77f2f8986 100644 --- a/compiler/packages/snap/src/sprout/shared-runtime-type-provider.ts +++ b/compiler/packages/snap/src/sprout/shared-runtime-type-provider.ts @@ -72,6 +72,10 @@ export function makeSharedRuntimeTypeProvider({ }, }; } 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: { @@ -86,6 +90,10 @@ export function makeSharedRuntimeTypeProvider({ }, }; } 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: { From 9df57a0165b0d8c51529232dc190ff5a47d0144c Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 5 Sep 2024 15:24:43 -0700 Subject: [PATCH 7/7] 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] --- .../src/HIR/Environment.ts | 2 + .../src/HIR/Globals.ts | 49 ++++++++++++++++--- ...name-not-typed-as-hook-namespace.expect.md | 25 ++++++++++ ...r-hook-name-not-typed-as-hook-namespace.js | 5 ++ ...ider-hook-name-not-typed-as-hook.expect.md | 2 +- ...vider-nonhook-name-typed-as-hook.expect.md | 2 +- 6 files changed, 77 insertions(+), 8 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-hook-name-not-typed-as-hook-namespace.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-hook-name-not-typed-as-hook-namespace.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts index 643e1f42dc82c..8bec4c4b16a35 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -738,6 +738,8 @@ export class Environment { this.#globals, this.#shapes, moduleConfig, + moduleName, + loc, ); } else { moduleType = 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 index 2812394300ad5..c923882900cc2 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts @@ -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. @@ -535,6 +537,8 @@ export function installTypeConfig( globals: GlobalRegistry, shapes: ShapeRegistry, typeConfig: TypeConfig, + moduleName: string, + loc: SourceLocation, ): Global { switch (typeConfig.kind) { case 'type': { @@ -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: @@ -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, }); @@ -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: { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-hook-name-not-typed-as-hook-namespace.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-hook-name-not-typed-as-hook-namespace.expect.md new file mode 100644 index 0000000000000..5f352281b3798 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-hook-name-not-typed-as-hook-namespace.expect.md @@ -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 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-hook-name-not-typed-as-hook-namespace.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-hook-name-not-typed-as-hook-namespace.js new file mode 100644 index 0000000000000..3a2f646569e10 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-hook-name-not-typed-as-hook-namespace.js @@ -0,0 +1,5 @@ +import ReactCompilerTest from 'ReactCompilerTest'; + +function Component() { + return ReactCompilerTest.useHookNotTypedAsHook(); +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-hook-name-not-typed-as-hook.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-hook-name-not-typed-as-hook.expect.md index 2765a0fc7cf11..9d863ba0cbc7a 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-hook-name-not-typed-as-hook.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-hook-name-not-typed-as-hook.expect.md @@ -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 | ``` diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-nonhook-name-typed-as-hook.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-nonhook-name-typed-as-hook.expect.md index 7eac1b30a5a75..ff1f4373b423c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-nonhook-name-typed-as-hook.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-nonhook-name-typed-as-hook.expect.md @@ -17,7 +17,7 @@ function Component() { 2 | 3 | function Component() { > 4 | return
{notAhookTypedAsHook()}
; - | ^^^^^^^^^^^^^^^^^^^ 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 | ```