Skip to content

Conversation

Liozou
Copy link
Contributor

@Liozou Liozou commented Jul 9, 2020

This PR fixes a method ambiguity in StaticArrays.jl. I found it while testing my package (which depends on StaticArrays.jl) with Aqua.jl and it popped up because I was overloading convert (for something completely unrelated).

Before this PR:

julia> using StaticArrays

julia> Scalar{Int}[SVector{1,Int}(3)]
ERROR: MethodError: convert(::Type{Scalar{Int64}}, ::SVector{1,Int64}) is ambiguous. Candidates:
  convert(::Type{SA}, a::AbstractArray) where SA<:(Scalar{T} where T) in StaticArrays at /home/liozou/.julia/dev/StaticArrays/src/Scalar.jl:12
  convert(::Type{SA}, sa::StaticArray) where SA<:StaticArray in StaticArrays at /home/liozou/.julia/dev/StaticArrays/src/convert.jl:10
Possible fix, define
  convert(::Type{SA}, ::StaticArray) where SA<:(Scalar{T} where T)
Stacktrace:
 [1] setindex!(A::Vector{Scalar{Int64}}, x::SVector{1,Int64}, i1::Int64)
   @ Base ./array.jl:849
 [2] getindex(#unused#::Type{Scalar{Int64}}, x::SVector{1,Int64})
   @ Base ./array.jl:416
 [3] top-level scope
   @ none:1

julia> Scalar[SVector{1,Float64}(2.0)]
ERROR: MethodError: convert(::Type{Scalar{T} where T}, ::SVector{1,Float64}) is ambiguous. Candidates:
  convert(::Type{SA}, a::AbstractArray) where SA<:(Scalar{T} where T) in StaticArrays at /home/liozou/.julia/dev/StaticArrays/src/Scalar.jl:12
  convert(::Type{SA}, sa::StaticArray) where SA<:StaticArray in StaticArrays at /home/liozou/.julia/dev/StaticArrays/src/convert.jl:10
Possible fix, define
  convert(::Type{SA}, ::StaticArray) where SA<:(Scalar{T} where T)
Stacktrace:
 [1] setindex!(A::Vector{Scalar{T} where T}, x::SVector{1,Float64}, i1::Int64)
   @ Base ./array.jl:849
 [2] getindex(#unused#::Type{Scalar{T} where T}, x::SVector{1,Float64})
   @ Base ./array.jl:416
 [3] top-level scope
   @ none:1

With this PR:

julia> Scalar{Int}[SVector{1,Int}(3)]
1-element Vector{Scalar{Int64}}:
 Scalar{Int64}((3,))

julia> Scalar[SVector{1,Float64}(2.0)]
1-element Vector{Scalar{T} where T}:
 Scalar{Float64}((2.0,))

@Liozou
Copy link
Contributor Author

Liozou commented Jul 9, 2020

I just noticed how close that PR is to #774 (I don't know how I didn't see that before)... If don't think they are actually equivalent, but feel free to close as a dup if you think both issues should be addressed directly in #774 for instance.
CI failures on nightly look unrelated.

@tkf
Copy link
Member

tkf commented Jul 10, 2020

Great! So, can we set this to 0 for modern Julia now?

const allowable_ambiguities =
if VERSION < v"1.1"
4
elseif VERSION < v"1.2"
2
else
1
end

@Liozou
Copy link
Contributor Author

Liozou commented Jul 10, 2020

Great! So, can we set this to 0 for modern Julia now?

const allowable_ambiguities =
if VERSION < v"1.1"
4
elseif VERSION < v"1.2"
2
else
1
end

It looks like so! I just did that in the last commit.
Nightly is still broken for different reasons but I don't have any ambiguity locally for both v1.5.0-rc1 and master. Since CI smiles upon the other Julia versions, there should be no ambiguity left for julia versions >= v1.2.0! And this also means that any project relying on StaticArrays can safely use Aqua.jl (or any automatic ambiguity detection system) in their test suite without risking a test failure due to StaticArrays, which is great.

Caveat though: I'm talking about ambiguities being detected by Test.detect_ambiguities(Base, LinearAlgebra, StaticArrays), which doesn't seem to see #774 for instance, so there might still be some internal ambiguities left.

@tkf
Copy link
Member

tkf commented Jul 11, 2020

Caveat though: I'm talking about ambiguities being detected by Test.detect_ambiguities(Base, LinearAlgebra, StaticArrays), which doesn't seem to see #774 for instance, so there might still be some internal ambiguities left.

Oh, interesting. So it looks like detect_ambiguities doesn't detect the ambiguity in the constructor unless we specify Core as well? I guess this is because parentmodule(Type) === Core. The output of detect_ambiguities(Core, Base, LinearAlgebra, StaticArrays) is rather sad...

I guess it means I should add Core by default in Aqua.jl (though this is a breaking change).

Copy link
Member

@tkf tkf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tightening the ambiguity bounds!

It looks like the issue in nightly is due to JuliaLang/julia#36107 so it's not relevant to this PR.

I think this is good to go since the new convert definition is compatible with the existing ones. Let's leave it for a few days just in case @c42f or @andyferris want to have a look at it.

Copy link
Member

@c42f c42f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@tkf tkf merged commit ad583c9 into JuliaArrays:master Jul 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants