-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Implement the descriptor protocol #2266
Conversation
(Can you come up with more descriptive commit messages for your local commits? They won't end up in the final merge because we do squash merges, but it's nice to have meaningful local commits.) |
@gvanrossum: I was planning on using git rebase --autosquash, so those commit messages just mean "These will be squashed into the 'Implement the descriptor protocol' commit" |
That said, if you'd rather I either pre-squashed, or named my intermediate commits more meaningfully, I can do that. Whatever makes reviewing easier for you all. |
Don't pre-squash. Give future intermediate commits meaningful messages.
(It's fine to use "git commit --amend" locally if you haven't pushed to
your branch yet.)
|
Ok, will do. |
Hey Calen, I appreciate your enthusiasm, but are you sure you haven't bitten off more than you can chew? That test file gives the same errors without your patch, so I'm not sure why you are using it as a test. (With your patch there's an extra error that you don't quote, which seems to be due to dunder_set not being a Type at all: "test_descriptors.py:60: error: Can't set a data descriptor with no set method". But it seems you've been focusing on the non-lvalue branch of your code, which makes sense.) Also: if you add And:
I'm not sure why you bring #1136 into it -- your test doesn't even contain the word I'm not sure what to recommend you do next. As you may guess from the sheer length of the discussion in #244 (and that we haven't gotten rid of the special-casing of If you want to work on it by yourself, I recommend starting with much simpler test cases (most test cases in test-data/unit are only a few lines, just enough to demonstrate a single small thing in the code). Maybe start with a read-only property of a fixed type first? |
I was using that test file as a test because I had already verified that the case of non-generic descriptors was already working. I've updated the file to show that (and also fixed the error with I'm mentioning #1136 precisely because my test doesn't have the word overload in it. The second error in my test case is because
With that signature, I think the second error would go away, because the type checker could correctly distinguish between the access by class and the access by instance cases (although that might take some additional adjustment to the checker code that I don't know about yet. Maybe I'll add a stub file just to test that case out.) I'm happy to continue digging through some of these issues on my own. One question that it would be great to get answered, though, is "What's the best way to engage the type-checker to eliminate generic variables?" My guess is that I need to plug in the knowledge of |
Actually, now that I'm reading up more on |
Ok, so I have the basics of descriptors working (with a couple of test-cases added). As I think about it a bit more, though, I'm wondering whether the code that I've added to the checker should be somewhere else. Is there a strong distinction in mypy right now between assigning types to expressions and checking the consistency of types in statements? If so, this code should probably be in the former location ( |
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.
So, it seems your big test from the initial comment now passes? Suppose you replaced the definition of property from typeshed with that one, could you then rip out the custom property support from mypy? What about staticmethod, classmethod?
(FWIW I couldn't quite follow what you changed in that big test, but I recall the line numbers for the errors being smaller last time, so I figured you updated it. :-)
class A: | ||
f = D() | ||
|
||
a = A() # type. A |
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.
Are you trying to make something that looks like a type annotation but isn't? I'd be more explicit about that, e.g. # type is A
.
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.
Oops. No, that's a typo. I think there are a couple of the Property tests w/ the same typo, so I'll fix them too.
a = A() # type. A | ||
s = '' # type: str | ||
s = a.f | ||
a = a.f # E: Incompatible types in assignment (expression has type "str", variable has type "A") |
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.
A simpler test would just use reveal_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.
Fixed.
else: | ||
# TODO: Per the docs: "If [a descriptor] does not define __get__(), | ||
# then accessing the attribute will return the descriptor object | ||
# itself unless there is a value in the object’s instance dictionary". |
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.
Hm... I think this describes the default behavior when there is no descriptor at all (i.e. search instance dict, class dict, superclass dict etc.).
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.
Ah, that's true. I'll take a stab at making it work (since it may just require falling through to the rest of the typing code).
I think where you've got this is right. semanal.py is (mostly) for matching
references to definitions. Statements are type-checked in checker.py which
for expressions passes the buck to checkexpr.py, which is the main customer
for checkmember.py.
|
Yeah, the big test passes (I had added checks to verify that non-generic descriptors were working correctly). I'll break it down into smaller example tests inside I tried replacing the special-casing of I haven't tried staticmethod or classmethod yet, but they seem more straightforward. |
If it makes sense, I was thinking that removing the special casing for Are there any other tests you'd like to see in this? I think I got all of the cases... |
I think that some of the special casing for |
@JukkaL: Yeah, that was my suspicion, and why I was leaning towards tackling those in separate PRs. |
Then what are your ambitions for the current PR? Does it do what you need
for your application (assuming you have a specific application in mind)?
|
I think so. I'm looking to typecheck something very-much like Django's Field objects, which the generic data descriptor test cases seem to cover. I'm also happy to come back and improve things if I find that there's more subtlety in the type-checking that I'm looking for than I'm aware of now. |
Then it would be good to include a more complex test case in the unit tests
(but not quite as complete as the one you have in the PR description).
Also, I'm currently overcommitted, it may be a while before I can truly
review this PR. If one of the other mypy core devs wants to have a look
please do!
|
I think the two tests of Is there any coverage reporting done during the mypy tests? It would be nice to know whether I'm hitting all the lines/branches that I added. |
Sadly we don't have test coverage. If you feel like an engineering
challenge it would be great! Maybe it could be integrated with our Travis
CI runs (though I'd be sad if it became much slower -- perhaps coverage
should only be run with one of the Python versions, perhaps 3.6 beta).
|
Ok, I've added coverage reporting to the pytest tests (they were the easiest to cover, but running other python code under coverage is fairly straightforward as well, if that ends up being important). I've also set the coverage files to upload to codecov.io, which is free for open source projects. Right now, it's only reporting coverage for the edx fork, but you can see an example here: https://codecov.io/gh/edx/mypy/branch/test-coverage, and could choose to enable reporting for the Looks like I have a few more test cases to write. |
Current coverage is 83.52% (diff: 76.59%)
|
Cool, the test coverage report is very exciting! It won't be very actionable until it covers all (or at least most) test cases, since a lot of the lines shown as uncovered actually have test coverage, but the tests just aren't included in the stats. Could you send the test coverage reporting as a separate PR, as it's not directly related to this PR? |
@JukkaL: Yes. @gvanrossum mentioned that you'd prefer that I not rebase this branch while review is in progress, so I'll leave the coverage commit in this PR, but submit it as a separate PR as well. |
Ok, I've added some more tests, and made sure that class attribute access to descriptors works properly. This needs a rebase, since it conflicts with the coverage changes that were already merged to master. I can do that whenever works best for the reviewers on your side. |
You can rebase as long as you don't squash. Dropping the coverage commit
here is preferred.
|
@gvanrossum: Yeah, @JukkaL suggested I rebase (on gitter), so I just did that (which removed the commit coverage commit, and squashed in the handful of auto-squash commits at the beginning of the PR) |
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 is part 1 of the review only! I only read the unit tests so far (they form a great way to understand what the PR is about). I'll try to review the code too, soon.
s = '' # type: str | ||
s = a.f | ||
a = a.f # E: Incompatible types in assignment (expression has type "str", variable has type "A") | ||
a = A() # type: A |
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 doesn't even need the # type: A
.
|
||
|
||
[case testAccessingNonDataDescriptor] | ||
class D(): |
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.
We usually don't write ()
for a class without base 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.
Fixed.
|
||
[case testAccessingNonDataDescriptor] | ||
class D(): | ||
def __get__(self, instance: Any, owner: Any) -> 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.
I tried some variations on this and it looks the declared type of instance and owner is not taken into account when verifying that the descriptor makes sense. E.g. on the f = D()
line below and a.f
later are still valid even if I change Any to int here. I'd expect them to be required to match C and Type[C] in the example below.
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.
Yeah, I think I need to add some checking to validate that __get__
and __set__
definitions are correctly typed in definitions. Unless you have a pointer on where that should be checked, I'll do some digging and see if there are any other checks of that sort.
pass | ||
|
||
class A: | ||
f = D() # type: Union[D, 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.
This test feels fishy. Without this # type
the assignment self.f = 'some string'
should pass, without a # type: str
there. And the revealed type for a.f should be D. It feels as if the explicit annotations are trying to paper over something missing in the implementation.
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.
Yeah, fixed.
class D(Generic[T, V]): | ||
def __init__(self, value: V) -> None: | ||
self.value = value | ||
def __get__(self, instance: T, owner: Type[T]) -> V: |
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.
Hm. I'd like to see a test where D is generic in V but not in T first. And I'd somehow hoped that the type declarations inside class A below would not be needed.
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've converted these tests to be only generic on V
(which, as I think about it, makes more sense anyway).
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 had to convert the descriptors back to being generic over T, because otherwise mypy couldn't disambiguate the two different calling conventions.
pass | ||
|
||
class A: | ||
f = D(10) # type: D[A, int] |
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.
Again, these type annotations shouldn't be needed?
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.
When I run the tests without these annotations, I get "Need type annotation for variable", pointing to the line where the descriptor is defined.
Testing with other annotations, I see that it's an issue with the generic T
, in particular. I fixed up the tests to remove T
from the class-level generics, which allowed me to remove a bunch of the annotations that you thought should be unnecessary.
A.f = D('some string') # E: Argument 1 to "D" has incompatible type "str"; expected "int" | ||
|
||
|
||
-- Multiple inheritance, non-object built-in class as base |
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 next two tests were actually deleted. I guess you accidentally restored them as part of a merge resolution?
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.
Fixed.
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.
Fixed.
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, here's the rest of my review. (I'm still learning this part of mypy myself, I don't have everything here in my "active vocabulary" yet, so it's possible that I'm not always asking the right questions or pointing you in the right direction. Sorry about that.)
return None | ||
|
||
if not dunder_set: | ||
msg.fail("Can't set a data descriptor with no __set__ method", node) |
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 don't see any tests checking for this error. (Nor for others, e.g. "set must be callable" below).
msg.fail("__set__ must be callable", node) | ||
return AnyType() | ||
|
||
return dunder_set_type.arg_types[1] |
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 assumes __set__
has at least two arguments. But you don't check for that.
I think this is also approximately the point where you might need to check that the first argument matches the class in which the descriptor was encountered (typically 'A' in your tests).
typ = map_instance_to_supertype(descriptor, dunder_get.info) | ||
dunder_get_type = expand_type_by_instance(bound_method, typ) | ||
|
||
if not isinstance(dunder_get_type, (Overloaded, CallableType)): |
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 90% sure you can use FunctionLike here.
msg.fail("Unable to find an applicable overload for __get__", node) | ||
return AnyType() | ||
|
||
return dunder_get_type.ret_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.
And somewhere here you should probably check that dunder_get_type can actually be called with the given class or instance.
if is_class_access: | ||
arg_types = [NoneTyp(), TypeType(owner)] | ||
else: | ||
arg_types = [descriptor, TypeType(owner)] |
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.
It seems arg_types is only used if dunder_get_type is Overloaded (tested below).
Here's a little example lifted almost exactly from StackOverflow: # See http://stackoverflow.com/questions/3203286/how-to-create-a-read-only-class-property-in-python
from typing import *
T = TypeVar('T')
V = TypeVar('V')
class classproperty(Generic[T, V]):
def __init__(self, getter: Callable[[Type[T]], V]) -> None:
self.getter = getter
def __get__(self, instance: Any, owner: Type[T]) -> V:
# instance is really None, but we don't care
return self.getter(owner)
class C:
@classproperty # <-- error here
def foo(cls) -> int:
return 42
reveal_type(C.foo) This gives an error (the first one, on line 20, i.e. the
(UPDATE: One solution would be to get rid of T and just use Any. But that feels like cheating.) |
@gvanrossum: I think that error on line 20 is actually because mypy knows/assumes too much about what type the first argument to a method will be. In particular, it seems like it's assuming that the first argument is always Is it possible that mypy is "cheating", and knows something special about |
In fact: https://github.com/python/mypy/blob/master/mypy/checkmember.py#L316-L342 seems to do just that. It's special-cased for classmethods. Personally, I'd lean towards landing the existing support for descriptors, and then working on removing the existing special-casing as much as possible, so as to not expand the scope of this particular PR too much. |
I chatted w. @JukkaL and he hasn't had time to review this yet, so let's
wait for him.
|
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 almost ready to merge -- I have just a few minor comments.
@@ -349,6 +350,7 @@ def analyze_class_attribute_access(itype: Instance, | |||
builtin_type: Callable[[str], Instance], | |||
not_ready_callback: Callable[[str, Context], None], | |||
msg: MessageBuilder, | |||
chk: 'mypy.checker.TypeChecker', |
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 is unused?
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.
Removed.
f = D(10) # type: D[A, int] | ||
g = D('10') # type: D[A, str] | ||
reveal_type(A.f) # E: Revealed type is 'd.D[__main__.A*, builtins.int*]' | ||
reveal_type(A.g) # E: Revealed type is 'd.D[__main__.A*, 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.
Also reveal the type of A().f
and A().g
.
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.
Added.
a.g = '' | ||
a.g = 1 # E: Argument 2 to "__set__" of "D" has incompatible type "int"; expected "str" | ||
|
||
[case testAccessingGenericDescriptorFromClass] |
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.
Can you add a test case where __get__
is overloaded but not generic. For example:
class Base: pass
class D:
@overload
def __get__(self, inst: None, own: Type[Base]) -> str: pass
@overload
def __get__(self, inst: Base, own: Type[Base]) -> int: pass
class A(Base):
f = D()
Then access a descriptor attribute through A().f
and A.f
. Also, check what happens if A
is not a subclass of Base
and we access the descriptor.
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.
Added a couple of test cases for these.
Ah you probably have to rebase to get the right test output. We recently simplified the output from mypy. |
Thanks for the final updates! |
Added #2563 tracking the classproperty example. |
(One thing we do know: this code does not cause new errors on our internal codebases.) |
Taken from python/mypy#2266 (comment). These annotations allow for static analysis to correctly return the correct type.
Taken from python/mypy#2266 (comment). These annotations allow for static analysers to correctly return the type of a decorated method.
This is an attempt to implement the descriptor protocol (as per #244).
However, it currently is suffering from being being unable to resolve type variables during assignment, and I'm not sure what I would need to resolve that. For instance, using this implementation of properties:
When checked with mypy, we get the following errors:
The second error is because we don't have overloading outside of stub files (see #1136), I think, but the former is the one I'm not sure how to resolve.