Skip to content

Better support for converter in attrs plugin. #4607

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 3 commits into from
Feb 23, 2018
Merged

Better support for converter in attrs plugin. #4607

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cb16545
Fix crash in attrs_plugin when using a converter
euresti Feb 17, 2018
6321073
Better support for converter in attrs plugin.
euresti Feb 17, 2018
e226f3e
Merge branch 'master' into fix_attr_right
euresti Feb 21, 2018
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
14 changes: 12 additions & 2 deletions mypy/plugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@

from abc import abstractmethod
from functools import partial
from typing import Callable, List, Tuple, Optional, NamedTuple, TypeVar
from typing import Callable, List, Tuple, Optional, NamedTuple, TypeVar, Dict

import mypy.plugins.attrs
from mypy.nodes import (
Expression, StrExpr, IntExpr, UnaryExpr, Context, DictExpr, ClassDef,
TypeInfo, SymbolTableNode
TypeInfo, SymbolTableNode, MypyFile
)
from mypy.tvar_scope import TypeVarScope
from mypy.types import (
Expand Down Expand Up @@ -61,6 +61,7 @@ def named_generic_type(self, name: str, args: List[Type]) -> Instance:
class SemanticAnalyzerPluginInterface:
"""Interface for accessing semantic analyzer functionality in plugins."""

modules = None # type: Dict[str, MypyFile]
options = None # type: Options
msg = None # type: MessageBuilder

Expand Down Expand Up @@ -93,6 +94,15 @@ def class_type(self, info: TypeInfo) -> Type:
def lookup_fully_qualified(self, name: str) -> SymbolTableNode:
raise NotImplementedError

@abstractmethod
def lookup_fully_qualified_or_none(self, name: str) -> Optional[SymbolTableNode]:
raise NotImplementedError

@abstractmethod
def lookup_qualified(self, name: str, ctx: Context,
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to use only this function, rather than all four lookup functions. But if it is not easy to fix, then ignore this, I will fix this later.

suppress_errors: bool = False) -> Optional[SymbolTableNode]:
raise NotImplementedError


# A context for a function hook that infers the return type of a function with
# a special signature.
Expand Down
30 changes: 20 additions & 10 deletions mypy/plugins/attrs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import mypy.plugin # To avoid circular imports.
from mypy.exprtotype import expr_to_unanalyzed_type, TypeTranslationError
from mypy.fixup import lookup_qualified_stnode
from mypy.nodes import (
Context, Argument, Var, ARG_OPT, ARG_POS, TypeInfo, AssignmentStmt,
TupleExpr, ListExpr, NameExpr, CallExpr, RefExpr, FuncBase,
Expand Down Expand Up @@ -54,14 +55,21 @@ def argument(self, ctx: 'mypy.plugin.ClassDefContext') -> Argument:
if self.converter_name:
# When a converter is set the init_type is overriden by the first argument
# of the converter method.
converter = ctx.api.lookup_fully_qualified(self.converter_name)
converter = lookup_qualified_stnode(ctx.api.modules, self.converter_name, True)
if not converter:
# The converter may be a local variable. Check there too.
converter = ctx.api.lookup_qualified(self.converter_name, self.info, True)

if (converter
and converter.type
and isinstance(converter.type, CallableType)
and converter.type.arg_types):
init_type = converter.type.arg_types[0]
else:
init_type = None
init_type = AnyType(TypeOfAny.from_error)
Copy link
Member

Choose a reason for hiding this comment

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

The rule is to show error message for Anys that are from_error. In this case you can show something like Cannot determine type of converter function, or unsupported converter.

elif self.converter_name == '':
# This means we had a converter but it's not of a type we can infer.
init_type = AnyType(TypeOfAny.from_error)
Copy link
Member

Choose a reason for hiding this comment

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

The above comment applies also here.


if init_type is None:
if ctx.api.options.disallow_untyped_defs:
Expand Down Expand Up @@ -317,14 +325,16 @@ def _attribute_from_attrib_maker(ctx: 'mypy.plugin.ClassDefContext',
def _get_converter_name(converter: Optional[Expression]) -> Optional[str]:
"""Return the full name of the converter if it exists and is a simple function."""
# TODO: Support complex converters, e.g. lambdas, calls, etc.
if (converter
and isinstance(converter, RefExpr)
and converter.node
and isinstance(converter.node, FuncBase)
and converter.node.type
and isinstance(converter.node.type, CallableType)
and converter.node.type.arg_types):
return converter.node.fullname()
if converter:
if (isinstance(converter, RefExpr)
and converter.node
and isinstance(converter.node, FuncBase)
and converter.node.type
and isinstance(converter.node.type, CallableType)
and converter.node.type.arg_types):
return converter.node.fullname()
# Signal that we have an unsupported converter.
return ''
return None


Expand Down
15 changes: 15 additions & 0 deletions test-data/unit/check-attr.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,6 +532,21 @@ class C:

[builtins fixtures/list.pyi]

[case testAttrsUsingUnsupportedConverter]
import attr
class Thing:
def do_it(self, int) -> str:
...
thing = Thing()
def factory(default: int):
...
@attr.s
class C:
x: str = attr.ib(converter=thing.do_it)
y: str = attr.ib(converter=lambda x: x)
z: str = attr.ib(converter=factory(8))
Copy link
Member

Choose a reason for hiding this comment

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

These should give errors until supported.

By the way I am thinking about how to support this. One possible way is to just accept type checker at a later stage. This would require a second part of the plugin that would be triggered on visiting this TypeInfo in type checker. We also need to be sure to mark all nodes that need this type as deferred. TBH I am sure good support for converters worth all this (including potential performance problems). So maybe we can just propose a workaround like always explicitly define your converters i.e. use

def str_first_item(seq: List[int]) -> int:
    """A docstring for why one needs this"""
    return str(seq[0])

@attr.s
class C:
    x: str = attr.ib(converter=str_first_item)

instead of

attr.s
class C:
    x: str = attr.ib(converter=lambda x: str(x[0]))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. That's what I was already thinking for my own code. Should I mention this in the error message? "only functions supported" or something like that?

Copy link
Member

Choose a reason for hiding this comment

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

Should I mention this in the error message? "only functions supported" or something like that?

Yes, this will be helpful. Maybe like "only named functions" (to make it clear that lambdas are not fully supported).

reveal_type(C) # E: Revealed type is 'def (x: Any, y: Any, z: Any) -> __main__.C'
[builtins fixtures/list.pyi]

[case testAttrsUsingConverterAndSubclass]
import attr
Expand Down
177 changes: 177 additions & 0 deletions test-data/unit/check-incremental.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3745,6 +3745,183 @@ class C(A, B):
[out1]
[out2]

[case testAttrsIncrementalConverterInSubmodule]
from a.a import A
reveal_type(A)
[file a/__init__.py]
[file a/a.py]
from typing import Optional
def converter(s:Optional[int]) -> int:
...

import attr
@attr.s
class A:
x: int = attr.ib(converter=converter)

[builtins fixtures/list.pyi]
[out1]
main:2: error: Revealed type is 'def (x: Union[builtins.int, builtins.None]) -> a.a.A'
[out2]
main:2: error: Revealed type is 'def (x: Union[builtins.int, builtins.None]) -> a.a.A'

[case testAttrsIncrementalConverterManyStyles]
from base import Base
reveal_type(Base)
from subclass import A, B
reveal_type(A)
reveal_type(B)
from submodule.base import SubBase
reveal_type(SubBase)
from submodule.subclass import AA, BB
reveal_type(AA)
reveal_type(BB)
from submodule.subsubclass import SubAA, SubBB
reveal_type(SubAA)
reveal_type(SubBB)

[file foo.py]
from typing import Optional
def maybe_int(x: Optional[int]) -> int:
...
[file bar.py]
from typing import Optional
def maybe_bool(x: Optional[bool]) -> bool:
...
[file base.py]
from typing import Optional
import attr
import bar
from foo import maybe_int
def maybe_str(x: Optional[str]) -> str:
...
@attr.s
class Base:
x: int = attr.ib(converter=maybe_int)
y: str = attr.ib(converter=maybe_str)
z: bool = attr.ib(converter=bar.maybe_bool)
[file subclass.py]
from typing import Optional
import attr
from base import Base
@attr.s
class A(Base): pass

import bar
from foo import maybe_int
def maybe_str(x: Optional[str]) -> str:
...
@attr.s
class B(Base):
xx: int = attr.ib(converter=maybe_int)
yy: str = attr.ib(converter=maybe_str)
zz: bool = attr.ib(converter=bar.maybe_bool)

[file submodule/__init__.py]
[file submodule/base.py]
from typing import Optional
import attr
import bar
from foo import maybe_int
def maybe_str(x: Optional[str]) -> str:
...
@attr.s
class SubBase:
x: int = attr.ib(converter=maybe_int)
y: str = attr.ib(converter=maybe_str)
z: bool = attr.ib(converter=bar.maybe_bool)

[file submodule/subclass.py]
from typing import Optional
import attr
from base import Base
@attr.s
class AA(Base): pass

import bar
from foo import maybe_int
def maybe_str(x: Optional[str]) -> str:
...
@attr.s
class BB(Base):
xx: int = attr.ib(converter=maybe_int)
yy: str = attr.ib(converter=maybe_str)
zz: bool = attr.ib(converter=bar.maybe_bool)

[file submodule/subsubclass.py]
from typing import Optional
import attr
from .base import SubBase
@attr.s
class SubAA(SubBase): pass

import bar
from foo import maybe_int
def maybe_str(x: Optional[str]) -> str:
...
@attr.s
class SubBB(SubBase):
xx: int = attr.ib(converter=maybe_int)
yy: str = attr.ib(converter=maybe_str)
zz: bool = attr.ib(converter=bar.maybe_bool)
[builtins fixtures/list.pyi]
[out1]
main:2: error: Revealed type is 'def (x: Union[builtins.int, builtins.None], y: Union[builtins.str, builtins.None], z: Union[builtins.bool, builtins.None]) -> base.Base'
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit hard to read. Could you please change this to instead checking types of things by assignments that would cause an error on the second run only? For example:

import a
[file a.py]
from b import thing
x: str = thing # OK
[file a.py.2]
from b import thing
x: int = thing # Error
[file b.py]
thing = 'thing'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. It'll be more like checking class creation though.

main:4: error: Revealed type is 'def (x: Union[builtins.int, builtins.None], y: Union[builtins.str, builtins.None], z: Union[builtins.bool, builtins.None]) -> subclass.A'
main:5: error: Revealed type is 'def (x: Union[builtins.int, builtins.None], y: Union[builtins.str, builtins.None], z: Union[builtins.bool, builtins.None], xx: Union[builtins.int, builtins.None], yy: Union[builtins.str, builtins.None], zz: Union[builtins.bool, builtins.None]) -> subclass.B'
main:7: error: Revealed type is 'def (x: Union[builtins.int, builtins.None], y: Union[builtins.str, builtins.None], z: Union[builtins.bool, builtins.None]) -> submodule.base.SubBase'
main:9: error: Revealed type is 'def (x: Union[builtins.int, builtins.None], y: Union[builtins.str, builtins.None], z: Union[builtins.bool, builtins.None]) -> submodule.subclass.AA'
main:10: error: Revealed type is 'def (x: Union[builtins.int, builtins.None], y: Union[builtins.str, builtins.None], z: Union[builtins.bool, builtins.None], xx: Union[builtins.int, builtins.None], yy: Union[builtins.str, builtins.None], zz: Union[builtins.bool, builtins.None]) -> submodule.subclass.BB'
main:12: error: Revealed type is 'def (x: Union[builtins.int, builtins.None], y: Union[builtins.str, builtins.None], z: Union[builtins.bool, builtins.None]) -> submodule.subsubclass.SubAA'
main:13: error: Revealed type is 'def (x: Union[builtins.int, builtins.None], y: Union[builtins.str, builtins.None], z: Union[builtins.bool, builtins.None], xx: Union[builtins.int, builtins.None], yy: Union[builtins.str, builtins.None], zz: Union[builtins.bool, builtins.None]) -> submodule.subsubclass.SubBB'
[out2]
main:2: error: Revealed type is 'def (x: Union[builtins.int, builtins.None], y: Union[builtins.str, builtins.None], z: Union[builtins.bool, builtins.None]) -> base.Base'
main:4: error: Revealed type is 'def (x: Union[builtins.int, builtins.None], y: Union[builtins.str, builtins.None], z: Union[builtins.bool, builtins.None]) -> subclass.A'
main:5: error: Revealed type is 'def (x: Union[builtins.int, builtins.None], y: Union[builtins.str, builtins.None], z: Union[builtins.bool, builtins.None], xx: Union[builtins.int, builtins.None], yy: Union[builtins.str, builtins.None], zz: Union[builtins.bool, builtins.None]) -> subclass.B'
main:7: error: Revealed type is 'def (x: Union[builtins.int, builtins.None], y: Union[builtins.str, builtins.None], z: Union[builtins.bool, builtins.None]) -> submodule.base.SubBase'
main:9: error: Revealed type is 'def (x: Union[builtins.int, builtins.None], y: Union[builtins.str, builtins.None], z: Union[builtins.bool, builtins.None]) -> submodule.subclass.AA'
main:10: error: Revealed type is 'def (x: Union[builtins.int, builtins.None], y: Union[builtins.str, builtins.None], z: Union[builtins.bool, builtins.None], xx: Union[builtins.int, builtins.None], yy: Union[builtins.str, builtins.None], zz: Union[builtins.bool, builtins.None]) -> submodule.subclass.BB'
main:12: error: Revealed type is 'def (x: Union[builtins.int, builtins.None], y: Union[builtins.str, builtins.None], z: Union[builtins.bool, builtins.None]) -> submodule.subsubclass.SubAA'
main:13: error: Revealed type is 'def (x: Union[builtins.int, builtins.None], y: Union[builtins.str, builtins.None], z: Union[builtins.bool, builtins.None], xx: Union[builtins.int, builtins.None], yy: Union[builtins.str, builtins.None], zz: Union[builtins.bool, builtins.None]) -> submodule.subsubclass.SubBB'

[case testAttrsIncrementalConverterInFunction]
import attr
def foo() -> None:
def foo(x: str) -> int:
...
@attr.s
class A:
x: int = attr.ib(converter=foo)
reveal_type(A)
[builtins fixtures/list.pyi]
[out1]
main:8: error: Revealed type is 'def (x: str?) -> __main__.A@5'
Copy link
Member

Choose a reason for hiding this comment

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

Why str is shown as unbound type here? This may cause problems later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll investigate

Copy link
Member

Choose a reason for hiding this comment

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

I'll investigate

This one actually worries me most. It would be great to fix at least this one before the release.

[out2]
main:8: error: Revealed type is 'def (x: str?) -> __main__.A@5'

[case testAttrsIncrementalConverterInSubmoduleForwardRef]
from a.a import A
reveal_type(A)
[file a/__init__.py]
[file a/a.py]
from typing import List
def converter(s:F) -> int:
...

import attr
@attr.s
class A:
x: int = attr.ib(converter=converter)

F = List[int]

[builtins fixtures/list.pyi]
[out1]
main:2: error: Revealed type is 'def (x: builtins.list[builtins.int]) -> a.a.A'
[out2]
main:2: error: Revealed type is 'def (x: builtins.list[builtins.int]) -> a.a.A'


[case testAttrsIncrementalThreeRuns]
from a import A
A(5)
Expand Down