contextlib.contextmanager raises dataclasses.FrozenInstanceError when handling a frozen dataclass exception

[mattclay]

Bug report

In Python 3.11, raising a frozen dataclass as an exception inside a contextlib.contextmanager context will raise a dataclasses.FrozenInstanceError exception. This issue does not occur in Python 3.10.

This is a result of the changes made in #92202

Reproducer

import contextlib
import dataclasses


@dataclasses.dataclass(frozen=True)
class Notice(Exception):
    pass


@contextlib.contextmanager
def demo():
    yield


try:
    with demo():
        raise Notice()
except Notice:
    pass

Expected Result

$ python3.10 ./repro.py

Actual Result

$ python3.11 ./repro.py 
Traceback (most recent call last):
  File "/usr/lib/python3.11/contextlib.py", line 155, in __exit__
    self.gen.throw(typ, value, traceback)
  File "/home/mclay/code/mattclay/contextlib-repro/./repro.py", line 12, in demo
    yield
  File "/home/mclay/code/mattclay/contextlib-repro/./repro.py", line 17, in <module>
    raise Notice()
Notice

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/mclay/code/mattclay/contextlib-repro/./repro.py", line 16, in <module>
    with demo():
  File "/usr/lib/python3.11/contextlib.py", line 188, in __exit__
    exc.__traceback__ = traceback
    ^^^^^^^^^^^^^^^^^
  File "<string>", line 4, in __setattr__
dataclasses.FrozenInstanceError: cannot assign to field '__traceback__'

Your environment

$ python3.10 -V
Python 3.10.6
$ python3.11 -V
Python 3.11.0
$ source /etc/os-release; echo $PRETTY_NAME
Ubuntu 22.04.1 LTS
$ uname -a
Linux redstone 5.11.0-27-generic #29~20.04.1-Ubuntu SMP Wed Aug 11 15:58:17 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

[salomon-smekecohen]

A similar thing happens when trying to raise Thrift exceptions, since they are also immutable.

[ericvsmith]

Shouldn’t we just document that exceptions must allow attribute assignment? That is, they can’t be “frozen”.

[mattclay]

Is there a reason not to use this?

object.__setattr__(exc, '__traceback__', traceback)

Instead of the existing:

exc.__traceback__ = traceback

[ericvsmith]

I don't think we want to change all assignments to look like that. Where else in the stdlib would we need this, if we wanted to support frozen dataclasses everywhere? This also might break hashing assumptions. I'd rather just document that the classes must be mutable.

[mattclay]

@ericvsmith I only see 4 of those assignments in https://github.com/python/cpython/blob/main/Lib/contextlib.py -- was there something else you were referring to? As for hashing, at least the default hash implementation should be safe, since it is based on the dataclass fields.

[ericvsmith]

My concern is the precedent. I presume there are plenty of places in the stdlib where assignments are done to attributes on user-defined classes. I don't want to change every one of them to use this pattern of working around "frozen-ness".

Why is it so important that exception subclasses be frozen? And why wouldn't that argument apply to other user-defined classes used by the stdlib?

[mattclay]

My concern is the precedent. I presume there are plenty of places in the stdlib where assignments are done to attributes on user-defined classes. I don't want to change every one of them to use this pattern of working around "frozen-ness".

Would it make more sense for dataclasses to allow changing __traceback__ instead? That seems to be what is suggested here: #92118 (comment)

Why is it so important that exception subclasses be frozen?

It worked until 3.11. This seemed to be an unintentional side-effect of the change.

And why wouldn't that argument apply to other user-defined classes used by the stdlib?

User-defined classes could allow changing __traceback__, but the behavior of frozen dataclasses isn't user defined.

[nitzmahone]

Wouldn't removing __traceback__ from the field list in the generated __setattr__ condition here be sufficient to solve this?

[carljm]

Wouldn't removing __traceback__ from the field list in the generated __setattr__ condition here be sufficient to solve this?

No, because of the type(self) is cls condition which is ORed with the field-names condition.

The intent of the design is that a frozen dataclass freezes all attributes, including inherited ones, but a mutable subclass of a frozen dataclass can add new mutable attributes.

There is something a bit strange in this design, though. A frozen dataclass inheriting a mutable type will make the base type's attributes immutable, but if you have a mutable subclass of the frozen dataclass, suddenly the base class attributes become writable again (because there's no way to tell apart a mutable attribute from a non-dataclass base, vs a mutable attribute added by a mutable subclass.)

This inconsistency is enough to convince me that ideally we should just remove the type(self) is cls condition, and make frozen dataclasses freeze only their own attributes. I'm not sure if we can make that change safely though; there may be code relying on the current behavior.

[yilei]

FWIW: attrs explicitly allows __traceback__ to be set on frozen classes: python-attrs/attrs#1081 due to this.

[ericvsmith]

@yilei : Do you know if they also exclude it from a hash calculation?

[yilei]

@ericvsmith Not 100% sure, but I think it's excluded from hash. By default, with the new attrs.define API it hashes (and compares) by instance id just like regular Python exceptions. Here is the relevant auto_exc: bool nob that controls this behavior: https://www.attrs.org/en/stable/api-attr.html#attr.s:~:text=code%20is%20undefined.-,auto_exc%20(bool)%20%E2%80%93,-If%20the%20class

If the class subclasses BaseException (which implicitly includes any subclass of any exception), the following happens to behave like a well-behaved Python exceptions class:

• the values for eq, order, and hash are ignored and the instances compare and hash by the instance’s ids (N.B. attrs will not remove existing implementations of hash or the equality methods. It just won’t add own ones.),
• all attributes that are either passed into init or have a default value are additionally available as a tuple in the args attribute,
• the value of str is ignored leaving str to base classes.

When auto_exc is off and frozen, neither eq nor hash considers __traceback__.

[Badg]

I thought I might chime in with another voice, because I've also stumbled on various problems with frozen dataclass exceptions, for example here. Incidentally, that thread includes an additional use case for where frozen instances (or rather, their hashing behavior) is particularly useful. Something I mentioned in that thread, which might be relevant in response to this bit:

This inconsistency is enough to convince me that ideally we should just remove the type(self) is cls condition, and make frozen dataclasses freeze only their own attributes. I'm not sure if we can make that change safely though; there may be code relying on the current behavior.

What about adding an additional parameter to @dataclass that gives the new behavior? (and also potentially deprecating the old behavior). For example, @dataclass(frozen_fields=True).

For what it's worth, when I first started using dataclasses, I assumed this to be the actual behavior. If your primary motivation for freezing the dataclass is to provide hashability based on the dataclass fields, then it makes intuitive sense that only the fields themselves would enforce immutability -- because the hash is only calculated on the fields, and therefore only the fields need to be frozen to provide a safe and stable hash function. It was kind of a big surprise to find out that this wasn't the case; it felt like a huge collateral impact for just a simple 11-character frozen=True kwarg.

Additionally, because that isn't the case, there's a huge gigantic asterisk on inheritance when it comes to dataclasses, or even things as docile as caching things as private instance attributes. I find myself needing the object.__setattr__ trick surprisingly frequently on frozen dataclasses, almost always buried within an unpythonic def set_cache(self, value): method. So having a way to restrict dataclass freezing to just the fields on the dataclass would be a huge quality of life improvement for me.

[terryjreedy]

#117211 was closed as a duplicate of this issue. The context was a test failing. It was agreed that some doc that exceptions must remain mutable should be added somewhere, to be discussed here.