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.

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

Jump to bottom

[attrs plugin] Support kw_only=True #6107

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 all commits
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
79 changes: 61 additions & 18 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 All @@ -25,6 +25,7 @@
if MYPY:
from typing_extensions import Final

KW_ONLY_PYTHON_2_UNSUPPORTED = "kw_only is not supported in Python 2"

# The names of the different functions that create classes or arguments.
attr_class_makers = {
Expand Down Expand Up @@ -56,12 +57,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 +134,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 +165,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 +190,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 +200,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_PYTHON_2_UNSUPPORTED, 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 +232,19 @@ def _get_frozen(ctx: 'mypy.plugin.ClassDefContext') -> bool:
return False


def _analyze_class(ctx: 'mypy.plugin.ClassDefContext', auto_attribs: bool) -> List[Attribute]:
"""Analyze the class body of an attr maker, its parents, and return the Attributes found."""
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.

auto_attribs=True means we'll generate attributes from type annotations also.
kw_only=True means that all attributes created here will be keyword only args in __init__.
"""
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 +283,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 +334,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 +371,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 +403,13 @@ def _attribute_from_attrib_maker(ctx: 'mypy.plugin.ClassDefContext',

# Read all the arguments from the call.
init = _get_bool_argument(ctx, rvalue, 'init', True)
# Note: If the class decorator says kw_only=True the attribute is ignored.
# See https://github.com/python-attrs/attrs/issues/481 for explanation.
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_PYTHON_2_UNSUPPORTED, stmt)
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 +443,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]
50 changes: 50 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,56 @@ 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

[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)
[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.


[case testAttrsUpdateBaseKwOnly]
from b import B
B(5)
[file a.py]
import attr
@attr.s()
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

[file a.py.2]
import attr
@attr.s(kw_only=True)
class A:
a = attr.ib(15) # type: int
[builtins fixtures/attr.pyi]
[out]
==
main:2: error: Too many positional arguments for "B"

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
Loading