Skip to content

Fix method ambiguities in SparseArrays #30120

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 23, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions stdlib/SparseArrays/src/higherorderfns.jl
Original file line number Diff line number Diff line change
Expand Up @@ -973,6 +973,7 @@ function _copy(f, args...)
parevalf, passedargstup = capturescalars(f, args)
return _copy(parevalf, passedargstup...)
end
_copy(f) = throw(MethodError(_copy, (f,))) # avoid method ambiguity

function _shapecheckbc(f, args...)
_aresameshape(args...) ? _noshapecheck_map(f, args...) : _diffshape_broadcast(f, args...)
Expand Down Expand Up @@ -1006,10 +1007,6 @@ end
_copyto!(parevalf, dest, passedsrcargstup...)
end

struct CapturedScalars{F, Args, Order}
Copy link
Member Author

Choose a reason for hiding this comment

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

git grep CapturedScalars shows me that there is no code using it. Maybe some remnant from experimenting with capturescalars? This was irrelevant to this PR but I thought it's OK to remove it here (as I'm touching capturescalars).

args::Args
end

# capturescalars takes a function (f) and a tuple of mixed sparse vectors/matrices and
# broadcast scalar arguments (mixedargs), and returns a function (parevalf, i.e. partially
# evaluated f) and a reduced argument tuple (passedargstup) containing only the sparse
Expand All @@ -1024,9 +1021,13 @@ end
# Work around losing Type{T}s as DataTypes within the tuple that makeargs creates
@inline capturescalars(f, mixedargs::Tuple{Ref{Type{T}}, Vararg{Any}}) where {T} =
capturescalars((args...)->f(T, args...), Base.tail(mixedargs))
@inline capturescalars(f, mixedargs::Tuple{Ref{Type{T}}, Ref{Type{S}}, Vararg{Any}}) where {T, S} =
# This definition is identical to the one above and necessary only for
# avoiding method ambiguity.
capturescalars((args...)->f(T, args...), Base.tail(mixedargs))
@inline capturescalars(f, mixedargs::Tuple{SparseVecOrMat, Ref{Type{T}}, Vararg{Any}}) where {T} =
capturescalars((a1, args...)->f(a1, T, args...), (mixedargs[1], Base.tail(Base.tail(mixedargs))...))
@inline capturescalars(f, mixedargs::Tuple{Union{Ref,AbstractArray{0}}, Ref{Type{T}}, Vararg{Any}}) where {T} =
@inline capturescalars(f, mixedargs::Tuple{Union{Ref,AbstractArray{<:Any,0}}, Ref{Type{T}}, Vararg{Any}}) where {T} =
Copy link
Member Author

Choose a reason for hiding this comment

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

This was irrelevant to this PR but I suppose AbstractArray{0} was a typo?

capturescalars((args...)->f(mixedargs[1], T, args...), Base.tail(Base.tail(mixedargs)))

nonscalararg(::SparseVecOrMat) = true
Expand Down
4 changes: 4 additions & 0 deletions stdlib/SparseArrays/test/ambiguous_exec.jl
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# This file is a part of Julia. License is MIT: https://julialang.org/license

using Test, SparseArrays
@test detect_ambiguities(SparseArrays; imported=true, recursive=true) == []
24 changes: 24 additions & 0 deletions stdlib/SparseArrays/test/higherorderfns.jl
Original file line number Diff line number Diff line change
Expand Up @@ -632,4 +632,28 @@ end
@test minimum(sparse([1, 2], [1, 2], ones(Int32, 2)), dims = 1) isa Matrix
end

@testset "Issue #30118" begin
@test ((_, x) -> x).(Int, spzeros(3)) == spzeros(3)
@test ((_, _, x) -> x).(Int, Int, spzeros(3)) == spzeros(3)
@test ((_, _, _, x) -> x).(Int, Int, Int, spzeros(3)) == spzeros(3)
@test_broken ((_, _, _, _, x) -> x).(Int, Int, Int, Int, spzeros(3)) == spzeros(3)
Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like capturescalars has type inference issue when it has to recurse a lot (> 3). I wounder if capturescalars could be implemented more naturally (for humans and maybe for Julia as well?) if you used @generated function. Any reason why capturescalars is implemented based on recursion?

end

using SparseArrays.HigherOrderFns: SparseVecStyle

@testset "Issue #30120: method ambiguity" begin
# HigherOrderFns._copy(f) was ambiguous. It may be impossible to
# invoke this from dot notation and it is an error anyway. But
# when someone invokes it by accident, we want it to produce a
# meaningful error.
err = try
copy(Broadcast.Broadcasted{SparseVecStyle}(rand, ()))
catch err
err
end
@test err isa MethodError
@test !occursin("is ambiguous", sprint(showerror, err))
@test occursin("no method matching _copy(::typeof(rand))", sprint(showerror, err))
end

end # module
8 changes: 8 additions & 0 deletions stdlib/SparseArrays/test/sparse.jl
Original file line number Diff line number Diff line change
Expand Up @@ -2343,4 +2343,12 @@ end
@test_throws MethodError sprandn(T, 5, 5, 0.5)
end

@testset "method ambiguity" begin
# Ambiguity test is run inside a clean process.
# https://github.com/JuliaLang/julia/issues/28804
script = joinpath(@__DIR__, "ambiguous_exec.jl")
cmd = `$(Base.julia_cmd()) --startup-file=no $script`
@test success(pipeline(cmd; stdout=stdout, stderr=stderr))
end

end # module