Skip to content
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

Recognize different setter type for properties. #11643

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

esoma
Copy link
Contributor

@esoma esoma commented Nov 29, 2021

I hit #3004 so I took a stab at it, it's not super elegant, but properties are kind of hacky already 😄. I hope it's not the case that maintainers want to de-hackify properties before tackling this issue, but I can understand if that's the case.


These changes should be backwards compatible in that plugins using the current is_property/is_settable_property node attributes will still function as they do now. That is, the set type of those "properties" will be the same as the get's return type. This only affects "true" decorated properties.


This change makes a particular error more verbose:

class Foo:
    @property
    def bar(self) -> int: ...
    @bar.setter
    def bar(self, value: int) -> None: ...
class Foo2(Foo):
    @property
    def bar(self) -> int: ...

In master this emits:

main.py:7: error: Read-only property cannot override read-write property

But with this PR it emits:

main.py:7: error: Read-only property cannot override read-write property
main.py:8: error: Signature of "bar" incompatible with supertype "Foo"
main.py:8: note:      Superclass:
main.py:8: note:          @overload
main.py:8: note:          def bar(self) -> int
main.py:8: note:          @overload
main.py:8: note:          def bar(self, value: int) -> None
main.py:8: note:      Subclass:
main.py:8: note:          def bar(self) -> str

Because the setter is missing. This is consistent with similar errors (if the getter's signature differs the signature error will emit in both branches), so I considered this acceptable.


A note regarding narrowing, since that has some recent discussion in the issue. I'd describe the narrowing behavior as "unchanged" in that mypy will continue to narrow properties. "assymetric" properties (as they have been described where the getter and setter of a property have different types) will narrow if types overlap. This seems (to me) to be the most reasonable way of maintaining the current narrowing behavior. Code is easier than words:

from typing import Union
class Foo:
    @property
    def bar(self) -> Union[int, list]: ...
    @bar.setter
    def bar(self, value: Union[str, int]) -> None: ...

f = Foo()
reveal_type(f.bar) # Union[int, list]
f.bar = '123' # str does not overlap with Union[int, list], so no narrowing occurs
reveal_type(f.bar) # Union[int, list] 
f.bar = 1 # int does not overlaps with Union[int, list], so narrowing occurs
reveal_type(f.bar) # int
f.bar = '123' # str does not overlap with Union[int, list], so narrowing is undone
reveal_type(f.bar) # Union[int, list] 

It may be worth mentioning that the issue mentioned here: #3004 (comment) is not addressed by this PR.

@github-actions

This comment has been minimized.

@esoma

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@esoma
Copy link
Contributor Author

esoma commented Nov 30, 2021

I'm not really sure what's going on with the werkzeug tests/ diffs. I don't seem to be able to reproduce it properly locally. Any ideas @hauntsaninja ?

@hauntsaninja
Copy link
Collaborator

When mypy switched to modular typeshed, Jukka added something that limits the number of errors mypy outputs, so I'm guessing the new "unused type ignore" errors are pushing out pre-existing errors. While presumably nice for users, let me see if there's a way to switch that behaviour off for mypy-primer

@hauntsaninja
Copy link
Collaborator

Changed in hauntsaninja/mypy_primer@5d50d01

@github-actions

This comment has been minimized.

@esoma
Copy link
Contributor Author

esoma commented Dec 5, 2021

This doesn't work when the property definition comes from the cache, working on that. Edit: Fixed.

@github-actions

This comment has been minimized.

@sobolevn
Copy link
Member

Please, take a look at python/typing#985

@lukaszmoroz
Copy link

Any update on this?

@github-actions
Copy link
Contributor

github-actions bot commented Mar 5, 2022

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

python-htmlgen (https://github.com/srittau/python-htmlgen)
+ test_htmlgen/element.py:218: error: Unused "type: ignore" comment
+ test_htmlgen/element.py:224: error: Unused "type: ignore" comment
+ test_htmlgen/element.py:225: error: Unused "type: ignore" comment
+ test_htmlgen/element.py:254: error: Unused "type: ignore" comment

urllib3 (https://github.com/urllib3/urllib3)
+ src/urllib3/connection.py:172: error: Unused "type: ignore" comment

werkzeug (https://github.com/pallets/werkzeug)
+ src/werkzeug/exceptions.py:205: error: Unused "type: ignore" comment
+ src/werkzeug/sansio/response.py:139: error: Unused "type: ignore" comment
+ src/werkzeug/sansio/response.py:151: error: Unused "type: ignore" comment
+ src/werkzeug/wrappers/response.py:740: error: Unused "type: ignore" comment
+ src/werkzeug/test.py:389: error: Unused "type: ignore" comment
+ src/werkzeug/debug/__init__.py:284: error: Incompatible types in assignment (expression has type "None", variable has type "str")  [assignment]

sphinx (https://github.com/sphinx-doc/sphinx)
+ sphinx/util/logging.py:494: error: Unused "type: ignore" comment

zulip (https://github.com/zulip/zulip)
+ zerver/lib/actions.py:4843: error: Unused "type: ignore" comment

porcupine (https://github.com/Akuli/porcupine)
+ porcupine/plugins/pastebin.py:106: error: Unused "type: ignore" comment

pandas (https://github.com/pandas-dev/pandas)
+ pandas/core/arrays/period.py:348: error: Signature of "freq" incompatible with supertype "DatetimeLikeArrayMixin"  [override]
+ pandas/core/arrays/period.py:348: note:      Superclass:
+ pandas/core/arrays/period.py:348: note:          @overload
+ pandas/core/arrays/period.py:348: note:          def freq(self) -> Any
+ pandas/core/arrays/period.py:348: note:          @overload
+ pandas/core/arrays/period.py:348: note:          def freq(self, value: Any) -> Any
+ pandas/core/arrays/period.py:348: note:      Subclass:
+ pandas/core/arrays/period.py:348: note:          def freq(self) -> BaseOffset

aiohttp (https://github.com/aio-libs/aiohttp)
+ aiohttp/web_fileresponse.py:118: error: Unused "type: ignore" comment
+ aiohttp/web_fileresponse.py:119: error: Unused "type: ignore" comment
+ aiohttp/web_fileresponse.py:263: error: Unused "type: ignore" comment
+ aiohttp/web_fileresponse.py:264: error: Unused "type: ignore" comment

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.

4 participants