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

What to do about setters of a different type than their property? #3004

Open
sixolet opened this issue Mar 15, 2017 · 55 comments
Open

What to do about setters of a different type than their property? #3004

sixolet opened this issue Mar 15, 2017 · 55 comments
Labels
false-positive mypy gave an error on correct code feature priority-0-high topic-descriptors Properties, class vs. instance attributes

Comments

@sixolet
Copy link
Collaborator

sixolet commented Mar 15, 2017

Consider this code:

import typing
class A:
    @property
    def f(self) -> int:
        return 1
    @f.setter  # Possible error for defining a setter that doesn't accept the type the @property returns
    def f(self, x: str) -> None:
        pass
a = A()
a.f = ''  # Possibly not an error, but currently mypy gives an error
a.f = 1  # Certainly should be an error, unless we disallowed 
         # defining the setter this way in the first place.
reveal_type(a.f)  # E: Revealed type is 'builtins.int'

Whatever we decide, I'm happy to build a PR; I have this code loaded into my head.

@sixolet
Copy link
Collaborator Author

sixolet commented Mar 15, 2017

In other words "with arbitrary setters, you can have an lvalue be of a different type than its corresponding rvalue"

@gvanrossum
Copy link
Member

IIRC we debated a similar issues when descriptors were added and decided that it's a perverse style that we just won't support. If you really have this you can add # type: ignore.

@sixolet
Copy link
Collaborator Author

sixolet commented Mar 15, 2017

Right now, you have to # type: ignore the line that says a.f = '', but not the definition. Should you have to # type: ignore the setter definition too? My inclination is yes, but I'm not sure of this.

@gvanrossum
Copy link
Member

What does a discrepancy between __get__ and __set__ (when using as a descriptor) do? I think that's the example to follow.

@srittau
Copy link
Contributor

srittau commented Feb 23, 2018

One real-life case where I encountered this a few times now is a normalizing property like this:

from typing import Set, Iterable


class Foo:

    def __init__(self) -> None:
        self._foo = set()  # type: Set[int]

    @property
    def foo(self) -> Set[int]:
        return self._foo

    @foo.setter
    def foo(self, v: Iterable[int]) -> None:
        self._foo = set(v)


Foo().foo = [1, 2, 3]

I like this implementation, because it allows a user of the cleverly named class Foo not to care about the exact type of the property, while Foo can use the best representation internally, while also giving additional guarantees when accessing the property.

Using the same type in the getter and setter would complicate the life of its users.

@dacut
Copy link

dacut commented May 19, 2019

I disagree that this is "perverse" -- contravariance in the setter is one example, e.g. where the getter returns Set[x] and the setter takes Collection[x]:

from typing import Collection, Set

class X():
    @property
    def hello(self) -> Set[str]:
        return {"x", "y"}
    
    @hello.setter
    def hello(self, value: Collection[str]) -> None:
        pass

x = X()
x.hello = ["1", "2", "3"]
% mypy --version
mypy 0.701
% mypy mypy3004.py
mypy3004.py:13: error: Incompatible types in assignment (expression has type "List[str]", variable has type "Set[str]")

In my current case, I'm being even stricter in the getter, returning FrozenSet[x] to prevent accidental misuse of the returned collection (thinking it will change the attribute in the object itself).

@ilevkivskyi
Copy link
Member

What does a discrepancy between __get__ and __set__ (when using as a descriptor) do? I think that's the example to follow.

Descriptors actually support different types as one would expect. However, properties were implemented before descriptors (using some heavy special-casing) so they don't support this.

I think this is a valid feature to support. I have seen this a lot recently in internal code bases, mostly in context similar to example by @srittau (canonical representation). This however may be tricky to implement (because of the special-casing I mentioned). Maybe a better strategy would be to just make property a regular descriptor in mypy (I believe we already have an issue for that), then this will be supported automatically.

@ilevkivskyi
Copy link
Member

FTR, the main issue about properties is #220

@chadrik
Copy link
Contributor

chadrik commented Sep 9, 2019

Here's another example that I don't find terribly perverse. Using None to indicate "use the default value":

class A:

    def __init__(self, label=None):
        # type: (Optional[str]) -> None
        self._user_label = None  # type: Optional[str]
        self.label = label

    @property
    def label(self):
        # type: () -> str
        return self._user_label or self.default_label()

    @label.setter
    def label(self, value):
        # type: (Optional[str]) -> None
        self._user_label = value

    def default_label(self):
        # type: () -> str
        return self.__class__.__name__

@ilevkivskyi ilevkivskyi added the false-positive mypy gave an error on correct code label Dec 5, 2019
@ilevkivskyi
Copy link
Member

I am going to remove the "needs discussion" label. IMO it is now pretty clear we should support this, the only discussion is what is the best way to implement this.

@joelberkeley-secondmind
Copy link

joelberkeley-secondmind commented May 14, 2020

perhaps I'm missing sth, but doesn't that mean you can get

class X: pass

class Foo:
   @property
   def foo(self) -> int:
       ...

   @foo.setter
   def foo(self, o: Union[X, int]) -> None:
       ...

foo = Foo()
x = X()
foo.bar = x
assert foo.bar != x

I wonder if there are cases where this makes sense (e.g. where equality still holds - perhaps this can work in the Iterable/Set example), but others where I also feel it seems odd e.g. using None for default values

My request would be for there to be a flag to turn this on, and it to be off by default

@warsaw
Copy link
Member

warsaw commented Jun 19, 2020

I think this is the problem I am encountering now with mypy 0.781. Here's my distilled example:

from datetime import timedelta
from typing import Union

Interval = Union[timedelta, int]


class Foo:
    def __init__(self):
        self._x = timedelta(seconds=15)

    @property
    def x(self) -> timedelta:
        return self._x

    @x.setter
    def x(self, delta: Interval) -> None:
        if isinstance(delta, timedelta):
            self.x = delta
        else:
            self.x = timedelta(seconds=delta)


foo = Foo()
foo.x = 7
foo.x = timedelta(seconds=8)

And the error I'm getting:

% mypy foo.py 
foo.py:24: error: Incompatible types in assignment (expression has type "int", variable has type "timedelta")
Found 1 error in 1 file (checked 1 source file)

The idea behind the x property is that its type "is" a timedelta but you can set it with an int or a timedelta and the former always gets coerced to the latter. I think those are pretty pedestrian semantics!

Seems like this is the same problem described in this issue. I can understand that mypy may not be able to infer this.

@dacut
Copy link

dacut commented Jun 19, 2020

I wonder if there are cases where this makes sense (e.g. where equality still holds - perhaps this can work in the Iterable/Set example), but others where I also feel it seems odd e.g. using None for default values

My request would be for there to be a flag to turn this on, and it to be off by default

@joelberkeley-pio, what you're asking for is a Python feature, not a MyPy feature. This is part of the whole point of properties -- to divorce the getters and setters from exposing bare values of members, so we don't need to have setX() and getX() like in Java.

For example, this is also valid Python (and, apart from the contrived simplification, actually used in some libraries):

from typing import Union
import requests

class RemoteObject:
    def __init__(self, object_id: str):
        self.object_id = object_id

    @property
    def value(self) -> float:
        return float(requests.get(f"https://example.com/{self.object_id}/value").text)

    @value.setter
    def value(self, new_value: Union[int, float]) -> None:
        requests.post(f"https://example.com/{self.object_id}/value",
                      data=str(new_value).encode("utf-8"))

Anything could happen on the remote server between the time the value is set and retrieved. There is absolutely no expectation that the values should be identical.

@lucventurini
Copy link

For the record, this can be done at least in the __init__ to prevent the mypy error:

class Example:
    
    def __init__(self, value=None):
        self._value = frozenset()
        type(self).value.fset(value)

    @property
    def value(self) -> frozenset[int]:
        return self._value

    @value.setter
    def value(self, value: Iterable[float, int] | None) -> None:
        if value is None:
            self._value = frozenset()
        else:
            self._value = frozenset(int(_) for _ in value)

That said, I would argue that in this case it should be mypy to support native Python methods, rather than forcing developers to use an awkward syntax to use features of the Python language.

@amgcc
Copy link

amgcc commented Feb 19, 2024

As a counter-example to the "perverse" comment, let me provide a use case of why this is needed. Pandas dataframes have a columns field with is defined as pd.Index but frequently is assigned a list of str values. Because of this mypy limitation, I am required to manually create a pd.Index any time I want to take a column projection. This is very counter-productive.

More details here.

@wjakob
Copy link

wjakob commented Feb 28, 2024

I was surprised to run into this issue and then read Guido's message calling such functionality "perverse". There is also precedent: for example, __getitem__ and __setitem__ can return and take different types. I don't see how this is any different.

My use case is a vector library where a user might, say, zero-initialize a component of a 2D vector:

v = Vector2f(...)
v.x = 0

These vectors are themselves vectorized, i.e., the "x" component is a slice of a larger data structure where the setter performs an implicit conversion that MyPy sadly cannot type-check. The alternative of having to put explicit casts everywhere is surprisingly ugly.

So this is all to say that I think the "perverse" comment is not seeing the big picture.

@mscuthbert
Copy link

Thank you @wjakob (and @amgcc) -- I think that the initial "perverse" comments have softened and that there is general consensus now that different getter and setter types (like in Typescript and other languages) are desirable both in numeric programming and in other areas.

What's needed to move this issue from Green (Open) to Purple (Merged) are (1) people who know the typing system of MyPy and Pyright and other systems who can make implementations, and (2) technical writers who can explain the changes/situation to others.

Given the huge goodwill of the community that would come from getting these changes in, are there people who would volunteer for either (1) or (2)?

flamingbear added a commit to flamingbear/xarray that referenced this issue Mar 1, 2024
mypy doesn't handle the case we have and this is the official suggestion.
python/mypy#3004 (comment)
jvansanten added a commit to jvansanten/pybind11-stubgen that referenced this issue Mar 8, 2024
This is intended to support contravariant setters (e.g.  fget
returning a concrete container, but fset taking any collection),
for the case that mypy eventually supports them (see:
python/mypy#3004)
@gentlegiantJGC
Copy link

gentlegiantJGC commented Mar 16, 2024

Delving into the code a bit, it looks like the important bit is mypy.checker.TypeChecker.check_assignment which finds the valid types for the variable being assigned to and checks that it is compatible with the value being set.

This line finds the type of the variable being assigned to.

lvalue_type, index_lvalue, inferred = self.check_lvalue(lvalue)

Down here expands member assignment.

mypy/mypy/checker.py

Lines 2987 to 2990 in bcb3747

instance_type = self.expr_checker.accept(lvalue.expr)
rvalue_type, lvalue_type, infer_lvalue_type = self.check_member_assignment(
instance_type, lvalue_type, rvalue, context=rvalue
)

Here are some examples of the current implementation.

Normal variable

class Test:
    val: int


t = Test()
t.val = 2.5

check_lvalue returns builtins.int which is correct.

Custom descriptor

from __future__ import annotations
from typing import Any, overload


class MyInt:
    @overload
    def __get__(self, obj: None, objtype: None) -> MyInt: ...

    @overload
    def __get__(self, obj: object, objtype: type[object]) -> int: ...

    def __get__(self, obj: Any, objtype: Any = None) -> int | MyInt:
        return 1

    def __set__(self, obj: Any, value: int | float) -> None:
        pass


class Test:
    val = MyInt()


t = Test()
t.val = 2.5

check_lvalue returns MyInt which check_member_assignment analyses the __set__ method of.

Property

class Test:
    @property
    def val(self) -> int:
        return 1

    @val.setter
    def val(self, val: int | float) -> None:
        pass


t = Test()
t.val = 2.5

check_lvalue returns builtins.int which check_member_assignment generates an error about.

I would have thought it would be more correct if check_lvalue returns a property class through which __get__ and __set__ can be evaluated. I don't know if this is possible though because the property is defined in stages.

Edit: Here is the part where it pulls the getter return value.

mypy/mypy/checkmember.py

Lines 314 to 318 in bcb3747

if method.is_property:
assert isinstance(method, OverloadedFuncDef)
first_item = method.items[0]
assert isinstance(first_item, Decorator)
return analyze_var(name, first_item.var, typ, info, mx)

Edit2: I think if analyze_instance_member_access can return a class with __get__ and __set__ methods, the rest of the code will handle it using the descriptor logic which is how it is actually implemented.

RazerM added a commit to RazerM/werkzeug that referenced this issue Apr 19, 2024
mypy doesn't use the type of setters as of 1.9.0 (see python/mypy#3004),
but I think it's still good to have these be accurate (maybe the other
type checkers work better here).
RazerM added a commit to RazerM/werkzeug that referenced this issue Apr 19, 2024
mypy doesn't use the type of setters as of 1.9.0 (see python/mypy#3004),
but I think it's still good to have these be accurate (maybe the other
type checkers work better here).
RazerM added a commit to RazerM/werkzeug that referenced this issue Apr 19, 2024
mypy doesn't use the type of setters as of 1.9.0 (see python/mypy#3004),
but I think it's still good to have these be accurate (maybe the other
type checkers work better here).
RazerM added a commit to RazerM/werkzeug that referenced this issue Apr 19, 2024
mypy doesn't use the type of setters as of 1.9.0 (see python/mypy#3004),
but I think it's still good to have these be accurate (maybe the other
type checkers work better here).

mypy's recommendation is to use `# type: ignore` comments if setter
types don't match getters, which you see when setting no_cache to True.
JochenSiegWork added a commit to basf/MolPipeline that referenced this issue May 17, 2024
    - Ignore None type in setter for descriptor_list to enable
      optional input validation and an optional None.
    - See python/mypy#3004 as reference.
Rafiot added a commit to pandora-analysis/pandora that referenced this issue Jun 14, 2024
davidism pushed a commit to RazerM/werkzeug that referenced this issue Jul 12, 2024
Cache-Control no-transform directive is a boolean

no-transform has no arguments as a request or response directive (RFC
9111). Prior to this fix, cc.no_transform would return None whether the
directive is present or not.

Cache-Control min-fresh directive requires argument

The type for this property is `int | None`, so getting `"*"` for a
malformed directive is surprising. I think dropping the empty value here
is better than fixing the type.

Fix CacheControl getter type stubs

- cache_control_property with type=bool never return None
- some non-bool types were marked as returning bool instead of str
- max_stale can return "*" in addition to int or None

Reflect immutability of RequestCacheControl in type stubs

Fix CacheControl setter type stubs

mypy doesn't use the type of setters as of 1.9.0 (see python/mypy#3004),
but I think it's still good to have these be accurate (maybe the other
type checkers work better here).

mypy's recommendation is to use `# type: ignore` comments if setter
types don't match getters, which you see when setting no_cache to True.

Support must-understand response directive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
false-positive mypy gave an error on correct code feature priority-0-high topic-descriptors Properties, class vs. instance attributes
Projects
None yet
Development

No branches or pull requests