Attrs: Feedback to next generation APIs

[hynek]

This is the issue to discuss the defaults and behavior of attr.define(), attr.mutable(), and attr.frozen() before they are finalized inside the new attrs namespace.

Feedback that we find justified will be spun out into separate issues.

Known issues:

• mypy doesn't know about the new-style APIs yet and there's nothing _we_ can do about it. However you can use this mypy plugin for now (for more details see https://www.attrs.org/en/stable/extending.html#wrapping-the-decorator). PR is submitted in https://github.com/python/mypy/pull/9396.
• __eq__ and __ne__ aren't correctly auto-detected (fixed) #670
• Unannotated attributes fail if define is called with arguments/parantheses (fixed) #673
• Inheriting from frozen classes requires to manually set on_setattr to None (fixed). https://github.com/python-attrs/attrs/issues/668#issuecomment-678689981)

[rouge8]

Overall I love the new APIs. When updating some code to use @attr.define instead of @attr.s(auto_attribs=True), I was surprised by one behavior of slotted classes -- you don't seem to be able to use unittest.mock.patch.object() on methods on instances of slotted classes. It's not a result of anything in attrs (in the code snippet below I don't use attrs at all), but it might be worth noting in the docs since making slots=True the default behavior means attrs will be how many people encounter this for the first time.

import unittest.mock

class Slotted:
    __slots__ = ()

    def method(self):
        return 1

s = Slotted()
assert s.method() == 1

with unittest.mock.patch.object(s, "method", return_value=3):
    assert s.method() == 3

# Errors with:
# Traceback (most recent call last):
#   File "/usr/local/Cellar/[email protected]/3.8.5/Frameworks/Python.framework/Versions/3.8/lib/python3.8/unittest/mock.py", line 1490, in __enter__
#     setattr(self.target, self.attribute, new_attr)
# AttributeError: 'Slotted' object attribute 'method' is read-only

[hynek]

Ah yeah, thanks for that reminder, I will add it to the glossary!

[euresti]

Hmm. I'm not sure I like the hybrid behavior.

Imagine I forget to annotate something. Now instead of getting a nice error at import time I have to wait until I try to create something.

from attr import define, field

@define()
class A:
    x: int
    y = field(8)

Oh this case is actually pretty bad because I might not catch it even if i do A(8)

[hynek]

I thought about this for a while too.

I decided it’s worth the comfort because the only possible confusing error state is the one you describe and it’s impossible to miss even short-term when an attribute is missing. I’m open to being convinced otherwise but ISTM that in practice this trade-off is worth it. 🤔

[hynek]

How about the following: auto_attrib=None = Hybrid, everything else means what it means now?

[euresti]

What's the default value? (I suggest auto_attribs=True but that's my personal preference because I always write type annotated code)

I'd also like it if auto_attrib=None would raise if it found a mixture of attribs and annotations. Is it ok to add this to _transform_attrs or is affecting attr.s not something we should do?

[hynek]

What's the default value? (I suggest auto_attribs=True but that's my personal preference because I always write type annotated code)

I'd also like it if auto_attrib=None would raise if it found a mixture of attribs and annotations. Is it ok to add this to _transform_attrs or is affecting attr.s not something we should do?

Well None would be default which means “guess”. I’m Camp True too, but I don’t want to punish the typing haters for whine attrs is currently the only refuge.

I don’t thing an error on a mix of attr.ib and annotations is feasible because there a valid use cases for that. Maybe raise an error but add an option to override it?

[euresti]

I don't really see a good use for this:

@define()
class A:
    x: int
    y = field(8)

A.x won't exist and you won't be able to add it because of __slots__. Worst of all, mypy will think it exists.

Perhaps this could be ok:

@define()
class A:
    x: int = 72
    y = field(8)

But one could argue that it should be:

@define()
class A:
    x: ClassVar[int] = 72
    y = field(8)

Because it's gonna be a Class attribute not an instance one.

[euresti]

Oh and thinking about it more auto_attribs=None is a fine default value. Because if I use annotations they'll work the way I want them. (i.e. I don't have to add auto_attribs=True everywhere.

[hynek]

To be clear: it’s 100% not gonna be necessary to pass slots=True or auto_attribs=True because that’s _my_ settings. 😅 The q now is how to handle your scenario. Truth to be told we can raise an error and when someone complains, we can still add an option to allow for it?

[euresti]

That option could be auto_attribs=False. :) Or did you want some kind of global disable?

[hynek]

God No 😅! I’m just trying to find a way where the common use case is not passing anything and it just works – ideally without exposing the users to sharp edges.

[euresti]

Just did a quick regex change on our code base, looking for trouble. I did find 2 interesting issues:

1. A blind replace of attr.dataclass with attr.define yielded this which raises an error.

@attr.define(frozen=True)
class Base:
   ...

ValueError: Frozen classes can't use on_setattr.

1. So I updated it to @attr.frozen. But then I hit another error:

@attr.frozen
class Base:
 ...

@attr.define
class Child(Base):
   ...

ValueError: Frozen classes can't use on_setattr.
This time on Child.

So one just has to treat frozen=True as a special case. Not a big thing but since we're talking about how the API feels.

[euresti]

I'm working on the mypy plugin but I am gonna need to know the final answer on how "hybrid" mode wants to work if there are both annotations and attribs. I think the options are:

1. Take only the ones that have = attr.ib()
2. Take only the annotated ones.
3. Take both.
4. Raise an exception.

