Skip to content

Commit

Permalink
fix new type instability from getindex(::String, r::UnitRange{Int})
Browse files Browse the repository at this point in the history
After investigating the cause of the invalidation reported in
#55583, it was found that the issue arises only when `r`
is propagated as an extended lattice element, such as
`PartialStruct(UnitRange{Int}, ...)`, for the method of
`getindex(::String, r::UnitRange{Int})`. Specifically, the path at
https://github.com/JuliaLang/julia/blob/cebfd7bc66153b82c56715cb1cb52dac7df8eac8/base/compiler/typeinfer.jl#L809-L815
is hit, so the direct cause was the recursion limit for constant
inference.

To explain in more detail, within the slow path of `nextind` which is
called inside `getindex(::String, ::UnitRange{Int})`, the 1-argument
`@assert` is used https://github.com/JuliaLang/julia/blob/cebfd7bc66153b82c56715cb1cb52dac7df8eac8/base/strings/string.jl#L211.
The code related to `print` associated with this `@assert` further uses
`getindex(::String, ::UnitRange{Int})`, causing the recursion limit.
This recursion limit is only for constant inference, which is why we
saw this regression only for the `PartialStruct` case.
Moreover, since this recursion limit occurs within the
`@assert`-related code, this issue did not arise until now (i.e. until
#49260 was merged).

As a solution, I considered improving the recursion limit itself, but
decided that keeping the current code for the recursion limit of
constant inference is safer. Ideally, this should be addressed on the
compiler side, but there is certainly deep recursion in this case. As an
easier solution, this commit resolves the issue by changing the 1-arg
`@assert` to the 2-arg version.

- replaces #55583
  • Loading branch information
aviatesk committed Aug 31, 2024
1 parent e22e4de commit d87f645
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 3 deletions.
5 changes: 3 additions & 2 deletions base/compiler/typeinfer.jl
Original file line number Diff line number Diff line change
Expand Up @@ -864,7 +864,7 @@ function typeinf_edge(interp::AbstractInterpreter, method::Method, @nospecialize
end
end
if ccall(:jl_get_module_infer, Cint, (Any,), method.module) == 0 && !generating_output(#=incremental=#false)
add_remark!(interp, caller, "Inference is disabled for the target module")
add_remark!(interp, caller, "[typeinf_edge] Inference is disabled for the target module")
return EdgeCallResult(Any, Any, nothing, Effects())
end
if !is_cached(caller) && frame_parent(caller) === nothing
Expand Down Expand Up @@ -897,7 +897,7 @@ function typeinf_edge(interp::AbstractInterpreter, method::Method, @nospecialize
end
frame = InferenceState(result, cache_mode, interp) # always use the cache for edge targets
if frame === nothing
add_remark!(interp, caller, "Failed to retrieve source")
add_remark!(interp, caller, "[typeinf_edge] Failed to retrieve source")
# can't get the source for this, so we know nothing
if cache_mode == CACHE_MODE_GLOBAL
engine_reject(interp, ci)
Expand All @@ -918,6 +918,7 @@ function typeinf_edge(interp::AbstractInterpreter, method::Method, @nospecialize
return EdgeCallResult(frame.bestguess, exc_bestguess, edge, effects, volatile_inf_result)
elseif frame === true
# unresolvable cycle
add_remark!(interp, caller, "[typeinf_edge] Unresolvable cycle")
return EdgeCallResult(Any, Any, nothing, Effects())
end
# return the current knowledge about this cycle
Expand Down
2 changes: 1 addition & 1 deletion base/strings/string.jl
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ end
i = i′
@inbounds l = codeunit(s, i)
(l < 0x80) | (0xf8 l) && return i+1
@assert l >= 0xc0
@assert l >= 0xc0 "invalid codeunit"
end
# first continuation byte
(i += 1) > n && return i
Expand Down

0 comments on commit d87f645

Please sign in to comment.