Type piracy checks for union varargs "too rough"?

[dkarrasch]

While bumping Aqua.jl in the test project of LinearMaps.jl, I came across "newly detected" piracy issues, that had not been reported on Aqua.jl versions pre v0.7. One concrete example is the following:

Possible type-piracy detected:
[1] hcat(As::Union{LinearAlgebra.UniformScaling, LinearMap, AbstractVecOrMat{T} where T}...) @ LinearMaps ~/.julia/dev/LinearMaps/src/blockmap.jl:87
...

Of course, there is some danger of piracy here because the argument tuple may contain only LinearAlgebra objects. In Julia v1.9 (on which the test was run), however, we have a method

hcat(A::Union{UniformScaling, AbstractVecOrMat}...)
     @ LinearAlgebra /Applications/Julia-1.9.app/Contents/Resources/julia/share/julia/stdlib/v1.9/LinearAlgebra/src/uniformscaling.jl:411

which catches the "dangerous" case. All of this was conciously designed, so that LinearMaps's method can only be called if there is at least one LinearMap typed object among the arguments, which means there's no piracy going on. In the above referred to PR, I am even adopting the method signature to different julia versions, but no mercy by Aqua.jl on the piracy detection front, unfortunately.

[lgoettgens]

Let me explain a few things:
Aqua.jl v0.7 did not change anything regarding what is type piracy. The main change was which methods to check for type piracy. Your cases did not occur for v0.6.x, because you did not import Base.hcat into your project and only defined methods in a fully qualified form. This has been changed in (#156).

To your main point:
I am not sure whether your hcat method constitutes as type piracy (in the existence of the one from LinearAlgebra). Someone more experienced can maybe help here.

The point is that Aqua's type piracy check works by checking one method at a time, and only checking this one method without regarding any of its surroundings, other dispatches etc. So I wouldn't know how to easily tackle what your observed, even in the case that we declare it a bug in Aqua.jl.

[dkarrasch]

I see. Thanks for clarifying. It would be a very strong feature enhancement if the piracy check could, for union arguments as above, something along the lines "what if I removed the owned types from the union and ask by @which which method gets called, and if it's still the same method (in the above case from LinearMaps.jl), then call it type piracy, otherwise not". I'd suspect that the above described extension pattern is not uncommon.

[lgoettgens]

To your main point: I am not sure whether your hcat method constitutes as type piracy (in the existence of the one from LinearAlgebra). Someone more experienced can maybe help here.

I started a thread on slack about that: https://julialang.slack.com/archives/C67910KEH/p1695887352340349
@mateuszbaran states there that your case is indeed type piracy.

[mateuszbaran]

It may be beneficial to explain what could go wrong in different cases of type piracy instead of putting everything in one bag.

module PkgA
    struct C end
    bar(x::C) = 1
    bar(x::Vector) = 2
end

module PkgB 
    import PkgA: bar
    struct D end
    bar(x::PkgA.C) = 1 # very bad
    bar(xs::D...) = 2 # very bad (the zero-argument version)
    bar(x::Vector{<:D}) = 3 # moderately bad: bar(Union{}[]) now calls this
    bar(x::Vector{D}) = 4 # slightly bad (may cause invalidations)
    bar(x::Union{C,D}) = 5 # slightly bad (a change in PkgA may turn it into piracy)
    #                        (for example changing bar(x::C) = 1 to bar(x::Union{C,Int}) = 1)
end

(that's a rough idea, may require iterating upon)

[lgoettgens]

Thanks @mateuszbaran, that helps a lot!

[LilithHafner]

Unions are bad for type piracy. Unless I'm missing something, if a type signature is piracy, adding elements to existing unions or turning existing types into unions will never fix the problem entirely.

Big +1 for "It may be beneficial to explain what could go wrong in different cases of type piracy instead of putting everything in one bag."

It's much more informative and motivating to hear "PkgA could define bar(::Union{C, Int}) and you would get an ambiguity on bar(C())" than "bar(::Union{C, D}) is type piracy, don't do it."

[jishnub]

I'd suggest separating bad design from explicit type-piracy. Bad design (such as ones that might potentially become piracy if other packages implement changes) may be warnings, whereas explicit type-piracies may be errors.

E.g. in the example above, bar(x::Vector{D}) may not be a bad design if this is explicitly documented to be the intended use case. For example, when implementing a custom broadcast style, one is required to define

Base.similar(bc::Broadcasted{DestStyle}, ::Type{ElType})

which matches the pattern above.
Similarly, the bar(x::Vector{<:D}) example matches the often-used bar(x::Type{<:AbstractType}) case, which is also a suggestion in the custom broadcast rule implementation:

Base.BroadcastStyle(::Type{<:MyType}) = MyStyle()

Any such method will also catch Type{Union{}} and the following call will become ambiguous:

Base.BroadcastStyle(::Type{Union{}})

Probably this isn't a problem?