add a small test for hashability of bare classes #296
Conversation
matthiasdiener
force-pushed
the
test-bare-class
branch
from
26328fb to
4d5a7eb
Compare
pytools/test/test_persistent_dict.py
Outdated
| class WithoutUpdateMethod: | ||
| pass | ||
|
|
||
| assert keyb(WithoutUpdateMethod) == keyb(WithoutUpdateMethod) |
There was a problem hiding this comment.
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 id() as the one available at the advertised name, before using that name for hashing.
There was a problem hiding this comment.
Hmm, I'm not sure I understand. Do you mean, every time test_class_hashing is called, the class is different? But this isn't really tested in this line, perhaps more like in the next line assert keyb(WithoutUpdateMethod) == "da060d601d180d4c"?
There was a problem hiding this comment.
Do you mean, every time
test_class_hashingis called, the class is different?
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 __qualname__ (won't work for local classes) and seeing if it comes back with the same id (just to be safe).
There was a problem hiding this comment.
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?
pytools/persistent_dict.py
Outdated
| ) | ||
| else: | ||
| # Name collision or local class; fall back | ||
| self.rec(key_hash, f"{key.__module__}.{key.__qualname__}_{id(key)}") |
There was a problem hiding this comment.
I think local classes should be unhashable, i.e. this should be an error.
matthiasdiener
force-pushed
the
test-bare-class
branch
from
ae0a065 to
b30b29e
Compare
2 participants
No description provided.