property.__get__: overload to model class-access behavior
#13769
Conversation
Add a new overload for `property.__get__` to model the behavior that a property returns itself when accessed on a class object (when `instance` is set to `None`). The relevant section in the pseudo-implementation of `property` can be found at [1]. [1]: https://github.com/python/cpython/blob/87d9983994e9a423e9e0050b1bbee52ebaf84367/Objects/descrobject.c#L1541-L1542
|
Diff from mypy_primer, showing the effect of this PR on open source code: spark (https://github.com/apache/spark)
+ python/pyspark/sql/session.py:166: error: Signature of "__get__" incompatible with supertype "property" [override]
+ python/pyspark/sql/session.py:166: note: Superclass:
+ python/pyspark/sql/session.py:166: note: @overload
+ python/pyspark/sql/session.py:166: note: def __get__(self, None, type, /) -> classproperty
+ python/pyspark/sql/session.py:166: note: @overload
+ python/pyspark/sql/session.py:166: note: def __get__(self, Any, type | None = ..., /) -> Any
+ python/pyspark/sql/session.py:166: note: Subclass:
+ python/pyspark/sql/session.py:166: note: def __get__(self, instance: Any, owner: Any = ...) -> Builder
pydantic (https://github.com/pydantic/pydantic)
- pydantic/_internal/_decorators.py:199: error: Incompatible return value type (got "Callable[[VarArg(Any), KwArg(Any)], ReturnType] | Any", expected "PydanticDescriptorProxy[ReturnType]") [return-value]
+ pydantic/_internal/_decorators.py:199: error: Incompatible return value type (got "Callable[[VarArg(Any), KwArg(Any)], ReturnType] | Any | property", expected "PydanticDescriptorProxy[ReturnType]") [return-value]
|
There was a problem hiding this comment.
This LGTM. The new mypy_primer hit seems like a true positive: the override of __get__ there in their property subclass really does have different (and arguably Liskov-incompatible) behaviour to property.__get__: https://github.com/apache/spark/blob/beb509fe6d79ada842e14ee7c4a7c55d874fd8b7/python/pyspark/sql/session.py#L166-L171. Mypy is able to detect the incompatible override with the new stub, whereas it couldn't previously.
They also already have a type: ignore on the method, so I doubt it'll concern them too much to have to add another 😆
| @@ -1281,6 +1281,9 @@ class property: | |||
| def getter(self, fget: Callable[[Any], Any], /) -> property: ... | |||
| def setter(self, fset: Callable[[Any, Any], None], /) -> property: ... | |||
| def deleter(self, fdel: Callable[[Any], None], /) -> property: ... | |||
| @overload | |||
| def __get__(self, instance: None, owner: type, /) -> Self: ... | |||
There was a problem hiding this comment.
The second argument is optional and unused, we should reflect that:
| def __get__(self, instance: None, owner: type, /) -> Self: ... | |
| def __get__(self, instance: None, owner: Unused = None, /) -> Self: ... |
(Unused might need to be imported from _typeshed. Also, we might need to silence mypy's overlap warning.)
There was a problem hiding this comment.
The second argument is optional
I don't think that's true? If None is supplied for the first parameter, it's invalid to leave the second parameter unspecified:
>>> class Foo:
... @property
... def bar(self): return 42
...
>>> b = Foo.__dict__["bar"]
>>> b.__get__(None)
Traceback (most recent call last):
File "<python-input-2>", line 1, in <module>
b.__get__(None)
~~~~~~~~~^^^^^^
TypeError: __get__(None, None) is invalidOnly if I provide the second paramter is it valid to give None for the first parameter:
>>> b.__get__(None, Foo)
<property object at 0x102f3c860>It's true that if you provide an instance as the first parameter, rather than None, then it's okay to omit the second parameter:
>>> b.__get__(Foo())
42but that's already covered by the second overload immediately below?
There was a problem hiding this comment.
I don't think that's true? If
Noneis supplied for the first parameter, it's invalid to leave the second parameter unspecified:
Which means that both the linked pseudo code is wrong and that the C implementation contradicts the specification, but it is what it is ...
There was a problem hiding this comment.
Yeah, I think @sharkdp would be the first to tell you that we've been having "a lot of fun" recently at red-knot trying to figure out which bits of documentation, specification and pseudo-code regarding the descriptor protocol are actually precise descriptions of the real runtime behaviour, and which are just approximations 😆
There was a problem hiding this comment.
Which means that both the linked pseudo code is wrong and that the C implementation contradicts the specification, but it is what it is ...
Right. There is some precedent for modeling __get__ this way in typeshed. For example, see types.FunctionType or builtins.classmethod:
Lines 111 to 114 in 977f300
Lines 160 to 163 in 977f300
For the property descriptor, it's not particularly important, because the owner attribute is not used. But for some descriptors, it is essential that owner is passed in when instance = None. For example, classmethod.__get__ needs the owner argument to create a <bound method Owner.some_class_method of <class 'Owner'>.
srittau
dismissed
their stale review
Actual C implementation is actually incorrect.
Add a new overload for
property.__get__to model the behavior that a property returns itself when accessed on a class object (wheninstanceis set toNone).The relevant section in the pseudo-implementation of
propertycan be found here.Note: This is more of an exploratory change. There is no immediate problem that this solves.