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

allow specializing Base.hash for enum types without overwriting method #49777

Merged
merged 1 commit into from
May 23, 2023

Conversation

stev47
Copy link
Contributor

@stev47 stev47 commented May 11, 2023

Previously @enum defined Base.hash(::MyEnumType, ::UInt) on the user-defined enum type MyEnumType.
When the user wants to specialize the hash function for his own enum type he will define exactly that method signature again which overwrites it and leads to the warning

WARNING: Method definition hash(TestPackage.MyEnumType, UInt64) in module TestPackage at Enums.jl:210 overwritten at [...]
  ** incremental compilation may be fatally broken for this module **

This commit changes @enum so that an internal method is used instead which is called through a fallback Base.hash(::Enum, ::UInt).

@stev47 stev47 requested a review from KristofferC May 11, 2023 20:58
@stev47 stev47 added the backport 1.9 Change should be backported to release-1.9 label May 11, 2023
Copy link
Member

@NHDaly NHDaly left a comment

Choose a reason for hiding this comment

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

LGTM - this makes a lot of sense and seems like a clear improvement to me. 👍 Thanks!

base/Enums.jl Show resolved Hide resolved
@vtjnash
Copy link
Member

vtjnash commented May 11, 2023

I feel like we should delete this, since hashing is already defined for them via objectid, and it is mostly illogical to use an Enum as a dictionary key, as it makes a better/faster array index.

@stev47
Copy link
Contributor Author

stev47 commented May 12, 2023

I feel like we should delete this, since hashing is already defined for them via objectid, and it is mostly illogical to use an Enum as a dictionary key, as it makes a better/faster array index.

In my usecase the enum is used as part of bigger datastructures where the hash is computed recursively. I need to specialize the hash function since the default one (both pre 1.9 and 1.9) is not deterministic across sessions:

module M @enum E A end
hash(M.A)

will yield different hashes for new sessions.

@vtjnash
Copy link
Member

vtjnash commented May 12, 2023

That is fine. I am suggesting this PR be updated to just delete this code, as it is redundant with the existing definition of hash.

@stev47
Copy link
Contributor Author

stev47 commented May 12, 2023

I'm fine with deleting it but maybe @KristofferC who introduced it in #30500 can comment.

@KristofferC
Copy link
Member

KristofferC commented May 12, 2023

as it is redundant with the existing definition of hash.

IIRC, the reason it was added was because the hash of enums came up in a profile traces. From #30500:

I have also seen it make my hash function start to allocate when implementing hash for structs containing enums.

So I rather keep it.

@vtjnash
Copy link
Member

vtjnash commented May 12, 2023

That seems a mistaken reasoning. We can teach codegen about object_id, it just has never seemed to appear in profiles, so it was not worth the effort.

@KristofferC
Copy link
Member

That's also fine (if it is actually done). Removing it because there is a possibility in doing it another way sounds like a mistaken reasoning to me.

test/enums.jl Outdated Show resolved Hide resolved
@stev47 stev47 requested a review from KristofferC May 16, 2023 07:05
@KristofferC KristofferC mentioned this pull request May 19, 2023
51 tasks
test/enums.jl Outdated
Comment on lines 184 to 185
@enum HashEnum3 Enum3_a=1
@test only(methods(hash, (HashEnum3, UInt))).sig != Tuple{typeof(hash), HashEnum3, UInt64}
Copy link
Member

Choose a reason for hiding this comment

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

@KristofferC can you check that this test matches what you had in mind?

test/enums.jl Outdated Show resolved Hide resolved
@NHDaly
Copy link
Member

NHDaly commented May 22, 2023

Merged in latest master in the hopes the test errors resolve. Can you squash into the commit description you want? Otherwise we can squash+merge from here

Previously `@enum` defined `Base.hash(::MyEnumType, ::UInt)` on the
user-defined enum type `MyEnumType`.
When the user wants to specialize the hash function for his own enum
type he will define exactly that method signature again which overwrites
it and leads to the warning

    WARNING: Method definition hash(TestPackage.MyEnumType, UInt64) in module TestPackage at Enums.jl:210 overwritten at [...]
      ** incremental compilation may be fatally broken for this module **

This commit changes `@enum` so that an internal method is used instead
which is called through a fallback `Base.hash(::Enum, ::UInt)`.
@stev47
Copy link
Contributor Author

stev47 commented May 23, 2023

Can you squash into the commit description you want? Otherwise we can squash+merge from here

ok, squashed into one commit

@KristofferC KristofferC merged commit 22551a2 into JuliaLang:master May 23, 2023
Comment on lines +30 to +31
_enum_hash(x::Enum, h::UInt) = hash(x, h)
Base.hash(x::Enum, h::UInt) = _enum_hash(x, h)
Copy link
Member

Choose a reason for hiding this comment

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

The fallback for _enum_hash has to be something else, otherwise these two methods just call each other resulting in stack overflows (fredrikekre/EnumX.jl#5).

Copy link
Contributor Author

@stev47 stev47 May 24, 2023

Choose a reason for hiding this comment

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

Indeed, the assumption was that using @enum would create specialized hash functions to dispatch to.
This should fix it:

Suggested change
_enum_hash(x::Enum, h::UInt) = hash(x, h)
Base.hash(x::Enum, h::UInt) = _enum_hash(x, h)
_enum_hash(x::Enum, h::UInt) = invoke(hash, Tuple{Any, UInt}, x, h)
Base.hash(x::Enum, h::UInt) = _enum_hash(x, h)

Could you test and open a pull request?

Copy link
Member

Choose a reason for hiding this comment

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

I guess I can when I find the time, but I am not the one who caused this so it is more tempting for me to press the revert button.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

opened #49964

@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label May 26, 2023
KristofferC pushed a commit that referenced this pull request Jun 27, 2023
…hod (#49777)

Previously `@enum` defined `Base.hash(::MyEnumType, ::UInt)` on the
user-defined enum type `MyEnumType`.
When the user wants to specialize the hash function for his own enum
type he will define exactly that method signature again which overwrites
it and leads to the warning

    WARNING: Method definition hash(TestPackage.MyEnumType, UInt64) in module TestPackage at Enums.jl:210 overwritten at [...]
      ** incremental compilation may be fatally broken for this module **

This commit changes `@enum` so that an internal method is used instead
which is called through a fallback `Base.hash(::Enum, ::UInt)`.

(cherry picked from commit 22551a2)
KristofferC pushed a commit that referenced this pull request Jun 27, 2023
KristofferC pushed a commit that referenced this pull request Jun 27, 2023
…hod (#49777)

Previously `@enum` defined `Base.hash(::MyEnumType, ::UInt)` on the
user-defined enum type `MyEnumType`.
When the user wants to specialize the hash function for his own enum
type he will define exactly that method signature again which overwrites
it and leads to the warning

    WARNING: Method definition hash(TestPackage.MyEnumType, UInt64) in module TestPackage at Enums.jl:210 overwritten at [...]
      ** incremental compilation may be fatally broken for this module **

This commit changes `@enum` so that an internal method is used instead
which is called through a fallback `Base.hash(::Enum, ::UInt)`.

(cherry picked from commit 22551a2)
KristofferC pushed a commit that referenced this pull request Jun 27, 2023
jmert added a commit to jmert/BitFlags.jl that referenced this pull request Nov 12, 2023
jmert added a commit to jmert/BitFlags.jl that referenced this pull request Nov 13, 2023
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.

5 participants