Make kw_only=True behavior consistent with dataclasses

[huzecong]

Summary

This PR resolve #980.

Before this change

When kw_only=True is set on a class, all attributes are made keyword-only, including those from base classes. If an attribute sets kw_only=False, that setting is ignored and it is still made keyword-only.

After this change

When kw_only=True is set on a class, only the attributes defined in that class that doesn't explicitly set kw_only=False are made keyword-only. Notably this is also the behavior for dataclasses.

See TestKeywordOnlyAttributes.{test_kw_only_inheritance,test_kw_only_inheritance_force_kw_only} for an example of the old and new behaviors.

Implementation

The default for kw_only in attr.ib/attrs.field is changed from False to None. When set to None, the attribute's kw_only mirrors the value set on the class.

An additional back-compat force_kw_only argument is added to attr.s/attrs.define. When set to True, the old behavior is restored for the class. The default is False (new behavior) for attrs.define and True (old behavior) for attr.s.

Backwards compatibility

This change makes certain attributes that were previously kw-only no longer kw-only. As such, all call sites where we construct an instance of an attrs class should be backwards compatible.

If no attribute in an attrs class and all of its base classes explicitly sets kw_only=False, then that class is also backwards compatible.

If there are kw_only=False attributes, then it is possible for the class to fail on import due to attribute ordering. For example:

@attrs.define(kw_only=True) class A: a: int b: int = attrs.field(default=1, kw_only=False) @attrs.define class B(A): c: int

Previously, B.__init__ would have the signature (self, c, *, a, b=...), but with the new behavior this is wrong because c follows b, a non-kw-only attribute with a default.

My hope is that this is rare enough --- since kw_only=False was the default on attr.ib/attrs.field and doesn't actually do anything when the class sets kw_only=True, very few users should've set it. A quick scan in my work code base, which extensively uses attrs with thousands of attrs-decorated classes, shows only one usage of field-level kw_only=False.

Pull Request Check List

• Do not open pull requests from your main branch – use a separate branch!
  • There's a ton of footguns waiting if you don't heed this warning. You can still go back to your project, create a branch from your main branch, push it, and open the pull request from the new branch.
  • This is not a pre-requisite for your pull request to be accepted, but you have been warned.
• Added tests for changed code.
  Our CI fails if coverage is not 100%.
• New features have been added to our Hypothesis testing strategy.
• Changes or additions to public APIs are reflected in our type stubs (files ending in .pyi).
  • ...and used in the stub test file tests/typing_example.py.
  • If they've been added to attr/__init__.pyi, they've also been re-imported in attrs/__init__.pyi.
• Updated documentation for changed code.
  • New functions/classes have to be added to docs/api.rst by hand.
  • Changes to the signatures of @attr.s() and @attrs.define() have to be added by hand too.
  • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives.
    The next version is the second number in the current release + 1.
    The first number represents the current year.
    So if the current version on PyPI is 22.2.0, the next version is gonna be 22.3.0.
    If the next version is the first in the new year, it'll be 23.1.0.
    • If something changed that affects both attrs.define() and attr.s(), you have to add version directives to both.
• Documentation in .rst and .md files is written using semantic newlines.
• Changes (and possible deprecations) have news fragments in changelog.d.
• Consider granting push permissions to the PR branch, so maintainers can fix minor issues themselves without pestering you.

[huzecong]

Additionally, changes also need to be made for type-checkers / IDEs to support this new behavior, although I'm not too concerned because most of them aren't currently fully correct anyway. And also, the behavior we're changing to is the same as dataclasses behavior.

Here's an example to illustrate:

import inspect import attrs from typing import TYPE_CHECKING @attrs.define() class A: a_1: int a_2: int @attrs.define(kw_only=True) class B(A): b_1: int = 1 b_2: int = attrs.field(kw_only=False) @attrs.define() class C(B): c_1: int c_2: int = 2 print(inspect.signature(A.__init__)) print(inspect.signature(B.__init__)) print(inspect.signature(C.__init__)) if TYPE_CHECKING: reveal_type(A.__init__) reveal_type(B.__init__) reveal_type(C.__init__) 

---

The correct signatures, before this change, are:

(self, a_1: int, a_2: int) -> None (self, *, a_1: int, a_2: int, b_1: int = 1, b_2: int) -> None (self, a_1: int, a_2: int, c_1: int, c_2: int = 2, *, b_1: int = 1, b_2: int) -> None 

and after:

(self, a_1: int, a_2: int) -> None (self, a_1: int, a_2: int, b_2: int, *, b_1: int = 1) -> None (self, a_1: int, a_2: int, b_2: int, c_1: int, c_2: int = 2, *, b_1: int = 1) -> None 

---

Here's what mypy shows:

untitled.py:25: note: Revealed type is "def (self: untitled.A, a_1: builtins.int, a_2: builtins.int)" untitled.py:26: note: Revealed type is "def (self: untitled.B, a_1: builtins.int, a_2: builtins.int, *, b_1: builtins.int =, b_2: builtins.int)" untitled.py:27: note: Revealed type is "def (self: untitled.C, a_1: builtins.int, a_2: builtins.int, c_1: builtins.int, c_2: builtins.int =, *, b_1: builtins.int =, b_2: builtins.int)" 

So mypy converts all attributes in the current class, ignoring attribute-level kw_only=False settings, but doesn't go into base classes.

pyre and pyright doesn't even seem to support attrs.

---

Here's what PyCharm shows for autocompletion:

(a_1, a_2) (*, a_1, a_2, b_1, b_2) (c_1, c_2, *, a_1, a_2, b_1, b_2) 

