Skip to content

Keep Extension hashable after the dataclass conversion - #427

Open
MohammedAlkindi wants to merge 2 commits into
pypa:mainfrom
MohammedAlkindi:fix/extension-keep-identity-hash
Open

Keep Extension hashable after the dataclass conversion#427
MohammedAlkindi wants to merge 2 commits into
pypa:mainfrom
MohammedAlkindi:fix/extension-keep-identity-hash

Conversation

@MohammedAlkindi

Copy link
Copy Markdown

Closes #426.

Symptom

Since the dataclass conversion (#373), Extension is unhashable — hash(Extension('x', ['y.c'])) raises TypeError, 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 with TypeError: unhashable type: 'WinExt_pythonwin' (their workaround: mhammond/pywin32#2787).

Mechanism

@dataclass defaults to eq=True, which generates __eq__; a class that defines __eq__ without __hash__ gets __hash__ = None. Before the conversion Extension defined neither, so it had identity equality and the inherited hash.

The generated __eq__ is itself a second behaviour change: two distinct Extension objects with the same field values now compare equal, so ext in some_list can 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, and eq=False keeps all of that while restoring the equality/hash contract Extension has 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:

FAILED distutils/tests/test_extension.py::TestExtension::test_extension_is_hashable
1 failed, 3 passed, 2 skipped

Pass-after:

4 passed, 2 skipped

Wider suite (distutils/tests, excluding test_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.

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.

@abravalheri abravalheri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread distutils/extension.py Outdated

@lenient_dataclass()
# eq=False keeps the identity-based equality and hashability Extension has
# always had. The dataclass conversion is for type annotations only; the

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dataclass conversion is for type annotations only;

I wouldn't say that phrase particularly, dataclasses are genuine quality of life improvement IMO...

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworded in e4ff1d3 — agreed, that phrase undersold the conversion. The comment now gives the actual rationale for keeping identity semantics (mutable working state) instead of characterizing what #373 was for.

@MohammedAlkindi

Copy link
Copy Markdown
Author

Thanks @abravalheri — both questions are fair, and I think they resolve the same way, so taking them together.

Is value-equality desirable for Extension? I don't think it can be made sound while Extension stays mutable — and mutable working state is how extensions are used in practice: build_ext subclasses rewrite extension attributes as the build progresses, and the site that surfaced this regression (pywin32) tracks the live objects by container membership while doing exactly that. With a field-value __eq__, two extensions can be equal at configuration time and unequal partway through the build, so any membership decision made on value depends on when it runs. Identity is the one relation that stays true for the object's whole lifetime, and it's what every pre-#373 caller got from the object defaults.

On __hash__ = object.__hash__ with the generated __eq__: I'd argue that trades a loud failure for a silent one. Ran on this repo's main (CPython 3.13.13) with exactly that patch applied:

>>> 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 misses

Equal 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 TypeError at least fails loudly at the first set().

If value semantics are genuinely wanted for Extension at some point, I'd say that's its own feature rather than something to back into while unbreaking downstream: it would want frozen=True (or tuple-ified fields) so __hash__ can be value-based and the eq/hash contract holds. eq=False restores exactly the pre-#373 contract, which seemed the most reviewable step.

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.

@Avasam

Avasam commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

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.

@MohammedAlkindi

Copy link
Copy Markdown
Author

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 CAOShurong left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@MohammedAlkindi

Copy link
Copy Markdown
Author

Thanks for running that, and for neutralizing the distutils-precedence hook first. That step is easy to skip and it is what makes the baseline trustworthy rather than a measurement of whatever setuptools happened to install.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Extension is unhashable since the dataclass conversion (#373) — breaks setup scripts that collect extensions in sets

4 participants