python-attrs / attrs

Python Classes Without Boilerplate
https://www.attrs.org/
MIT License
5.28k stars 369 forks source link

question about mypy error with optional() validator and non-None default #515

Open dcrosta opened 5 years ago

dcrosta commented 5 years ago

With attrs 19.1.0/py 3.7.2, I have the following attrbug.py:

from typing import Optional

from attr import validators
import attr

@attr.s
class DoesntWorkWithMyPy:
    required_bool: bool = attr.ib(
        validator=validators.optional(validators.instance_of(bool)),
        default=False,
    )

    optional_bool: Optional[bool] = attr.ib(
        validator=validators.instance_of(bool),
        default=False,
    )

@attr.s
class DoesWorkWithMyPy:
    required_bool: bool = attr.ib(
        validator=validators.instance_of(bool),
        default=False,
    )

if __name__ == "__main__":
    # neither of these raises, because all attribs specify defaults
    DoesWorkWithMyPy()
    DoesntWorkWithMyPy()

Running mypy, I see thee following (I think wrong?) error:

$ mypy attrbug.py
attrbug.py:9: error: Argument "validator" has incompatible type "Callable[[Any, Attribute[Optional[bool]], Optional[bool]], Any]"; expected "Union[Callable[[Any, Attribute[bool], bool], Any], Sequence[Callable[[Any, Attribute[bool], bool], Any]], None]"

That's from the required_bool attribute of the DoesntWorkWithMyPy class. MyPy doesn't complain about any of the other invocations used here, as you can see, and I don't think it probably ought to complain about this one either.

I think what's going on is that I have said that the attribute is non-Optional on the class (because I specify a default value), but uses an "optional" validator. I had (originally) specified it there so that instances could be created without specifying a value for this parameter, assuming that the validators & converters happen before the default is applied; it seems however that the default applies before the validator, which is why both optional_bool and DoesWorkWithMyPy.required_bool work. Is this new understanding correct & reliable?

euresti commented 5 years ago

The default value does get passed through the validator:

@attr.s
class Foo:
     x = attr.ib(default=88, validator=validators.instance_of(bool))

Foo()

Hmm. I'm wondering if this is one of those "covariance" issues in mypy.
That being said, I think required_bool should be an Optional[bool] like this:

required_bool: Optional[bool] = attr.ib(
        validator=validators.optional(validators.instance_of(bool)),
        default=False,
    )

Since your validator is allowing None through.

dcrosta commented 5 years ago

Yeah, I think there are two things here: first, my understanding about the order of operations, now corrected, and the actual typing I want, which is the version from DoesWorkWithMyPy, thankfully.

Second, whether there's an issue in the typing. I don't have a super-strong intuition, but it seems like if the result of the attr.ib is definitely a bool, that it should be typed in a way that satisfies mypy's Optional[bool].

henryiii commented 4 years ago

I think this is related:

I want to make a attribute like this:

mass = attr.ib(-1, converter=lambda v: None if v < 0 else v)

However, after fixing the issue with converters requiring names, and adding typing, this still fails to work:

def _none_or_positive_converter(value):
     # type: (Any) -> Optional[float]
     return None if value < 0 else value
...
mass: Optional[float] = attr.ib(-1., converter=_none_or_positive_converter)

I get a a MyPy error because it wants the converter to return a float, not an Optional[float].

This is my current workaround:

def _none_or_positive_converter(value):
     # type: (Any) -> float
     return None if value < 0 else value  # type: ignore
...
euresti commented 4 years ago

Yeah so the problem is that attr.ib wants the arguments to match and it uses the type of default (i.e. -1) as a higher precedence. So mypy says great

attr.ib(float, converter=method that returns Optional[float])

Well that converter is wrong. It doesn't return a float, it returns an optional float. There are 2 ways to trick mypy into doing the right thing:

mass = attr.ib(cast(Optional[float], -1), converter=_none_or_positive_converter)

Or if you have a bunch of these:

def maybe_float(x: float) -> Optional[float]:
    return x

mass = attr.ib(maybe_float(-1), converter=_none_or_positive_converter)

I realize both of these are pretty terrible. I do think there is a mypy bug about this but I don't have time to look for it right now.

henryiii commented 4 years ago

Thanks! That does trick it in a better way than ignoring the type. It's also easy to use a typed variable (minus_one: Optional[float] = -1) since I have the same one over and over. It still seems like the typing should use type= or the variable type instead of default's type. Should the typing system for attrs be part of attrs instead of sitting in Mypy or type shed (not sure where it is now), by the way? It sounds like it is not.

euresti commented 4 years ago

This particular issue is stubs related which are part of attrs. (That's all the .pyi files in the codebase). There is also a plugin which lives in mypy for testability reasons. But that's not what you're hitting now.