Skip to content

Add opt-in flag for method hook defining-class fullnames#21650

Open
itxsamad1 wants to merge 3 commits into
python:masterfrom
itxsamad1:fix/plugin-hook-docstring
Open

Add opt-in flag for method hook defining-class fullnames#21650
itxsamad1 wants to merge 3 commits into
python:masterfrom
itxsamad1:fix/plugin-hook-docstring

Conversation

@itxsamad1

@itxsamad1 itxsamad1 commented Jun 26, 2026

Copy link
Copy Markdown

Summary

This adds an opt-in flag for the defining-class behavior discussed in #19181.

By default, plugin method hooks still receive the call-site class name (e.g. Derived.method), so existing plugins keep working. If you set use_method_hook_defining_class = True in config (or pass --use-method-hook-defining-class), hooks get the class where the method was defined instead (Base.method).

Fixes #19181

Test plan

  • Ran the plugin hook tests locally, including the new defining-class regression test with the flag enabled

@github-actions

This comment has been minimized.

@A5rocks

A5rocks commented Jun 27, 2026

Copy link
Copy Markdown
Collaborator

I think we should fix this instead of documenting errata, and that seems to be consensus in the issue too.

method_fullname() now resolves the class where a method was defined via get_containing_type_info(), matching the documented plugin hook semantics (Base.method rather than Derived.method for inherited calls).

Fixes python#19181
@itxsamad1 itxsamad1 changed the title docs: clarify get_method_signature_hook fullname uses call-site class Fix method plugin hooks to use defining class fullname Jun 28, 2026
@itxsamad1

Copy link
Copy Markdown
Author

Updated per @A5rocks feedback — this is now a code fix, not a docstring errata update.

What changed: ExpressionChecker.method_fullname() now uses TypeInfo.get_containing_type_info() to pass the defining class name (e.g. Base.method) to get_method_signature_hook / get_method_hook, instead of the call-site class (Derived.method).

Tests: Added testMethodSignatureHookUsesDefiningClass with a plugin that hooks only on __main__.Base.method and verifies Derived().method() triggers it. Updated the arg_names test plugin accordingly for inherited methods.

Note: plugins that registered hooks on derived-class fullnames only (without also hooking the base class) may need updating — this aligns with the original documented intent per the issue discussion.

@itxsamad1

Copy link
Copy Markdown
Author

Updated per @A5rocks feedback — this is now a code fix, not docs-only.

method_fullname() uses TypeInfo.get_containing_type_info() so inherited calls like Derived().method() invoke plugin hooks with Base.method (the defining class), matching the documented/intended semantics discussed in #19181.

Added regression test estMethodSignatureHookUsesDefiningClass and ran 276 plugin-related tests locally.

@itxsamad1 itxsamad1 changed the title Fix method plugin hooks to use defining class fullname Fix method plugin hooks to use defining class fullname (fixes #19181) Jun 28, 2026
@A5rocks

A5rocks commented Jun 28, 2026

Copy link
Copy Markdown
Collaborator

Sorry but the issue also describes a backward compatibility approach to take. Anyways if this PR is purely LLM generated then we will close it.

@github-actions

This comment has been minimized.

Keep call-site class names as the default for plugin method hooks, and add an opt-in flag to use defining-class names so plugins can migrate gradually.

Fixes python#19181
@itxsamad1 itxsamad1 changed the title Fix method plugin hooks to use defining class fullname (fixes #19181) Add opt-in flag for method hook defining-class fullnames Jun 28, 2026
@itxsamad1

Copy link
Copy Markdown
Author

Sorry about the duplicate comments earlier — that was my mistake.

Re the backward-compat point: I reworked this so the old behavior stays the default. Hooks still get the call-site class unless you opt in with \use_method_hook_defining_class = True\ (or --use-method-hook-defining-class).

The new test only enables that flag, so existing plugin tests should behave the same as before. Happy to adjust naming or rollout if you'd prefer a different migration path.

@github-actions

Copy link
Copy Markdown
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wording in mypy.plugin.Plugin.get_method[_signature]_hook docstring

3 participants