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
Generated inner constructor should have parameters constrained. #49
Comments
This is a slightly breaking change. The following no longer runs:
So we would have to bump the major version number. |
I tried playing with this as it surprised me a bit and I don't see Julia enforcing that behavior with the default constructor. Are we sure we want to do this?
|
(which I "corrected" to)
After further research, I wrote JuliaLang/julia#51253 |
I have offered to fix Julia's documentation at JuliaLang/julia#51253. Whatever the correct specification of the correct behavior of Julia is, this package should
I suspect that fixing this issue may permit this package to interoperate with other macros better. |
I don't see anything particularly wrong with the constructor: julia> macroexpand(Main, quote
@auto_hash_equals cache=true struct S269{T<:Integer}
x::T
end
end)
quote
#= REPL[9]:2 =#
begin
#= REPL[9]:2 =#
struct S269{T <: Integer}
#= REPL[9]:3 =#
x::T
_cached_hash::UInt
function S269{T}(x) where T <: Integer
#= /home/tim/.julia/packages/AutoHashEquals/8dq3O/src/impl.jl:256 =#
#= /home/tim/.julia/packages/AutoHashEquals/8dq3O/src/impl.jl:257 =#
new(x, (Base).hash(x, 0xa63963209f14699e))
end
end
... Maybe it could be improved like this: function S269{T}(x) where T <: Integer
y = convert(T, x)
new(y, (Base).hash(y, 0xa63963209f14699e))
end That would eliminate a corner case: |
In fact, it doesn't even need to be two different types: julia> @auto_hash_equals cache=true struct S{T}
x::T
end
S
julia> s1, s2 = S{Float32}(1.0), S{Float32}(1.0+eps())
(S{Float32}(1.0f0), S{Float32}(1.0f0))
julia> s1 == s2
false
julia> s1.x == s2.x
true This happens because This is why a good mental model for default constructors is the following (where all constructors are explicit): struct S{T}
x::T
S{T}(x::T) where T = new{T}(x) # constructor 1
end
S{T}(x) where T = S{T}(convert(T, x)::T) # constructor 2
S(x) = S{typeof(x)}(x) # constructor 3 It's not exactly what we do (we effectively inline constructor 1 into constructor 2) but it's the right way to think about it. Moreover, this design would fix the bug here: compute the cached hash only in the inner constructor Presumably this is essentially what you mean by "should have parameters constrained," but you also need the |
@timholy What is wrong with the current behavior is that it defines one constructor method rather than two. The default constructor has two methods. If there is no source equivalent to the set of constructor methods automatically provided, the documentation should say that. As far as I can tell, the default constructors are equivalent to this for a parametric type:
The hand-inlined type inference in your definition of However, this only applies to parametric types. Non-parametric types have a much simpler expansion, documented in JuliaLang/julia#51253 . That PR doesn't provide the expansion for parametric types, which is already (incorrectly) documented in a later section of the document modified by that PR. I'll modify that PR to better handle the case of parametric types, including the required conversion on the second constructor method, above. |
The bug illustrated in the first code-block of #49 (comment) is fixed by the suggestion in the second code-block of #49 (comment), function S269{T}(x) where T <: Integer
y = convert(T, x)
new(y, (Base).hash(y, 0xa63963209f14699e))
end Try it; you'll see that adding a Focusing on counting the number of methods doesn't seem so important: f(x) = 1
f(x::Int) = 1
f(x::Float64) = 1 This is three methods, but one would be forgiven for suggesting that one could delete the last two (and it's probably better if one does). |
For example, let's try the implementation I think you're advocating: julia> struct CachedHash{T}
x::T
cachedhash::UInt
CachedHash{T}(x::T) where T = new{T}(x, hash(x, 0xa63963209f14699e))
CachedHash{T}(x) where T = new{T}(x, hash(x, 0xa63963209f14699e))
end
julia> s1, s2 = CachedHash{Float32}(1.0), CachedHash{Float32}(1.0 + eps());
julia> dump(s1)
CachedHash{Float32}
x: Float32 1.0f0
cachedhash: UInt64 0x691e5307da57bbf4
julia> dump(s2)
CachedHash{Float32}
x: Float32 1.0f0
cachedhash: UInt64 0xc58b93505afe7c50 Oops. You have |
@timholy The current behavior of |
According to the Julia documentation at https://docs.julialang.org/en/v1/manual/constructors/#man-inner-constructor-methods
But
@auto_hash_equals
generates the constructor with unconstrained parameter types. Those parameters should be constrained.The text was updated successfully, but these errors were encountered: