Re-widen custom properties after narrowing

[ilevkivskyi]

Fixes #10399

This is another smaller cleanup for #7724. The current logic as documented is IMO correct, for attributes (either properties or custom descriptors) with setter type different from getter type, we narrow the attribute type in an assignment if:

• The attribute is "normalizing", i.e. getter type is a subtype of setter type (e.g. Sequence[Employee] is normalized to tuple[Employee, ...])
• The given r.h.s. type in the assignment is a subtype of getter type (and thus transitively the setter as well), e.g. tuple[Manager, ...] vs tuple[Employee, ...] in the example above.

The problem was that this logic was implemented too literally, as a result assignments that didn't satisfy these two rules were simply ignored (thus making previous narrowing incorrectly "sticky"). In fact, we also need to re-widen previously narrowed types whenever second condition is not satisfied.

(I also decided to rename one variable name to make it more obvious.)

[sterliakov]

Only now I finally realized that mypy narrows types of asymmetric properties. Is such a pattern really that popular in wild? In my head any property with a setter means "please do not try to narrow that from assignment" in bold, toxic yellow letters...

[JukkaL]

I believe that the vast majority of setters probably just assign the value as is (with some additional operations, such as checking validity), and the getter will return it. There's no guarantee that this is the case -- but narrowing types of regular attributes is already generally unsafe but still supported, since not having it would be quite tedious.

[sterliakov]

@JukkaL I agree that's true iff the property is symmetric. If it is not, narrowing is too optimistic IMO, I'd prefer to see no narrowing by assignment at all for properties with different getter and setter types. I can easily envision the setter implementation (omitted in this test) creating a C instance and caching it, and narrowing to D would be incorrect.

I think this is quite different from regular attribute narrowing: there is some common sense expectation that attributes are "good citizens", but it does not apply to properties and especially asymmetric ones.

[sterliakov]

To be focused, I think the following implementation is idiomatic, and narrowing by assignment should not happen here:

from collections.abc import Iterable

class SuperList(list[int]):
    def non_list_method(self): ...

class Demo:
    _foo: list[int]
    
    @property
    def foo(self) -> list[int]:
        return self._foo
    
    @foo.setter
    def foo(self, val: Iterable[int]) -> None:
        self._foo = list(val)


d = Demo()
reveal_type(d.foo)  # N: Revealed type is "builtins.list[builtins.int]"
d.foo = SuperList([])
# Huh? Looks like an easy mistake to make, and `mypy` now supports it!
reveal_type(d.foo)  # N: Revealed type is "__main__.SuperList"
d.foo.non_list_method()  # AttributeError, of course

[ilevkivskyi]

@sterliakov

I think the following implementation is idiomatic

Yeah, I am writing subclasses of list every day :-) Seriously however:

• Do you think we didn't consider such examples?
• This is an intentional decision (and is already a compromise), because some people asked for this.
• It has been here for ~6-7 years (it was added for properties and custom descriptors at different time), and AFAIK no one complained about the unsafety.

[sterliakov]

Since you're asking...

I dunno whether this was considered and intentional - sometimes things like this may really get missed. Sometimes decisions made in past are challenged and eventually reconsidered. Since such decisions are almost always undocumented (or difficult to trace at least), I can't say for sure. And I know you were working on these parts, so you're the right person to raise my concerns to:) And I don't think 6-7 years timeline applies to properties - you only finished asymmetric properties support last month or so, the implementation is essentially newborn.

To me doing such narrowing for asymmetric properties is a mistake - I don't think this would cause a lot of false positives in real code, and this is exactly the kind of possible bug I want mypy to catch early. It's not the hill I'm going to die on, but a significant annoyance.

(though I should've probably opened a ticket to discuss this outside of a semi-related PR)

[ilevkivskyi]

And I don't think 6-7 years timeline applies to properties

It does, before I implemented the proper support, the setter type was automatically the getter type, so mypy happily narrowed it if the type of the value assigned was a subtype of the getter type.

To me doing such narrowing for asymmetric properties is a mistake

But what if a person actually implements your example correctly? I.e. with

    @foo.setter
    def foo(self, val: Iterable[int]) -> None:
        if isinstance(val, list):
            self._foo = val
        else:
            self._foo = list(val)

Also what if the getter/setter types are unions (especially with None)? Anyway, there were no new material information since that decision was made, so it stays.

[sterliakov]

But what if a person actually implements your example correctly? I.e. with

But that's not necessarily "correctly"! If that class modifies self._foo elsewhere, the least surprising behaviour would be to always copy in setter to avoid modifying caller-provided value iff it happens to be a list.

[ilevkivskyi]

Right, the least surprising behavior is when consequences of x.foo = [1, 2, 3] depend on whether foo is a regular attribute or a fancy property :-)

A bit more of educational content for you: should we prohibit overriding regular attributes with such properties? I mean we always narrow regular attributes, but x in x.foo in fact may be an instance of a subclass.

[github-actions]

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