Skip to content

Re-widen custom properties after narrowing #19296

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 19, 2025
+27 −10
Merged

Re-widen custom properties after narrowing #19296

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5f7e2c6
Re-widen custom properties after narrowing
ilevkivskyi Jun 14, 2025
e9c58d8
Fix typo
ilevkivskyi Jun 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 8 additions & 10 deletions mypy/checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4580,7 +4580,7 @@ def check_member_assignment(
self,
lvalue: MemberExpr,
instance_type: Type,
attribute_type: Type,
set_lvalue_type: Type,
rvalue: Expression,
context: Context,
) -> tuple[Type, Type, bool]:
Expand All @@ -4597,23 +4597,21 @@ def check_member_assignment(
if (isinstance(instance_type, FunctionLike) and instance_type.is_type_obj()) or isinstance(
instance_type, TypeType
):
rvalue_type, _ = self.check_simple_assignment(attribute_type, rvalue, context)
return rvalue_type, attribute_type, True
rvalue_type, _ = self.check_simple_assignment(set_lvalue_type, rvalue, context)
return rvalue_type, set_lvalue_type, True

with self.msg.filter_errors(filter_deprecated=True):
get_lvalue_type = self.expr_checker.analyze_ordinary_member_access(
lvalue, is_lvalue=False
)

# Special case: if the rvalue_type is a subtype of both '__get__' and '__set__' types,
# and '__get__' type is narrower than '__set__', then we invoke the binder to narrow type
# Special case: if the rvalue_type is a subtype of both '__get__' type, and
# '__get__' type is narrower than '__set__', then we invoke the binder to narrow type
# by this assignment. Technically, this is not safe, but in practice this is
# what a user expects.
rvalue_type, _ = self.check_simple_assignment(attribute_type, rvalue, context)
infer = is_subtype(rvalue_type, get_lvalue_type) and is_subtype(
get_lvalue_type, attribute_type
)
return rvalue_type if infer else attribute_type, attribute_type, infer
rvalue_type, _ = self.check_simple_assignment(set_lvalue_type, rvalue, context)
rvalue_type = rvalue_type if is_subtype(rvalue_type, get_lvalue_type) else get_lvalue_type
return rvalue_type, set_lvalue_type, is_subtype(get_lvalue_type, set_lvalue_type)

def check_indexed_assignment(
self, lvalue: IndexExpr, rvalue: Expression, context: Context
Expand Down
19 changes: 19 additions & 0 deletions test-data/unit/check-narrowing.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2593,3 +2593,22 @@ def baz(item: Base) -> None:
reveal_type(item) # N: Revealed type is "__main__.<subclass of "__main__.Base" and "__main__.BarMixin">"
item.bar()
[builtins fixtures/isinstance.pyi]

[case testCustomSetterNarrowingReWidened]
class B: ...
class C(B): ...
class C1(B): ...
class D(C): ...

class Test:
@property
def foo(self) -> C: ...
@foo.setter
def foo(self, val: B) -> None: ...

t: Test
t.foo = D()
reveal_type(t.foo) # N: Revealed type is "__main__.D"
Copy link
Collaborator

@sterliakov sterliakov Jun 15, 2025

Choose a reason for hiding this comment

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

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...

Copy link
Collaborator

@JukkaL JukkaL Jun 16, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@sterliakov sterliakov Jun 18, 2025

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator

@sterliakov sterliakov Jun 18, 2025

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@ilevkivskyi ilevkivskyi Jun 18, 2025

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator

@sterliakov sterliakov Jun 18, 2025

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

@ilevkivskyi ilevkivskyi Jun 18, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@sterliakov sterliakov Jun 19, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@ilevkivskyi ilevkivskyi Jun 19, 2025

Choose a reason for hiding this comment

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

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.

t.foo = C1()
reveal_type(t.foo) # N: Revealed type is "__main__.C"
[builtins fixtures/property.pyi]