Repository navigation
Support method plugin hooks on unions - #6560
Conversation
Michael0x2a
left a comment
Michael0x2a
left a comment
There was a problem hiding this comment.
I think looks good to me -- I couldn't really find anything I wanted to give feedback on.
I guess I am a little uncomfortable with how many different check_call_blah methods we're accumulating, but I don't think that's really relevant to this pull request.
I think we should merge this first -- I can add the extra tests to my PR.
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
Thanks for fixing this! Generally looks good, left some ideas about making the relevant code a bit easier to understand.
|
|
||
| The given callee type overrides the type of the callee | ||
| expression. | ||
|
|
There was a problem hiding this comment.
The above docstring part was a bit confusing. It looks like we aren't actually overriding the callee type but to reusing a previously calculated callee type. This looks like an optimization. Maybe update the docstring to mention that? Otherwise it's not clear whether recalculating the callee type is correct.
| The given callee type overrides the type of the callee | ||
| expression. | ||
|
|
||
| The 'callable_name' and 'object_type' are used to call plugin hooks. |
There was a problem hiding this comment.
Also 'member' is used for plugin hooks only? Maybe be more explicit and say that they are only used for this purpose.
| in_literal_context=self.is_literal_context()) | ||
| self.msg.enable_errors() | ||
| item = self.narrow_type_from_binder(e.callee, item) | ||
| # TODO: This check is not one-to-one with non-special-cased behavior. |
There was a problem hiding this comment.
Do you know what issues this may cause? Currently it's not clear if this is a serious problem or not.
There was a problem hiding this comment.
Actually I found a cleaner solution, I will push an update soon.
| callable_name, callee_type, e.args, e.arg_kinds, e, e.arg_names, object_type) | ||
| # Unions are special-cased to allow plugins to act on each item in the union. | ||
| elif member is not None and isinstance(object_type, UnionType): | ||
| res = [] # type: List[Type] |
There was a problem hiding this comment.
Can you move the elif block to a separate method? Now the largest part of this function handles a rare special case, obscuring the main flows.
| original_type = original_type or base_type | ||
| # Unions are special-cased to allow plugins to act on each element of the union. | ||
| if isinstance(base_type, UnionType): | ||
| res = [] # type: List[Type] |
There was a problem hiding this comment.
Similar to above, it would be better to move this block to a separate function.
Conflicts: test-data/unit/check-ctypes.test test-data/unit/check-typeddict.test
|
@Michael0x2a @JukkaL I addressed the comments from last round, do you have any more comments here? |
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
Looks good, though I didn't look at all the recent checker updates in much detail. Just a few comments about tests.
| if isinstance(x.meth, int): | ||
| reveal_type(x.meth) # N: Revealed type is 'builtins.int' | ||
| else: | ||
| reveal_type(x.meth(int())) # N: Revealed type is 'builtins.int' |
There was a problem hiding this comment.
Since the plugin switches types between int/str and the existing types are also int/str, it's a bit hard to see what is going on. What about using more than two separate types?
There was a problem hiding this comment.
OK, I now have different return types for two meth methods, and correspondingly different type conversions for the two hooks: str -> int, and float -> int, this way the call would fail if a wrong hook would act, and hopefully this is now easier to read.
| class Foo: | ||
| def meth(self, x: str) -> str: ... | ||
| class Bar: | ||
| def meth(self, x: int) -> str: ... |
There was a problem hiding this comment.
If each meth variant would have a distinct return type, the test would perhaps catch more issues.
| if isinstance(x.meth, int): | ||
| reveal_type(x.meth) # N: Revealed type is 'builtins.int' | ||
| else: | ||
| reveal_type(x.meth(int())) # N: Revealed type is 'builtins.int' |
There was a problem hiding this comment.
This is similar to above -- maybe use more than two separate types.
3 participants
Fixes #6117
Fixes #5930
Currently both our plugin method hooks don't work with unions. This PR fixes this with three things:
visit_call_expr_inner()(which is a long method already) tocheck_call_expr_with_callee_type()(which is a short method).check_call_expr_with_callee_type()(normal method calls) andcheck_method_call_by_name()(dunder/operator method calls).The week point is interaction with binder, but IMO this is the best we can have for now. I left a comment mentioning that check for overlap should be consistent in two functions.
In general, I don't like special-casing, but I spent several days thinking of other solutions, and it looks like special-casing unions in couple more places is the only reasonable way to fix unions-vs-plugins interactions.
This PR may interfere with #6558 that fixes an "opposite" problem, hopefully they will work together unmodified, so that accessing union of literals on union of typed dicts works. Whatever PR lands second, should add a test for this.