Allow instantiation of Type[A], if A is abstract#2853
Allow instantiation of Type[A], if A is abstract#2853gvanrossum merged 7 commits intopython:masterfrom
Conversation
|
#2430 depends on this PR (although the test should be removed there) |
gvanrossum
left a comment
There was a problem hiding this comment.
Thanks! I finally got to looking at this. I like the general idea.
test-data/unit/check-abstract.test
Outdated
| [out] | ||
|
|
||
| [case testInstantiationAbstractsInType] | ||
| # flags: --python-version 3.6 |
There was a problem hiding this comment.
I take it this is only because of the var: Type[A] below? If this behavior is supposed to work for all Python versions maybe you can rewrite that part of the test using a function? (Also it's a rather long test; maybe it would be better split up.)
There was a problem hiding this comment.
Since recently --python-version flag is not needed. Also I split the test into three smaller and added a test case for @classmethod.
mypy/checker.py
Outdated
| else: | ||
| rvalue_type = self.check_simple_assignment(lvalue_type, rvalue, lvalue) | ||
|
|
||
| # Special case |
There was a problem hiding this comment.
I added a detailed comment here.
mypy/checker.py
Outdated
|
|
||
| # Special case | ||
| if ( | ||
| isinstance(rvalue_type, CallableType) and rvalue_type.is_type_obj() and |
There was a problem hiding this comment.
I'm not keen on this formatting.
mypy/checker.py
Outdated
| isinstance(lvalue_type, TypeType) and | ||
| isinstance(lvalue_type.item, Instance) and lvalue_type.item.type.is_abstract | ||
| ): | ||
| self.fail("Cannot only assign non-abstract classes" |
There was a problem hiding this comment.
"Cannot only" -> "Cannot" (I presume).
There was a problem hiding this comment.
Oh, sorry, it should be "Can only" (this is probably a worst kind of mistakes).
| if callee.is_concrete_type_obj() and callee.type_object().is_abstract: | ||
| if (callee.is_type_obj() and callee.type_object().is_abstract | ||
| # Exceptions for Type[...] and classmethod first argument | ||
| and not callee.from_type_type and not callee.is_classmethod_class): |
There was a problem hiding this comment.
Maybe also except static methods?
I found a case in our codebase that goes basically like this:
class Logger:
@staticmethod
def log(a: Type[C]): ...
class C:
@classmethod
def action(cls) -> None:
Logger.log(cls) #E: Only non-abstract class can be given where 'Type[C]' is expected
@abstractmethod
...(I'm far from positive that it's because of this line though.)
There was a problem hiding this comment.
This code looks reasonable, I added a corresponding exception to check_arg.
mypy/checkexpr.py
Outdated
| messages.does_not_return_value(caller_type, context) | ||
| elif isinstance(caller_type, DeletedType): | ||
| messages.deleted_as_rvalue(caller_type, context) | ||
| # Only non-abstract class could be given where Type[...] is expected |
|
@gvanrossum |
|
LGTM. We're holding off merging until we've sorted out some issues with recent mypy and typeshed commits. |
|
@gvanrossum Could this be merged soon? Sorry for bothering you again, this will be helpful to avoid merge conflicts with my work on protocols. |
|
NP. We're finally at a point where we can merge PRs with confidence again. |
Fixes #1843 (It was also necessary to fix few minor things to make this work correctly)
The rules are simple, assuming we have:
then
The same applies to variables:
Also there is an option for people who want to pass abstract classes around: type aliases, they work as before. For non-abstract
A,Type[A]also works as before.@gvanrossum You could be interested in this.
My intuition why you opened #1843 is when someone writes annotation
Type[A]with an abstractA, then most probably one wants a class object that implements a certain protocol, not just inherits fromA.EDIT: as discussed in python/peps#224 this behaviour is good for both protocols and usual ABCs.