Skip to content

Commit

Permalink
inference: don't taint :consistent on use of undefined isbitstype…
Browse files Browse the repository at this point in the history 
…-fields (#54136)

After #52169, there is no need to taint `:consistent`-cy on accessing
undefined `isbitstype` field since the value of the fields is freezed
and thus never transmute across multiple uses.
  • Loading branch information
@aviatesk
aviatesk authored Apr 30, 2024
1 parent 578a8f1 commit 69036e1
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 109 deletions.
58 changes: 0 additions & 58 deletions base/compiler/tfuncs.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1240,54 +1240,6 @@ end
return rewrap_unionall(R, s00)
end

@nospecs function getfield_notuninit(typ0, name)
if isa(typ0, Const)
# If the object is Const, then we know exactly the bit patterns that
# must be returned by getfield if not an error
return true
end
typ0 = widenconst(typ0)
typ = unwrap_unionall(typ0)
if isa(typ, Union)
return getfield_notuninit(rewrap_unionall(typ.a, typ0), name) &&
getfield_notuninit(rewrap_unionall(typ.b, typ0), name)
end
isa(typ, DataType) || return false
isabstracttype(typ) && !isconstType(typ) && return false # cannot say anything about abstract types
if typ.name.n_uninitialized == 0
# Types such as tuples and named tuples that can't be instantiated with undefined fields,
# so we don't need to be conservative here
return true
end
if !isa(name, Const)
isvarargtype(name) && return false
if !hasintersect(widenconst(name), Union{Int,Symbol})
return true # no undefined behavior if thrown
end
# field isn't known precisely, but let's check if all the fields can't be
# initialized with undefined value so to avoid being too conservative
fcnt = fieldcount_noerror(typ)
fcnt === nothing && return false
all(i::Int->is_undefref_fieldtype(fieldtype(typ,i)), (datatype_min_ninitialized(typ)+1):fcnt) && return true
return false
end
name = name.val
if isa(name, Symbol)
fidx = fieldindex(typ, name, false)
fidx === nothing && return true # no undefined behavior if thrown
elseif isa(name, Int)
fidx = name
else
return true # no undefined behavior if thrown
end
fcnt = fieldcount_noerror(typ)
fcnt === nothing && return false
0 < fidx ≤ fcnt || return true # no undefined behavior if thrown
fidx ≤ datatype_min_ninitialized(typ) && return true # always defined
ftyp = fieldtype(typ, fidx)
is_undefref_fieldtype(ftyp) && return true # always initialized
return false
end
# checks if a field of this type is guaranteed to be defined to a value
# and that access to an uninitialized field will cause an `UndefRefError` or return zero
# - is_undefref_fieldtype(String) === true
Expand Down Expand Up @@ -2462,16 +2414,6 @@ function getfield_effects(𝕃::AbstractLattice, argtypes::Vector{Any}, @nospeci
# :consistent if the argtype is immutable
consistent = (is_immutable_argtype(obj) || is_mutation_free_argtype(obj)) ?
ALWAYS_TRUE : CONSISTENT_IF_INACCESSIBLEMEMONLY
# taint `:consistent` if accessing `isbitstype`-type object field that may be initialized
# with undefined value: note that we don't need to taint `:consistent` if accessing
# uninitialized non-`isbitstype` field since it will simply throw `UndefRefError`
# NOTE `getfield_notuninit` conservatively checks if this field is never initialized
# with undefined value to avoid tainting `:consistent` too aggressively
# TODO this should probably taint `:noub`, however, it would hinder concrete eval for
# `REPLInterpreter` that can ignore `:consistent-cy`, causing worse completions
if !(length(argtypes) ≥ 2 && getfield_notuninit(obj, argtypes[2]))
consistent = ALWAYS_FALSE
end
noub = ALWAYS_TRUE
bcheck = getfield_boundscheck(argtypes)
nothrow = getfield_nothrow(𝕃, argtypes, bcheck)
Expand Down
53 changes: 2 additions & 51 deletions test/compiler/effects.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -236,65 +236,17 @@ end
# Effect modeling for Core.compilerbarrier
@test Base.infer_effects(Base.inferencebarrier, Tuple{Any}) |> Core.Compiler.is_removable_if_unused

# allocation/access of uninitialized fields should taint the :consistent-cy
# effects modeling for allocation/access of uninitialized fields
struct Maybe{T}
x::T
Maybe{T}() where T = new{T}()
Maybe{T}(x) where T = new{T}(x)
Maybe(x::T) where T = new{T}(x)
end
Base.getindex(x::Maybe) = x.x

struct SyntacticallyDefined{T}
x::T
end

import Core.Compiler: Const, getfield_notuninit
for T = (Base.RefValue, Maybe) # both mutable and immutable
for name = (Const(1), Const(:x))
@test getfield_notuninit(T{String}, name)
@test getfield_notuninit(T{Integer}, name)
@test getfield_notuninit(T{Union{String,Integer}}, name)
@test getfield_notuninit(Union{T{String},T{Integer}}, name)
@test !getfield_notuninit(T{Int}, name)
@test !getfield_notuninit(T{<:Integer}, name)
@test !getfield_notuninit(T{Union{Int32,Int64}}, name)
@test !getfield_notuninit(T, name)
end
# throw doesn't account for undefined behavior
for name = (Const(0), Const(2), Const(1.0), Const(:y), Const("x"),
Float64, String, Nothing)
@test getfield_notuninit(T{String}, name)
@test getfield_notuninit(T{Int}, name)
@test getfield_notuninit(T{Integer}, name)
@test getfield_notuninit(T{<:Integer}, name)
@test getfield_notuninit(T{Union{Int32,Int64}}, name)
@test getfield_notuninit(T, name)
end
# should not be too conservative when field isn't known very well but object information is accurate
@test getfield_notuninit(T{String}, Int)
@test getfield_notuninit(T{String}, Symbol)
@test getfield_notuninit(T{Integer}, Int)
@test getfield_notuninit(T{Integer}, Symbol)
@test !getfield_notuninit(T{Int}, Int)
@test !getfield_notuninit(T{Int}, Symbol)
@test !getfield_notuninit(T{<:Integer}, Int)
@test !getfield_notuninit(T{<:Integer}, Symbol)
end
# should be conservative when object information isn't accurate
@test !getfield_notuninit(Any, Const(1))
@test !getfield_notuninit(Any, Const(:x))
# tuples and namedtuples should be okay if not given accurate information
for TupleType = Any[Tuple{Int,Int,Int}, Tuple{Int,Vararg{Int}}, Tuple{Any}, Tuple,
NamedTuple{(:a, :b), Tuple{Int,Int}}, NamedTuple{(:x,),Tuple{Any}}, NamedTuple],
FieldType = Any[Int, Symbol, Any]
@test getfield_notuninit(TupleType, FieldType)
end
# skip analysis on fields that are known to be defined syntactically
@test Core.Compiler.getfield_notuninit(SyntacticallyDefined{Float64}, Symbol)
@test Core.Compiler.getfield_notuninit(Const(Main), Const(:var))
@test Core.Compiler.getfield_notuninit(Const(Main), Const(42))
# high-level tests for `getfield_notuninit`
@test Base.infer_effects() do
Maybe{Int}()
end |> !Core.Compiler.is_consistent
Expand Down Expand Up @@ -904,7 +856,6 @@ end |> Core.Compiler.is_foldable_nothrow
@test Base.infer_effects(Tuple{WrapperOneField{Float64}, Symbol}) do w, s
getfield(w, s)
end |> Core.Compiler.is_foldable
@test Core.Compiler.getfield_notuninit(WrapperOneField{Float64}, Symbol)
@test Base.infer_effects(Tuple{WrapperOneField{Symbol}, Symbol}) do w, s
getfield(w, s)
end |> Core.Compiler.is_foldable
Expand Down Expand Up @@ -1103,7 +1054,7 @@ function f3_optrefine(x)
@fastmath sqrt(x)
return x
end
@test !Core.Compiler.is_consistent(Base.infer_effects(f2_optrefine; optimize=false))
@test !Core.Compiler.is_consistent(Base.infer_effects(f3_optrefine; optimize=false))
@test Core.Compiler.is_consistent(Base.infer_effects(f3_optrefine, (Float64,)))

# Check that :consistent is properly modeled for throwing statements
Expand Down

2 comments on commit 69036e1

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Executing the daily package evaluation, I will reply here when finished:

@nanosoldier runtests(isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Please sign in to comment.