Decide what to do about Base.memhash value

[vtjnash]

In reviewing some code patterns, I noticed that most uses in the ecosystem of Base.memhash are unsound (are reimplementing the same function in Base in order to drop the mandatory GC roots), but a few previously used to be okay. Now all of them are unsound since the implementation changed in #57509 in a major breaking way for most of the users. We should consider whether we want to fix this constant (which used to be defined to be the definition of AbstractString hashing) or delete it entirely (so that all users are sound, by virtue of throwing an exception, and can switch to the new hashing API) or define Base.hash on AbstractString so that users can delete their unsound implementations and fallback to a correct one (given that Base already defines == on AbstractString, not defining hash was already incorrect)

[JeffBezanson]

or define Base.hash on AbstractString

This is already defined?

[JeffBezanson]

I think we should delete memhash; that might be a bit mean but it's internal so we're allowed to do it.

[vtjnash]

Implemented, yes, but badly, with eager conversion instead of Char iteration:

julia/base/strings/basic.jl

Line 367 in 9f69961

hash(s::AbstractString, h::UInt) = hash(String(s), h)

[adienes]

this is maybe possibly potentially a faster fallback for AbstractString ? of course it would be even better probably have yet another full hash_bytes clone on codeunits but that's more of a pain

edit: nevermind it's super slow

function hash(s::AbstractString, h::UInt)
    x = big(0)
    b = 8sizeof(codeunit(s))
    N = ncodeunits(s)
    for i in 0:(N-1)
        Base._hash_shl!(x, b)
        Base.GMP.MPZ.add_ui!(x, codeunit(s, N-i))
    end
    Base.hash_integer(x, h)
end

[LilithHafner]

From triage: let's remove the internal so that folks who are using it get errors (that they were warned of because it's internal) rather than wrong answers (which would also be valid but less nice)

See also: #59172, #59173

[vtjnash]

Note that before people go too crazy on reimplementing this themselves, that I hope to have a PR soon to implement a fast generic version so that it is sufficient just to delete the bad methods in those packages.

[adienes]

ok, I'll mention that in the issues, thanks

[PallHaraldsson]

@vtjnash, should this be on the 1.12 milestone for sure? Since rapidhash3 (v1 also previously) is only on 1.13.

before people go too crazy on reimplementing this themselves

That's exactly what I started to to, since it seemed like only 5 more trivial looking PRs to those packages, but then I stopped after seeing your message. Needs your PR happen soon before 1.12? Or even could rapidhash3 now be backported to 1.12, since it's been merged? It seems very localized (and helpful for speed...) so could it override RC status?