Call get_method_hook when methods are used in decorators #10675
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When a method is used in a decorator, pass the full name to plugins
through the get_method_hook function, just like we would normally do for
other places that methods got called. Previously, in these situations,
we would call get_function_hook instead with '{method_name} of
{class_name}', which loses information about which module the class is
located in, and gives information about a method to the hook meant for
functions.
Currently this is limited to situations where the method is accessed in
the decorator itself, so
@obj.meth
def f() -> None: ...
would work but
method = obj.meth
@method
def f() -> None: ...
probably wouldn't work.
Add a test case that makes sure that methods used as decorators are passed to the get_method_hook with the correct name. This was fixed in the previous commit, so this test is currently passing.
Add a test that checks if plugins are correctly called with
'{class_name}.{method_name}' when a method is assigned to a local
variable and then used as a decorator, instead of the current behavior,
which is calling the plugin with '{module_name}.{local_variable_name}'
where local_variable_name is the variable that the method was assigned
to.
This is currently marked as xfail because this doesn't work right now.
The `fullname is None` check is redundant because of the outer if statement, and self.type_map stores Type, not Optional[Type], so the `object_type is not None` check is unnecessary.
Reproducing the bug that causes a method name like 'a of Foo' to get passed to get_function_hook happens any time a method is used in a decorator, meaning the overloading is unnecessary for the test.
msullivan
approved these changes
Jun 20, 2021
Remove test marked with xfail because getting it to work probably would require a lot of other changes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, when methods are used in decorators, instead of correctly calling
get_method_hookwith'{module_name}.{class_name}.{method_name}'(withmodule_name,class_name, andmethod_namefilled in with their correct values),get_function_hookis called instead with'{method_name} of {class_name}'. This PR fixes this bug by callingget_method_hookcorrectly.Technically, this may break some plugins if they relied on the fact that
get_function_hookwould be called on methods used in decorators, because with this PR those calls get moved toget_method_hook. I'm guessing that this is rare, but I don't know for sure if any plugins rely on that.Currently, this only works when you access the method in the decorator itself, instead of assigning the method to a local variable. So, for example (assuming
objis an object that has a methodmeth), this works:but this doesn't:
Currently, the second case causes mypy to pass
__main__.methodtoget_function_hookinstead of passing the same thing as the first case. I added a test that's currently marked as xfail for the second case, but I'm not really sure how to fix it.Test Plan
I added a test to check-custom-plugin.test to make sure that the correct method name is getting passed to plugins and that it is getting passed to the correct plugin hook.