Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add promote_rule methods instead of promote_type #56779

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add promote_rule methods instead of promote_type #56779

wants to merge 6 commits into from

Conversation

jishnub
Copy link
Contributor

@jishnub jishnub commented Dec 8, 2024

In this PR, we move some promote_type methods to promote_rule, so that packages that wish to override this behavior only need to define the appropriate promote_rule method. E.g., currently, if one wishes to define promote_type(T, T) to return something other than T, they would need to specialize promote_type. With this PR, they may specialize promote_rule instead, which seems to be what the promotion documentation asks for.

This is inspired by the specific example in #54138.
cc @nsajko

Note that this PR changes

julia> struct A end

julia> promote_rule(A, A)
Union{}

to

julia> promote_rule(A, A)
A

Previously, this was being handled directly by promote_type.

Some of the methods added to promote_result also lead to an informative error in conflicting cases:

julia> struct A end

julia> struct B end

julia> Base.promote_rule(::Type{A}, ::Type{B}) = A

julia> Base.promote_rule(::Type{B}, ::Type{A}) = B

julia> promote_type(A, B)
ERROR: ArgumentError: promote_type(A, B) failed as conflicting `promote_rule`s were detected with either argument being a possible result.
Stacktrace:
 [1] _throw_promote_type_fail(A::Type, B::Type)
   @ Base ./promotion.jl:343
 [2] promote_result(::Type{A}, ::Type{B}, ::Type{A}, ::Type{B})
   @ Base ./promotion.jl:346
 [3] promote_type(::Type{A}, ::Type{B})
   @ Base ./promotion.jl:312
 [4] top-level scope
   @ REPL[5]:1

Currently, this leads to a stack-overflow, as promote_result calls promote_type internally, creating a cycle.

@jishnub jishnub added the types and dispatch Types, subtyping and method dispatch label Dec 8, 2024
@jishnub jishnub changed the title Add more promote_rule methods instead of promote_type Add promote_rule methods instead of promote_type Dec 9, 2024
@martinholters
Copy link
Member

if one wishes to define promote_type(T, T) to return something other than T

That sounds like a bad idea. Why would one want to do that? And shouldn't promote then also be changed to not just return its arguments if they're all of the same type? Otherwise, promote(x, y)::promote_type(typeof(x), typeof(y) might fail to hold. But would that also imply that operations should call promote even if the arguments are of the same type? Should promote then be called again on the result until a fixed point is reached?

@jishnub
Copy link
Contributor Author

jishnub commented Dec 10, 2024 •
edited
Loading

In most cases, this specific example is probably a bad idea, but in certain specific cases this might be practical. One case is static numbers, where something like static(1) and static(2) can't be represented by the same type, and it might be convenient if it was possible to promote such numbers to Int, even at the expense of losing the static nature. Static.jl has currently decided against this (SciML/Static.jl#98), but I can see arguments either way. A second case is that of Odd integers in https://github.com/jishnub/OddEvenIntegers.jl, which I had written mainly to access odd half-integer types such as 1/2 that were useful in basis conversions in ApproxFun.jl. For convenience, it makes sense for Odd(1) + Odd(1) to produce 2 instead of throwing an error. One may explicitly define every such operation instead of defining a promotion rule, but that seems excessive in this case (although Static.jl has to do this in most cases to preserve the type-domain value).

The point about promote(x, y)::promote_type(typeof(x), typeof(y) is a good one, and I hadn't considered this in the example above. I'm unsure if this is necessary in promote, though, and perhaps a convert may be added to ensure that this holds?

Notwithstanding the less-than-convincing example, the idea here is that packages should add methods to promote_rule and not to promote_type. I understand that the methods for promote_type were the ones that are usually not expected to be overridden, but as pointed out in the linked issue above, several packages have needed to add methods to promote_type for their purposes. It might make sense to redirect them to promote_rule instead.

vtjnash
vtjnash reviewed Dec 10, 2024
View reviewed changes
base/promotion.jl
Comment on lines +343 to +350
# avoid recursion if the types don't change under promote_rule, and throw an informative error instead
function _throw_promote_type_fail(A::Type, B::Type)
throw(ArgumentError(LazyString("`promote_type(", A, ", ", B, ")` failed as conflicting `promote_rule`s were ",
"detected with either argument being a possible result.")))
end
promote_result(::Type{T},::Type{S},::Type{T},::Type{S}) where {T,S} = _throw_promote_type_fail(T, S)
promote_result(::Type{T},::Type{S},::Type{S},::Type{T}) where {T,S} = _throw_promote_type_fail(T, S)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, this these lines are unrelated to the rest of the change, and improve the error message for promote_rule mistakes (explicit error instead of a stack overflow crash), so these can be merged separately from the other controversial changes?

@vtjnash
Copy link
Member

vtjnash commented Dec 10, 2024

A second case is that of Odd

That is a mistake to add those rules, as promote_type is never acceptable to override (we may make this an error someday). The correct place to add those rules for addition would be in + itself.
https://github.com/jishnub/OddEvenIntegers.jl/blob/c60602052f5c858b118d185bb6f4dc4c8ccd575a/src/OddEvenIntegers.jl#L78-L79

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtjnash vtjnash vtjnash left review comments

Assignees
No one assigned
Labels
types and dispatch Types, subtyping and method dispatch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jishnub @martinholters @vtjnash