-
Notifications
You must be signed in to change notification settings - Fork 27
add a small test for hashability of bare classes #296
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
4d5a7eb
43cea0b
e35c58a
a722191
b30b29e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -525,9 +525,52 @@ def update_persistent_hash(self, key_hash, key_builder): | |
| assert keyb(MyABC3) != keyb(MyABC) != keyb(MyABC3()) | ||
|
|
||
|
|
||
| class WithoutUpdateMethodGlobal: | ||
| pass | ||
|
|
||
|
|
||
| class CollidingNameClass: | ||
| pass | ||
|
|
||
|
|
||
| def test_class_hashing() -> None: | ||
| keyb = KeyBuilder() | ||
|
|
||
| class WithoutUpdateMethod: | ||
| pass | ||
|
|
||
| assert keyb(WithoutUpdateMethod) == keyb(WithoutUpdateMethod) | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Arguably, this should not work. This is a different class every time the function gets called. It might be useful to see if the class type has the same
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I'm not sure I understand. Do you mean, every time
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes. My point is that a function-local class does not have a globally valid name and thus should not be hashable. This can be tested by attempting to import it by its
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TIL, thanks! What do you think of e35c58a? I'm worried that this may cause extra cache misses since some tags that we use might be local classes? |
||
| assert keyb(WithoutUpdateMethodGlobal) == keyb(WithoutUpdateMethodGlobal) | ||
|
|
||
| # This doesn't work with the function-local class "WithoutUpdateMethod", because | ||
| # local classes are instantiated at each function call, and thus their hash | ||
| # includes the id() of the class: | ||
| # assert keyb(WithoutUpdateMethod) == "N/A" | ||
| assert keyb(WithoutUpdateMethodGlobal) == "49c4673089d30507" | ||
|
|
||
| with pytest.raises(TypeError): | ||
| # does not have update_persistent_hash() = > will raise | ||
| keyb(WithoutUpdateMethod()) | ||
|
|
||
| with pytest.raises(TypeError): | ||
| # does not have update_persistent_hash() = > will raise | ||
| keyb(WithoutUpdateMethodGlobal()) | ||
|
|
||
| # {{{ test for name collisions between top-level and function-local classes | ||
|
|
||
| def make_colliding_name_class(): | ||
| class CollidingNameClass: | ||
| pass | ||
|
|
||
| return CollidingNameClass | ||
|
|
||
| top_level_cls = CollidingNameClass | ||
| shadowed_cls = make_colliding_name_class() | ||
|
|
||
| assert keyb(top_level_cls) != keyb(shadowed_cls) | ||
|
|
||
| # }}} | ||
|
|
||
| class WithUpdateMethod: | ||
| def update_persistent_hash(self, key_hash, key_builder): | ||
| # Only called for instances of this class, not for the class itself | ||
|
|
@@ -548,7 +591,6 @@ class TagClass2(Tag): | |
| assert keyb(TagClass()) != keyb(TagClass2()) | ||
|
|
||
| assert keyb(TagClass()) == "7b3e4e66503438f6" | ||
| assert keyb(TagClass2) == "690b86bbf51aad83" | ||
|
|
||
| @tag_dataclass | ||
| class TagClass3(Tag): | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think local classes should be unhashable, i.e. this should be an error.