Skip to content
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

Define a method for hash(::Type, ::UInt) #49636

Merged
merged 1 commit into from
May 6, 2023
Merged

Define a method for hash(::Type, ::UInt) #49636

merged 1 commit into from
May 6, 2023

Conversation

ararslan
Copy link
Member

@ararslan ararslan commented May 4, 2023

Currently, hash(::Type, ::UInt) uses objectid, which can have some odd behavior for types: in particular, subsequent identical type-valued variable definitions can have objectids which differ from the first such definition. This has some bizarre downstream effects when e.g. using types as the values of a Set or the keys of a Dict. See #49620 for examples.

There is an internal type_hash C function used for caching types but isn't exposed to Julia, as Jameson pointed out in the linked issue. This commit exposes it as jl_type_hash which is then used via ccall to define a method hash(::Type, ::UInt). This method then fixes #49620. Note, however, that this does not affect the differing objectids for otherwise identical types.

I've marked this as a bugfix and notably the linked issue is present at least as far back as Julia 1.6, so it could be worth backporting to release-1.6, release-1.9, and perhaps release-1.8 if we're going to do any more of those. I'm not sure whether that's allowed though for changes that add to the C API.

@ararslan ararslan added types and dispatch Types, subtyping and method dispatch bugfix This change fixes an existing bug hashing labels May 4, 2023
src/jltypes.c Outdated Show resolved Hide resolved
Currently, `hash(::Type, ::UInt)` uses `objectid`, which can have some
odd behavior for types: in particular, subsequent identical type-valued
variable definitions can have `objectid`s which differ from the first
such definition. This has some bizarre downstream effects when e.g.
using types as the values of a `Set` or the keys of a `Dict`. See issue
49620 for examples.

There is an internal `type_hash` C function used for caching types but
isn't exposed to Julia, as Jameson pointed out in the linked issue. This
commit exposes it as `jl_type_hash` which is then used via `ccall` to
define a method `hash(::Type, ::UInt)`. This method then fixes #49620.
Note, however, that this does not affect the differing `objectid`s for
otherwise identical types.
@ararslan ararslan requested a review from vtjnash May 5, 2023 22:38
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label May 6, 2023
@vtjnash
Copy link
Sponsor Member

vtjnash commented May 6, 2023

error looks unrelated (to this PR)

Error in testset error:
Test Failed at C:\buildkite-agent\builds\win2k22-amdci6-3\julialang\julia-master\julia-9249904fd3\share\julia\test\error.jl:116
  Expression: value <: Exception
   Evaluated: Main.Test70Main_errorshow.TypeCompareError <: Exception

@vtjnash vtjnash merged commit 633d1ae into master May 6, 2023
@vtjnash vtjnash deleted the aa/typehash branch May 6, 2023 04:31
@ararslan
Copy link
Member Author

ararslan commented May 6, 2023

Marking for backport to 1.6 and 1.9 since it's a bugfix but feel free to remove the labels if it's not appropriate to backport due to the addition to the C API

@ararslan ararslan added backport 1.6 Change should be backported to release-1.6 backport 1.9 Change should be backported to release-1.9 labels May 6, 2023
@vtjnash
Copy link
Sponsor Member

vtjnash commented May 6, 2023

I thought the C API for this was fairly recent, but if not, it seems worthwhile to backport

@oscardssmith oscardssmith removed the merge me PR is reviewed. Merge when all tests are passing label May 6, 2023
KristofferC pushed a commit that referenced this pull request May 8, 2023
Currently, `hash(::Type, ::UInt)` uses `objectid`, which can have some
odd behavior for types: in particular, subsequent identical type-valued
variable definitions can have `objectid`s which differ from the first
such definition. This has some bizarre downstream effects when e.g.
using types as the values of a `Set` or the keys of a `Dict`. See issue
49620 for examples.

There is an internal `type_hash` C function used for caching types but
isn't exposed to Julia, as Jameson pointed out in the linked issue. This
commit exposes it as `jl_type_hash` which is then used via `ccall` to
define a method `hash(::Type, ::UInt)`. This method then fixes #49620.
Note, however, that this does not affect the differing `objectid`s for
otherwise identical types.

(cherry picked from commit 633d1ae)
@KristofferC KristofferC mentioned this pull request May 8, 2023
51 tasks
@ararslan
Copy link
Member Author

ararslan commented May 8, 2023

I thought the C API for this was fairly recent, but if not, it seems worthwhile to backport

If it is then how do we fix #49620 for prior versions?

@KristofferC
Copy link
Sponsor Member

KristofferC commented May 26, 2023

Is this intended:

julia> typeof(a)
Destroy{FockSpace{Symbol}, Symbol, Int64, Nothing}

julia> typeof(a')
Create{FockSpace{Symbol}, Symbol, Int64, Nothing}

julia> hash(a)
0xc35039ec05f5ae3b

julia> hash(a')
0xc35039ec05f5ae3b

julia> hash(a) == hash(a')
true

It causes https://github.com/qojulia/QuantumCumulants.jl/blob/7a00cbbdbc3c702668689900f96303607ed65c6a/test/test_fock.jl#LL8C1-L13C34 to fail with this backported.

@ararslan
Copy link
Member Author

Is this intended:

I assume not, and I'm not sure why that's happening. @vtjnash, any ideas?

@vtjnash
Copy link
Sponsor Member

vtjnash commented May 27, 2023

It is not wrong, but is fairly unexpected. We might not be bixmixing with the TypeName hash enough?

@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label May 27, 2023
@KristofferC
Copy link
Sponsor Member

I droped this PR from backports 1.9 until the effects are better understood (#49975).

@nalimilan
Copy link
Member

This triggers an annoying bug when building StatsBase docs (#49620) so it would be nice to find a way to backport it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.6 Change should be backported to release-1.6 bugfix This change fixes an existing bug hashing types and dispatch Types, subtyping and method dispatch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

hash(::Type) inconsistent with some signature tuples (sometimes)
6 participants