Suggestion: Overriding class-level kw_only=True option in attributes

[huzecong]

First of all, thank you for building such a wonderful package! It is now an essential part of both my work and my hobby projects.

This issue is similar to #481, but I think it's more suitable to open a new issue for discussion instead of reviving a 4-year-old thread.

kw_only can be set both on the class decorator and on attributes. When kw_only=False on the class (this is the default), one can set kw_only=True on attributes to make only those attributes keyword-only in the constructor. However, when kw_only=True on the class, all attributes (including those defined in base classes) become keyword-only, and setting kw_only=False on attributes will not work.

The following code snippet implements the logic described above:

attrs/src/attr/_make.py

Lines 564 to 566 in 7091b1f

	if kw_only:
	own_attrs = [a.evolve(kw_only=True) for a in own_attrs]
	base_attrs = [a.evolve(kw_only=True) for a in base_attrs]

In my opinion, this is a little confusing, since many of the other options that exist on both the class and attributes do give you a way to opt-out for a subset of attributes. For example, eq (and friends), repr, and init all allow you to exclude an attribute from the auto-generated functions. It feels natural to assume the same for kw_only; I don't have exact stats, but I know quite a few coworkers who were surprised by the current behavior.

It would also be useful to allow overriding. For example, I have a class whose ideal constructor signature should look like this:

SomeObject(x, y, *, kw1=..., kw2=..., kw3=..., ...)  # imagine there's a lot more kw-only args

where x and y are required, and all the rest are optional with defaults, usually some obscure setting that can be tweaked but are rarely tweaked. To implement such an interface, I would have to set kw_only=False on the class, and kw_only=True on most of the attributes. To make matters worse, I need to do the same for all new attributes in all of its subclasses as well. It would be beneficial if we could just set kw_only=True on the class, and override a few attributes with kw_only=False.

---

If we are to allow overriding kw_only on attributes, this is how I imagine it should behave:

• The class's kw_only defaults to False, as it does today.
• The attribute's kw_only defaults to None. If it is set to True or False, then it stays kw-only or non-kw-only regardless of the class's setting; if it is not set (None), then it reflects the class's kw_only value.
• Note that this also means that a subclass cannot control the kw_only-ness of its base attributes.

I'm open for debate on the semantics and I'd love input on how this can be improved or made more natural or useful.

Now, I would love to put up a pull request for this change, but I realize that this would break backwards-compability, so I'd like to seek permission from maintainers before I go ahead and do this. I'm also open to introducing a new class-level option (e.g. kw_only_overridable, or something less horrendous) that's disabled by default, similar to how collect_by_mro works. Please let me know if you think this proposal makes sense, and how we should move forward. Thanks!

[hynek]

That seems like the common semantics for other parameters, where do you expect things to break backwards-compatibility?

[huzecong]

That seems like the common semantics for other parameters, where do you expect things to break backwards-compatibility?

I agree it's common semantics for other parameters, but the current state is that kw_only doesn't follow the semantics, so changing that would result in different behaviors for existing code.

Here's a hypothetical example:

@attrs.define
class Base:
  a: int = attrs.field()
  b: int = attrs.field(kw_only=False)
  c: int = attrs.field(kw_only=True)

@attrs.define(kw_only=True)
class Derived1(Base):
  pass

@attrs.define(kw_only=False)
class Derived2(Base):
  pass

Base and Derived2 would stay the same regardless of the proposed change. But for Derived1:

• In the current state, it would have all three fields as kw-only, despite setting kw_only=False on the field b.
• If we change the semantics, only a and c would be kw-only, and we'd get an exception because the non-kw-only field b is after a kw-only field.

Although no one would write code like this, it illustrates a case where the behavior differs if we change the semantics of kw_only.

Of course, I support making the change since I think it's more intuitive and could reduce boilerplate code for some of my projects, but at the same time I realize that attrs has a backwards-compatible guarantee and I don't want to break that. I'm just wondering what's the best thing to do here.

[hynek]

How about – and I suspect that's how it's handled elsewhere – we bake the kw_only-ness into a class's Attribute when they're created?

In your example a would become kw_only=False because that's what it is. Subclasses wouldn't know how it became what it is.

[huzecong]

Right, that's how I would change it as well. I actually had a patch locally that looks like this — a very simple change:

