-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Support for declared_type
, to give types to decorated functions
#3291
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 the PR! Looks good, I just have some ideas about test cases.
|
||
T = TypeVar('T') | ||
|
||
|
||
def TypedDict(typename: str, fields: Dict[str, Type[T]]) -> Type[dict]: pass | ||
|
||
class NoReturn: pass | ||
|
||
def decorated_type(t: Type) -> Callable[[T], T]: 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.
The argument type should be Any
, since some valid types aren't type objects (e.g. None
).
test-data/unit/check-functions.test
Outdated
|
||
@decorated_type # E: "decorated_type" must have a type as an argument | ||
@dec | ||
def f(): 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.
Add reveal_type(f)
.
test-data/unit/check-functions.test
Outdated
|
||
@decorated_type() # E: "decorated_type" takes exactly one argument | ||
@dec | ||
def f(): 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.
Add reveal_type(f)
.
test-data/unit/check-functions.test
Outdated
|
||
@dec | ||
@decorated_type(Callable[[int], str]) # E: "decorated_type" must be the topmost decorator | ||
def f(): 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.
Add reveal_type(f)
.
test-data/unit/check-functions.test
Outdated
|
||
def dec(f: T) -> T: pass | ||
|
||
@decorated_type(Callable[[int], str]) # E: Incompatible types (inferred decorated type Callable[[str], str], declared decorated type Callable[[int], str]) |
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.
Add test where the declared type is a proper supertype of the inferred type. It should be okay -- i.e. test that the subtype checks goes the right way.
test-data/unit/check-functions.test
Outdated
def f(): pass | ||
|
||
[builtins fixtures/dict.pyi] | ||
|
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.
Add test cases that use staticmethod
and classmethod
together with decorated_type
.
Can you also fix the merge conflict? This needs https://github.com/python/typeshed/pull/1230/files before this is usable outside tests. I'll merge this PR first. |
decorated_type
, to give types to decorated functionsdeclared_type
, to give types to decorated functions
@JukkaL ping seems ready after relevant typeshed change. |
Not a review, just wondering if this might be a candidate for implementing through the new plugin architecture, once #3534 lands? |
If we're proposing this as a general typing feature (not something mypy-specific), which I think we are, then this should ideally be placed in typing_extensions, which is now a thing: https://github.com/python/typing/tree/master/typing_extensions. (I withdraw my suggestion to consider this for the plugin architecture.) |
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 some minor nits. Thanks for implementing this! Apologies about the long review delay -- I got distracted by various things including vacation and internal team changes, and I took a long time to finish the review I started a while ago.
@@ -734,6 +734,188 @@ a = None # type: A | |||
a.f() | |||
a.f(None) # E: Too many arguments for "f" of "A" | |||
|
|||
[case testMethodWithDeclaredDecoratedType] | |||
|
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.
Style nit: Redundant empty line (and similar cases in other test cases).
y: int = foo.f(1) # E: Incompatible types in assignment (expression has type "str", variable has type "int") | ||
|
||
reveal_type(foo.f) # E: Revealed type is 'def (builtins.int) -> builtins.str' | ||
|
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.
Another context where we don't tend to have empty lines.
from mypy_extensions import declared_type | ||
|
||
@declared_type(Callable[[int], str]) | ||
def f(x): 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.
Maybe reveal type of x
inside f
to ensure that the declared type doesn't get propagated.
"""Declare the type of a declaration. | ||
|
||
This is useful for declaring a more specific type for a decorated class or | ||
function definition than the decorator provides as a return value. |
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.
I wonder if there is a context where it's useful to use this to decorate a class?
@@ -2633,6 +2633,19 @@ def visit_decorator(self, dec: Decorator) -> None: | |||
elif refers_to_fullname(d, 'typing.no_type_check'): | |||
dec.var.type = AnyType() | |||
no_type_check = True | |||
elif isinstance(d, CallExpr) and ( | |||
refers_to_fullname(d.callee, 'typing.declared_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.
As discussed above, maybe also accept typing_extensions.declared_type
.
self.fail('"declared_type" takes exactly one argument', d) | ||
else: | ||
dec.var.type = self.expr_to_analyzed_type(d.args[0]) | ||
elif (refers_to_fullname(d, 'typing.declared_type') or |
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.
Same as above.
def dec(f: T) -> T: pass | ||
|
||
@declared_type(Callable[[int], str]) # E: Incompatible types (inferred decorated type Callable[[str], str], declared decorated type Callable[[int], str]) | ||
@dec |
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.
Also test declared_type
together with another decorator that modifies the signature of the function. Test both a good usage and an invalid usage (where the declared type is incompatible).
|
||
# Note that the decorated type must account for the `cls` argument -- It's applied pre-binding | ||
class Foo: | ||
@declared_type(Callable[[Any, int], str]) |
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.
Test similar case where you use Type[...]
type for the cls
argument.
|
||
# Note that the decorated type must account for the `self` argument -- It's applied pre-binding | ||
class Foo: | ||
@declared_type(Callable[[Any, int], str]) |
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.
Test similar case where you use a non-Any
type for the self argument, such as Foo
here.
e.var.type = sig | ||
if e.var.type is not None: | ||
# We have a declared type, check it. | ||
self.check_subtype(sig, e.var.type, e, |
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.
@JukkaL Shouldn't this be other way around?
I thought that the declared type is expected to be more narrow/precise than the inferred.
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.
I think that this is right -- this is not a cast but a declaration. But it would be good to have a test case where the ordering of this check is important.
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.
The problem with this is that it will be then impossible to give a more precise type for a decorated function when it is known. From the discussion on python/peps tracker I somehow understood that this should work like a cast. I however could imagine situations where one might want to declare narrower argument types, so maybe this check is not needed and we can allow both ways?
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 can be used to declare a more precise type with respect to Any
types (Any
is considered a subtype of everything).
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, I see, I was thinking about hijacking this decorator for #2087
I'm closing the PR since there hasn't been activity in a long time. Feel free to reopen if you wish to continue working on this. |
This is an implementation of python/peps#242 in mypy_extensions instead of typing for now (but with forward compat when it gets to typing)