Skip to content

Update property.fget, property.fset, and property.fdel #7520

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

Closed
wants to merge 1 commit into from

Conversation

jpy-git
Copy link
Contributor

@jpy-git jpy-git commented Mar 21, 2022

Closes #7516.

class A:
    def __init__(self, foo: int) -> None:
        self._foo = foo

    @property
    def foo(self) -> int:
        return self._foo
    
    @foo.setter
    def foo(self, foo: int) -> None:
        self._foo = foo
    
    @foo.deleter
    def foo(self) -> None:
        del self._foo

a = A(1)
print(A.foo.fget(a))

A.foo.fset(a, 2)
print(a.foo)

A.foo.fdel(a)

These property methods take property type as first arg and then the desired instance.

@github-actions

This comment has been minimized.

@jpy-git
Copy link
Contributor Author

jpy-git commented Mar 21, 2022

On seeing that primer diff, maybe typeshed is right and this is a pyright issue?

@JelleZijlstra
Copy link
Member

JelleZijlstra commented Mar 21, 2022

Mypy and pyright disagree here. Here's a minimal example:

from typing import Callable

class X:
    f: Callable[[], int]
    g: Callable[["X"], int]

reveal_type(X().f())
reveal_type(X().g())

Pyright complains about the second call (Expected 1 more positional argument), mypy about the first one (main.py:8: error: Attribute function "f" with type "Callable[[], int]" does not accept self argument).

I feel like pyright is right here, because the behavior where self gets implicitly bound on instances is specific to functions, not all callables. But the type system sometimes conflates functions with callables, so I can see where mypy is coming from.

In either case, the discrepancy is unfortunate because it means we can't write typeshed stubs that satisfy both type checkers. Perhaps we should raise a discussion at the github.com/python/typing repo, come to a consensus, and update one type checker or the other.

@Akuli
Copy link
Collaborator

Akuli commented Mar 21, 2022

I find mypy's behavior weird, because not all callables are descriptors. In #4484, I used this workaround for it:

class _Foo(Protocol):
    def __call__(self) -> int: ...

class X:
    f: _Foo

I believe this works consistently in mypy and pyright, but I haven't checked.

@jpy-git
Copy link
Contributor Author

jpy-git commented Mar 21, 2022

So @Akuli's workaround seems to fix @JelleZijlstra's minimal example. However, if I try and make the same changes to property I still get errors.

For the code in the linked issue:

class A:
    @property
    def foo(self):
        return 'a'

A.foo.fget(A())

mypy: "Callable[[A], Any]" has no attribute "fget"
pyright: Expected 0 positional arguments

Seems like mypy can't infer that A.foo should have property type and pyright has a similar issue as discussed above (but doesn't seem to be helped by the workaround)

Will make an issue in the typing repo for discussion and close this PR for now

@AlexWaygood
Copy link
Member

Let's see what mypy_primer says about this now that mypy 0.981 is released.

@AlexWaygood AlexWaygood reopened this Sep 28, 2022
@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

