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

Hash function missing from subclass #483

Closed
vitalbmcdonald opened this issue Jan 11, 2019 · 5 comments
Closed

Hash function missing from subclass #483

vitalbmcdonald opened this issue Jan 11, 2019 · 5 comments

Comments

@vitalbmcdonald
Copy link

When creating a subclass of an attr object, I would expect setting cmp=False to cause the subclass to be hashable. Right now it inherits the hash settings from its parent and ignores the cmp settings for hashing.

import attr

@attr.s
class Foo:
    pass

@attr.s(cmp=False)
class Bar(Foo):
    pass

assert Foo.__hash__ is None, "Foo incorrectly has a hash method"
## passes

assert Bar.__hash__ is object.__hash__, "Bar is missing the identity hash method"
## fails

The issue I believe is this elif block assumes that the class is inheriting from object and thereby getting its __hash__ method. Adding a line to explicitly add the __hash__ method would fix the problem.

The workaround I am using is just adding the __hash__ method after the class is defined, so

@attr.s(cmp=False)
class Bar(Foo):
    pass

Bar.__hash__ = object.__hash__
@vitalbmcdonald
Copy link
Author

As a note, it would also be nice if subclasses always inherited __hash__ based on parents, something like:

import attr

@attr.s
class Foo:
    pass

@attr.s(cmp=False)
class Bar(Foo):
    pass

## workaround from issue description
Bar.__hash__ = object.__hash__

@attr.s
class Baz(Bar):
    pass

assert Baz.__hash__ is object.__hash__, "Baz is missing the identity hash method"

Perhaps defaulting cmp/frozen to None and then doing a similar mro search as with attributes to determine if the hash method is needed.

@hynek
Copy link
Member

hynek commented Jan 16, 2019

Hm the problem here is twofold:

  1. attrs in general has a “hands off policy” by default. Telling it cmp=False means “don't write cmp methods” and not “make sure there’s not a cmp method”. For example if you wanted to write your own cmp methods, you'd set cmp to False and then implement them on the body.
  2. and effectively this would be an incredibly destructive and breaking change. :-/

Do you realize that you can write:

@attr.s(cmp=False)
class Bar(Foo):
    __hash__ = object.__hash__

?

That seems more idiomatic than doing implicit black magic behind the users’ back.

@wsanchez
Copy link

Marked as Bug because it's filed as such and invalid because it's intentional, as Hynek notes.

@vitalbmcdonald Do you think the documentation is insufficiently clear here? I could see an argument that this text in the Hashing page:

The correct way to achieve hashing by id is to set @attr.s(cmp=False).

…might imply that cmp=False does make sure that you get object's behavior.

@vitalbmcdonald
Copy link
Author

vitalbmcdonald commented Jan 18, 2019

@hynek, good call on declaring the hash method in the class definition, I'm not sure why I had it separated out.

@wsanchez, as far as the documentation, I did get the impression that setting cmp=False was the way to explicitly set the hash functionality to use the id when in reality it is whatever hash function is inherited.

But the real root of my confusion is a misunderstanding on how python hash functions worked by default. For instances:

class Foo:
    def __eq__(self, other):
        return super().__eq__(self, other)

class Bar(Foo):
    pass

assert Bar.__hash__ is None

I would have thought Bar.__hash__ was object.__hash__ in the above scenario, but it is actually None. So in vanilla python I would have to set the __hash__ function manually too.

Thanks for the help!

@wsanchez
Copy link

OK so it sounds like the docs could use a small tweak to say it's inheriting the method, since it sort of implies that attrs ensures an ID test when cmp=False.

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

No branches or pull requests

3 participants