This feels like the old collect_by_mro=False algorithm.

---

Here's what VSCode shows for autocompletion:

(a_1: int, a_2: int) -> A (a_1: int, a_2: int, b_2: int, *, b_1: int = 1) -> B (a_1: int, a_2: int, b_2: int, c_1: int, c_2: int = 2, *, b_1: int = 1) -> C 

which interestingly is our desired after behavior. My guess is that the VSCode LSP has support dataclass_transform, so it followed dataclasses behavior, which is what we're changing to.

[hynek]

The reason why docs are failing is because you're using

`kw_only` 

which is is short for

:any:`kw_only` 

Please use asterisks when referring to arguments. Sadly, there's no good way to have Markdown docstrings for now.

[hynek]

Thank you, this is an absolute amazing PR, including the supplied comments.

Any reason why it is a Draft? The changelog is gonna be important, but other than that it seems fine, I think?

As you write and as far as I can tell, the only important case we will be breaking here is:

@attrs.define(kw_only=True) class A: a: int b: int = attrs.field(default=1, kw_only=False) @attrs.define class B(A): c: int

Everything else would only get weakened and keep working.

The use-case is very much up Hyrum's wheelhouse, but also an accidental thing that works that shouldn't and that can be fixed on both ends: either making A force_kw_only or adding kw_only to B, so there is an easy way forward.

---

I think the biggest question here is how fast @Tinche (presumably) will be able to get Mypy fixed. Pyright does support attrs btw, but also only via dataclass transform, so it makes sense to move our modern APIs there.

---

The biggest attrs users I know of are Airflow and Hypothesis so I guess it would make sense to ping @ashb and @Zac-HD whether they think this is something problematic.

[Zac-HD]

👋 happy to chime in - as always, thanks @hynek for a great library, and thanks @huzecong for a great patch!

Hypothesis turns out not to use kw_only=False at all... and in fact when I search GitHub, setting it on a field seems extremely rare.

• caveat: the search misses multi-line calls to attrs.field(), which are probably systematically more complex... but so rare I still feel fine about going forward.
• It also looks to me like the airflow use is safe with respect to this change, though I'd suggest one of you checking; and at that point I guess you might as well check the other eighteen files on all of public github!
• Overall this seems like a nice semantic change, with minimal compat implications in practice.

My main concern is actually that using non-kw-only arguments with inheritance sounds super confusing to me, but it also seems reasonable to make that users' problem. (I'd just make kw-only=True the default 😉)

[huzecong]

Thanks for the reviews and suggestions!

@hynek Your analysis of the breaking case is correct and I agree that the workaround is simple. I also agree that fixing mypy is essential for the adoption of the new feature, but it's also not a blocker. And good to know that pyright supports it via dataclass transform!

I left the PR as a draft because I haven't had time to write new docs and the changelog etc. I'll find time for it later today or tomorrow, and then take it out of draft. Code should be ready!

@Zac-HD Thanks for the pointer! I changed to regex to /(attrs?\.field|attr\.?ib)\(\n?.*kw_only=False.*\n?\)/ to cover old-style APIs and simple line-wrapped cases, and found 30 results. I went through all of them and I don't think any of them would be affected:

• Many results are just people putting a copy of airflow or other libraries inside their repo. And I agree that the airflow usage is safe.
• A lot of usages aren't even setting kw_only=True on the containing class. I guess some users prefer explicitness.
• Among the rest with kw_only=True classes, none of them have a kw_only=False subclass. Also none of them look like abstract base classes that users would subclass, but I can't be sure.
  This was a lot fewer usages than I expected and that's great!

Also I agree that mixing kw_only=False/True is confusing. The concrete usage I had that prompted the original issue was:

• We have a base class with a few required attributes (e.g. name, version), and bunch of subclasses each adding its own custom attributes, all with reasonable defaults. Think a bunch of configuration classes.
• All the subclass attributes have kw_only=True, partly because some base class attrs have defaults, and partly because they're very specific and would be confusing to not be kwargs.
• But because we still want the base class attributes to be not kw-only (so you could write MyConfig("NAME", 2) instead of name="NAME", version=2; not a big deal but users really like that), we have to mark the subclasses as kw_only=False and set kw_only=True on the subclass attributes.
• With this new feature landed, we could just set kw_only=True on subclasses and that won't affect base class attributes.

[huzecong]

Turns out there's not much docs & changelog changes :)

[hynek]

We need to expand this a bit, and the file name must be changed from change to breaking, such that it jumps out at people.

The first line is OK, but we should add the one example that we know that's gonna break and how to work around it. I absolutely hate it, when a package says they broke something and won't tell me how to fix it. 😅

Maybe lead with something like "This shouldn't be a problem for most users, unless …"?

[huzecong]

I added a description of what exactly has changed and the breaking example above. Do you think this is too verbose, or is it good?

[hynek]

it's a tiny bit verbose and doesn't do semantic new lines 🤓 but it's 100% fine – I'll merge and fumble around until I'm happy myself.

thanks for your endless patience! 💛

[huzecong]

Haha happy to contribute to the library! And thanks for the discussions and reviews. I'll read up on this semantic new line practice too 😁

[ashb]

Yeah I'm with @Zac-HD here, and these changes as described fine for Airflow, and in the very very unlikely event it breaks we'll fix our code

[hynek]

JFTR, I've looked at another notable attrs user https://github.com/python-trio/trio and couldn't find any dangerous/relevant use of kw_only. The tests pass with current main too, so I hope they should be fine, too.

aiohttp uses only old-school attr.s that hasn't changed and doesn't use kw_only at all.