Fix return type in broadcast when there are Type arguments

[pabloferz]

Fixes #19419

[KristofferC]

@nanosoldier runbenchmarks(ALL, vs = ":master")

[Sacha0]

Am I making a mistake below? ziptype now returns the correct type, but inference seems unhappy with the _default_eltype-first-generator-zip mechanism:

julia> begin
           ftype(f, A) = typeof(f)
           ftype(f, A...) = typeof(a -> f(a...))
           ftype(T::Type, A) = Type{T}
           ftype(T::Type, A...) = Type{T}
           ziptype(A) = Tuple{eltype(A)}
           ziptype(T::Type) = Tuple{Type{T}}
           ziptype(A, B) = Iterators.Zip2{ziptype(A), ziptype(B)}
           @inline ziptype(A, B, C, D...) = Iterators.Zip{ziptype(A), ziptype(B, C, D...)}
       end
ziptype (generic function with 4 methods)

julia> As = (Int, [1,2,3])
(Int64,[1,2,3])

julia> zt = ziptype(As...)
Base.Iterators.Zip2{Tuple{Type{Int64}},Tuple{Int64}}

julia> ft = ftype(round, As...)
##1#2{Base.#round}

julia> Base._default_eltype(Base.Generator{zt,ft})
Any

yet

julia> Core.Inference.return_type(round, Tuple{Type{Int},Int})
Int64

(I imagine there is a good reason for the _default_eltype-first-generator-zip mechanism? I.e. rather than simply constructing a call to return_type like that above?) Best!

[nanosoldier]

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

[pabloferz]

@Sacha0 I don't understand all the underlying details in inference all too well, but I'll try to convey what I can grasp from the problems that led to the current mechanism, which has the limitation you noticed.

Core.Inference.return_type is inherently non-inferable, but it gets a special treatment during inference, I believe, when called inside a pure function, so that rules out using it inside broadcast_c directly. So you might wonder if you can write a pure function, say foo, that calls Core.Inference.return_type, takes a function and its argument types, and which you can embed in broadcast. I think a difficulty with this, is that if you don't know the function you'll be calling, you can't ensure the pureness of foo. Additionally, you can't mark foo as pure given the way inference works.

Maybe there is another approach that could work, but I haven't been able to devise one. The only thing that I have found to work most of the time is the _default_eltype-first-generator-zip mechanism.

[vtjnash]

Core.Inference.return_type is inherently non-inferable, but it gets a special treatment during inference, I believe, when called inside a pure functio

That's backwards. It's distinctly not pure, so it must never be called from a function annotated as such. But since the bound on its return value is actually inferrable, it has special handling inside type inference. (I still need to look at this PR to understand if it's correct, but I've been afk for Thanksgiving)

[pabloferz]

@vtjnash Thanks for clarifying. That makes more sense.

[pabloferz]

Just a question then (to understand the situation better). Under what conditions the bound of return_type cannot be inferred or when it fails to get handled during inference? (given it is not called from a function annotated as pure)

[Sacha0]

IIUC, the eltype of the type ziptype constructs is a tuple, which necessitates ftype(f, A...) = typeof(a -> f(a...)) rather than ftype(f, A...) = f. Is that correct? If so, why does ftype(T::Type, A...) = Type{T} suffice rather than something like ftype(T::Type, A...) = typeof(a -> T(a...))? Thanks!

[Sacha0]

It's distinctly not pure, so it must never be called from a function annotated as such.

@vtjnash: promote_eltype_op is annotated pure, and calls promote_op. promote_op calls _default_eltype, which in turn calls Core.Inference.return_type. So IIUC, promote_eltype_op is invalid? Thanks!

[vtjnash]

Absolutely.

[pabloferz]

The use of @pure in promote_eltype_op is a reminiscent of the time when promote_op wasn't relying on inference. I removed it and simplified the whole mechanism so it works better.

[pabloferz]

Could we ask @nanosoldier again how is the performance?

[Sacha0]

💯

[Sacha0]

@nanosoldier runbenchmarks(ALL, vs = ":master")

[Sacha0]

The most recent simplifications look great! Do they make the test result now inferable? Best!

[pabloferz]

The most recent simplifications look great! Do they make the test result now inferable?

Yep

[nanosoldier]

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

[Sacha0]

With broadcast(round, Int, [1]) now inferred, drop the ::Vector{Int}? Also, shouldn't these combined @test-@inferred tests be written @test @inferred(broadcast(round, Int, [1])) == [1] instead? Otherwise the @inferred checks whether the == expression is inferable rather than the LHS? Likewise several of the preceding tests. Best!

[Sacha0]

These changes look great. Travis, Appveyor, and Nanosoldier approve. Absent objections or requests for time, I plan to merge this following the test touchups. Best!

[Sacha0]

(As an aside, though this PR dramatically reduces the amount of red in @code_warntype broadcast(round, Int, [1, 2, 3]), some red remains. Perhaps worth looking into at a later point? Best!)

[pabloferz]

Tests fixed. The AV error was an unrelated server failure. Some of the map_newindexer methods in Broadcast would have to be changed to reduce those red marks, but that can be done later as you point out (those do not affect the overall inferability of broadcast).

[Sacha0]

Thanks again @pabloferz! 💯

[KristofferC]

This (probably) caused the following regression:

julia> using StaticArrays

julia> s = rand(SVector{3});

julia> Base.Test.@inferred s * 0.0

Ping @andyferris

[KristofferC]

Not saying that there is anything wrong with this PR, it could be how StaticArray uses broadcast. Just bringing it up.

[pabloferz]

The problem with StaticArrays comes from the fact that the new approach now suffers (I believe) a manifestation of #15276. The old approach didn't had this problem but it is unable get things like broadcast(round, Int, [1]) to be inferable.

I could implement a mixed approach between the one in this PR and the previous mechanism that handles (except for some possible corner cases) both issues.

Another option is see if something can be done on the inference end @JeffBezanson?

Here is an example of something that used to work with the old approach but currently does not

foo(f, A, n) = broadcast(x -> f(x, n), A)
foo(+, [1.0, 2.0, 3.0], 1) # Not inferable but it was before

[Sacha0]

Ouch. Just confirmed

foo(f, A, n) = broadcast(x -> f(x, n), A)
Base.Test.@inferred foo(+, [1.0, 2.0, 3.0], 1)

previously worked (with concrete inferred type), but now fails. Odd though, the _default_eltype-first-generator-zip mechanism appeared to likewise fail in my testing for this related discourse thread? Testing further. Best!

[Sacha0]

Checked, both the example above and that from the linked discourse thread fail on master, but both work on the commit prior to the merge commit for this pull request. I must've made a mistake when testing for the related discourse thread? Best!

[tkelman]

If there isn't an easy fix to be had soon, could at least add some test_broken cases that we would ideally like to work, for tracking purposes

[Sacha0]

the new approach now suffers (I believe) a manifestation of #15276. The old approach didn't had this problem but it is unable get things like broadcast(round, Int, [1]) to be inferable.

Ref. @yuyichao's comment on discourse.