diff --git a/src/attr/_make.py b/src/attr/_make.py
index 4d1afe3..6faf525 100644
--- a/src/attr/_make.py
+++ b/src/attr/_make.py
@@ -109,7 +109,7 @@ def attrib(
     type=None,
     converter=None,
     factory=None,
-    kw_only=False,
+    kw_only=None,
     eq=None,
     order=None,
     on_setattr=None,
@@ -561,9 +561,10 @@ def _transform_attrs(
             cls, {a.name for a in own_attrs}
         )

-    if kw_only:
-        own_attrs = [a.evolve(kw_only=True) for a in own_attrs]
-        base_attrs = [a.evolve(kw_only=True) for a in base_attrs]
+    own_attrs = [
+        a.evolve(kw_only=kw_only) if a.kw_only is None else a
+        for a in own_attrs
+    ]

     attrs = base_attrs + own_attrs

To reiterate, I think it's totally possible (and quite easy) to allow such behavior, and I think such behavior is more intuitive than the current state. However, as my previous example, the change would be backwards-incompatible in certain cases, and I was looking for guidance on how to handle that.

[hynek]

Sorry for being obtuse, but how is your example backwards-incompatible? If you bake your Attributes, a shouldn't change unless you re-define it?

[huzecong]

No worries! Sorry for not explaining this properly. You're right that in my example, a.kw_only doesn't change unless being redefined. However, that is not what's happening in the current implementation:

attrs/src/attr/_make.py

Lines 542 to 544 in c860e9d

	if kw_only:
	own_attrs = [a.evolve(kw_only=True) for a in own_attrs]
	base_attrs = [a.evolve(kw_only=True) for a in base_attrs]

In the current implementation, if a subclass sets kw_only=True on the class, kw_only is overridden for all attributes, whether it's defined in this class or a base class. This is also the example that I gave above, where the behavior for Derived1 would be different were we to make this change. I do think it's a change for the better, but I'd just want to make it clear that this would be backwards-incompatible.

Let me know if you have questions, or maybe if I'm missing something!

[hynek]

Sorry for the delay, I had to get structlog out of the door and I'm bad at deep multi-tasking. 🙈

I think I'm starting to sense what you consider breaking here: that if someone set kw_only=False, it's currently ignored but would trigger then.

Of course there's Hyrum's law, but kw_only=False is currently meaningless and shouldn't be set by anyone on attribute-level.

We had 100 million downloads per month less back then, but IIRC, we've always did the same move in this case: switch defaults to None and let them still mean what the old default was. Then add explicit meaning.

---

Have you tried your changes (haven't looked too closely at them tho) and checked if our test suite still passes? 🤔

[huzecong]

No worries!

Have you tried your changes (haven't looked too closely at them tho) and checked if our test suite still passes? 🤔

So the following is the patch I applied on top of the current master:

diff --git a/src/attr/__init__.pyi b/src/attr/__init__.pyi
index 7aff65c..ea2ad21 100644
--- a/src/attr/__init__.pyi
+++ b/src/attr/__init__.pyi
@@ -174,7 +174,7 @@ def attrib(
     type: None = ...,
     converter: None = ...,
     factory: None = ...,
-    kw_only: bool = ...,
+    kw_only: Optional[bool] = ...,
     eq: Optional[_EqOrderType] = ...,
     order: Optional[_EqOrderType] = ...,
     on_setattr: Optional[_OnSetAttrArgType] = ...,
@@ -195,7 +195,7 @@ def attrib(
     type: Optional[Type[_T]] = ...,
     converter: Optional[_ConverterType] = ...,
     factory: Optional[Callable[[], _T]] = ...,
-    kw_only: bool = ...,
+    kw_only: Optional[bool] = ...,
     eq: Optional[_EqOrderType] = ...,
     order: Optional[_EqOrderType] = ...,
     on_setattr: Optional[_OnSetAttrArgType] = ...,
@@ -215,7 +215,7 @@ def attrib(
     type: Optional[Type[_T]] = ...,
     converter: Optional[_ConverterType] = ...,
     factory: Optional[Callable[[], _T]] = ...,
-    kw_only: bool = ...,
+    kw_only: Optional[bool] = ...,
     eq: Optional[_EqOrderType] = ...,
     order: Optional[_EqOrderType] = ...,
     on_setattr: Optional[_OnSetAttrArgType] = ...,
@@ -235,7 +235,7 @@ def attrib(
     type: object = ...,
     converter: Optional[_ConverterType] = ...,
     factory: Optional[Callable[[], _T]] = ...,
-    kw_only: bool = ...,
+    kw_only: Optional[bool] = ...,
     eq: Optional[_EqOrderType] = ...,
     order: Optional[_EqOrderType] = ...,
     on_setattr: Optional[_OnSetAttrArgType] = ...,
@@ -252,7 +252,7 @@ def field(
     metadata: Optional[Mapping[Any, Any]] = ...,
     converter: None = ...,
     factory: None = ...,
-    kw_only: bool = ...,
+    kw_only: Optional[bool] = ...,
     eq: Optional[bool] = ...,
     order: Optional[bool] = ...,
     on_setattr: Optional[_OnSetAttrArgType] = ...,
@@ -272,7 +272,7 @@ def field(
     metadata: Optional[Mapping[Any, Any]] = ...,
     converter: Optional[_ConverterType] = ...,
     factory: Optional[Callable[[], _T]] = ...,
-    kw_only: bool = ...,
+    kw_only: Optional[bool] = ...,
     eq: Optional[_EqOrderType] = ...,
     order: Optional[_EqOrderType] = ...,
     on_setattr: Optional[_OnSetAttrArgType] = ...,
@@ -291,7 +291,7 @@ def field(
     metadata: Optional[Mapping[Any, Any]] = ...,
     converter: Optional[_ConverterType] = ...,
     factory: Optional[Callable[[], _T]] = ...,
-    kw_only: bool = ...,
+    kw_only: Optional[bool] = ...,
     eq: Optional[_EqOrderType] = ...,
     order: Optional[_EqOrderType] = ...,
     on_setattr: Optional[_OnSetAttrArgType] = ...,
@@ -310,7 +310,7 @@ def field(
     metadata: Optional[Mapping[Any, Any]] = ...,
     converter: Optional[_ConverterType] = ...,
     factory: Optional[Callable[[], _T]] = ...,
-    kw_only: bool = ...,
+    kw_only: Optional[bool] = ...,
     eq: Optional[_EqOrderType] = ...,
     order: Optional[_EqOrderType] = ...,
     on_setattr: Optional[_OnSetAttrArgType] = ...,
diff --git a/src/attr/_make.py b/src/attr/_make.py
index 4e6846a..6ee2914 100644
--- a/src/attr/_make.py
+++ b/src/attr/_make.py
@@ -97,7 +97,7 @@ def attrib(
     type=None,
     converter=None,
     factory=None,
-    kw_only=False,
+    kw_only=None,
     eq=None,
     order=None,
     on_setattr=None,
@@ -545,9 +545,10 @@ def _transform_attrs(
             cls, {a.name for a in own_attrs}
         )
 
-    if kw_only:
-        own_attrs = [a.evolve(kw_only=True) for a in own_attrs]
-        base_attrs = [a.evolve(kw_only=True) for a in base_attrs]
+    own_attrs = [
+        a.evolve(kw_only=kw_only) if a.kw_only is None else a
+        for a in own_attrs
+    ]
 
     attrs = base_attrs + own_attrs
 
diff --git a/src/attr/_next_gen.py b/src/attr/_next_gen.py
index 79e8a44..7160e9c 100644
--- a/src/attr/_next_gen.py
+++ b/src/attr/_next_gen.py
@@ -165,7 +165,7 @@ def field(
     metadata=None,
     converter=None,
     factory=None,
-    kw_only=False,
+    kw_only=None,
     eq=None,
     order=None,
     on_setattr=None,

All but one test pass. The failing test is tests/test_make.py::TestTransformAttrs::test_kw_only, which tests the behavior of kw_only, and it's kind of expected that it fails.

I'm actually not sure what's the better behavior, with respect to base classes. If a base class A sets kw_only=False and a subclass B sets kw_only=True, and all of their attributes have kw_only=None (defaulting to the class's setting), what should the behavior be? I can think of three different options:

• A class's kw_only setting only affects its own attributes. In this case, B's own attributes will have kw_only=True, while the inherited attributes from A will still have kw_only=False.
• A class's kw_only setting affects its own & all of its base attributes that has kw_only=None.
• To be even fancier, we can allow kw_only=None on classes. Then a class's kw_only setting (if not None) will affect its own & all of its base attributes whose class sets kw_only=None.

What I implemented is option 1, just because it's easiest to implement. Of the other two options, option 2 makes more sense to me, while option 3 sounds unnecessarily complicated. Curious to know what you think.

I think I'm starting to sense what you consider breaking here: that if someone set kw_only=False, it's currently ignored but would trigger then.

Of course there's Hyrum's law, but kw_only=False is currently meaningless and shouldn't be set by anyone on attribute-level.

We had 100 million downloads per month less back then, but IIRC, we've always did the same move in this case: switch defaults to None and let them still mean what the old default was. Then add explicit meaning.

So I think that the only case where this would be backwards-incompatible is if someone has a kw_only=True class with an attribute that explicitly sets kw_only=False. Since both are explicitly set, I'm not sure switching defaults here would help. I personally don't care too much about this use case, but this does count as backwards-incompatible and I'm afraid that I might break random code out there. But if you're not too worried about this, I can move forward with a PR.