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.

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

Informative error in promotion with conflicting promote_rules #57507

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

Conversation

jishnub
Copy link
Contributor

@jishnub jishnub commented Feb 23, 2025

This is the non-controversial change from #56779 that improves the error message in type promotion.

On master, promotion with conflicting promote_rule definitions leads to a stack overflow:

julia> struct PromoteB end

julia> struct PromoteA end

julia> Base.promote_rule(::Type{PromoteA}, ::Type{PromoteB}) = PromoteA

julia> Base.promote_rule(::Type{PromoteB}, ::Type{PromoteA}) = PromoteB

julia> promote_type(PromoteA, PromoteB)
Warning: detected a stack overflow; program state may be corrupted, so further execution might be unreliable.
ERROR: StackOverflowError:
Stacktrace:
      [1] promote_result(::Type, ::Type, ::Type{PromoteA}, ::Type{PromoteB})
        @ Base ./promotion.jl:337
      [2] promote_type
        @ ./promotion.jl:317 [inlined]--- the above 2 lines are repeated 79982 more times ---

In this PR, this is changed to an ArgumentError:

julia> promote_type(PromoteA, PromoteB)
ERROR: ArgumentError: promote_type(PromoteA, PromoteB) failed, as conflicting promote_rule definitions were detected with both PromoteA and PromoteB being possible results.

@nsajko
Copy link
Contributor

nsajko commented Feb 23, 2025

s/imformative/informative, for the title.

I think this fixes #13193? EDIT: it doesn't

@nsajko nsajko added the bugfix This change fixes an existing bug label Feb 23, 2025
@jishnub jishnub changed the title Imformative error in promotion with conflicting promote_rules Informative error in promotion with conflicting promote_rules Feb 24, 2025
Comment on lines +340 to +343
promote_result(::Type,::Type,::Type{T},::Type{T}) where {T} = T
# If only one promote_rule is defined, use the definition directly
promote_result(::Type,::Type,::Type{T},::Type{Bottom}) where {T} = T
promote_result(::Type,::Type,::Type{Bottom},::Type{T}) where {T} = T
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these methods are not necessary

Suggested change
promote_result(::Type,::Type,::Type{T},::Type{T}) where {T} = T
# If only one promote_rule is defined, use the definition directly
promote_result(::Type,::Type,::Type{T},::Type{Bottom}) where {T} = T
promote_result(::Type,::Type,::Type{Bottom},::Type{T}) where {T} = T

nsajko added a commit to nsajko/julia that referenced this pull request Feb 24, 2025
Don't use recursion, delete the `promote_result` function.

Use loop known to terminate, because of a hardcoded limit on the
iteration count.

Closes JuliaLang#57507, this PR encompasses the fixes from that PR and is more
comprehensive.

Fixes JuliaLang#13193
@nsajko

This comment was marked as outdated.

nsajko added a commit to nsajko/julia that referenced this pull request Feb 24, 2025
Don't use recursion, delete the `promote_result` function.

Use loop known to terminate, because of a hardcoded limit on the
iteration count.

Closes JuliaLang#57507, this PR encompasses the fixes from that PR and is more
comprehensive.

Fixes JuliaLang#13193
nsajko added a commit to nsajko/julia that referenced this pull request Feb 24, 2025
Don't use recursion, delete the `promote_result` function.

Use loop known to terminate, because of a hardcoded limit on the
iteration count.

Closes JuliaLang#57507, this PR encompasses the fixes from that PR and is more
comprehensive.

Fixes JuliaLang#13193
@nsajko nsajko added the error messages Better, more actionable error messages label Feb 25, 2025
@nsajko
Copy link
Contributor

nsajko commented Feb 25, 2025

more comprehensive PR: #57517

I reduced the scope of that PR, so now it doesn't strictly encompass this PR. However, if my PR gets merged, this PR will not be a bugfix PR any more, seeing as my PR will already prevent a stack overflow. It might still make sense to merge both for the better error messages.

nsajko added a commit to nsajko/julia that referenced this pull request Feb 26, 2025
Don't use recursion, delete the `promote_result` function.

Use loop known to terminate, because of a hardcoded limit on the
iteration count.

Closes JuliaLang#57507, this PR encompasses the fixes from that PR and is more
comprehensive.

Fixes JuliaLang#13193
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug error messages Better, more actionable error messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants