Support and document a vector-based API

[charleskawczynski]

While the existing interface is ideal if users are work on packages with no instances of method ambiguities, piracy and unbound method argument types. That is just not the case. The interface @test_X(SomePackage) (e.g., @test_ambiguities(SomePackage)) works in the case that a package is able to pass this test, but it's otherwise unhelpful.

Similarly, I've seen @test_broken isempty(Aqua.detect_ambiguities(SomePackage)), while this works for packages that don't pass @test_ambiguities, however it still has downsides:

• It doesn't prevent more ambiguities from entering the package, and
• It only encourages contributors to fix all of the issues, and not only some of them.

I would like to propose changing this package's api (and update documentation to encourage users to fix their issues) to always return a vector of the offending issues, similar to how Aqua.detect_ambiguities(SomePackage) works. We can then document Aqua.jl's use as:

@test length(Aqua.detect_ambiguities(SomePackage)) ≤ 27

This way, users can incrementally fix issues they may have, while preventing new ones from entering.

[lgoettgens]

Thanks for your issue. I just noticed that the stable docs are out of date (#181), so please refer to the dev docs.

There is a kwarg broken for most tests as a simple pattern to say that this particular test is broken.

Now towards your main point: I don't think that tracking a number of ambiguities is the right way forward, as this only counts the number, but not the type of ambiguity. For your use-case, I would put all functions with ambiguities into the exclude kwarg, and then gradually remove them once they are fixed.

There already is the function you want to have as an internal (called in https://github.com/JuliaTesting/Aqua.jl/blob/3cd8d0ea4496010c4b0c25954e6171c2b8a67004/src/ambiguities.jl#L88), but I would refrain from making it part of the public API (at least for now) as Aqua needs to tangle around many weird things and internals of julia

Furthermore, for counting ambiguities in your code, you can just use Test.detect_ambiguities from julia stdlib.

[charleskawczynski]

I don't think that tracking a number of ambiguities is the right way forward, as this only counts the number, but not the type of ambiguity.

...

For your use-case, I would put all functions with ambiguities into the exclude kwarg, and then gradually remove them once they are fixed.

I agree that my suggestion of using the number of ambiguities (or instances of type piracy, unbound method args etc.) is a less precise than using the exclude kwarg, however, I suspect that your suggestion is much more complicated to add than my own in many real world situations. And some tests are better than none for package users.

Let's take method ambiguities for a package I'm working on, for example:

method_ambiguity = ((::Type{T})(x::Static.StaticInteger) where T<:Real @ Static ~/.julia/packages/Static/dLrtk/src/Static.jl:421, (var"#ctor-self#"::Type{ClimaCore.Utilities.PlusHalf{I}} where I<:Integer)(i) @ ClimaCore.Utilities dev/ClimaCore.jl/src/Utilities/plushalf.jl:13)
method_ambiguity = (broadcasted(::Base.Broadcast.DefaultArrayStyle{N}, op, x::Number, r::FillArrays.AbstractFill{T, N}) where {T, N} @ FillArrays ~/.julia/packages/FillArrays/FwMoZ/src/fillbroadcast.jl:236, broadcasted(fs::Base.Broadcast.DefaultArrayStyle{0}, ::Type{T}, args...) where T<:AxisTensor @ ClimaCore.Fields dev/ClimaCore.jl/src/Fields/broadcast.jl:294)
method_ambiguity = (ldiv!(L::LinearAlgebra.LU{<:Any, <:ArrayLayouts.LayoutMatrix}, B::ArrayLayouts.LayoutVector) @ ArrayLayouts ~/.julia/packages/ArrayLayouts/LXeCF/src/factorizations.jl:444, ldiv!(A::LinearAlgebra.LU, x::ClimaCore.Fields.FieldVector) @ ClimaCore.Fields dev/ClimaCore.jl/src/Fields/fieldvector.jl:259)
method_ambiguity = (broadcasted(::Base.Broadcast.DefaultArrayStyle{N}, op, r::FillArrays.AbstractFill{T, N}, x::Number) where {T, N} @ FillArrays ~/.julia/packages/FillArrays/FwMoZ/src/fillbroadcast.jl:235, broadcasted(fs::Base.Broadcast.DefaultArrayStyle{0}, ::Type{T}, args...) where T<:AxisTensor @ ClimaCore.Fields dev/ClimaCore.jl/src/Fields/broadcast.jl:294)
method_ambiguity = (broadcasted(::Base.Broadcast.DefaultArrayStyle{N}, op, r::FillArrays.AbstractFill{T, N}, x::Ref) where {T, N} @ FillArrays ~/.julia/packages/FillArrays/FwMoZ/src/fillbroadcast.jl:237, broadcasted(fs::Base.Broadcast.DefaultArrayStyle{0}, ::Type{T}, args...) where T<:AxisTensor @ ClimaCore.Fields dev/ClimaCore.jl/src/Fields/broadcast.jl:294)
method_ambiguity = (copyto!(dest::AbstractArray, bc::Base.Broadcast.Broadcasted{<:BlockArrays.AbstractBlockStyle{NDims}, <:Any, <:Any, Args}) where {NDims, Args<:Tuple} @ BlockArrays ~/.julia/packages/BlockArrays/4FBcw/src/blockbroadcast.jl:190, copyto!(dest::ClimaCore.Fields.FieldVector, bc::Base.Broadcast.Broadcasted{<:Base.Broadcast.AbstractArrayStyle{0}}) @ ClimaCore.Fields dev/ClimaCore.jl/src/Fields/fieldvector.jl:218)
method_ambiguity = (broadcasted(::Base.Broadcast.DefaultArrayStyle, op, a::FillArrays.AbstractFill, b::FillArrays.AbstractFill) @ FillArrays ~/.julia/packages/FillArrays/FwMoZ/src/fillbroadcast.jl:95, broadcasted(fs::Base.Broadcast.DefaultArrayStyle{0}, ::Type{T}, args...) where T<:AxisTensor @ ClimaCore.Fields dev/ClimaCore.jl/src/Fields/broadcast.jl:294)
method_ambiguity = (copyto!(dest::AbstractArray, bc::Base.Broadcast.Broadcasted{<:GPUArraysCore.AbstractGPUArrayStyle}) @ GPUArrays ~/.julia/packages/GPUArrays/5XhED/src/host/broadcast.jl:46, copyto!(dest::ClimaCore.Fields.FieldVector, bc::Base.Broadcast.Broadcasted{<:Base.Broadcast.AbstractArrayStyle{0}}) @ ClimaCore.Fields dev/ClimaCore.jl/src/Fields/fieldvector.jl:218)
method_ambiguity = (copyto!(dest::AbstractArray, B::Base.Broadcast.Broadcasted{<:StaticArraysCore.StaticArrayStyle}) @ StaticArrays ~/.julia/packages/StaticArrays/dTwvg/src/broadcast.jl:65, copyto!(dest::ClimaCore.Fields.FieldVector, bc::Base.Broadcast.Broadcasted{<:Base.Broadcast.AbstractArrayStyle{0}}) @ ClimaCore.Fields dev/ClimaCore.jl/src/Fields/fieldvector.jl:218)
method_ambiguity = (broadcasted(::Base.Broadcast.DefaultArrayStyle{N}, op, x::Ref, r::FillArrays.AbstractFill{T, N}) where {T, N} @ FillArrays ~/.julia/packages/FillArrays/FwMoZ/src/fillbroadcast.jl:238, broadcasted(fs::Base.Broadcast.DefaultArrayStyle{0}, ::Type{T}, args...) where T<:AxisTensor @ ClimaCore.Fields dev/ClimaCore.jl/src/Fields/broadcast.jl:294)
method_ambiguity = (broadcasted(::Base.Broadcast.DefaultArrayStyle{N}, op, r::FillArrays.AbstractFill{T, N}) where {T, N} @ FillArrays ~/.julia/packages/FillArrays/FwMoZ/src/fillbroadcast.jl:78, broadcasted(fs::Base.Broadcast.DefaultArrayStyle{0}, ::Type{T}, args...) where T<:AxisTensor @ ClimaCore.Fields dev/ClimaCore.jl/src/Fields/broadcast.jl:294)
method_ambiguity = (ldiv!(L::LinearAlgebra.LU{<:Any, <:ArrayLayouts.LayoutMatrix}, B) @ ArrayLayouts ~/.julia/packages/ArrayLayouts/LXeCF/src/factorizations.jl:422, ldiv!(A::LinearAlgebra.LU, x::ClimaCore.Fields.FieldVector) @ ClimaCore.Fields dev/ClimaCore.jl/src/Fields/fieldvector.jl:259)
method_ambiguity = (ldiv!(L::LinearAlgebra.LU{<:Any, <:SubArray{<:Any, 2, <:ArrayLayouts.LayoutMatrix}}, B) @ ArrayLayouts ~/.julia/packages/ArrayLayouts/LXeCF/src/factorizations.jl:422, ldiv!(A::LinearAlgebra.LU, x::ClimaCore.Fields.FieldVector) @ ClimaCore.Fields dev/ClimaCore.jl/src/Fields/fieldvector.jl:259)

With my suggested interface, adding this test is as simple as

    ambs = Aqua.detect_ambiguities(ClimaCore; recursive = true)
    @test length(ambs) ≤ 13

How would this look using the exclude kwarg?

Furthermore, for counting ambiguities in your code, you can just use Test.detect_ambiguities from julia stdlib.

Yes, I'm aware of this function, I just like the idea of this package having a clear, simple, and consistent API.

I think using the exclude kwarg may make sense for simple cases, with few ambiguities, but, frankly, there are many julia packages that don't use Aqua, and it's frustrating to deal with all of the resulting issues. I'm happy to put time into the ecosystem with adding these tests, however, given how many packages need these tests, I feel that I could have a larger impact by simply restricting the number of offending problems.

Can we at least add support for these style of tests?

[charleskawczynski]

Not for nothing, but I have the same issue with JETjl. Is it actually realistic for me to add excludes when ═════ 1743 possible errors found ═════?

[lgoettgens]

I can have a look when I find some time if/how we can offer such functions as part of the public API without fixing too much of the Aqua internals.

[fingolfin]

Actually I don't think exclude is much better here than a total number of ambiguities -- if I exclude some function foo, then I won't notice if additional ambiguities for that function are introduced. E.g. in one project I work on there are currently 14 ambiguities for divexact; I would like to know if we introduce additional ones; but also if we fix some of them.

If instead I could get a dictionary with entries of the form :divexact => 14 I could implement a much more precise check. (Here I would hope that also kwcalls such as kwcall(::Any, ::typeof(AbstractAlgebra.divexact), a::Nemo.ZZRingElem, b::AbstractAlgebra.FracElem) could be "normalized" to just AbstractAlgebra.divexact or divexact)

Of course even more powerful would be if the full list of ambiguities could be put into a serialized form, including information on the full signature (so that I can recognize the case when in one PR, one ambiguity is fixed but another one is introduced). However, this is to me really optional, as that seems like a relatively scarce scenarios...

Also, we shouldn't let the perfect be the enemy of the good, i.e. some imperfect improvement here (which can be improved incrementally over time) beats a never implemented perfect solution any day.