`cumsum` gives bad error message when collection elements can't be added

[LilithHafner]

julia> sum(sincos.(1:10)) # Good error
ERROR: MethodError: no method matching +(::Tuple{Float64, Float64}, ::Tuple{Float64, Float64})

Closest candidates are:
  +(::Any, ::Any, ::Any, ::Any...)
   @ Base operators.jl:587

Stacktrace:
 [1] add_sum(x::Tuple{Float64, Float64}, y::Tuple{Float64, Float64})
   @ Base ./reduce.jl:24
 [2] _mapreduce
   @ ./reduce.jl:440 [inlined]
 [3] _mapreduce_dim
   @ ./reducedim.jl:365 [inlined]
 [4] mapreduce
   @ ./reducedim.jl:357 [inlined]
 [5] _sum
   @ ./reducedim.jl:1015 [inlined]
 [6] _sum
   @ ./reducedim.jl:1014 [inlined]
 [7] sum(a::Vector{Tuple{Float64, Float64}})
   @ Base ./reducedim.jl:1010
 [8] top-level scope
   @ REPL[24]:1

julia> cumsum(sincos.(1:10)) # Bad error
ERROR: ArgumentError: cannot convert a value to Union{} for assignment
Stacktrace:
  [1] convert(T::Core.TypeofBottom, x::Tuple{Float64, Float64})
    @ Base ./essentials.jl:315
  [2] setindex!(A::Vector{Union{}}, x::Tuple{Float64, Float64}, i1::Int64)
    @ Base ./array.jl:1021
  [3] accumulate_pairwise!
    @ ./accumulate.jl:29 [inlined]
  [4] _cumsum!(out::Vector{Union{}}, v::Vector{Tuple{Float64, Float64}}, dim::Int64, ::Base.ArithmeticRounds)
    @ Base ./accumulate.jl:57
  [5] _cumsum!
    @ ./accumulate.jl:60 [inlined]
  [6] #cumsum!#889
    @ ./accumulate.jl:53 [inlined]
  [7] cumsum!
    @ ./accumulate.jl:51 [inlined]
  [8] #cumsum#890
    @ ./accumulate.jl:115 [inlined]
  [9] cumsum
    @ ./accumulate.jl:113 [inlined]
 [10] cumsum(x::Vector{Tuple{Float64, Float64}})
    @ Base ./accumulate.jl:146
 [11] top-level scope
    @ REPL[25]:1

In 1.9 the error was

julia> cumsum(sincos.(1:10))
ERROR: MethodError: Base.ArithmeticStyle(::Type{Union{}}) is ambiguous.

[rofinn]

The issue is that cumsum and many of the other accumulators in that file are calling promote_op to infer the return eltype. The docstring doesn't recommend using promote_op, but in this case we don't have much of an option. The indexing error makes sense cause you can't assign anything to Vector{Union{}} and reduce_first(+, Tuple{Int, Int}) is fine prior to insertion.

I don't think it makes sense to throw a MethodError from those calls, but we might be able to modify promote_op to include throw_error=false. Then in places where returning Union{} will error anyways we could have it consistently do error with something like "Cannot infer return type for +(Tuple{Float64, Float64}, Tuple{Float64, Float64})". Would an error message like that be helpful?

[LilithHafner]

Yes! Though inference returning Union{} is not "cannot infer" it's "inferred that it cannot return".

"+(Tuple{Float64, Float64}, Tuple{Float64, Float64}) does not return"

[rofinn]

Oh, right, cause not being able to infer the type should still fallback to Any. Okay, I can make a PR later if you're fine with that wording and it being an ArgumentError?

[LilithHafner]

Thanks! Feel free to ping me in the PR.

[vtjnash]

I am a little bit unclear on why #53461 is thought to fix this. The issue here seems to be that in the implementation of accumulate, it was assumed that promote_op(add_sum, T, T) >: promote_op(reduce_first, typeof(add_sum), T), but that is not a particularly well-founded assumption, since the later is defined to return either T or something equivalent (such as Int for SmallInt), but this is simply one example of why that assumption is not valid.

I think either the correct value there is either some sort of Union{promote_op(add_sum, T, T), promote_op(reduce_first, typeof(add_sum), T)} or typejoin, or a restructuring of the logic of that accumulate function to avoid trying to force the input type to be equal to the output type, which is not a constraint imposed by the documentation of accumulate.

For example, this sample code is also in the docstring, but errors when simple variants of it are attempted to be used:

julia> accumulate(Pair, [i^2 for i in 1:3])
ERROR: MethodError: Cannot `convert` an object of type Int64 to an object of type Pair{Int64, Int64}
The function `convert` exists, but no method is defined for this combination of argument types.

Closest candidates are:
  Pair{A, B}(::Any, ::Any) where {A, B}
   @ Base Base.jl:219
  convert(::Type{Pair{A, B}}, ::Pair{A, B}) where {A, B}
   @ Base pair.jl:61
  convert(::Type{Pair{A, B}}, ::Pair) where {A, B}
   @ Base pair.jl:62
  ...

Stacktrace:
 [1] setindex!(A::Vector{Pair{Int64, Int64}}, x::Int64, i::Int64)
   @ Base ./array.jl:976
 [2] _accumulate1!(op::Type{Pair}, B::Vector{Pair{Int64, Int64}}, v1::Int64, A::Vector{Int64}, dim::Int64)
   @ Base ./accumulate.jl:435
 [3] _accumulate!(op::Type, B::Vector{Pair{Int64, Int64}}, A::Vector{Int64}, dims::Nothing, init::Nothing)
   @ Base ./accumulate.jl:363
 [4] #accumulate!#991
   @ ./accumulate.jl:348 [inlined]
 [5] accumulate(op::Type, A::Vector{Int64}; dims::Nothing, kw::@Kwargs{})
   @ Base ./accumulate.jl:291
 [6] accumulate(op::Type, A::Vector{Int64})
   @ Base ./accumulate.jl:278
 [7] top-level scope
   @ REPL[12]:1

[rofinn]

restructuring of the logic of that accumulate function to avoid trying to force the input type to be equal to the output type, which is not a constraint imposed by the documentation of accumulate.

👍🏻 If we can avoid promote_op altogether, I think that would be best.

[martinholters]

Ceterum censeo promote_op esse delendam.