Make deprecated promote_eltype_op type stable

[TotalVerb]

The suggestion for a replacement for the deprecated promote_eltype_op was type-unstable, so this replaces it with a type-stable version. Nevertheless, there is very little reason for this function to be called at all. Arguably it may even better to suggest a Base._broadcast_eltype as a replacement.

cc @martinholters @Sacha0 @pabloferz
related #19814

[Sacha0]

Given #19814 (comment), perhaps instead follow the typical deprecation approach, i.e. move the former logic of promote_eltype_op and broadcast_elwise_op into base/deprecated.jl and inject a depwarn into those methods? Best!

[TotalVerb]

@Sacha0 Unfortunately, the typical deprecation logic requires a replacement, and the former logic of those functions to too complex to admit a short replacement.

[TotalVerb]

Perhaps a possible approach would be to deprecate this function without replacement, keeping the logic from before.

[Sacha0]

Unfortunately, the typical deprecation logic requires a replacement, and the former logic of those functions to too complex to admit a short replacement.

Not following you? The typical approach to this sort of deprecation seems to be e.g.

# Deprecate promote_eltype_op
_promote_eltype_op(::Any) = Any
_promote_eltype_op(op, A) = (@_inline_meta; _promote_op(op, eltype(A)))
_promote_eltype_op(op, A, B) = (@_inline_meta; _promote_op(op, eltype(A), eltype(B)))
_promote_eltype_op(op, A, B, C, D...) = (@_inline_meta; _promote_eltype_op(op, eltype(A), _promote_eltype_op(op, B, C, D...)))
@inline function promote_eltype_op(op, A, B, C, D...)
    depwarn("promote_eltype_op deprecated etc etc...")
    _promote_eltype_op(op, A, B, C, D...)
end

[TotalVerb]

Indeed, that is what I meant by my last comment. This is a deprecation without replacement.

(Most deprecations seem to be done with replacement.)

[TotalVerb]

@Sacha0 I believe I will implement that, but what should be done about the deprecation message? Should we recommend Base.Broadcast._broadcast_eltype?

[Sacha0]

Indeed, that is what I meant by my last comment. This is a deprecation without replacement.

Cheers, same page now.

I believe I will implement that, but what should be done about the deprecation message? Should we recommend Base.Broadcast._broadcast_eltype?

Recommending Base.Broadcast._broadcast_eltype seems reasonable on this end. (Re. additionally mentioning promote_op: On the one hand, mentioning promote_op for the one and two argument cases might be useful to former promote_eltype_op users. On the other hand, with the direction in #19669, suggesting promote_op seems ill-advised.) Best!

[TotalVerb]

I have some reservations about recommending a function prefixed with an underscore. Perhaps we should rename it to and recommend Base.Broadcast.broadcast_eltype.

[Sacha0]

I have some reservations about recommending a function prefixed with an underscore. Perhaps we should rename it to and recommend Base.Broadcast.broadcast_eltype.

Likewise: I'm not certain whether encouraging _broadcast_eltype's use outside of base is wise in general, in part because the underscore of magic and fear strikes me as appropriate (given that _broadcast_eltype does not provide broadcast's output's eltype in general, and is a bit finicky besides). So if we do mention Base.Broadcast._broadcast_eltype in the depwarn, retaining the underscore and providing simultaneous cautions might be wise. Thoughts? Best!

[TotalVerb]

I think a good compromise would be to point at #19669, i.e.

promote_eltype_op is deprecated and should not be used in the future.
See https://github.com/JuliaLang/julia/issues/19669 for more information.

[Sacha0]

I think a good compromise would be to point at #19669

Sounds good! :)

[TotalVerb]

@Sacha0, thank you for your comments and input. I have modified the PR as discussed.

[Sacha0]

LGTM. Could also deprecate broadcast_elwise_op more gently (in case there are other uses outside base), but perhaps not worth the effort? Best!

[TotalVerb]

A quick search turned up no uses of broadcast_elwise_op outside of Base.

[pabloferz]

This LGTM too.

broadcast_elwise_op was introduced during the evolution of broadcast in 0.6, and I doubt there was anyone following these changes and adapting code in packages as they appeared and changed along the way (except for possibly @nalimilan, @TotalVerb and @martinholters)

[TotalVerb]

Appveyor failure seems to be a libgit2 problem.

[martinholters]

I don't think ensuring a deprecated function is inferable is worth the effort; but as it has been spent anyway, why not.

[Sacha0]

(Added milestone so that merging these changes won't slip through the cracks. Best!)

[Sacha0]

I don't think ensuring a deprecated function is inferable is worth the effort; but as it has been spent anyway, why not.

Were your work dependent on that deprecated function, I wager your position would be different 😉. Best!

[kshyatt]

Like @martinholters said, AV fail seems unrelated. And we have a bunch of 👍 for this, so I am going to merge.