limit recursion depth to prevent stack overflow in 2-arg promote_type

[nsajko]

Replace the recursive application of promote_rule by a harcoded (with metapogramming) sequence of steps, up to a limited depth.

Now that recursion is gone, promote_rule can constant fold more often.

The simplest cases of conflicting promote_rule definitions are detected even before hitting the depth limit, as in #57507. This is just to provide better UX, because of the more precise error message in that case and the shorter stack trace.

Fixes #13193

Closes #57507

[adienes]

what is the advantage over #57507 ? this seems to add quite a bit more complexity and includes scary things like hardcoded constants

[nsajko]

what is the advantage over #57507 ?

This PR prevents a much wider class of stack overflows. In particular, #57507 doesn't fix #13193 (see the test here for an example), while this PR should.

scary things like hardcoded constants

Usually I avoid hardcoded constants as best as I can, but in this case there's no way to guarantee (local) termination without hardcoding a cutoff. Another approach would be to stick with recursion, but limit the depth, however in any case it definitely seems necessary to have some kind of hardcoded limit.

promote_type shouldn't run without bound just because of faulty/conflicting promote_rules. Or, worse, overflow the stack. Or, even worse, cause the compiler itself to run without bound while it's trying to compile the promote_type method.

[nsajko]

This is how the behavior for #13193 looks with this PR, due to the limited recursion depth:

julia> struct SIQuantity{T<:Number} <: Number; end

julia> Base.promote_rule(::Type{SIQuantity{T}}, ::Type{SIQuantity{S}}) where {T, S} = SIQuantity{promote_type(T,S)}

julia> Base.promote_rule(::Type{SIQuantity{T}}, ::Type{S}) where {T, S<:Number} = SIQuantity{promote_type(T,S)}

julia> struct Interval{T<:Number} <: Number; end

julia> Base.promote_rule(::Type{Interval{T}}, ::Type{Interval{S}}) where {T, S} = Interval{promote_type(T,S)}

julia> Base.promote_rule(::Type{Interval{T}}, ::Type{S}) where {T, S<:Number} = Interval{promote_type(T,S)}

julia> promote_type(Interval{Int}, SIQuantity{Int})
ERROR: ArgumentError: `promote_type`: recursion depth limit reached, giving up; check for faulty/conflicting/missing `promote_rule` methods
Stacktrace:
  [1] _promote_type_binary(::Type, ::Type, ::Tuple{})
    @ Base ./promotion.jl:308
  [2] promote_result(::Type, ::Type, ::Type{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{Int64}}}}}}}}}}}}, ::Type{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Int64}}}}}}}}}}}}, l::Tuple{})
    @ Base ./promotion.jl:360
  [3] _promote_type_binary(::Type{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Int64}}}}}}}}}}}, ::Type{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{Int64}}}}}}}}}}}, recursion_depth_limit::Tuple{Nothing})
    @ Base ./promotion.jl:325
  [4] promote_result(::Type, ::Type, ::Type{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Int64}}}}}}}}}}}, ::Type{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{Int64}}}}}}}}}}}, l::Tuple{Nothing})
    @ Base ./promotion.jl:360
  [5] _promote_type_binary(::Type{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{Int64}}}}}}}}}}, ::Type{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Int64}}}}}}}}}}, recursion_depth_limit::Tuple{Nothing, Nothing})
    @ Base ./promotion.jl:325
  [6] promote_result(::Type, ::Type, ::Type{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{Int64}}}}}}}}}}, ::Type{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Int64}}}}}}}}}}, l::Tuple{Nothing, Nothing})
    @ Base ./promotion.jl:360
  [7] _promote_type_binary
    @ ./promotion.jl:325 [inlined]
  [8] promote_result(::Type, ::Type, ::Type{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Int64}}}}}}}}}, ::Type{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{Int64}}}}}}}}}, l::Tuple{Nothing, Nothing, Nothing})
    @ Base ./promotion.jl:360
  [9] _promote_type_binary
    @ ./promotion.jl:325 [inlined]
 [10] promote_result(::Type, ::Type, ::Type{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{Int64}}}}}}}}, ::Type{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Int64}}}}}}}}, l::NTuple{4, Nothing})
    @ Base ./promotion.jl:360
 [11] _promote_type_binary
    @ ./promotion.jl:325 [inlined]
 [12] promote_result(::Type, ::Type, ::Type{Interval{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Int64}}}}}}}, ::Type{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Interval{Int64}}}}}}}, l::NTuple{5, Nothing})
    @ Base ./promotion.jl:360
 [13] _promote_type_binary
    @ ./promotion.jl:325 [inlined]
 [14] promote_result(::Type, ::Type, ::Type{Interval{SIQuantity{Interval{SIQuantity{Interval{Int64}}}}}}, ::Type{SIQuantity{Interval{SIQuantity{Interval{SIQuantity{Int64}}}}}}, l::NTuple{6, Nothing})
    @ Base ./promotion.jl:360
 [15] _promote_type_binary
    @ ./promotion.jl:325 [inlined]
 [16] promote_result(::Type, ::Type, ::Type{Interval{SIQuantity{Interval{SIQuantity{Int64}}}}}, ::Type{SIQuantity{Interval{SIQuantity{Interval{Int64}}}}}, l::NTuple{7, Nothing})
    @ Base ./promotion.jl:360
 [17] _promote_type_binary
    @ ./promotion.jl:325 [inlined]
 [18] promote_result(::Type, ::Type, ::Type{Interval{SIQuantity{Interval{Int64}}}}, ::Type{SIQuantity{Interval{SIQuantity{Int64}}}}, l::NTuple{8, Nothing})
    @ Base ./promotion.jl:360
 [19] _promote_type_binary
    @ ./promotion.jl:325 [inlined]
 [20] promote_result(::Type, ::Type, ::Type{Interval{SIQuantity{Int64}}}, ::Type{SIQuantity{Interval{Int64}}}, l::NTuple{9, Nothing})
    @ Base ./promotion.jl:360
 [21] _promote_type_binary
    @ ./promotion.jl:325 [inlined]
 [22] promote_type(::Type{Interval{Int64}}, ::Type{SIQuantity{Int64}})
    @ Base ./promotion.jl:340
 [23] top-level scope
    @ REPL[7]:1

