Merge pull request JuliaLang#46111 from JuliaLang/avi/correct-consistent

effects: fix correctness issues of `:consistent`-cy analysis

base/compiler/abstractinterpretation.jl

@@ -1887,18 +1887,21 @@ function abstract_eval_cfunction(interp::AbstractInterpreter, e::Expr, vtypes::V
 end
 
 function abstract_eval_value_expr(interp::AbstractInterpreter, e::Expr, vtypes::VarTable, sv::InferenceState)
- if e.head === :static_parameter
+ head = e.head
+ if head === :static_parameter
  n = e.args[1]::Int
  t = Any
  if 1 <= n <= length(sv.sptypes)
  t = sv.sptypes[n]
  end
  return t
- elseif e.head === :boundscheck
+ elseif head === :boundscheck
  return Bool
- else
+ elseif head === :the_exception
+ tristate_merge!(sv, Effects(EFFECTS_TOTAL; consistent=ALWAYS_FALSE))
  return Any
  end
+ return Any
 end
 
 function abstract_eval_special_value(interp::AbstractInterpreter, @nospecialize(e), vtypes::VarTable, sv::InferenceState)
@@ -1963,7 +1966,6 @@ function abstract_eval_statement(interp::AbstractInterpreter, @nospecialize(e),
  end
  elseif ehead === :new
  t, isexact = instanceof_tfunc(abstract_eval_value(interp, e.args[1], vtypes, sv))
- is_nothrow = true
  if isconcretedispatch(t)
  ismutable = ismutabletype(t)
  fcount = fieldcount(t)
@@ -1972,6 +1974,7 @@ function abstract_eval_statement(interp::AbstractInterpreter, @nospecialize(e),
  ats = Vector{Any}(undef, nargs)
  local anyrefine = false
  local allconst = true
+ local is_nothrow = true
  for i = 1:nargs
  at = widenconditional(abstract_eval_value(interp, e.args[i+1], vtypes, sv))
  ft = fieldtype(t, i)
@@ -1989,12 +1992,22 @@ function abstract_eval_statement(interp::AbstractInterpreter, @nospecialize(e),
  end
  ats[i] = at
  end
+ if fcount > nargs && any(i::Int -> !is_undefref_fieldtype(fieldtype(t, i)), (nargs+1):fcount)
+ # allocation with undefined field leads to undefined behavior and should taint `:consistent`-cy
+ consistent = ALWAYS_FALSE
+ elseif ismutable
+ # mutable object isn't `:consistent`, but we can still give the return
+ # type information a chance to refine this `:consistent`-cy later
+ consistent = TRISTATE_UNKNOWN
+ else
+ consistent = ALWAYS_TRUE
+ end
  # For now, don't allow:
  # - Const/PartialStruct of mutables (but still allow PartialStruct of mutables
  # with `const` fields if anything refined)
  # - partially initialized Const/PartialStruct
  if fcount == nargs
- if !ismutable && allconst
+ if consistent === ALWAYS_TRUE && allconst
  argvals = Vector{Any}(undef, nargs)
  for j in 1:nargs
  argvals[j] = (ats[j]::Const).val
@@ -2004,12 +2017,11 @@ function abstract_eval_statement(interp::AbstractInterpreter, @nospecialize(e),
  t = PartialStruct(t, ats)
  end
  end
+ nothrow = is_nothrow ? ALWAYS_TRUE : ALWAYS_FALSE
  else
- is_nothrow = false
+ consistent = nothrow = ALWAYS_FALSE
  end
- tristate_merge!(sv, Effects(EFFECTS_TOTAL;
- consistent = !ismutabletype(t) ? ALWAYS_TRUE : TRISTATE_UNKNOWN,
- nothrow = is_nothrow ? ALWAYS_TRUE : ALWAYS_FALSE))
+ tristate_merge!(sv, Effects(EFFECTS_TOTAL; consistent, nothrow))
  elseif ehead === :splatnew
  t, isexact = instanceof_tfunc(abstract_eval_value(interp, e.args[1], vtypes, sv))
  is_nothrow = false # TODO: More precision

base/compiler/tfuncs.jl

@@ -896,7 +896,7 @@ function _getfield_tfunc(@nospecialize(s00), @nospecialize(name), setfield::Bool
  end
  isa(s, DataType) || return Any
  isabstracttype(s) && return Any
- if s <: Tuple && !(Int <: widenconst(name))
+ if s <: Tuple && !hasintersect(widenconst(name), Int)
  return Bottom
  end
  if s <: Module
@@ -971,6 +971,57 @@ function _getfield_tfunc(@nospecialize(s00), @nospecialize(name), setfield::Bool
  return rewrap_unionall(R, s00)
 end
 
+function getfield_notundefined(@nospecialize(typ0), @nospecialize(name))
+ typ = unwrap_unionall(typ0)
+ if isa(typ, Union)
+ return getfield_notundefined(rewrap_unionall(typ.a, typ0), name) &&
+ getfield_notundefined(rewrap_unionall(typ.b, typ0), name)
+ end
+ isa(typ, DataType) || return false
+ if typ.name === Tuple.name || typ.name === _NAMEDTUPLE_NAME
+ # tuples and named tuples 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)), 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
+ ftyp = fieldtype(typ, fidx)
+ is_undefref_fieldtype(ftyp) && return true
+ return fidx ≤ datatype_min_ninitialized(typ)
+end
+# checks if a field of this type will not be initialized with undefined value
+# and the access to that uninitialized field will cause and `UndefRefError`, e.g.,
+# - is_undefref_fieldtype(String) === true
+# - is_undefref_fieldtype(Integer) === true
+# - is_undefref_fieldtype(Any) === true
+# - is_undefref_fieldtype(Int) === false
+# - is_undefref_fieldtype(Union{Int32,Int64}) === false
+function is_undefref_fieldtype(@nospecialize ftyp)
+ return !has_free_typevars(ftyp) && !allocatedinline(ftyp)
+end
+
 function setfield!_tfunc(o, f, v, order)
  @nospecialize
  if !isvarargtype(order)
@@ -1817,6 +1868,14 @@ function builtin_effects(f::Builtin, argtypes::Vector{Any}, @nospecialize(rt))
  end
  s = s::DataType
  consistent = !ismutabletype(s) ? ALWAYS_TRUE : ALWAYS_FALSE
+ # access to `isbitstype`-field initialized with undefined value leads to undefined behavior
+ # so should taint `:consistent`-cy while access to uninitialized non-`isbitstype` field
+ # throws `UndefRefError` so doesn't need to taint it
+ # NOTE `getfield_notundefined` conservatively checks if this field is never initialized
+ # with undefined value so that we don't taint `:consistent`-cy too aggressively here
+ if f === Core.getfield && !getfield_notundefined(s, argtypes[2])
+ consistent = ALWAYS_FALSE
+ end
  if f === Core.getfield && !isvarargtype(argtypes[end]) && getfield_boundscheck(argtypes) !== true
  # If we cannot independently prove inboundsness, taint consistency.
  # The inbounds-ness assertion requires dynamic reachability, while

test/compiler/effects.jl

@@ -60,6 +60,121 @@ end |> !Core.Compiler.is_consistent
  return nothing
 end |> Core.Compiler.is_foldable
 
+# :the_exception expression should taint :consistent-cy
+global inconsistent_var::Int = 42
+function throw_inconsistent() # this is still :consistent
+ throw(inconsistent_var)
+end
+function catch_inconsistent()
+ try
+ throw_inconsistent()
+ catch err
+ err
+ end
+end
+@test !Core.Compiler.is_consistent(Base.infer_effects(catch_inconsistent))
+cache_inconsistent() = catch_inconsistent()
+function compare_inconsistent()
+ a = cache_inconsistent()
+ global inconsistent_var = 0
+ b = cache_inconsistent()
+ global inconsistent_var = 42
+ return a === b
+end
+@test !compare_inconsistent()
+# return type information shouldn't be able to refine it also
+function catch_inconsistent(x::T) where T
+ v = x
+ try
+ throw_inconsistent()
+ catch err
+ v = err::T
+ end
+ return v
+end
+@test !Core.Compiler.is_consistent(Base.infer_effects(catch_inconsistent, (Int,)))
+cache_inconsistent(x) = catch_inconsistent(x)
+function compare_inconsistent(x::T) where T
+ x = one(T)
+ a = cache_inconsistent(x)
+ global inconsistent_var = 0
+ b = cache_inconsistent(x)
+ global inconsistent_var = 42
+ return a === b
+end
+@test !compare_inconsistent(3)
+
+# allocation/access of uninitialized fields should taint the :consistent-cy
+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
+
+import Core.Compiler: Const, getfield_notundefined
+for T = (Base.RefValue, Maybe) # both mutable and immutable
+ for name = (Const(1), Const(:x))
+ @test getfield_notundefined(T{String}, name)
+ @test getfield_notundefined(T{Integer}, name)
+ @test getfield_notundefined(T{Union{String,Integer}}, name)
+ @test getfield_notundefined(Union{T{String},T{Integer}}, name)
+ @test !getfield_notundefined(T{Int}, name)
+ @test !getfield_notundefined(T{<:Integer}, name)
+ @test !getfield_notundefined(T{Union{Int32,Int64}}, name)
+ @test !getfield_notundefined(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_notundefined(T{String}, name)
+ @test getfield_notundefined(T{Int}, name)
+ @test getfield_notundefined(T{Integer}, name)
+ @test getfield_notundefined(T{<:Integer}, name)
+ @test getfield_notundefined(T{Union{Int32,Int64}}, name)
+ @test getfield_notundefined(T, name)
+ end
+ # should not be too conservative when field isn't known very well but object information is accurate
+ @test getfield_notundefined(T{String}, Int)
+ @test getfield_notundefined(T{String}, Symbol)
+ @test getfield_notundefined(T{Integer}, Int)
+ @test getfield_notundefined(T{Integer}, Symbol)
+ @test !getfield_notundefined(T{Int}, Int)
+ @test !getfield_notundefined(T{Int}, Symbol)
+ @test !getfield_notundefined(T{<:Integer}, Int)
+ @test !getfield_notundefined(T{<:Integer}, Symbol)
+end
+# should be conservative when object information isn't accurate
+@test !getfield_notundefined(Any, Const(1))
+@test !getfield_notundefined(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_notundefined(TupleType, FieldType)
+end
+
+# TODO add equivalent test cases for `Ref` once we handle mutability more nicely
+@test Base.infer_effects() do
+ Maybe{Int}()
+end |> !Core.Compiler.is_consistent
+@test Base.infer_effects() do
+ Maybe{Int}()[]
+end |> !Core.Compiler.is_consistent
+@test !fully_eliminated() do
+ Maybe{Int}()[]
+end
+@test Base.infer_effects() do
+ Maybe{String}()
+end |> Core.Compiler.is_consistent
+@test Base.infer_effects() do
+ Maybe{String}()[]
+end |> Core.Compiler.is_consistent
+@test Base.return_types() do
+ Maybe{String}()[] # this expression should be concrete evaluated
+end |> only === Union{}
+
 # effects propagation for `Core.invoke` calls
 # https://github.com/JuliaLang/julia/issues/44763
 global x44763::Int = 0