-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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...) | ||
|
@@ -1006,10 +1007,6 @@ end | |
_copyto!(parevalf, dest, passedsrcargstup...) | ||
end | ||
|
||
struct CapturedScalars{F, Args, Order} | ||
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 | ||
|
@@ -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} = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was irrelevant to this PR but I suppose |
||
capturescalars((args...)->f(mixedargs[1], T, args...), Base.tail(Base.tail(mixedargs))) | ||
|
||
nonscalararg(::SparseVecOrMat) = true | ||
|
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) == [] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like |
||
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 |
There was a problem hiding this comment.
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 withcapturescalars
? This was irrelevant to this PR but I thought it's OK to remove it here (as I'm touchingcapturescalars
).