Skip to content

[attrs plugin] Support kw_only=True #6107

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
Dec 30, 2018
Merged

[attrs plugin] Support kw_only=True #6107

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
d266b11
[attrs plugin] Support kw_only=True
euresti Dec 20, 2018
228782e
CR
euresti Dec 27, 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
70 changes: 53 additions & 17 deletions mypy/plugins/attrs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
Context, Argument, Var, ARG_OPT, ARG_POS, TypeInfo, AssignmentStmt,
TupleExpr, ListExpr, NameExpr, CallExpr, RefExpr, FuncBase,
is_class_var, TempNode, Decorator, MemberExpr, Expression,
SymbolTableNode, MDEF, JsonDict, OverloadedFuncDef
SymbolTableNode, MDEF, JsonDict, OverloadedFuncDef, ARG_NAMED_OPT, ARG_NAMED
)
from mypy.plugins.common import (
_get_argument, _get_bool_argument, _get_decorator_bool_argument, add_method
Expand Down Expand Up @@ -56,12 +56,13 @@ class Attribute:
"""The value of an attr.ib() call."""

def __init__(self, name: str, info: TypeInfo,
has_default: bool, init: bool, converter: Converter,
has_default: bool, init: bool, kw_only: bool, converter: Converter,
context: Context) -> None:
self.name = name
self.info = info
self.has_default = has_default
self.init = init
self.kw_only = kw_only
self.converter = converter
self.context = context

Expand Down Expand Up @@ -132,17 +133,23 @@ def argument(self, ctx: 'mypy.plugin.ClassDefContext') -> Argument:
# Convert type not set to Any.
init_type = AnyType(TypeOfAny.unannotated)

if self.kw_only:
arg_kind = ARG_NAMED_OPT if self.has_default else ARG_NAMED
else:
arg_kind = ARG_OPT if self.has_default else ARG_POS

# Attrs removes leading underscores when creating the __init__ arguments.
return Argument(Var(self.name.lstrip("_"), init_type), init_type,
None,
ARG_OPT if self.has_default else ARG_POS)
arg_kind)

def serialize(self) -> JsonDict:
"""Serialize this object so it can be saved and restored."""
return {
'name': self.name,
'has_default': self.has_default,
'init': self.init,
'kw_only': self.kw_only,
'converter_name': self.converter.name,
'converter_is_attr_converters_optional': self.converter.is_attr_converters_optional,
'context_line': self.context.line,
Expand All @@ -157,6 +164,7 @@ def deserialize(cls, info: TypeInfo, data: JsonDict) -> 'Attribute':
info,
data['has_default'],
data['init'],
data['kw_only'],
Converter(data['converter_name'], data['converter_is_attr_converters_optional']),
Context(line=data['context_line'], column=data['context_column'])
)
Expand All @@ -181,6 +189,7 @@ def attr_class_maker_callback(ctx: 'mypy.plugin.ClassDefContext',
frozen = _get_frozen(ctx)
cmp = _get_decorator_bool_argument(ctx, 'cmp', True)
auto_attribs = _get_decorator_bool_argument(ctx, 'auto_attribs', auto_attribs_default)
kw_only = _get_decorator_bool_argument(ctx, 'kw_only', False)

if ctx.api.options.python_version[0] < 3:
if auto_attribs:
Expand All @@ -190,8 +199,11 @@ def attr_class_maker_callback(ctx: 'mypy.plugin.ClassDefContext',
# Note: This will not catch subclassing old-style classes.
ctx.api.fail("attrs only works with new-style classes", info.defn)
return
if kw_only:
ctx.api.fail("kw_only is not supported in Python 2", ctx.reason)
return

attributes = _analyze_class(ctx, auto_attribs)
attributes = _analyze_class(ctx, auto_attribs, kw_only)

# Save the attributes so that subclasses can reuse them.
ctx.cls.info.metadata['attrs'] = {
Expand Down Expand Up @@ -219,13 +231,15 @@ def _get_frozen(ctx: 'mypy.plugin.ClassDefContext') -> bool:
return False


def _analyze_class(ctx: 'mypy.plugin.ClassDefContext', auto_attribs: bool) -> List[Attribute]:
def _analyze_class(ctx: 'mypy.plugin.ClassDefContext',
auto_attribs: bool,
kw_only: bool) -> List[Attribute]:
Copy link
Member

Choose a reason for hiding this comment

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

Could you please document kw_only (and auto_attribs) in the docstring?

"""Analyze the class body of an attr maker, its parents, and return the Attributes found."""
own_attrs = OrderedDict() # type: OrderedDict[str, Attribute]
# Walk the body looking for assignments and decorators.
for stmt in ctx.cls.defs.body:
if isinstance(stmt, AssignmentStmt):
for attr in _attributes_from_assignment(ctx, stmt, auto_attribs):
for attr in _attributes_from_assignment(ctx, stmt, auto_attribs, kw_only):
# When attrs are defined twice in the same body we want to use the 2nd definition
# in the 2nd location. So remove it from the OrderedDict.
# Unless it's auto_attribs in which case we want the 2nd definition in the
Expand Down Expand Up @@ -264,19 +278,34 @@ def _analyze_class(ctx: 'mypy.plugin.ClassDefContext', auto_attribs: bool) -> Li
# Check the init args for correct default-ness. Note: This has to be done after all the
# attributes for all classes have been read, because subclasses can override parents.
last_default = False
last_kw_only = False

for attribute in attributes:
if attribute.init:
if not attribute.has_default and last_default:
ctx.api.fail(
"Non-default attributes not allowed after default attributes.",
attribute.context)
last_default |= attribute.has_default
if not attribute.init:
continue

if attribute.kw_only:
# Keyword-only attributes don't care whether they are default or not.
last_kw_only = True
continue

if not attribute.has_default and last_default:
ctx.api.fail(
"Non-default attributes not allowed after default attributes.",
attribute.context)
if last_kw_only:
ctx.api.fail(
"Non keyword-only attributes are not allowed after a keyword-only attribute.",
attribute.context
)
last_default |= attribute.has_default

return attributes


def _attributes_from_assignment(ctx: 'mypy.plugin.ClassDefContext',
stmt: AssignmentStmt, auto_attribs: bool) -> Iterable[Attribute]:
stmt: AssignmentStmt, auto_attribs: bool,
kw_only: bool) -> Iterable[Attribute]:
"""Return Attribute objects that are created by this assignment.

The assignments can look like this:
Expand All @@ -300,11 +329,11 @@ def _attributes_from_assignment(ctx: 'mypy.plugin.ClassDefContext',
if (isinstance(rvalue, CallExpr)
and isinstance(rvalue.callee, RefExpr)
and rvalue.callee.fullname in attr_attrib_makers):
attr = _attribute_from_attrib_maker(ctx, auto_attribs, lhs, rvalue, stmt)
attr = _attribute_from_attrib_maker(ctx, auto_attribs, kw_only, lhs, rvalue, stmt)
if attr:
yield attr
elif auto_attribs and stmt.type and stmt.new_syntax and not is_class_var(lhs):
yield _attribute_from_auto_attrib(ctx, lhs, rvalue, stmt)
yield _attribute_from_auto_attrib(ctx, kw_only, lhs, rvalue, stmt)


def _cleanup_decorator(stmt: Decorator, attr_map: Dict[str, Attribute]) -> None:
Expand Down Expand Up @@ -337,17 +366,19 @@ def _cleanup_decorator(stmt: Decorator, attr_map: Dict[str, Attribute]) -> None:


def _attribute_from_auto_attrib(ctx: 'mypy.plugin.ClassDefContext',
kw_only: bool,
lhs: NameExpr,
rvalue: Expression,
stmt: AssignmentStmt) -> Attribute:
"""Return an Attribute for a new type assignment."""
# `x: int` (without equal sign) assigns rvalue to TempNode(AnyType())
has_rhs = not isinstance(rvalue, TempNode)
return Attribute(lhs.name, ctx.cls.info, has_rhs, True, Converter(), stmt)
return Attribute(lhs.name, ctx.cls.info, has_rhs, True, kw_only, Converter(), stmt)


def _attribute_from_attrib_maker(ctx: 'mypy.plugin.ClassDefContext',
auto_attribs: bool,
kw_only: bool,
lhs: NameExpr,
rvalue: CallExpr,
stmt: AssignmentStmt) -> Optional[Attribute]:
Expand All @@ -367,6 +398,11 @@ def _attribute_from_attrib_maker(ctx: 'mypy.plugin.ClassDefContext',

# Read all the arguments from the call.
init = _get_bool_argument(ctx, rvalue, 'init', True)
kw_only |= _get_bool_argument(ctx, rvalue, 'kw_only', False)
Copy link
Member

Choose a reason for hiding this comment

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

I noticed this (a bit counterintuitive) behavior:

import attr
@attr.s(kw_only=True)
class A:
    a = attr.ib(kw_only=False)

A(1)  # Error

The logic here captures it correctly, but I would add a short comment that this is intentional.

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 filed an issue in attrs to see if this is expected: python-attrs/attrs#481

if kw_only and ctx.api.options.python_version[0] < 3:
ctx.api.fail("kw_only is not supported in Python 2", stmt)
Copy link
Member

Choose a reason for hiding this comment

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

I would avoid repeating the same error message, and instead define a constant.

return None

# TODO: Check for attr.NOTHING
attr_has_default = bool(_get_argument(rvalue, 'default'))
attr_has_factory = bool(_get_argument(rvalue, 'factory'))
Expand Down Expand Up @@ -400,7 +436,7 @@ def _attribute_from_attrib_maker(ctx: 'mypy.plugin.ClassDefContext',
converter = convert
converter_info = _parse_converter(ctx, converter)

return Attribute(lhs.name, ctx.cls.info, attr_has_default, init, converter_info, stmt)
return Attribute(lhs.name, ctx.cls.info, attr_has_default, init, kw_only, converter_info, stmt)


def _parse_converter(ctx: 'mypy.plugin.ClassDefContext',
Expand Down
86 changes: 86 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 @@ -934,3 +934,89 @@ T = TypeVar("T", bytes, str)
class A(Generic[T]):
v: T
[builtins fixtures/attr.pyi]

[case testAttrsKwOnlyAttrib]
import attr
@attr.s
class A:
a = attr.ib(kw_only=True)
A() # E: Missing named argument "a" for "A"
A(15) # E: Too many positional arguments for "A"
A(a=15)
[builtins fixtures/attr.pyi]

[case testAttrsKwOnlyClass]
import attr
@attr.s(kw_only=True, auto_attribs=True)
class A:
a: int
b: bool
A() # E: Missing named argument "a" for "A" # E: Missing named argument "b" for "A"
A(b=True, a=15)
[builtins fixtures/attr.pyi]

[case testAttrsKwOnlyClassNoInit]
import attr
@attr.s(kw_only=True)
class B:
a = attr.ib(init=False)
b = attr.ib()
B(b=True)
[builtins fixtures/attr.pyi]

[case testAttrsKwOnlyWithDefault]
import attr
@attr.s
class C:
a = attr.ib(0)
b = attr.ib(kw_only=True)
c = attr.ib(16, kw_only=True)
C(b=17)
[builtins fixtures/attr.pyi]

[case testAttrsKwOnlyClassWithMixedDefaults]
import attr
@attr.s(kw_only=True)
class D:
a = attr.ib(10)
b = attr.ib()
c = attr.ib(15)
D(b=17)
[builtins fixtures/attr.pyi]


[case testAttrsKwOnlySubclass]
import attr
@attr.s
class A2:
a = attr.ib(default=0)
@attr.s
class B2(A2):
b = attr.ib(kw_only=True)
B2(b=1)
[builtins fixtures/attr.pyi]

[case testAttrsNonKwOnlyAfterKwOnly]
import attr
@attr.s(kw_only=True)
class A:
a = attr.ib(default=0)
@attr.s
class B(A):
b = attr.ib() # E: Non keyword-only attributes are not allowed after a keyword-only attribute.
@attr.s
class C:
a = attr.ib(kw_only=True)
b = attr.ib(15) # E: Non keyword-only attributes are not allowed after a keyword-only attribute.
[builtins fixtures/attr.pyi]

[case testAttrsKwOnlyPy2]
# flags: --py2
import attr
@attr.s(kw_only=True) # E: kw_only is not supported in Python 2
class A(object):
x = attr.ib()
@attr.s
class B(object):
x = attr.ib(kw_only=True) # E: kw_only is not supported in Python 2
[builtins_py2 fixtures/bool.pyi]
31 changes: 31 additions & 0 deletions test-data/unit/fine-grained.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1001,6 +1001,37 @@ class A:
==
main:2: error: Unsupported left operand type for < ("B")

[case testAttrsUpdateKwOnly]
[file a.py]
import attr
@attr.s(kw_only=True)
class A:
a = attr.ib(15) # type: int

[file b.py]
from a import A
import attr
@attr.s(kw_only=True)
class B(A):
b = attr.ib("16") # type: str

B(b="foo", a=7)

[file b.py.2]
from a import A
import attr
@attr.s()
class B(A):
b = attr.ib("16") # type: str

B(b="foo", a=7)

Copy link
Member

Choose a reason for hiding this comment

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

This empty line is not needed.

[builtins fixtures/attr.pyi]
[out]
==
b.py:5: error: Non keyword-only attributes are not allowed after a keyword-only attribute.
Copy link
Member

Choose a reason for hiding this comment

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

This test doesn't test much of incremental logic. I would also add a test where base class in a.py is updated, while subclass is unchanged.



Copy link
Member

Choose a reason for hiding this comment

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

We prefer single empty line between tests.

[case testAddBaseClassMethodCausingInvalidOverride]
import m
class B(m.A):
Expand Down
16 changes: 14 additions & 2 deletions test-data/unit/lib-stub/attr.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ def attrib(default: None = ...,
type: None = ...,
converter: None = ...,
factory: None = ...,
kw_only: bool = ...,
) -> Any: ...
# This form catches an explicit None or no default and infers the type from the other arguments.
@overload
Expand All @@ -35,6 +36,7 @@ def attrib(default: None = ...,
type: Optional[Type[_T]] = ...,
converter: Optional[_ConverterType[_T]] = ...,
factory: Optional[Callable[[], _T]] = ...,
kw_only: bool = ...,
) -> _T: ...
# This form catches an explicit default argument.
@overload
Expand All @@ -49,6 +51,7 @@ def attrib(default: _T,
type: Optional[Type[_T]] = ...,
converter: Optional[_ConverterType[_T]] = ...,
factory: Optional[Callable[[], _T]] = ...,
kw_only: bool = ...,
) -> _T: ...
# This form covers type=non-Type: e.g. forward references (str), Any
@overload
Expand All @@ -63,6 +66,7 @@ def attrib(default: Optional[_T] = ...,
type: object = ...,
converter: Optional[_ConverterType[_T]] = ...,
factory: Optional[Callable[[], _T]] = ...,
kw_only: bool = ...,
) -> Any: ...

@overload
Expand All @@ -75,8 +79,12 @@ def attrs(maybe_cls: _C,
init: bool = ...,
slots: bool = ...,
frozen: bool = ...,
weakref_slot: bool = ...,
str: bool = ...,
auto_attribs: bool = ...) -> _C: ...
auto_attribs: bool = ...,
kw_only: bool = ...,
cache_hash: bool = ...,
) -> _C: ...
@overload
def attrs(maybe_cls: None = ...,
these: Optional[Mapping[str, Any]] = ...,
Expand All @@ -87,8 +95,12 @@ def attrs(maybe_cls: None = ...,
init: bool = ...,
slots: bool = ...,
frozen: bool = ...,
weakref_slot: bool = ...,
str: bool = ...,
auto_attribs: bool = ...) -> Callable[[_C], _C]: ...
auto_attribs: bool = ...,
kw_only: bool = ...,
cache_hash: bool = ...,
Copy link
Member

Choose a reason for hiding this comment

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

I noticed that some of these are missing in typeshed. Could you please make a typeshed update PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that attrs includes stubs in the package maybe removing them from typeshed is better. python-attrs/attrs#480

) -> Callable[[_C], _C]: ...


# aliases
Expand Down