-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Fix untyped decorator check for class instances #5509
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
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.
Thanks for contributing! Here are few comments.
It looks like a proper fix for the issue would be to just add another branch for Overloaded
type. I would do this in the same PR, but if you want a separate PR it is also OK.
mypy/checker.py
Outdated
method = typ.type.get_method('__call__') | ||
if method and method.type: | ||
return not is_typed_callable(method.type) | ||
return True |
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.
If an Instance
doesn't have a __call__
method this will return True
, which will generate an additional spurious error. While I believe it shouldn't.
Could you please add a test case for this?
mypy/checker.py
Outdated
return typ.implicit | ||
if isinstance(typ, CallableType): | ||
return typ.implicit | ||
if isinstance(typ, Instance): |
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.
if
-> elif
would be better here.
mypy/checker.py
Outdated
if isinstance(typ, Instance): | ||
method = typ.type.get_method('__call__') | ||
if method and method.type: | ||
return not is_typed_callable(method.type) |
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 seems a bit inconsistent with the above. I would either use typ.implicit
in both branches, or is_typed_callable
in both branches.
test-data/unit/check-flags.test
Outdated
|
||
@UntypedDecorator() # E: Untyped decorator makes function "f" untyped | ||
@TypedDecorator() | ||
def f() -> None: pass |
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.
Could you please add four separate checks that would check only first decorator, only second decorator, and both of them in different order. Each check should also have a reveal_type
for the function so that it is clear the behaviour is correct.
Thank you for the review. Updated with requested fixes + fix for the |
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.
Sorry for a delay with review, this looks good now!
… overloads (#9232) When --disallow-untyped-decorators is used, a callable-class decorator with a overloads on its __call__ implementation would incorrectly issue an "Untyped decorator makes function untyped" error, even when the overloads are typed properly. This has been reported in pytest, which does this: pytest-dev/pytest#7589. This fixes the problem by expanding on a previous fix in this area, #5509.
Running
mypy --disallow-untyped-decorators
when the decorator is a typed callable instance produces an error even thoughreveal_type
shows the correct type.Related to #4191 but I'll leave that one for a different PR.