Hmm. I thought #3 would be easy to implement (just don't raise the error) but it turns out we can't tell the order between annotated and un-annotated attributes.

class A:
   x: int
   y = attr.ib()

cls.__annotations__  ->   [x]
cls.__dict__.keys(). -> [y]

But which is actually first?

2 is kind of a weird one. Why throw away those attr.ib()s?

1 is relatively easy to implement. Because it's just a try catch.

4 Might require the user to add ClassVar in order to add it.

Oh I should note that I think this should still be fine:

@define
class A:
   x: int = field()
   y = field()

We shouldn't force you to have to annotate everything.
Hmm. That might make 4 harder to implement. :)

[hynek]

Just did a quick regex change on our code base, looking for trouble. I did find 2 interesting issues:

1. A blind replace of attr.dataclass with attr.define yielded this which raises an error.

@attr.define(frozen=True)
class Base:
   ...

ValueError: Frozen classes can't use on_setattr.

1. So I updated it to @attr.frozen. But then I hit another error:

@attr.frozen
class Base:
 ...

@attr.define
class Child(Base):
   ...

ValueError: Frozen classes can't use on_setattr.
This time on Child.

So one just has to treat frozen=True as a special case. Not a big thing but since we're talking about how the API feels.

Oof, both aren't great.

I guess we should set on_setattr=None and only use the default if the class isn't effectively frozen?

[hynek]

(I have moved the hybrid discussion to #676 since it's non-trivial and also very important)

[Drino]

I've played around with new generation API a little, trying to migrate a part of my project onto it to feel the taste.

First of all - little classes work perfectly and with less boilerplate (I almost always use dataclass(frozen=True)), that's great :)

But it also feels like slotted classes are bad choice for my case

• I sometimes use mixins to add a kw_only attrib (with converters and validators) to my class which results in multiple bases have instance lay-out conflict. I've ended up aliasing mixin = partial(define, slots=False, kw_only=True) for such classes, but I'm not sure if it's a great practice.
• I've also run into an issue with property if it's used for init=False attributes - I've got my property descriptor overridden by auto-generated slot descriptor :(

I hope attr.s/attr.dataclass wouldn't be removed or some sane shortcuts for dict-classes would be provided :)

[hynek]

attr.s/attr.dataclass definitely aren't going anywhere.

[genric]

Hi,
Why attr.field kw_only default is False?
https://github.com/python-attrs/attrs/blob/1e0e5664fb007cb8d22c5eb440468a2e252993ee/src/attr/_next_gen.py#L136
According to the docs(https://www.attrs.org/en/stable/api.html#attr.field) it should keyword-only, no? But it is the same as in:
https://github.com/python-attrs/attrs/blob/1e0e5664fb007cb8d22c5eb440468a2e252993ee/src/attr/_make.py#L110

[Drino]

Hi!
The documentation means that attr.field signature is keyword-only (all attributes are keyword-only - say, it's incorrect to write attr.field(5) and one should mention attribute name attr.field(default=5)).
The kw_only attribute default hadn't change and is False.

[genric]

Ah, ok :) I thought it was about kw_only being True by default(at least this is how I plan to use it most of the time). Thanks for clarification!

[Drino]

If your intention is to have a keyword-only constructor - you can use attr.s(kw_only=True) instead of passing argument to attr.ib manually all the time :)

[genric]

Yep :+1:, just had some weird constructs to work around pos/kw args with the older versions of attrs, so now migrating them to fields ...
Thanks again! Next generation APIs look great! (maybe some minor doc improvement :smiley: so people like me don't get confused)

[henryiii]

I'd second that, I thought it meant that changed for a second and was quite surprised at how large of a change would be, until I saw the * and guessed what it really meant.

[henryiii]

By the way, are the next gen APIs intentionally missing type info? Both define and field return Any, breaking the nice MyPy setup. Switching back to .ib and .s fixes it.

[euresti]

For field it shouldn't be returning Any there are overloads. Can you post the code of what you're seeing? As to define

mypy doesn't know about the new-style APIs yet and there's nothing we can do about it. However you can use this mypy plugin for now (for more details see https://www.attrs.org/en/stable/extending.html#wrapping-the-decorator). PR is submitted in python/mypy#9396.

The PR has been merged but there hasn't been a mypy release.

[henryiii]

Can you post the code of what you're seeing? As to define

Sure, I'm using pre-commit and mypy so don't have the original, but this minimal change back shows the problem: https://github.com/henryiii/hypernewsviewer/pull/1

The problem seems to be field is missing type annotations too; so later on it returns "Any" because MyPy can't deduce anything about it.

The PR has been merged but there hasn't been a mypy release.

MyPy releases are a little slow sometimes. Python 3.9 style list[int] is in master but not in a release yet either... :'( (really waiting for the int | float 3.10 syntax, though...)

[euresti]

Oh I see. You need to install the attrs package in the same virtual env as mypy is running from. This is because attrs ships its own stubs. There are attrs stubs in typeshed (which is shipped with mypy) but they are older and we're thinking of just removing them entirely. I'm not too familar with poetry so I'm not sure if there's a different way to configure that.

[henryiii]

There are attrs stubs in typeshed

ahh, I bet that explains it. I am at least some of the time running via pre-commit, and I don’t have attrs in the extra dependencies there. Though I really thought I saw this both ways. I’ll check tomorrow, but I bet that’s the problem, I didn’t know it could partially work with no attrs installed.