spark (https://github.com/apache/spark)
+ python/pyspark/pandas/frame.py:13489: error: Too few arguments  [call-arg]
+ python/pyspark/pandas/frame.py:13489: note: "fget" is considered instance variable, to make it class variable use ClassVar[...]
+ python/pyspark/pandas/frame.py:13489: error: Argument 1 has incompatible type "DataFrame[T]"; expected "property"  [arg-type]
+ python/pyspark/pandas/series.py:7264: error: Too few arguments  [call-arg]
+ python/pyspark/pandas/series.py:7264: note: "fget" is considered instance variable, to make it class variable use ClassVar[...]
+ python/pyspark/pandas/series.py:7264: error: Argument 1 has incompatible type "Series[T]"; expected "property"  [arg-type]
+ python/pyspark/pandas/groupby.py:3738: error: Too few arguments  [call-arg]
+ python/pyspark/pandas/groupby.py:3738: note: "fget" is considered instance variable, to make it class variable use ClassVar[...]
+ python/pyspark/pandas/groupby.py:3738: error: Argument 1 has incompatible type "DataFrameGroupBy"; expected "property"  [arg-type]
+ python/pyspark/pandas/groupby.py:3923: error: Too few arguments  [call-arg]
+ python/pyspark/pandas/groupby.py:3923: note: "fget" is considered instance variable, to make it class variable use ClassVar[...]
+ python/pyspark/pandas/groupby.py:3923: error: Argument 1 has incompatible type "SeriesGroupBy"; expected "property"  [arg-type]
+ python/pyspark/pandas/indexes/base.py:2672: error: Too few arguments  [call-arg]
+ python/pyspark/pandas/indexes/base.py:2672: note: "fget" is considered instance variable, to make it class variable use ClassVar[...]
+ python/pyspark/pandas/indexes/base.py:2672: error: Argument 1 has incompatible type "Index"; expected "property"  [arg-type]
+ python/pyspark/pandas/indexes/timedelta.py:128: error: Too few arguments  [call-arg]
+ python/pyspark/pandas/indexes/timedelta.py:128: note: "fget" is considered instance variable, to make it class variable use ClassVar[...]
+ python/pyspark/pandas/indexes/timedelta.py:128: error: Argument 1 has incompatible type "TimedeltaIndex"; expected "property"  [arg-type]
+ python/pyspark/pandas/indexes/multi.py:997: error: Too few arguments  [call-arg]
+ python/pyspark/pandas/indexes/multi.py:997: note: "fget" is considered instance variable, to make it class variable use ClassVar[...]
+ python/pyspark/pandas/indexes/multi.py:997: error: Argument 1 has incompatible type "MultiIndex"; expected "property"  [arg-type]
+ python/pyspark/pandas/indexes/datetimes.py:141: error: Too few arguments  [call-arg]
+ python/pyspark/pandas/indexes/datetimes.py:141: note: "fget" is considered instance variable, to make it class variable use ClassVar[...]
+ python/pyspark/pandas/indexes/datetimes.py:141: error: Argument 1 has incompatible type "DatetimeIndex"; expected "property"  [arg-type]
+ python/pyspark/pandas/window.py:205: error: Too few arguments  [call-arg]
+ python/pyspark/pandas/window.py:205: note: "fget" is considered instance variable, to make it class variable use ClassVar[...]
+ python/pyspark/pandas/window.py:205: error: Argument 1 has incompatible type "Rolling[FrameLike]"; expected "property"  [arg-type]
+ python/pyspark/pandas/window.py:900: error: Too few arguments  [call-arg]
+ python/pyspark/pandas/window.py:900: note: "fget" is considered instance variable, to make it class variable use ClassVar[...]
+ python/pyspark/pandas/window.py:900: error: Argument 1 has incompatible type "RollingGroupby[FrameLike]"; expected "property"  [arg-type]
+ python/pyspark/pandas/window.py:1438: error: Too few arguments  [call-arg]
+ python/pyspark/pandas/window.py:1438: note: "fget" is considered instance variable, to make it class variable use ClassVar[...]
+ python/pyspark/pandas/window.py:1438: error: Argument 1 has incompatible type "Expanding[FrameLike]"; expected "property"  [arg-type]
+ python/pyspark/pandas/window.py:1948: error: Too few arguments  [call-arg]
+ python/pyspark/pandas/window.py:1948: note: "fget" is considered instance variable, to make it class variable use ClassVar[...]
+ python/pyspark/pandas/window.py:1948: error: Argument 1 has incompatible type "ExpandingGroupby[FrameLike]"; expected "property"  [arg-type]
+ python/pyspark/pandas/window.py:2492: error: Too few arguments  [call-arg]
+ python/pyspark/pandas/window.py:2492: note: "fget" is considered instance variable, to make it class variable use ClassVar[...]
+ python/pyspark/pandas/window.py:2492: error: Argument 1 has incompatible type "ExponentialMoving[FrameLike]"; expected "property"  [arg-type]
+ python/pyspark/pandas/window.py:2593: error: Too few arguments  [call-arg]
+ python/pyspark/pandas/window.py:2593: note: "fget" is considered instance variable, to make it class variable use ClassVar[...]
+ python/pyspark/pandas/window.py:2593: error: Argument 1 has incompatible type "ExponentialMovingGroupby[FrameLike]"; expected "property"  [arg-type]
+ python/pyspark/pandas/resample.py:714: error: Too few arguments  [call-arg]
+ python/pyspark/pandas/resample.py:714: note: "fget" is considered instance variable, to make it class variable use ClassVar[...]
+ python/pyspark/pandas/resample.py:714: error: Argument 1 has incompatible type "DataFrameResampler"; expected "property"  [arg-type]
+ python/pyspark/pandas/resample.py:746: error: Too few arguments  [call-arg]
+ python/pyspark/pandas/resample.py:746: note: "fget" is considered instance variable, to make it class variable use ClassVar[...]
+ python/pyspark/pandas/resample.py:746: error: Argument 1 has incompatible type "SeriesResampler"; expected "property"  [arg-type]

@AlexWaygood
Copy link
Member

Hmm, that still looks pretty bad. And, looking at this issue again, I think we might have misdiagnosed a little bit here.

Following the release of mypy 0.981, mypy and pyright now agree on the snippet @JelleZijlstra posted in #7520 (comment): https://mypy-play.net/?mypy=latest&python=3.10&gist=276f0399f36c37f6e291f43b277cec65.

And looking at the stubs again, I think maybe typeshed is correct at the moment. Maybe this is just an issue with pyright's special-casing here.

@AlexWaygood
Copy link
Member

And indeed, I can no longer reproduce the error in #7516 using VSCode. So, it seems like this was a pyright problem, and that it's now fixed 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong type for fget of property
4 participants