Skip to content

Commit

Permalink
[REPLCompletions] fix #52099 by adjusting effects of HAMT methods (#…
Browse files Browse the repository at this point in the history
…52331)

After looking into #52099, I discovered that it has the same root cause
as #51548. Essentially, when a method that's not `!effect_free` is
applied to a `Const` value concretized by the `REPLInterpreter`'s
aggressive inference, since the `!effect_free` method will not be
concretized, it will eventually lead to the `Const` representing
unexpected object being returned.

This commit tries to fix the specific problem reported in #52099 by
enhancing the effects of `Base.HAMT` methods, so they're
concrete-evaled.

Admittedly, this is more of a quick fix than a complete solution, and
not the best one, but it was the simplest. A better solution might
involve implementing EA's handling of `Memory`-objects that allows the
compiler to automatically prove `:effect_free` in more scenarios. But
this would need a bigger overhaul, so I plan to tackle it in another PR.
  • Loading branch information
aviatesk authored Dec 1, 2023
1 parent bac3ba5 commit 58c1d51
Show file tree
Hide file tree
Showing 4 changed files with 18 additions and 7 deletions.
4 changes: 2 additions & 2 deletions base/compiler/optimize.jl
Original file line number Diff line number Diff line change
Expand Up @@ -698,7 +698,7 @@ function scan_non_dataflow_flags!(inst::Instruction, sv::PostOptAnalysisState)
flag = inst[:flag]
# If we can prove that the argmem does not escape the current function, we can
# refine this to :effect_free.
needs_ea_validation = (flag & IR_FLAGS_NEEDS_EA) == IR_FLAGS_NEEDS_EA
needs_ea_validation = has_flag(flag, IR_FLAGS_NEEDS_EA)
stmt = inst[:stmt]
if !needs_ea_validation
if !isterminator(stmt) && stmt !== nothing
Expand Down Expand Up @@ -730,7 +730,7 @@ end

function scan_inconsistency!(inst::Instruction, sv::PostOptAnalysisState)
flag = inst[:flag]
stmt_inconsistent = iszero(flag & IR_FLAG_CONSISTENT)
stmt_inconsistent = !has_flag(flag, IR_FLAG_CONSISTENT)
stmt = inst[:stmt]
# Special case: For `getfield` and memory operations, we allow inconsistency of the :boundscheck argument
(; inconsistent, tpdum) = sv
Expand Down
10 changes: 5 additions & 5 deletions base/hamt.jl
Original file line number Diff line number Diff line change
Expand Up @@ -66,21 +66,21 @@ mutable struct HAMT{K, V}
HAMT{K, V}() where {K, V} = new{K,V}(Vector{Union{Leaf{K, V}, HAMT{K, V}}}(undef, 0), zero(BITMAP))
end

@Base.assume_effects :nothrow function init_hamt(K, V, k, v)
# For a single element we can't have a hash-collision
Base.@assume_effects :nothrow :effect_free function init_hamt(K, V, k, v)
# For a single element we can't have a 'hash-collision
trie = HAMT{K,V}(Vector{Union{Leaf{K, V}, HAMT{K, V}}}(undef, 1), zero(BITMAP))
trie.data[1] = Leaf{K,V}(k,v)
return trie
end

function HAMT{K,V}((k,v)::Pair) where {K, V}
k = convert(K, k)
v = convert(V, v)
Base.@assume_effects :effect_free function HAMT{K,V}((k,v)::Pair{K,V}) where {K, V}
trie = init_hamt(K, V, k, v)
bi = BitmapIndex(HashState(k))
set!(trie, bi)
return trie
end
HAMT{K,V}(kv::Pair) where {K, V} = HAMT{K,V}(convert(Pair{K,V}, kv))

HAMT(pair::Pair{K,V}) where {K, V} = HAMT{K,V}(pair)

# TODO: Parameterize by hash function
Expand Down
8 changes: 8 additions & 0 deletions stdlib/REPL/test/replcompletions.jl
Original file line number Diff line number Diff line change
Expand Up @@ -2148,3 +2148,11 @@ end
let t = REPLCompletions.repl_eval_ex(:(unsafe_method(42)), @__MODULE__)
@test isnothing(t)
end

# https://github.com/JuliaLang/julia/issues/52099
const issue52099 = []
let t = REPLCompletions.repl_eval_ex(:(Base.PersistentDict(issue52099 => 3)), @__MODULE__)
if t isa Core.Const
@test length(t.val) == 1
end
end
3 changes: 3 additions & 0 deletions test/dict.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1107,6 +1107,9 @@ import Base.PersistentDict
@test hs.hash == hsr.hash
@test hs.depth == hsr.depth
@test hs.shift == hsr.shift

@test Core.Compiler.is_removable_if_unused(Base.infer_effects(Base.HAMT.init_hamt, (Type{Vector{Any}},Type{Int},Vector{Any},Int)))
@test Core.Compiler.is_removable_if_unused(Base.infer_effects(Base.HAMT.HAMT{Vector{Any},Int}, (Pair{Vector{Any},Int},)))
end
@testset "basics" begin
dict = PersistentDict{Int, Int}()
Expand Down

0 comments on commit 58c1d51

Please sign in to comment.