How not to commit type piracy when handling automatic conversion for field with Union type

General Usage
, ,
1

I’d like to handle the conversion in structs just like when you have a concrete type for a field, but when a field as a Union type like below:

struct A end

struct B
    x::Union{A, Int}
end

I’d like this to work:

B(true) # should return B(1)

I was able to make everything work with:

# note: these definitions are not entirely correct in any case, see the issue for more details
Base.convert(::Type{Union{A,T}}, x) where T = convert(T, x)
Base.convert(::Type{Union{A,T}}, x::T) where T = convert(T, x)
Base.convert(::Type{Union{A,T}} where T, x::A) = x

everything seems to work very well

julia> b1 = B(A())
B(A())

julia> b2 = B(1)
B(1)

julia> b3 = B(true) # automatic conversion
B(1)

but then I was informed by @jameson in this issue: Ambiguity in Base.convert on 1.8/1.9 but not on 1.10 and master · Issue #53224 · JuliaLang/julia · GitHub that I’m committing type-piracy, and indeed this now fails!

julia> Union{Nothing, Union{A, Int}}[1][1]
ERROR: MethodError: convert(::Type{Union{Nothing, A, Int64}}, ::Int64) is ambiguous.

Candidates:
  convert(::Type{Union{A, T}}, x::T) where T
    @ Main REPL[4]:1
  convert(::Type{T}, x) where T>:Nothing
    @ Base some.jl:37
  convert(::Type{Union{A, T}}, x) where T
    @ Main REPL[3]:1
  convert(::Type{T}, x::T) where T>:Nothing
    @ Base some.jl:36
  convert(::Type{T}, x::T) where T
    @ Base Base.jl:84
To resolve the ambiguity, try making one of the methods more specific, or adding a new method more specific than any of the existing applicable methods.

but without the above extensions of Base.convert it works fine:

julia> Union{Nothing, Union{A, Int}}[1][1]
1

Is there a way to achieve this without producing bugs?

2

Can’t you achieve the same thing with constructors rather than introducing a general conversion rule?

julia> struct B3
           x::Union{A, Int}
           B3(a::A) = new(a)
           B3(a) = new(convert(Int, a))
       end

julia> B3(1)
B3(1)

julia> B3(true)
B3(1)
1 Like
3

Seems like a very good idea, but I fail to see the generalization for my use case, for example

struct A end

struct B
    x::Union{A, Int64}
    y::Union{A, Int32}
    z::Union{A, Int16}
    k::Union{A, Int8}
end

doesn’t this need a lot of constructors with your idea?

4

ok I have it maybe:


julia> struct A end

julia> struct B
           x::Union{A, Int64}
           y::Union{A, Int32}
           z::Union{A, Int16}
           k::Union{A, Int8}
           B(x, y, z, k) = new(maybe_convert(x), maybe_convert(y), maybe_convert(z), maybe_convert(k))
           end

julia> maybe_convert(x::A) = x
maybe_convert (generic function with 1 method)

julia> maybe_convert(x::T) where {T} = convert(T, x)
maybe_convert (generic function with 2 methods)
1 Like
5

I think you need to pass the type to maybe_convert or else it doesn’t do anything, no?

julia> struct A end
julia> struct B
           x::Union{A, Int64}
           y::Union{A, Int32}
           z::Union{A, Int16}
           k::Union{A, Int8}
           B(x, y, z, k) = new(maybe_convert(Int64, x), maybe_convert(Int32, y), maybe_convert(Int16, z), maybe_convert(Int8, k))
           end

julia> maybe_convert(_, x::A) = x
maybe_convert (generic function with 1 method)

julia> maybe_convert(T, x) = convert(T, x)
maybe_convert (generic function with 2 methods)
1 Like
6

Lovely, thank you a lot!

7

I think you could even automate that… But not sure whether this code is a good idea in practice :slight_smile:

function maybe_convert_recursively(T::Union, x)
    try
        return convert(T.a, x)
    catch err
        err isa MethodError && return maybe_convert_recursively(T.b, x)
        rethrow(err)
    end
end
maybe_convert_recursively(T, x) = convert(T, x)

struct B
    x::Union{A, Int64}
    y::Union{A, Int32}
    z::Union{A, Int16}
    k::Union{A, Int8}
    function B(args...)
        length(args) == fieldcount(B) || error("You need to pass $(fieldcount(B)) arguments to construct a B. Got $(length(args)).")
        converted_args = map(maybe_convert_recursively, fieldtypes(B), args)
        new(converted_args...)
    end
end
1 Like
8

mmmh, interesting, but this will be a bit slower I guess in some cases :smiley:

I will use this inside a macro so I can actually create the syntax we discussed previously with a bit of effort

9

yeah, this is nice because it is recursive though, but I wouldn’t go this far, because I just want to mimick how normal structs work, I don’t think they have this kind of recursive behaviour

10

I actually don’t know whether this is slow or not… In principle it could compile away I think. And it actually does :smiley: See here (my function is called f not maybe_convert_recursively)

julia> @code_native debuginfo=:none dump_module=:false (()->f(Union{A,B2,Int32,String}, "asdf"))()
	.text
	push	rbp
	mov	rbp, rsp
	mov	rax, qword ptr [r13 + 16]
	movabs	rdi, 140250117903312
	movabs	rsi, 140250096741664
	mov	rax, qword ptr [rax + 16]
	mov	rax, qword ptr [rax]
	movabs	rax, offset f
	call	rax
	pop	rbp
	ret
	nop	word ptr cs:[rax + rax]

No error handling to be seen :slight_smile:
Edit: I actually take that back. I don’t know why I wrapped the function in an anonymous function but if you don’t do that then you clearly the error survive. More over if you simple profile the function it get clear, that it is in fact quite slow

julia> @btime f(Union{A,B2,Int32,String}, "asdf")
  97.152 μs (40 allocations: 1.34 KiB)
"asdf"

So don’t use this approach!

1 Like
11

When you hit the catch block it is usually slow in my experience, the try usually has small overhead instead