Open dkarrasch opened 12 months ago
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 (https://github.com/JuliaTesting/Aqua.jl/pull/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.
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.
To your main point: I am not sure whether your
hcat
method constitutes as type piracy (in the existence of the one fromLinearAlgebra
). 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.
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)
Thanks @mateuszbaran, that helps a lot!
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."
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:
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 methodwhich catches the "dangerous" case. All of this was conciously designed, so that
LinearMaps
's method can only be called if there is at least oneLinearMap
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.