maximum(::Broadcasted) can produce buggy outputs with stateful functions

[tkf]

Combining a stateful function, lazy broadcasted object, and reduction can produce a buggy result.

Here is a demo:

function badzero_before_second_pass(i)
    global FIRST_PASS
    if i == 1
        if FIRST_PASS
            FIRST_PASS = false
            return -0.0  # "badzero"
        else
            return 0.0
        end
    end
    return -1
end

using Base.Broadcast: broadcasted, instantiate

FIRST_PASS = true
@show maximum(instantiate(broadcasted(badzero_before_second_pass, 1:17)))

FIRST_PASS = true
@show maximum(badzero_before_second_pass.(1:17))

prints

maximum(instantiate(broadcasted(badzero_before_second_pass, 1:17))) = 0.0
maximum(badzero_before_second_pass.(1:17)) = -0.0

This is because maximum go over an array twice:

julia/base/reduce.jl

Lines 594 to 609 in acd0e83

	v = op(op(v1,v2),op(v3,v4))
	for i in start:last
	@inbounds ai = A[i]
	v = op(v, f(ai))
	end
	# enforce correct order of 0.0 and -0.0
	# e.g. maximum([0.0, -0.0]) === 0.0
	# should hold
	if isbadzero(op, v)
	for i in first:last
	x = @inbounds A[i]
	isgoodzero(op,x) && return x
	end
	end
	return v

This is not new in the sense as it could have happened with something like https://github.com/JuliaArrays/MappedArrays.jl.

(I'm not sure what should be done with it but I just thought it's worth recording in the issue. Personally, I think stateful function should be "banned" (i.e., declared to be an undefined behavior) in broadcasting but I've seen, e.g., @mbauman mentioning rand.() is a nice idiom so this is likely not everyone's favorite definition. In general, I think we need to formalize what to be expected from broadcasting. For example, sparse style broadcasting already expects some kind of pureness in the functions.)

[mbauman]

This is precisely why I've been dragging my feet on formalizing the creation of lazy broadcasted objects (#31088).