Fix joining a function against metaclass-using object constructors #13648
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
This pull request fixes python#9838. It turns out that when an object is using a metaclass, it uses that metaclass as the fallback instead of `builtins.type`. This caused the `if t.fallback.type.fullname != "builtins.type"` check we were performing in `join_similar_callables` and `combine_similar_callables` to pick the wrong fallback in the case where we were attempting to join a function against a constructor for an object that used a metaclass. This ended up causing a crash later for basically the exact same reason python#13576 caused a crash: using `abc.ABCMeta` causes `Callable.is_type_obj()` to return true, which causes us to enter a codepath where we call `Callable.type_object()`. But this function is not prepared to handle the case where the return type of the callable is a Union, causing an assert to fail. I opted to fix this by adjusting the join algorithm so it does `if t.fallback.type.fullname == "builtins.function"`. One question I did punt on -- what should happen in the case where one of the fallbacks is `builtins.type` and the other is a metaclass? I suspect it's impossible for this case to actually occur: I think mypy would opt to use the algorithm for joining two `Type[...]` entities instead of these callable joining algorithms. While I'm not 100% sure of this, the current approach of just arbitrarily picking one of the two fallbacks seemed good enough for now.
This comment has been minimized.
This comment has been minimized.
sobolevn
approved these changes
Sep 12, 2022
test-data/unit/check-classes.test
Outdated
|
|
||
| def f1(cond: bool) -> Any: | ||
| join = func if cond else WithMetaclass | ||
| return reveal_type(join()) # N: Revealed type is "Union[__main__.WithMetaclass, None]" |
There was a problem hiding this comment.
Suggested change
| return reveal_type(join()) # N: Revealed type is "Union[__main__.WithMetaclass, None]" | |
| reveal_type(join()) # N: Revealed type is "Union[__main__.WithMetaclass, None]" |
test-data/unit/check-classes.test
Outdated
|
|
||
| def f2(cond: bool) -> Any: | ||
| join = WithMetaclass if cond else func | ||
| return reveal_type(join()) # N: Revealed type is "Union[__main__.WithMetaclass, None]" |
There was a problem hiding this comment.
Suggested change
| return reveal_type(join()) # N: Revealed type is "Union[__main__.WithMetaclass, None]" | |
| reveal_type(join()) # N: Revealed type is "Union[__main__.WithMetaclass, None]" |
test-data/unit/check-classes.test
Outdated
|
|
||
| def f3(cond: bool) -> Any: | ||
| join = NormalClass if cond else WithMetaclass | ||
| return reveal_type(join()) # N: Revealed type is "builtins.object" |
There was a problem hiding this comment.
Suggested change
| return reveal_type(join()) # N: Revealed type is "builtins.object" | |
| reveal_type(join()) # N: Revealed type is "builtins.object" |
test-data/unit/check-classes.test
Outdated
|
|
||
| def f4(cond: bool) -> Any: | ||
| join = WithMetaclass if cond else NormalClass | ||
| return reveal_type(join()) # N: Revealed type is "builtins.object" |
There was a problem hiding this comment.
Suggested change
| return reveal_type(join()) # N: Revealed type is "builtins.object" | |
| reveal_type(join()) # N: Revealed type is "builtins.object" |
|
I'm guessing all of these tests would break if something like #12712 was ever merged (there seems to be a consensus that it would be good to do something like that if the regressions could be kept down to a minimum — #12395 (comment)). Is there a way of making the tests more future-proof for that? Or not worth bothering about? |
|
Actually, yes, I agree that a better way to trigger join is to use |
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
|
Yay Michael! |
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.
This pull request fixes #9838.
It turns out that when an object is using a metaclass, it uses that metaclass as the fallback instead of
builtins.type.This caused the
if t.fallback.type.fullname != "builtins.type"check we were performing injoin_similar_callablesand combine_similar_callables` to pick the wrong fallback in the case where we were attempting to join a function against a constructor for an object that used a metaclass.This ended up causing a crash later for basically the exact same reason discussed in #13576: using
abc.ABCMetacausesCallable.is_type_obj()to return true, which causes us to enter a codepath where we callCallable.type_object(). But this function is not prepared to handle the case where the return type of the callable is a Union, causing an assert to fail.I opted to fix this by adjusting the join algorithm so it does
if t.fallback.type.fullname == "builtins.function".One question I did punt on -- what should happen in the case where one of the fallbacks is
builtins.typeand the other is a metaclass?I suspect it's impossible for this case to actually occur: I think mypy would opt to use the algorithm for joining two
Type[...]entities instead of these callable joining algorithms. While I'm not 100% sure of this, the current approach of just arbitrarily picking one of the two fallbacks seemed good enough for now.