Skip to content

Commit

Permalink
effects: Do not over-taint :terminates in abstract cycle
Browse files Browse the repository at this point in the history
We already infer non-termination as part of the conservative effects
we assume at the point in the call-graph that recursion is detected.

As a result, it should be sound to allow this to propagate through
the Effects system naturally rather than eagerly marking our callers
as non-terminating.

Doing this is important to avoid tainting non-termination from purely
abstract cycles, where inference is forced to analyze methods that do
not correspond to real calls at runtime, such as `Base._return_type`.

Resolves #48983.
  • Loading branch information
topolarity committed Mar 22, 2023
1 parent 6d678fe commit fb38ae9
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 9 deletions.
8 changes: 0 additions & 8 deletions base/compiler/abstractinterpretation.jl
Original file line number Diff line number Diff line change
Expand Up @@ -667,14 +667,6 @@ function abstract_call_method(interp::AbstractInterpreter, method::Method, @nosp
elseif is_effect_overridden(method, :terminates_globally)
# this edge is known to terminate
effects = Effects(effects; terminates=true)
elseif edgecycle
# Some sort of recursion was detected.
if edge !== nothing && !edgelimited && !is_edge_recursed(edge, sv)
# no `MethodInstance` cycles -- don't taint :terminate
else
# we cannot guarantee that the call will terminate
effects = Effects(effects; terminates=false)
end
end

return MethodCallResult(rt, edgecycle, edgelimited, edge, effects)
Expand Down
1 change: 0 additions & 1 deletion base/compiler/typeinfer.jl
Original file line number Diff line number Diff line change
Expand Up @@ -785,7 +785,6 @@ function merge_call_chain!(interp::AbstractInterpreter, parent::InferenceState,
while true
add_cycle_backedge!(parent, child, parent.currpc)
union_caller_cycle!(ancestor, child)
merge_effects!(interp, child, Effects(EFFECTS_TOTAL; terminates=false))
child = parent
child === ancestor && break
parent = child.parent::InferenceState
Expand Down
11 changes: 11 additions & 0 deletions test/compiler/effects.jl
Original file line number Diff line number Diff line change
Expand Up @@ -805,5 +805,16 @@ unknown_sparam_nothrow2(x::Ref{Ref{T}}) where T = (T; nothing)
@test Core.Compiler.is_nothrow(Base.infer_effects(unknown_sparam_nothrow1, (Ref,)))
@test Core.Compiler.is_nothrow(Base.infer_effects(unknown_sparam_nothrow2, (Ref{Ref{T}} where T,)))

# purely abstract recursion should not taint :terminates
# https://github.com/JuliaLang/julia/issues/48983
abstractly_recursive1() = abstractly_recursive2()
abstractly_recursive2() = (Core.Compiler._return_type(abstractly_recursive1, Tuple{}); 1)
abstractly_recursive3() = abstractly_recursive2()
@test Core.Compiler.is_terminates(Base.infer_effects(abstractly_recursive3, ()))
actually_recursive1(x) = actually_recursive2(x)
actually_recursive2(x) = (x <= 0) ? 1 : actually_recursive1(x - 1)
actually_recursive3(x) = actually_recursive2(x)
@test !Core.Compiler.is_terminates(Base.infer_effects(actually_recursive3, (Int,)))

# https://github.com/JuliaLang/julia/issues/48856
@test Base.ismutationfree(Vector{Any}) == false

0 comments on commit fb38ae9

Please sign in to comment.