Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork3.1k
Allow instantiation of Type[A], if A is abstract#2853
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
elazarg commentedFeb 16, 2017
#2430 depends on this PR (although the test should be removed there) |
gvanrossum left a comment
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! 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I take it this is only because of thevar: 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
For what, exactly?
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 added a detailed comment here.
mypy/checker.py Outdated
| # Special case | ||
| if ( | ||
| isinstance(rvalue_type,CallableType)andrvalue_type.is_type_obj()and |
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'm not keen on this formatting.
mypy/checker.py Outdated
| isinstance(lvalue_type,TypeType)and | ||
| isinstance(lvalue_type.item,Instance)andlvalue_type.item.type.is_abstract | ||
| ): | ||
| self.fail("Cannot only assign non-abstract classes" |
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.
"Cannot only" -> "Cannot" (I presume).
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.
Oh, sorry, it should be "Can only" (this is probably a worst kind of mistakes).
| ifcallee.is_concrete_type_obj()andcallee.type_object().is_abstract: | ||
| if (callee.is_type_obj()andcallee.type_object().is_abstract | ||
| # Exceptions for Type[...] and classmethod first argument | ||
| andnotcallee.from_type_typeandnotcallee.is_classmethod_class): |
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 also except static methods?
I found a case in our codebase that goes basically like this:
classLogger:@staticmethoddeflog(a:Type[C]): ...classC:@classmethoddefaction(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This code looks reasonable, I added a corresponding exception tocheck_arg.
mypy/checkexpr.py Outdated
| messages.does_not_return_value(caller_type,context) | ||
| elifisinstance(caller_type,DeletedType): | ||
| messages.deleted_as_rvalue(caller_type,context) | ||
| # Only non-abstract class could be given where Type[...] is expected |
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 -> can (again :-)
ilevkivskyi commentedMar 21, 2017
@gvanrossum |
gvanrossum commentedMar 21, 2017
LGTM. We're holding off merging until we've sorted out some issues with recent mypy and typeshed commits. |
ilevkivskyi commentedMar 27, 2017
@gvanrossum Could this be merged soon? Sorry for bothering you again, this will be helpful to avoid merge conflicts with my work on protocols. |
gvanrossum commentedMar 27, 2017
NP. We're finally at a point where we can merge PRs with confidence again. |
Uh oh!
There was an error while loading.Please reload this page.
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 thatimplements a certain protocol, not just inherits fromA.EDIT: as discussed inpython/peps#224 this behaviour is good for both protocols and usual ABCs.