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.

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

auto_exc's hashing behavior and its documentation do not match #543

Closed
gabbard opened this issue Jun 14, 2019 · 3 comments · Fixed by #563
Closed

auto_exc's hashing behavior and its documentation do not match #543

gabbard opened this issue Jun 14, 2019 · 3 comments · Fixed by #563
Labels
Milestone

Comments

@gabbard
Copy link
Member

@gabbard gabbard commented Jun 14, 2019

The documentation for auto_exc says:

  - the values for *cmp* 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.),

However, the test for auto_exc says:

    Classes with auto_exc=True have a Exception-style __str__, are neither
        comparable nor hashable, and store the fields additionally in
        self.args.

and tests:

            with pytest.raises(TypeError):
                hash(e)

The documentation and the code should be made consistent. I noticed this because unittest complained that the exception type was unhashable when thrown from a test and I see some references to logging expecting hashable exceptions, so the documented behavior is probably better than the implemented behavior.

@hynek hynek added the Bug label Jun 17, 2019
@hynek
Copy link
Member

@hynek hynek commented Jul 20, 2019

Oh god, are there any docs on Exception hashability? This looks bonkers:

In [7]: hash(Exception()) == hash(Exception())
Out[7]: True

In [8]: hash(Exception(1)) == hash(Exception(1))
Out[8]: True

In [9]: hash(Exception(1)) == hash(Exception(2))
Out[9]: True

In [10]: hash(TypeError(1)) == hash(TypeError(2))
Out[10]: True

@gabbard
Copy link
Member Author

@gabbard gabbard commented Jul 23, 2019

@hynek: This confused me for quite a while, but I think I have tracked it down, helped by some further investigation by @berquist.

Note you don't get the same error if you first assign the exceptions to variables before doing the comparison. I think what is going on is:

  1. Exception and TypeError have hash and equality by id().
  2. The the first exception is created and its hash is taken from its memory location. Then it is no longer referenced, so it is deleted from memory. The second exception is created in the same memory location formerly occupied by the first exception and therefore has an identical id(). See https://github.com/satwikkansal/wtfpython#-deep-down-were-all-the-same- .

For the life of me I can't find any documentation of the requirements or conventions for __eq__ and __hash__ with respect to exceptions. There is definitely nothing about it in the obvious places in the Python documentation. This is unfortunately an extremely difficult thing to Google for, and I can't turn up anything else useful. Also, what appears to be the tests for the base exception classes in CPython have no tests related to either hashing or equality.

The actual behavior of the built-in exceptions seems to match what the attrs documentation says it will do (hashing and equality by id()), so it seems reasonable to switch the implementation to match the documentation.

@hynek hynek added this to the 19.2 milestone Aug 7, 2019
hynek added a commit that referenced this issue Aug 18, 2019
They now behave as the docs claim.

Fixes #543
@hynek hynek closed this in #563 Aug 19, 2019
hynek added a commit that referenced this issue Aug 19, 2019
* Make auto_exc=True classes hashable by ID

They now behave as the docs claim.

Fixes #543

* Add PR newsfragment

* Be more specific about NOP
@twm
Copy link

@twm twm commented Sep 30, 2019

For anyone searching for these errors, this issue can cause failures to format tracebacks that look like this:

Traceback (most recent call last):
  File "attrstb.py", line 19, in <module>
    traceback.print_exc()
  File "/usr/lib/python3.5/traceback.py", line 159, in print_exc
    print_exception(*sys.exc_info(), limit=limit, file=file, chain=chain)
  File "/usr/lib/python3.5/traceback.py", line 100, in print_exception
    type(value), value, tb, limit=limit).format(chain=chain):
  File "/usr/lib/python3.5/traceback.py", line 439, in __init__
    _seen.add(exc_value)
TypeError: unhashable type: 'UnhashableException'

Or, if you passed hash=True or frozen=True, hashing may fail when it encounters an unhashable type like list or dict that was passed to the exception constructor.

Passing hash=False is a simple and forward-compatible workaround, so in attrs 19.1.0 you can use auto_exc like this:

@attr.s(auto_exc=True, hash=False)
class MyException(Exception):
    foo = attr.ib()

hash=False causes the class to inherit object.__hash__. Such an exception type is compatible with the traceback module's formatting functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants