Skip to content

[react-compiler] Skip hook validation for method calls on untyped module imports #576

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
everettbu wants to merge 1 commit into from
Closed

[react-compiler] Skip hook validation for method calls on untyped module imports #576

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 60 additions & 2 deletions compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateHooksUsage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,15 @@ export function validateHooksUsage(
}
}

/**
* Track identifiers that were loaded from module imports (ImportDefault,
* ImportNamespace, ImportSpecifier) but whose type is unresolved. These
* represent third-party module imports where the type system doesn't know
* the actual types. Used to avoid false hook detection on method calls
* like `amCharts.useTheme()`. See #32109.
*/
const untypedImportIdentifiers = new Set<IdentifierId>();

const valueKinds = new Map<IdentifierId, Kind>();
function getKindForPlace(place: Place): Kind {
const knownKind = valueKinds.get(place.identifier.id);
Expand Down Expand Up @@ -232,6 +241,20 @@ export function validateHooksUsage(
} else {
setKind(instr.lvalue, Kind.Global);
}
/*
* Track identifiers from module imports with unresolved types,
* used to skip heuristic hook detection on method calls. See #32109.
*/
const binding = instr.value.binding;
if (
(binding.kind === 'ImportDefault' ||
binding.kind === 'ImportNamespace' ||
binding.kind === 'ImportSpecifier') &&
instr.lvalue.identifier.type.kind !== 'Object' &&
instr.lvalue.identifier.type.kind !== 'Function'
) {
untypedImportIdentifiers.add(instr.lvalue.identifier.id);
}
break;
}
case 'LoadContext':
Expand Down Expand Up @@ -339,11 +362,33 @@ export function validateHooksUsage(
}
case 'MethodCall': {
const calleeKind = getKindForPlace(instr.value.property);
/**
* For method calls where the receiver is a non-React module import
* with an unresolved type, hook-named properties were typed
* heuristically based on naming alone. We skip hook validation for
* these cases to avoid false positives with third-party APIs like
* `amCharts.useTheme()` that happen to use "use*" naming but are
* not React hooks. See https://github.com/facebook/react/issues/32109
*/
const receiverType = instr.value.receiver.identifier.type;
const isReceiverUntyped =
receiverType.kind !== 'Object' &&
receiverType.kind !== 'Function';
const isReceiverImport =
untypedImportIdentifiers.has(
instr.value.receiver.identifier.id,
);
const skipHookCheck = isReceiverUntyped && isReceiverImport;
Comment on lines +373 to +381

@greptile-apps greptile-apps Bot Feb 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant isReceiverUntyped check

The isReceiverUntyped check on line 374-376 is redundant with isReceiverImport on line 377-380. The untypedImportIdentifiers set (line 178) only adds identifiers that already passed the same untyped check (lines 253-254: type.kind !== 'Object' && type.kind !== 'Function'). So whenever isReceiverImport is true, isReceiverUntyped is guaranteed to also be true.

This isn't a bug — the redundancy adds a defensive check in case the identifier's type changes between LoadGlobal and MethodCall processing — but it may be worth a brief comment noting this is intentional for clarity.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateHooksUsage.ts
Line: 373-381

Comment:
**Redundant `isReceiverUntyped` check**

The `isReceiverUntyped` check on line 374-376 is redundant with `isReceiverImport` on line 377-380. The `untypedImportIdentifiers` set (line 178) only adds identifiers that already passed the same untyped check (lines 253-254: `type.kind !== 'Object' && type.kind !== 'Function'`). So whenever `isReceiverImport` is `true`, `isReceiverUntyped` is guaranteed to also be `true`.

This isn't a bug — the redundancy adds a defensive check in case the identifier's type changes between `LoadGlobal` and `MethodCall` processing — but it may be worth a brief comment noting this is intentional for clarity.

How can I resolve this? If you propose a fix, please make it concise.

const isHookCallee =
calleeKind === Kind.KnownHook || calleeKind === Kind.PotentialHook;
!skipHookCheck &&
(calleeKind === Kind.KnownHook ||
calleeKind === Kind.PotentialHook);
if (isHookCallee && !unconditionalBlocks.has(block.id)) {
recordConditionalHookError(instr.value.property);
} else if (calleeKind === Kind.PotentialHook) {
} else if (
!skipHookCheck &&
calleeKind === Kind.PotentialHook
) {
recordDynamicHookUsageError(instr.value.property);
}
/*
Expand Down Expand Up @@ -444,6 +489,19 @@ function visitFunctionExpression(errors: CompilerError, fn: HIRFunction): void {
instr.value.kind === 'CallExpression'
? instr.value.callee
: instr.value.property;
/**
* For MethodCall on untyped Global receivers, skip hook validation
* as the property was typed heuristically. See #32109.
*/
if (instr.value.kind === 'MethodCall') {
const receiverType = instr.value.receiver.identifier.type;
if (
receiverType.kind !== 'Object' &&
receiverType.kind !== 'Function'
) {
break;
}
Comment on lines +493 to +503

@greptile-apps greptile-apps Bot Feb 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Broader skip logic than main function

In validateHooksUsage, the skip requires both isReceiverUntyped && isReceiverImport, but here in visitFunctionExpression only isReceiverUntyped is checked (no import tracking). This means method calls on any untyped receiver (not just imports) will skip hook validation inside nested functions — e.g., localObj.useHook() inside a callback would also be skipped if localObj has an unresolved type.

This is likely acceptable since getHookKind relies on the same heuristic type system (getPropertyType assigns DefaultNonmutatingHook for use*-named properties on untyped receivers), so the reasoning is consistent. However, the comment on line 493 says "untyped Global receivers" which is misleading — the check doesn't verify the receiver is a global, just that its type is unresolved.

Consider updating the comment to reflect the actual behavior:

Suggested change
* For MethodCall on untyped Global receivers, skip hook validation
* as the property was typed heuristically. See #32109.
*/
if (instr.value.kind === 'MethodCall') {
const receiverType = instr.value.receiver.identifier.type;
if (
receiverType.kind !== 'Object' &&
receiverType.kind !== 'Function'
) {
break;
}
/**
* For MethodCall on receivers with unresolved types, skip hook
* validation as the property was typed heuristically. See #32109.
*/

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Prompt To Fix With AI 
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateHooksUsage.ts
Line: 493-503

Comment:
**Broader skip logic than main function**

In `validateHooksUsage`, the skip requires both `isReceiverUntyped && isReceiverImport`, but here in `visitFunctionExpression` only `isReceiverUntyped` is checked (no import tracking). This means method calls on *any* untyped receiver (not just imports) will skip hook validation inside nested functions — e.g., `localObj.useHook()` inside a callback would also be skipped if `localObj` has an unresolved type.

This is likely acceptable since `getHookKind` relies on the same heuristic type system (`getPropertyType` assigns `DefaultNonmutatingHook` for `use*`-named properties on untyped receivers), so the reasoning is consistent. However, the comment on line 493 says "untyped Global receivers" which is misleading — the check doesn't verify the receiver is a global, just that its type is unresolved.

Consider updating the comment to reflect the actual behavior:

```suggestion
          /**
           * For MethodCall on receivers with unresolved types, skip hook
           * validation as the property was typed heuristically. See #32109.
           */
```

<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>

How can I resolve this? If you propose a fix, please make it concise.

}
const hookKind = getHookKind(fn.env, callee.identifier);
if (hookKind != null) {
errors.pushErrorDetail(
Expand Down
Loading