Skip to content

Conversation

sjkelly
Copy link
Contributor

@sjkelly sjkelly commented May 12, 2020

Before:

julia> detect_ambiguities(Base, LinearAlgebra, StaticArrays)
2-element Array{Tuple{Method,Method},1}:
 (unsafe_convert(::Type{Ptr{T}}, a::Base.RefValue{SArray{S,T,D,L}}) where {S, T, D, L} in StaticArrays at /home/steve/.julia/dev/StaticArrays/src/SArray.jl:69, unsafe_convert(::Type{Ptr{Nothing}}, b::Base.RefValue{T}) where T in Base at refvalue.jl:30)
 (convert(::Type{SA}, a::AbstractArray) where SA<:(SArray{Tuple{},T,0,1} where T) in StaticArrays at /home/steve/.julia/dev/StaticArrays/src/Scalar.jl:13, convert(::Type{SA}, sa::StaticArray) where SA<:StaticArray in StaticArrays at /home/steve/.julia/dev/StaticArrays/src/convert.jl:10)

After:

julia> detect_ambiguities(Base, LinearAlgebra, StaticArrays)
1-element Array{Tuple{Method,Method},1}:
 (convert(::Type{SA}, a::AbstractArray) where SA<:(SArray{Tuple{},T,0,1} where T) in StaticArrays at /home/steve/.julia/dev/StaticArrays/src/Scalar.jl:13, convert(::Type{SA}, sa::StaticArray) where SA<:StaticArray in StaticArrays at /home/steve/.julia/dev/StaticArrays/src/convert.jl:10

The remaining ambiguity is covered by #774

@sjkelly
Copy link
Contributor Author

sjkelly commented May 12, 2020

The failure on nightly is reproducible locally. It also affects master.

@c42f
Copy link
Member

c42f commented May 14, 2020

It's mysterious to me that unsafe_convert(Ptr{Nothing}, Ref(SA[1,2])) works without this fix rather than throwing a MethodError. Maybe detect_ambiguities is not precise?

Relatedly, I do wonder whether it would be better to deprecate the associated cconvert, it's more explicit to have people manually wrap in a Ref if they need to pass an SVector to a ccall. But that's a separate issue.

In any case, thanks for fixing this. Let's merge it!

@c42f c42f merged commit 3372719 into JuliaArrays:master May 14, 2020
@c42f
Copy link
Member

c42f commented May 14, 2020

Ah I got a little trigger happy; we should also have reduced the ambiguity counter in the tests to go with this improvement - could I bother you to do that as well?

@c42f
Copy link
Member

c42f commented May 14, 2020

Relatedly, I do wonder whether it would be better to deprecate the associated cconvert

I started opening an issue about this, but in the process realized that the current behavior is pretty reasonable. So move on, nothing to see here :)

@sjkelly
Copy link
Contributor Author

sjkelly commented May 14, 2020

It still doesn't really make sense to me, but I am not well versed in the inference/matching mechanics. If I remember correctly the behaviour is the same on 1.0 and 1.1 as well.

Ah I got a little trigger happy; we should also have reduced the ambiguity counter in the tests to go with this improvement - could I bother you to do that as well?

I'll test on prior Julias and get bounds in a different PR. I think 1.0 still has 2 or 3 remaining. So maybe have tighter bounds for 1.4+.

@c42f
Copy link
Member

c42f commented May 14, 2020

I think 1.0 still has 2 or 3 remaining. So maybe have tighter bounds for 1.4+.

Perfect, much appreciated!

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.

2 participants