Keep Extension hashable after the dataclass conversion - #427
Keep Extension hashable after the dataclass conversion#427MohammedAlkindi wants to merge 2 commits into
Conversation
The dataclass conversion in pypa#373 was for type annotations, but the default eq=True defines __eq__ and thereby sets __hash__ to None, so Extension instances can no longer live in sets or dict keys. Setup scripts do exactly that: pywin32's build collects excluded extensions in a set and now fails on every platform with TypeError: unhashable type: 'WinExt_pythonwin' Pass eq=False so Extension keeps the identity-based equality and hash it has always had. Value equality was never part of the contract, and under it two distinct Extensions with the same name and sources start comparing equal, which changes 'ext in list' checks as well.
There was a problem hiding this comment.
Hi @MohammedAlkindi thank you for flagging this.
I have a semantic question: is it desirable for the comparison between 2 extension objects with identical inner state (i.e. all fields have "equal" values) to be different because they have a different object id?
For example, if a user is trying to exclude a specific Extension by comparison, would it make more sense to exclude any extension matching that description, regardless of whether it is the exact same object instance?
UPDATED:
I suppose one middle ground (assuming __eq__ is still desirable, which arguably may help people that want to check whether an object matches a specific extension description) is to keep __eq__ as it is and define:
__hash__ = object.__hash__(untested, and I am not sure how happy mypy would be about it).
The big semantic question is what Extension should be optimised for:
- two objects with the same state are equivalent; or
- objects have a nominal identity independent of their state.
The middle-ground "solution" above introduces a bit of awkwardness: __eq__ is optimised for state equivalence while __hash__ is optimised for nominal identity. The consequence is that it becomes possible to have ext1 == ext2 while at the same time hash(ext1) != hash(ext2), which Python docs strongly discourage.
P.S. My suspicion is that this may be yet another Hyrum's Law situation in setuptools/distutils: Extension was usable as a set element because it implicitly inherited Python's default object hashing implementation, but neither the implementation nor the tests seem to provide evidence that identity-based hashing was intended to be part of the API contract.
Another subtle point is that "hashable" and "identity-based" are not necessarily the same thing. Historically Extension happened to have both properties (so both the argument for backwards compatibility and Hyrum's law are valid), but the observed behaviour of being usable as a set element does not by itself imply identity semantics. Hashability and identity happened to coincide because of the inherited object methods, but one does not imply the other.
If the actual requirement is object identity, another option would be for users to opt into it explicitly:
seen = {id(ext)}
# or:
# seen = {id(ext): ext}
# if there is a requirement to keep the object alive via a reference count
if id(other_ext) in seen:
...rather than relying on the hashing semantics of Extension itself. This could be considered a clearer and potentially more resilient pattern...
Finally, as always it is more than fair to preserve a historical usage pattern if an important part of existing projects rely on it.
P.S.2: I am not a maintainer of distutils, just the author of the PR for the use of dataclasses.
|
|
||
| @lenient_dataclass() | ||
| # eq=False keeps the identity-based equality and hashability Extension has | ||
| # always had. The dataclass conversion is for type annotations only; the |
There was a problem hiding this comment.
The dataclass conversion is for type annotations only;
I wouldn't say that phrase particularly, dataclasses are genuine quality of life improvement IMO...
|
Thanks @abravalheri — both questions are fair, and I think they resolve the same way, so taking them together. Is value-equality desirable for On >>> e1 = Extension("demo", ["demo.c"]); e2 = Extension("demo", ["demo.c"])
>>> e1 == e2
True
>>> len({e1, e2})
2 # equal objects not collapsed
>>> d = {e1: "built"}
>>> d[e1], d.get(e2)
('built', None) # equal key missesEqual objects that hash differently means dedup that doesn't dedup and lookups that miss, with no traceback pointing at the cause — which is the scenario the "strongly discouraged" note in the docs is about. The current If value semantics are genuinely wanted for On the inline comment: you're right, and I've reworded it in e4ff1d3 — "type annotations only" undersold the conversion, which wasn't my intent. The comment now states the actual reason identity semantics are kept instead of characterizing #373's purpose. |
|
Haven't read through the entire discussion. I find the LLM text too verbose. But as far as pywin32 is concerned, I wouldn't rely on Extension being hashable. It was more of a side effect of optimizing a contain check by using a set. The whole piece of code that got affected is a bit hacky (checking if an extension is part of the built ones, to decide if more should ve built) and would benefit from being rewritten to have a flag that truly tracks the state we were interested in. The fix proposed here is very small anyway. |
|
Agreed on the flag: that's the real fix. This only restores the pre-dataclass behaviour so the pywin32 check keeps working until someone does it properly. Fine to close if you'd rather not carry the stopgap. |
CAOShurong
left a comment
There was a problem hiding this comment.
Verified end-to-end on the exact head e4ff1d36 (Windows 11, CPython 3.11.16, repo checked out at the PR head with setuptools' distutils-precedence hook neutralized so the local tree is what actually imports):
Baseline on main (same environment):
hash(Extension('x', ['y.c']))→TypeError: unhashable type: 'Extension'Extension.__hash__ is None→ True- two distinct instances with equal fields:
a == b→ True (value equality)
On this PR head:
hash(Extension(...))works;__hash__no longer None- identity semantics restored: equal-valued instances are not equal (
a == b→ False), and a dict keyed by one instance is not matched by an equal-values second instance (KeyError, as pre-conversion) - set membership / dict-key use works
Tests: distutils/tests/test_extension.py — 4 passed, 2 skipped, 0 failed, including the new test_extension_is_hashable, which pins both hashability and the identity semantics.
The eq=False rationale comment is accurate for how extensions are used as mutable working state during builds, and the regression test covers both halves of the regression (the crash and the quieter equality change). Approving — this is the right fix at the right layer; looking forward to it reaching setuptools' vendored copy so pywin32's workaround can be dropped.
|
Thanks for running that, and for neutralizing the |
Closes #426.
Symptom
Since the dataclass conversion (#373),
Extensionis unhashable —hash(Extension('x', ['y.c']))raisesTypeError, and any setup script that collects extensions in a set or dict key breaks. pywin32's build does exactly that and now fails on every platform withTypeError: unhashable type: 'WinExt_pythonwin'(their workaround: mhammond/pywin32#2787).Mechanism
@dataclassdefaults toeq=True, which generates__eq__; a class that defines__eq__without__hash__gets__hash__ = None. Before the conversionExtensiondefined neither, so it had identity equality and the inherited hash.The generated
__eq__is itself a second behaviour change: two distinctExtensionobjects with the same field values now compare equal, soext in some_listcan be true for an object that is not in the list.Fix
Pass
eq=False. #373's stated purpose was type annotations so they can be inherited by setuptools, andeq=Falsekeeps all of that while restoring the equality/hash contractExtensionhas had for its whole life. Nothing in this repo relies on the generated__eq__— the only==involving extensions in the codebase compares filename suffix strings (build_ext.py).Evidence
Run on Windows 11, CPython 3.13.13, at
main(3ed42426); the mechanism is platform-independent.Fail-before — product change reverted, new test kept:
Pass-after:
Wider suite (
distutils/tests, excludingtest_build_ext.py, which needs a C compiler this machine does not have):176 passed, 16 skipped, 2 xfailed— no regressions. I could not run the compile-dependent tests locally; CI is the authority there.The new test pins both halves of the contract: hashability (set membership, dict key) and identity semantics (equal-valued but distinct instances stay unequal), so a future conversion cannot silently flip either again.