Repository navigation
Conversation
JukkaL
left a comment
There was a problem hiding this comment.
Thanks for the PR! Looks good, I just have some ideas about test cases.
|
|
||
| class NoReturn: pass | ||
|
|
||
| def decorated_type(t: Type) -> Callable[[T], T]: pass No newline at end of file |
There was a problem hiding this comment.
The argument type should be Any, since some valid types aren't type objects (e.g. None).
|
|
||
| @decorated_type # E: "decorated_type" must have a type as an argument | ||
| @dec | ||
| def f(): pass |
|
|
||
| @decorated_type() # E: "decorated_type" takes exactly one argument | ||
| @dec | ||
| def f(): pass |
|
|
||
| @dec | ||
| @decorated_type(Callable[[int], str]) # E: "decorated_type" must be the topmost decorator | ||
| def f(): pass |
|
|
||
| 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.
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.
| def f(): pass | ||
|
|
||
| [builtins fixtures/dict.pyi] | ||
|
|
There was a problem hiding this comment.
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.) |
JukkaL
left a comment
There was a problem hiding this comment.
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.
| a.f(None) # E: Too many arguments for "f" of "A" | ||
|
|
||
| [case testMethodWithDeclaredDecoratedType] | ||
|
|
There was a problem hiding this comment.
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.
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.
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.
I wonder if there is a context where it's useful to use this to decorate a class?
| 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.
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 |
| 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.
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.
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.
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.
@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.
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.
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.
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.
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)