Skip to content

Decorators on top of @property are not supported #14461

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
analog-cbarber opened this issue Jan 16, 2023 · 19 comments · Fixed by #16571
Closed

Decorators on top of @property are not supported #14461

analog-cbarber opened this issue Jan 16, 2023 · 19 comments · Fixed by #16571
Labels
bug mypy got something wrong topic-descriptors Properties, class vs. instance attributes

Comments

@analog-cbarber
Copy link
Contributor

We have decorators that decorate the property not the getter function,
and which must be declared BEFORE the property decorator:

@mydecorator
@property
def foo(self):
    pass

Mypy does not support this and produces the error: Decorators on top of @property are not supported

It is fine if mypy does not support this yet, but why do we have to suffer this utterly useless warning? I shouldn't have to pollute my code with comments to shut up mypy.

Instead, mypy should either simply ignore such decorators or should at least provide an option to let the user globally
shut off such "not implemented" errors.

And if you are going to produce errors like this, at least give them a "not-implemented" category instead of just using "misc".

This is the unfixed part of bug #1362.

@analog-cbarber analog-cbarber added the bug mypy got something wrong label Jan 16, 2023
@lokesh1199
Copy link

Any workarounds for this issue. :(

@analog-cbarber
Copy link
Contributor Author

The only workaround is to add a type ignore comment:

@mydecorator # type: ignore
@property
def foo(self):
    pass

@analog-cbarber
Copy link
Contributor Author

It would be really easy to simply remove this error, but I have a feeling that such a PR would not be accepted.

Would be nice to hear from a maintainer what they think the correct option would be.

@lokesh1199
Copy link

lokesh1199 commented Jul 25, 2023

In my case I want to have a class property like below. When accessing the property just by using class, mypy infers it as callable. But mypy infers it correctly when the accessing the same class property through a instance.

class Test:
    _a = 10
    _b = 20

    def __init__(self) -> None:
        self._c = 20

    @classmethod
    @property
    def a(cls) -> int:
        return cls._a
    
    @classmethod
    @property
    def b(cls) -> int:
        return cls._b
    
    @property
    def c(self) -> int:
        return self._c


reveal_type(Test.a)
reveal_type(Test().a)
reveal_type(Test().b)
reveal_type(Test().c)

Output

test.py:23: note: Revealed type is "def () -> builtins.int"
test.py:24: note: Revealed type is "builtins.int"
test.py:25: note: Revealed type is "builtins.int"
test.py:26: note: Revealed type is "builtins.int"
Success: no issues found in 1 source file

Same output with putting type: ignore next to classmethod decorator

@analog-cbarber
Copy link
Contributor Author

Note that classmethod can only wrap properties in python 3.9 and 3.10, not versions before or after that, so you probably should avoid that if you can.

@phillies
Copy link

phillies commented Aug 4, 2023

Having a decorated property is actually the official way to create computed fields for pydantic. So I guess this is a good argument to support this syntax:
https://docs.pydantic.dev/latest/usage/computed_fields/

@analog-cbarber
Copy link
Contributor Author

I have yet to hear anyone explain why the current error is any way useful to anyone.

@TiansuYu
Copy link

TiansuYu commented Oct 1, 2023

Note that @computed_field is introduced in PydanticV2 that specifically asking adding this on top of @property decorator. It is waste in time at 2023 to check this.

@analog-cbarber
Copy link
Contributor Author

Still waiting to hear from a maintainer about removing this....

@analog-cbarber
Copy link
Contributor Author

With 1.7, the behavior is even worse. Now it still complains about not supporting the decorator but then also goes ahead and processes the decorator incorrectly.

Given this example:

import functools
from typing import Optional, Union, Callable, overload, Any

@overload
def decorate(prop: property) -> property:
    pass

@overload
def decorate() -> Callable[[property],property]:
    pass

def decorate(prop: Optional[property] = None) -> Any:
    if prop:
        return prop
    return functools.partial(decorate)


class MyClass:
    @decorate() 
    @property
    def foo(self) -> str:
        return "foo"

print(MyClass().foo)

I get:

src/foo.py:19: error: Decorators on top of @property are not supported  [misc]
src/foo.py:19: error: Argument 1 has incompatible type "Callable[[MyClass], str]"; expected "property"  [arg-type]
src/foo.py:24: error: "MyClass" not callable  [operator]
Found 3 errors in 1 file (checked 1 source file)

So it appears that it thinks the decorator is getting passed the getter function instead of the property object.

If I add a # type: ignore comment on the @decorator line, then I still get the bogus "MyClass" not callable error.

I can make mypy happy if I change the decorator signature to just say that it always returns a property:

@overload
def decorate(prop: property) -> property:
    pass

@overload
def decorate() -> property:
    pass

But that is obviously incorrect, so I basically have to change it to Any.

On top of that, the decorator is defined in a different library, and am not sure if I can word around the problem without doing a new release of the other library.

@TiansuYu
Copy link

I have decided to move away from mypy given the amount of false positives I have to silence in repo.

@mikkelam
Copy link

I have decided to move away from mypy given the amount of false positives I have to silence in repo.

instead of complaining, perhaps you should consider tapping the sponsor button. I believe it's also possible for you to create a PR to fix it!

@analog-cbarber
Copy link
Contributor Author

I can't speak for the commenter, but as far as submitting a PR, I volunteered to do that but only if a maintainer tells me my approach is ok (i.e. get rid of the pointless not-implemented error). I have way too much experience submitting PRs on projects that got rejected because the maintainers simply didn't want to do it that way or were too busy to even look at the PR. There are already 197 open pull requests on this project. I don't want to waste hours of my time only to add another zombie PR.

@mikkelam
Copy link

I can't speak for the commenter, but as far as submitting a PR, I volunteered to do that but only if a maintainer tells me my approach is ok (i.e. get rid of the pointless not-implemented error). I have way too much experience submitting PRs on projects that got rejected because the maintainers simply didn't want to do it that way or were too busy to even look at the PR. There are already 197 open pull requests on this project. I don't want to waste hours of my time only to add another zombie PR.

Then don't. The maintainers dont owe you anything. You're also free to fork the project and drive your own implementation

@analog-cbarber
Copy link
Contributor Author

Indeed, the maintainers owe us nothing but by the same token get nothing if they aren't willing or able to interact with potential contributors. I get it, the maintainers don't considers this to be an important enough issue to even waste a comment on. But that suggests they also aren't going to have time to review a pull request either. The fact that there are scores of old open PRs is not encouraging.

Don't get me wrong. I think this is a highly useful project. But just because it is useful doesn't mean I or anyone else should spend hours of their spare time working on a PR that no one will look at.

@JelleZijlstra
Copy link
Member

As a maintainer, I do think this issue should be fixed. However, I expect it's not easy to fix, and I'd be unlikely to find time to review a PR touching this part of mypy. Nevertheless, the first step towards a fix must be a PR; one of the other maintainers would hopefully be able to review it.

@analog-cbarber
Copy link
Contributor Author

That is the problem right there. If the nature of the fix is obvious a PR makes sense. But in this case I was suggesting that the not implemented error benefits no one and should be removed. That is the kind of thing that I would like to get at least a nod before I spend time on.

I have other open source projects I either maintain or contribute to in my limited spare time. I just want to spend that time doing work that will not go to waste.

And I do not want to come across as angry about this. I am not. I understand that people are busy and the project has other priorities.

@JelleZijlstra
Copy link
Member

I'd accept a PR adding a specific error code for this error, so that users can shut off the error if they want to. That should be an easy change.

It's important that we do produce an error here in strict mode, because if the decorator does not preserve the type of the property, we may infer incorrect types.

@AlexWaygood AlexWaygood added the topic-descriptors Properties, class vs. instance attributes label Nov 25, 2023
analog-cbarber added a commit to analog-cbarber/mypy that referenced this issue Nov 26, 2023
…on#14461)

Using a decorator before a @Property now results in
the narrower `prop-decorator` code, which is a subcode
of `misc` for backward compatibility.
@analog-cbarber
Copy link
Contributor Author

Ok. I submitted a PR.

JelleZijlstra pushed a commit that referenced this issue May 18, 2024
…) (#16571)

Using a decorator before a @Property now results in the narrower
`prop-decorator` code, which is a subcode of `misc` for backward
compatibility.

I would have preferred to add a more general Unsupported error code and
have this be a subcode of that, but this has to be a subcode of misc for
backward compatibility.

Fixes #14461
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong topic-descriptors Properties, class vs. instance attributes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants