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

Allow overriding attribute with a settable property #13475

Merged
merged 6 commits into from
Aug 22, 2022

Conversation

ilevkivskyi
Copy link
Member

@ilevkivskyi ilevkivskyi commented Aug 21, 2022

Fixes #4125

Previously the code compared the original signatures for properties. Now we compare just the return types, similar to how we do it in checkmember.py.

Note that we still only allow invariant overrides, which is stricter that for regular variables that where we allow (unsafe) covariance.

@github-actions

This comment has been minimized.

@ilevkivskyi
Copy link
Member Author

Unfortunately, the whole new bunch in homeassistant looks real. They have an wrongly typed custom property decorator, that just used to work by accident. (Because previously @property decorator was effectively ignored in override checks). cc @OttoWinter @balloob so you can prepare for when this is released.

The rest will be hopefully fixed with the latest commit (I adjusted the replacement error to have an old line number, error code, and skip it if supertype is Any).

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

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

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

spark (https://github.com/apache/spark)
+ python/pyspark/ml/classification.py:877: error: Signature of "summary" incompatible with supertype "HasTrainingSummary"  [override]
+ python/pyspark/ml/classification.py:3323: error: Signature of "summary" incompatible with supertype "HasTrainingSummary"  [override]

ibis (https://github.com/ibis-project/ibis)
- ibis/common/dispatch.py:107: error: Signature of "__doc__" incompatible with supertype "object"
+ ibis/common/dispatch.py:107: error: Cannot override writeable attribute with read-only property
+ ibis/backends/datafusion/__init__.py:79: error: Signature of "current_database" incompatible with supertype "BaseBackend"
+ ibis/backends/duckdb/__init__.py:85: error: Signature of "current_database" incompatible with supertype "BaseAlchemyBackend"
+ ibis/backends/duckdb/__init__.py:85: error: Signature of "current_database" incompatible with supertype "BaseBackend"

packaging (https://github.com/pypa/packaging)
+ packaging/specifiers.py:242: error: Signature of "prereleases" incompatible with supertype "BaseSpecifier"  [override]

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

discord.py (https://github.com/Rapptz/discord.py)
- discord/asset.py:346: error: Signature of "url" incompatible with supertype "AssetMixin"
+ discord/asset.py:346: error: Cannot override writeable attribute with read-only property
- discord/partial_emoji.py:227: error: Signature of "url" incompatible with supertype "AssetMixin"
+ discord/partial_emoji.py:227: error: Cannot override writeable attribute with read-only property
+ discord/invite.py:507: error: Cannot override writeable attribute with read-only property
- discord/emoji.py:160: error: Signature of "url" incompatible with supertype "AssetMixin"
+ discord/emoji.py:160: error: Cannot override writeable attribute with read-only property
+ discord/channel.py:213: error: Cannot override writeable attribute with read-only property
+ discord/channel.py:1066: error: Cannot override writeable attribute with read-only property
+ discord/channel.py:1539: error: Cannot override writeable attribute with read-only property
+ discord/channel.py:1787: error: Cannot override writeable attribute with read-only property
+ discord/channel.py:2068: error: Cannot override writeable attribute with read-only property

manticore (https://github.com/trailofbits/manticore)
+ manticore/platforms/linux.py:359: error: Signature of "closed" incompatible with supertype "FdLike"

pandas (https://github.com/pandas-dev/pandas)
+ pandas/core/series.py:3574: error: Unused "type: ignore" comment
+ pandas/core/series.py:3832: error: Unused "type: ignore" comment
+ pandas/core/series.py:5067: error: Unused "type: ignore" comment
+ pandas/core/series.py:5141: error: Unused "type: ignore" comment
+ pandas/core/series.py:5289: error: Unused "type: ignore" comment
+ pandas/core/series.py:5371: error: Unused "type: ignore" comment
+ pandas/core/series.py:5984: error: Unused "type: ignore" comment
+ pandas/core/series.py:6028: error: Unused "type: ignore" comment
+ pandas/core/series.py:6121: error: Unused "type: ignore" comment
+ pandas/core/series.py:6187: error: Unused "type: ignore" comment
+ pandas/core/frame.py:5244: error: Unused "type: ignore" comment
+ pandas/core/frame.py:5618: error: Unused "type: ignore" comment
+ pandas/core/frame.py:5711: error: Unused "type: ignore" comment
+ pandas/core/frame.py:6886: error: Unused "type: ignore" comment
+ pandas/core/frame.py:7015: error: Unused "type: ignore" comment
+ pandas/core/frame.py:11826: error: Unused "type: ignore" comment
+ pandas/core/frame.py:11870: error: Unused "type: ignore" comment
+ pandas/core/frame.py:11963: error: Unused "type: ignore" comment
+ pandas/core/frame.py:12029: error: Unused "type: ignore" comment

core (https://github.com/home-assistant/core)
+ homeassistant/components/recorder/models.py:173: error: Unused "type: ignore" comment
+ homeassistant/components/recorder/models.py:185: error: Unused "type: ignore" comment
+ homeassistant/components/recorder/models.py:200: error: Unused "type: ignore" comment
+ homeassistant/components/esphome/switch.py:44: error: Signature of "is_on" incompatible with supertype "ToggleEntity"  [override]
+ homeassistant/components/esphome/sensor.py:84: error: Signature of "native_value" incompatible with supertype "SensorEntity"  [override]
+ homeassistant/components/esphome/sensor.py:128: error: Signature of "native_value" incompatible with supertype "SensorEntity"  [override]
+ homeassistant/components/esphome/select.py:44: error: Signature of "current_option" incompatible with supertype "SelectEntity"  [override]
+ homeassistant/components/esphome/number.py:82: error: Signature of "native_value" incompatible with supertype "NumberEntity"  [override]
+ homeassistant/components/esphome/lock.py:57: error: Signature of "is_locked" incompatible with supertype "LockEntity"  [override]
+ homeassistant/components/esphome/lock.py:62: error: Signature of "is_locking" incompatible with supertype "LockEntity"  [override]
+ homeassistant/components/esphome/lock.py:67: error: Signature of "is_unlocking" incompatible with supertype "LockEntity"  [override]
+ homeassistant/components/esphome/lock.py:72: error: Signature of "is_jammed" incompatible with supertype "LockEntity"  [override]
+ homeassistant/components/esphome/light.py:138: error: Signature of "is_on" incompatible with supertype "ToggleEntity"  [override]
+ homeassistant/components/esphome/light.py:267: error: Signature of "brightness" incompatible with supertype "LightEntity"  [override]
+ homeassistant/components/esphome/light.py:272: error: Signature of "color_mode" incompatible with supertype "LightEntity"  [override]
+ homeassistant/components/esphome/light.py:282: error: Signature of "rgb_color" incompatible with supertype "LightEntity"  [override]
+ homeassistant/components/esphome/light.py:298: error: Signature of "rgbw_color" incompatible with supertype "LightEntity"  [override]
+ homeassistant/components/esphome/light.py:305: error: Signature of "rgbww_color" incompatible with supertype "LightEntity"  [override]
+ homeassistant/components/esphome/light.py:337: error: Signature of "effect" incompatible with supertype "LightEntity"  [override]
+ homeassistant/components/esphome/fan.py:120: error: Signature of "is_on" incompatible with supertype "FanEntity"  [override]
+ homeassistant/components/esphome/fan.py:120: error: Signature of "is_on" incompatible with supertype "ToggleEntity"  [override]
+ homeassistant/components/esphome/fan.py:125: error: Signature of "percentage" incompatible with supertype "FanEntity"  [override]
+ homeassistant/components/esphome/fan.py:147: error: Signature of "oscillating" incompatible with supertype "FanEntity"  [override]
+ homeassistant/components/esphome/fan.py:154: error: Signature of "current_direction" incompatible with supertype "FanEntity"  [override]
+ homeassistant/components/esphome/cover.py:73: error: Signature of "is_closed" incompatible with supertype "CoverEntity"  [override]
+ homeassistant/components/esphome/cover.py:79: error: Signature of "is_opening" incompatible with supertype "CoverEntity"  [override]
+ homeassistant/components/esphome/cover.py:84: error: Signature of "is_closing" incompatible with supertype "CoverEntity"  [override]
+ homeassistant/components/esphome/cover.py:89: error: Signature of "current_cover_position" incompatible with supertype "CoverEntity"  [override]
+ homeassistant/components/esphome/cover.py:96: error: Signature of "current_cover_tilt_position" incompatible with supertype "CoverEntity"  [override]
+ homeassistant/components/esphome/climate.py:223: error: Signature of "hvac_mode" incompatible with supertype "ClimateEntity"  [override]
+ homeassistant/components/esphome/climate.py:228: error: Signature of "hvac_action" incompatible with supertype "ClimateEntity"  [override]
+ homeassistant/components/esphome/climate.py:236: error: Signature of "fan_mode" incompatible with supertype "ClimateEntity"  [override]
+ homeassistant/components/esphome/climate.py:243: error: Signature of "preset_mode" incompatible with supertype "ClimateEntity"  [override]
+ homeassistant/components/esphome/climate.py:250: error: Signature of "swing_mode" incompatible with supertype "ClimateEntity"  [override]
+ homeassistant/components/esphome/climate.py:255: error: Signature of "current_temperature" incompatible with supertype "ClimateEntity"  [override]
+ homeassistant/components/esphome/climate.py:260: error: Signature of "target_temperature" incompatible with supertype "ClimateEntity"  [override]
+ homeassistant/components/esphome/climate.py:265: error: Signature of "target_temperature_low" incompatible with supertype "ClimateEntity"  [override]
+ homeassistant/components/esphome/climate.py:270: error: Signature of "target_temperature_high" incompatible with supertype "ClimateEntity"  [override]
+ homeassistant/components/esphome/media_player.py:74: error: Signature of "state" incompatible with supertype "MediaPlayerEntity"  [override]
+ homeassistant/components/esphome/media_player.py:74: error: Signature of "state" incompatible with supertype "Entity"  [override]
+ homeassistant/components/esphome/media_player.py:79: error: Signature of "is_volume_muted" incompatible with supertype "MediaPlayerEntity"  [override]
+ homeassistant/components/esphome/media_player.py:84: error: Signature of "volume_level" incompatible with supertype "MediaPlayerEntity"  [override]

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

I wonder if it's safe to have setter be invariant, but allow getter to be covariant. I'm looking at the example from packaging:

import typing

class X:
    @property
    def x(self) -> typing.Optional[bool]: ...

    @x.setter
    def x(self, value: bool) -> None: ...

class Y(X):
    @property
    def x(self) -> bool: ...

    @x.setter
    def x(self, value: bool) -> None: ...

@sobolevn
Copy link
Member

I wonder if it's safe to have setter be invariant, but allow getter to be covariant.

I think it is! Return type of getter can be the same or narrower, but argument of setter can be the same or wider. Just like regular methods.

@ilevkivskyi
Copy link
Member Author

Yes, this is safe, but it is a subject of a separate PR. It is non-trivial to enable, since mypy ignores the setter type, e.g. with that example x: X, x.x = None passes mypy. See #3004 for (much) longer discussion on this.

@Rocamonde
Copy link

Has this been released to pypi? Is there a way to track what merged issues have been released into what version?

@AlexWaygood
Copy link
Member

AlexWaygood commented Oct 11, 2022

Has this been released to pypi?

No, this didn't make it into 0.981 or 0.982. It will be in 0.99x.

Is there a way to track what merged issues have been released into what version?

If you go to the commit page here, you can see that the commit is included in the master branch, but has not been included in any release tags yet: 40dd719

Whereas if you look at bf143d9 , a few commits back, you can see that it's included in the 0.981 and 0.982 tags, as well as the master branch:

image

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.

Mypy disallows overriding an attribute with a property
5 participants