[adienes]

not sure if I'm 100% qualified to review beyond stylistic things but as far as I can tell I like this approach

it might make sense to run the benchmarks? I know this shouldn't change anything but promote_type is quite a highly used function so just in case

[nsajko]

Question for triage: does limiting the recursion depth seem like an acceptable minor change? It's a bugfix, as it prevents stack overflows, but hypothetically some user code could break if it relied on an awkward chain of promote_rules.

NB: an earlier version of this PR tried to do away with recursion altogether in favor of a loop, however that resulted in inference regressions.

NB: setting the recursion depth limit higher breaks bootstrap for some reason, so currently it's not possible to set the limit higher.

[nsajko]

@nanosoldier runtests()

[nanosoldier]

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

Report summary

❗ Packages that crashed

1 packages crashed only on the current version.

• An internal error was encountered: 1 packages

12 packages crashed on the previous version too.

✖ Packages that failed

38 packages failed only on the current version.

• Package fails to precompile: 1 packages
• Package has test failures: 2 packages
• Package tests unexpectedly errored: 3 packages
• Tests became inactive: 2 packages
• Test duration exceeded the time limit: 27 packages
• Test log exceeded the size limit: 3 packages

1054 packages failed on the previous version too.

✔ Packages that passed tests

23 packages passed tests only on the current version.

• Other: 23 packages

5334 packages passed tests on the previous version too.

~ Packages that at least loaded

9 packages successfully loaded only on the current version.

• Other: 9 packages

2968 packages successfully loaded on the previous version too.

➖ Packages that were skipped altogether

2 packages were skipped only on the current version.

• Package could not be installed: 2 packages

908 packages were skipped on the previous version too.

[nsajko]

The pkgeval seems OK, it doesn't seem like any packages hit the recursion depth limit. That said, it might be good to repeat the pkgeval once the bpart-related changes are more polished.

[LilithHafner]

PolynomialRings failure looks real (link that will go stale)

[nsajko]

PolynomialRings is buggy, doesn't have much to do with this PR specifically: tkluck/PolynomialRings.jl#10

EDIT: the current version of this PR doesn't break PolynomialRings.jl any more, even though it's that package's own fault for adding methods to promote_type.

EDIT: the current version of this PR will again break PolynomialRings.jl, unless it gets broken for an unrelated reason by #58720 first. But, again, that's the fault of PolynomialRings.jl.

[nsajko]

bump

[nsajko]

Fixed conflict in NEWS.md

[vtjnash]

I think you need to justify this PR more, since right now it reads as breaking and unpredictable with arbitrary thresholds that are based on seeing the initial PR wasn’t functional:

The above PkgEval run was with the recursion depth limit set to four. Now I raised the limit to eight to make it more certain that the limit wouldn't ever be reached in practice by valid code.

[nsajko]

need to justify this PR more

The motivation already seems exceedingly strong to me.

• This PR fixes serious bugs like stack overflows and hangs. On current master, running the first test set added in this PR results in a hang, and then a crash of the REPL when interrupting from the terminal. The second test set results in a stack overflow on the master branch.

• The current version of the PR also improves the effects.

right now it reads as breaking and unpredictable with arbitrary thresholds that are based on seeing the initial PR wasn’t functional

No, this is not at all what happened, you're misinterpreting my message that you quoted. What I was saying is that the PkgEval with the depth limit set to four was a success, with no packages hitting the depth limit. Thus, to be even more sure the PR is non-breaking, I further increased the depth limit after that point.

[nsajko]

bump

[adienes]

what would be the plan if someone comes along and complains that their promotion didn't converge? I would probably be of the opinion that if there's going to be a finite limit (before system-wide limits like stack overflows) it should be documented

[vtjnash]

I don't see any value proposition here in expanding this function call into a massive amount of code (using macros), just to break any existing code that relies on this. I'm sorry, I just don't get it