Pyright confused with property setter typing

[GBeauregard]

Hi, another report based on going through the rich codebase. I'm not convinced by pyright's warning here.

https://github.com/willmcgugan/rich/blob/256697915d9676b7d5cb6abd5559132ff5f92fdf/rich/table.py#L325

  /home/gregory/Documents/open/rich/rich/table.py:323:32 - error: Property setter value type is not assignable to the getter return type
    Type "PaddingDimensions" cannot be assigned to type "Tuple[int, int, int, int]"
      "int" is incompatible with "Tuple[int, int, int, int]"
      Tuple size mismatch; expected 4 but received 1
      Tuple size mismatch; expected 4 but received 2 (reportPropertyTypeMismatch)

I'm not sure what's going on here; the typing all seems correct. Does pyright assume the error based on the type of the argument alone? Is this intended?

Someone seems to have run into a more self contained example online: https://stackoverflow.com/questions/69111948/property-setter-value-type-is-not-assignable-to-the-getter-return-type-str-is

I'm headed out but I'll try to make a minimal example when I get back (but at first glance the stackoverflow example seems ok)

[erictraut]

The purpose of the reportPropertyTypeMismatch diagnostic check is to flag situations where your setter accepts a different type than the type returned by your getter. This asymmetry with property getters/setters is considered an undesirable coding practice by many, and this diagnostic check reports this condition. If you don't mind this asymmetry and want to allow it within your code base, you can disable this diagnostic check for your project. Or you can disable it for a specific file by adding the comment # pyright: reportPropertyTypeMismatch=false.

[GBeauregard]

Yeah okay, on reflection I think that's a reasonable lint and that transforming assignment should be in a separate method.

I think it's a bit surprising to have this type of warning from a type checker, but I don't know what other kind of linter could throw the error so it's probably fine.

Sorry for the noise!

[GBeauregard]

@erictraut I think maybe the error message could be reworded a bit? It read to me more like it's a runtime assignment mismatch and not a style complaint which caused my initial confusion. I don't think the existing wording is wrong per se, but maybe it could be signposted better that it's a style error (maybe verbs like "should" in combination with some sort of label indicating a style complaint?)

Thanks for all your work and prompt responses always

[GBeauregard]

I've been working on this one a lot the last two days, and I wanted to collect the information here for posterity and to maybe see if you had any thoughts on the interaction with descriptors.

1. setter/getter asymmetry actually doesn't type-check for mypy users of a library even though it type-checks on the library side: https://mypy-play.net/?mypy=latest&python=3.10&gist=1cb10416c97745cd4b7bc9dcc458109e. There is consensus this is a bug, but the issue is stale What to do about setters of a different type than their property? python/mypy#3004. I suspect there are a number of mypy-CI typed libraries in the Python ecosystem that aren't aware they're breaking downstream mypy users. I think it's definitely correct pyright doesn't throw an error here on the user side.
2. Both the mypy user-side issue and the pyright library-side lint can be worked around by using descriptors: https://gist.github.com/adamchainz/412d6bcad3232c6edaf69f9e4b741713 as suggested here https://twitter.com/AdamChainz/status/1450045498589646851. Since properties are descriptors under the hood, should pyright's lint maybe be demoted to running in strict mode? I'm not sure. Maybe not. I don't really think it should be gotten rid of.
3. Related: A few days ago I saw you commented on the reasoning for not allowing properties to override attributes Mypy disallows overriding an attribute with a property python/mypy#4125 -> looking into descriptors for the current issue has made me curious: what about descriptors there too? https://mypy-play.net/?mypy=latest&python=3.10&gist=74f076e464ae7ab9f3fdce9fd70b757f As it turns out, both mypy and pyright are fine with descriptors there. Shouldn't overriding attributes with properties be allowed in order to be consistent with descriptors? I find the argument in the thread really unconvincing.

I thought the interaction with descriptors was interesting even if you don't want to change anything, cheers.

[erictraut]

1. Yeah, I agree that consumers of an asymmetric interface shouldn't see an error in this case.

2. I would recommend against that solution. You're still employing the asymmetric anti-pattern, which is not a good idea for a descriptor. If you want asymmetry, I would argue that you should define separate accessor methods with names that make it clear that the contract does not use simple getter/setter semantics.

3. This isn't an inconsistency because YIntProp derives from int. If you remove the int base class from YIntProp, you will receive the same error as you would with a property.

[GBeauregard]

2. To be clear I'm totally on board with you here (removing the asymmetry is what I suggested in a PR: Many Miscellaneous Typing Fixes Textualize/rich#1604). The problem trying to be solved by rich is not primarily the pyright one (it can be ignored at the library-level), but the mypy one of avoiding breaking existing API while stopping the breakage of mypy users because of 1). I believe this was the rich maintainer's motivation for seeking the workaround.

3. That's (sorta?) fair I guess; deriving from int was a bit of a hack, but I'm not totally convinced. Probably because I wasn't convinced before anyway and it sways my judgement a bit.

Thanks for your thoughts as always.