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

Converter functions remove type annotations from __init__ #694

Closed
nosewings opened this issue Sep 24, 2020 · 12 comments
Closed

Converter functions remove type annotations from __init__ #694

nosewings opened this issue Sep 24, 2020 · 12 comments
Labels
Typing Typing/stub/Mypy/PyRight related bugs.

Comments

@nosewings
Copy link
Contributor

nosewings commented Sep 24, 2020

Example:

from typing import Union
import attr

def to_int(x: Union[str, int]) -> int:
    return int(x)

@attr.s
class Example:
    x: int = attr.ib(converter=to_int)

My expectation is that the generated __init__ should take a parameter x: Union[str, int]. Instead, the generated __init__ simply takes an untyped parameter x.

It seems like this could be solved by looking at the type annotations attached to the converter method and then annotating __init__ appropriately.

If this is a desirable feature, I can submit a PR.

@hynek hynek added the Typing Typing/stub/Mypy/PyRight related bugs. label Sep 26, 2020
@hynek
Copy link
Member

hynek commented Sep 26, 2020

I feel like this has come up before, but just to be clear: are you talking about whatever is in __annotations__ or about the behavior of mypy in these cases?

@nosewings
Copy link
Contributor Author

I'm just talking about __annotations__. In the previous example, we currently have

>>> Example.__init__.__annotations__
{'return': None}

whereas I would expect

>>> Example.__init__.__annotations__
{'x': typing.Union[str, int], 'return': None}

@euresti
Copy link
Contributor

euresti commented Sep 26, 2020

Ah. This happens here

if a.init is True and a.converter is None and a.type is not None:

        if a.init is True and a.converter is None and a.type is not None:
            annotations[arg_name] = a.type

@hynek
Copy link
Member

hynek commented Sep 27, 2020

So the q is why did the a.converter is None check instead of copying it over? It should be noted, that the fix needs to special-case the use of the pipe() converter.

@hynek
Copy link
Member

hynek commented Oct 19, 2020

@nosewings are you still game for a PR?

@nosewings
Copy link
Contributor Author

Yes. School has kept me busy, but I think I can get the PR out within the next few days.

@nosewings
Copy link
Contributor Author

nosewings commented Oct 22, 2020

I will note that a choice has to be made if both a converter and an explicit type annotation or type argument are present. From an implementation perspective, the simplest thing to do is to let a type argument override any converter annotations. However, that might have some unintuitive consequences. Consider the following:

import attr

def int2str(x: int) -> str:
    return str(x)

@attr.s
class A:
    a: str = attr.ib(converter=int2str)

A plausible reading of this code's intent is that the attribute a should have type str, but the argument a should have type int. However, given the implementation I've suggested above, the a: str annotation would override the converter's annotation, so __init__ would take an argument a: str.

@hynek
Copy link
Member

hynek commented Oct 26, 2020

I suspect this muddying might be reason, why we have punted on it?

I mean the correct annotation for a in init would be the argument type of the first converter (in this case int), correct?

@nosewings
Copy link
Contributor Author

nosewings commented Nov 2, 2020

@hynek Different users could have different intents, but I feel like it would be surprising if the type argument could be overridden implicitly.

In any case, I've submitted a PR. Of course, it's completely reasonable if the issue I've raised above makes you wary of implementing this change.

@sscherfke
Copy link
Contributor

IMHO, the type of the value argument of all of an attribute converters and validators must be the same.
This type should/could then be used to annotate the type of that attribute in __init__().

Vise versa, the return type of all converers must be the same and must also match an explicitly annotated type for an attribute. This is the type that a "getattr()" would return.

If the converters and validators do use type annotations and if these annotations do not mach, attrs should raise a TypeError while creating the class. This would avoid the need for attrs to handle ambiguities in the user's code. :)

Here is an example in pseudo code for illustration:

T_IN = Type(...)
T_OUT = Type(...)

def validator(value: T_IN) -> None:
    pass

def converter(value: T_IN) -> T_OUT:
    return T_OUT(value)

@attr.define
class C:
    x: T_OUT = attr.field(converter=converter, validator=validator)

# Resulting/generated class:
class C:
    def __init__(self, x: T_IN): -> None:
        self.__validate_x(x)
        self.x: T_OUT = self.__convert_x(x)

@hynek
Copy link
Member

hynek commented Nov 3, 2020

If the converters and validators do use type annotations and if these annotations do not mach, attrs should raise a TypeError while creating the class. This would avoid the need for attrs to handle ambiguities in the user's code. :)

Oof, I think this leads down a dangerous path with types that are actually compatible even though the look differently. This should be left to the type checking crowd (i.e. mypy) IMHO.

@hynek
Copy link
Member

hynek commented Jan 26, 2021

This has been fixed by #710

@hynek hynek closed this as completed Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Typing Typing/stub/Mypy/PyRight related bugs.
Projects
None yet
Development

No branches or pull requests

4 participants