Skip to content

Fix incorrect one-arg Base.hash for Block in NDTensors#1716

Merged
mtfishman merged 2 commits into
ITensor:mainfrom
adienes:fix-hash-one-arg
Mar 23, 2026
Merged

Fix incorrect one-arg Base.hash for Block in NDTensors#1716
mtfishman merged 2 commits into
ITensor:mainfrom
adienes:fix-hash-one-arg

Conversation

@adienes

@adienes adienes commented Mar 19, 2026

Copy link
Copy Markdown
Contributor

Generated as part of an ecosystem-wide audit for one-arg hash methods.

Summary

hash(b::Block) in NDTensors/src/blocksparse/block.jl defines a one-arg Base.hash (via import Base: hash in imports.jl). This means the two-arg fallback hash(x, h::UInt) uses objectid-based hashing, which can cause:

  • Correctness bugs: equal Blocks may hash differently when used as compound keys in Dict or Set
  • Performance issues: excessive method invalidation across the ecosystem
  • Julia 1.13+ breakage: the default hash seed is changing from zero(UInt) to a random value

The existing two-arg method also used addition (h + hash(b)) instead of proper hash chaining.

Fix

Replace both methods with a single proper two-arg hash(b::Block, h::UInt) = hash(b.hash, h) that chains the cached hash value through the seed.

Test plan

  • No custom == for Block; default struct equality compares data and hash fields, which is consistent with the new hash method
  • Block is used as Dict key (Dict{Block{NR}, Bool}), so correct Base.hash is important

This PR was generated with the assistance of generative AI.

Co-Authored-By: Claude noreply@anthropic.com

@codecov

codecov Bot commented Mar 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.38%. Comparing base (294c359) to head (c33645e).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1716      +/-   ##
==========================================
- Coverage   81.09%   79.38%   -1.71%     
==========================================
  Files          59       53       -6     
  Lines        4676     4556     -120     
==========================================
- Hits         3792     3617     -175     
- Misses        884      939      +55     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Generated as part of an ecosystem-wide audit for one-arg hash methods.

The `hash(b::Block)` method only defines a one-arg `Base.hash` (via
`import Base: hash` in imports.jl), which means the two-arg fallback
`hash(x, h::UInt)` uses `objectid`-based hashing instead. This can
cause correctness issues (equal objects hashing differently) and
performance problems (excessive invalidation). With Julia 1.13+, the
default hash seed is changing from `zero(UInt)` to a random value,
which will make the one-arg method produce different results each
session.

The existing two-arg method also used addition (`h + hash(b)`) instead
of proper hash chaining.

Fix by replacing both with a single proper two-arg method:
`hash(b::Block, h::UInt) = hash(b.hash, h)`.

Co-Authored-By: Claude <noreply@anthropic.com>
@mtfishman

Copy link
Copy Markdown
Member

Thanks @adienes! Sorry for all the PR noise, we are reworking some of our CI workflows and this PR exposed an issue when running downstream tests of private repositories from fork PRs, looks like that is fixed now and tests are passing so I'll merge this.

@mtfishman mtfishman merged commit 3a62e79 into ITensor:main Mar 23, 2026
13 of 14 checks passed
@adienes

adienes commented Mar 24, 2026

Copy link
Copy Markdown
Contributor Author

glad it ended up helpful! no worries about the noise. I had ~30 of these PRs open 😂 so inbox was already cooked

ipasichnyk pushed a commit to ipasichnyk/ITensors.jl that referenced this pull request May 25, 2026
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Matt Fishman <mtfishman@users.noreply.github.com>
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.

2 participants