-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Fix overload resolution for metaclass #3511
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.
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
Conversation
test-data/unit/check-classes.test
Outdated
reveal_type(f(e2)) # E: Revealed type is 'builtins.int' | ||
|
||
[out] | ||
main:6: error: An overloaded function outside a stub file must have an implementation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a bug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, there must be an implementation (or make this in a .pyi file in test).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But there is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not how overloads work. PEP 484:
In regular modules, a series of
@overload
-decorated definitions must be followed by exactly one non-@overload
-decorated definition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, sorry. I will add one.
51a8443
to
ce309a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a few ideas for test cases.
test-data/unit/check-classes.test
Outdated
class EM(type): pass | ||
class E(metaclass=EM): pass | ||
|
||
@overload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have a few more test cases:
- Test two overload variants which have two different metaclasses as argument types, and overloading based on metaclass.
- Test calling the overloaded function with a
str
argument (results should bestr
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now. Thanks for fixing this!
Fix #3452.