Skip to content

Avoid broken kind type handling when compiling isa #27736

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Jul 7, 2018
25 changes: 13 additions & 12 deletions base/compiler/abstractinterpretation.jl
Original file line number Diff line number Diff line change
Expand Up @@ -638,19 +638,20 @@ function abstract_call(@nospecialize(f), fargs::Union{Tuple{},Vector{Any}}, argt
if !isa(body, Type) && !isa(body, TypeVar)
return Any
end
has_free_typevars(body) || return body
if isa(argtypes[2], Const)
tv = argtypes[2].val
elseif isa(argtypes[2], PartialTypeVar)
ptv = argtypes[2]
tv = ptv.tv
canconst = false
else
return Any
if has_free_typevars(body)
if isa(argtypes[2], Const)
tv = argtypes[2].val
elseif isa(argtypes[2], PartialTypeVar)
ptv = argtypes[2]
tv = ptv.tv
canconst = false
else
return Any
end
!isa(tv, TypeVar) && return Any
body = UnionAll(tv, body)
end
!isa(tv, TypeVar) && return Any
theunion = UnionAll(tv, body)
ret = canconst ? AbstractEvalConstant(theunion) : Type{theunion}
ret = canconst ? AbstractEvalConstant(body) : Type{body}
return ret
end
return Any
Expand Down
33 changes: 22 additions & 11 deletions base/compiler/tfuncs.jl
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,15 @@ add_tfunc(throw, 1, 1, (@nospecialize(x)) -> Bottom, 0)
# returns (type, isexact)
# if isexact is false, the actual runtime type may (will) be a subtype of t
function instanceof_tfunc(@nospecialize(t))
if t === Bottom || t === typeof(Bottom)
return Bottom, true
elseif isa(t, Const)
if isa(t, Const)
if isa(t.val, Type)
return t.val, true
end
return Bottom, true
end
t = widenconst(t)
if t === Bottom || t === typeof(Bottom) || typeintersect(t, Type) === Bottom
return Bottom, true
elseif isType(t)
tp = t.parameters[1]
return tp, !has_free_typevars(tp)
Expand Down Expand Up @@ -385,21 +388,29 @@ add_tfunc(typeassert, 2, 2,
return typeintersect(v, t)
end, 4)
add_tfunc(isa, 2, 2,
function (@nospecialize(v), @nospecialize(t))
t, isexact = instanceof_tfunc(t)
function (@nospecialize(v), @nospecialize(tt))
t, isexact = instanceof_tfunc(tt)
if t === Bottom
# check if t could be equivalent to typeof(Bottom), since that's valid in `isa`, but the set of `v` is empty
# if `t` cannot have instances, it's also invalid on the RHS of isa
if typeintersect(widenconst(tt), Type) === Union{}
return Union{}
end
return Const(false)
end
if !has_free_typevars(t)
if t === Bottom
return Const(false)
elseif v ⊑ t
if isexact
if v ⊑ t
if isexact && isnotbrokensubtype(v, t)
return Const(true)
end
elseif isa(v, Const) || isa(v, Conditional) || isdispatchelem(v)
# this tests for knowledge of a leaftype appearing on the LHS
# (ensuring the isa is precise)
return Const(false)
elseif isexact && typeintersect(v, t) === Bottom
if !iskindtype(v) #= subtyping currently intentionally answers this query incorrectly for kinds =#
elseif typeintersect(v, t) === Bottom
# similar to `isnotbrokensubtype` check above, `typeintersect(v, t)`
# can't be trusted for kind types so we do an extra check here
if !iskindtype(v)
return Const(false)
end
end
Expand Down
8 changes: 7 additions & 1 deletion base/compiler/typeutils.jl
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@ function issingletontype(@nospecialize t)
return false
end

# Subtyping currently intentionally answers certain queries incorrectly for kind types. For
# some of these queries, this check can be used to somewhat protect against making incorrect
# decisions based on incorrect subtyping. Note that this check, itself, is broken for
# certain combinations of `a` and `b` where one/both isa/are `Union`/`UnionAll` type(s)s.
isnotbrokensubtype(a, b) = (!iskindtype(b) || !isType(a) || issingletontype(a.parameters[1]))

argtypes_to_type(argtypes::Array{Any,1}) = Tuple{anymap(widenconst, argtypes)...}

function isknownlength(t::DataType)
Expand All @@ -52,7 +58,7 @@ end
# return an upper-bound on type `a` with type `b` removed
# such that `return <: a` && `Union{return, b} == Union{a, b}`
function typesubtract(@nospecialize(a), @nospecialize(b))
if a <: b
if a <: b && isnotbrokensubtype(a, b)
return Bottom
end
if isa(a, Union)
Expand Down
9 changes: 7 additions & 2 deletions src/cgutils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1060,13 +1060,18 @@ static void emit_type_error(jl_codectx_t &ctx, const jl_cgval_t &x, Value *type,

static std::pair<Value*, bool> emit_isa(jl_codectx_t &ctx, const jl_cgval_t &x, jl_value_t *type, const std::string *msg)
{
// TODO: The subtype check below suffers from incorrectness issues due to broken
// subtyping for kind types (see https://github.com/JuliaLang/julia/issues/27078). For
// actual `isa` calls, this optimization should already have been performed upstream
// anyway, but having this optimization in codegen might still be beneficial for
// `typeassert`s if we can make it correct.
Optional<bool> known_isa;
jl_value_t *intersected_type = type;
if (x.constant)
known_isa = jl_isa(x.constant, type);
else if (jl_subtype(x.typ, type))
else if (jl_is_not_broken_subtype(x.typ, type) && jl_subtype(x.typ, type)) {
known_isa = true;
else {
} else {
intersected_type = jl_type_intersection(x.typ, type);
if (intersected_type == (jl_value_t*)jl_bottom_type)
known_isa = false;
Expand Down
1 change: 1 addition & 0 deletions src/julia.h
Original file line number Diff line number Diff line change
Expand Up @@ -1067,6 +1067,7 @@ JL_DLLEXPORT int jl_subtype_env_size(jl_value_t *t);
JL_DLLEXPORT int jl_subtype_env(jl_value_t *x, jl_value_t *y, jl_value_t **env, int envsz);
JL_DLLEXPORT int jl_isa(jl_value_t *a, jl_value_t *t);
JL_DLLEXPORT int jl_types_equal(jl_value_t *a, jl_value_t *b) JL_NOTSAFEPOINT;
JL_DLLEXPORT int jl_is_not_broken_subtype(jl_value_t *a, jl_value_t *b);
JL_DLLEXPORT jl_value_t *jl_type_union(jl_value_t **ts, size_t n);
JL_DLLEXPORT jl_value_t *jl_type_intersection(jl_value_t *a, jl_value_t *b);
JL_DLLEXPORT int jl_has_empty_intersection(jl_value_t *x, jl_value_t *y);
Expand Down
7 changes: 7 additions & 0 deletions src/subtype.c
Original file line number Diff line number Diff line change
Expand Up @@ -1165,6 +1165,13 @@ JL_DLLEXPORT int jl_types_equal(jl_value_t *a, jl_value_t *b)
return jl_subtype(a, b) && jl_subtype(b, a);
}

JL_DLLEXPORT int jl_is_not_broken_subtype(jl_value_t *a, jl_value_t *b)
{
// TODO: the final commented out check here isn't correct; it should be closer to the
// `issingletype` check used by `isnotbrokensubtype` in `base/compiler/typeutils.jl`
return !jl_is_kind(b) || !jl_is_type_type(a); // || jl_is_datatype_singleton((jl_datatype_t*)jl_tparam0(a));
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added this check before the problematic subtyping check in emit_isa and it allows the tests in the previously provided gist to pass, including the final heisenbuggy one. It's emulating the isbrokensubtype predicate we're using in the isa tfunc for this PR. As you can see by the TODO, this predicate is probably overly strict.

BTW, thanks to @maleadt, @DokFaust, @pablosanjose for looking into this. Much appreciated!

Would also be nice to get some review from core codegen contributors. If tests pass here, then I'm going to merge right away unless there are any objections.

int jl_tuple_isa(jl_value_t **child, size_t cl, jl_datatype_t *pdt)
{
if (jl_is_tuple_type(pdt) && !jl_is_va_tuple(pdt)) {
Expand Down
13 changes: 9 additions & 4 deletions test/compiler/compiler.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1179,7 +1179,7 @@ let isa_tfunc = Core.Compiler.T_FFUNC_VAL[
@test isa_tfunc(Array{Real}, Type{AbstractArray{Int}}) === Const(false)
@test isa_tfunc(Array{Real, 2}, Const(AbstractArray{Real, 2})) === Const(true)
@test isa_tfunc(Array{Real, 2}, Const(AbstractArray{Int, 2})) === Const(false)
@test isa_tfunc(DataType, Int) === Bool # could be improved
@test isa_tfunc(DataType, Int) === Union{}
@test isa_tfunc(DataType, Const(Type{Int})) === Bool
@test isa_tfunc(DataType, Const(Type{Array})) === Bool
@test isa_tfunc(UnionAll, Const(Type{Int})) === Bool # could be improved
Expand All @@ -1189,7 +1189,7 @@ let isa_tfunc = Core.Compiler.T_FFUNC_VAL[
@test isa_tfunc(typeof(Union{}), Const(Int)) === Const(false) # any result is ok
@test isa_tfunc(typeof(Union{}), Const(Union{})) === Const(false)
@test isa_tfunc(typeof(Union{}), typeof(Union{})) === Const(false)
@test isa_tfunc(typeof(Union{}), Union{}) === Const(false) # any result is ok
@test isa_tfunc(typeof(Union{}), Union{}) === Union{} # any result is ok
@test isa_tfunc(typeof(Union{}), Type{typeof(Union{})}) === Const(true)
@test isa_tfunc(typeof(Union{}), Const(typeof(Union{}))) === Const(true)
let c = Conditional(Core.SlotNumber(0), Const(Union{}), Const(Union{}))
Expand All @@ -1204,7 +1204,7 @@ let isa_tfunc = Core.Compiler.T_FFUNC_VAL[
@test isa_tfunc(Val{1}, Type{Val{T}} where T) === Bool
@test isa_tfunc(Val{1}, DataType) === Bool
@test isa_tfunc(Any, Const(Any)) === Const(true)
@test isa_tfunc(Any, Union{}) === Const(false) # any result is ok
@test isa_tfunc(Any, Union{}) === Union{} # any result is ok
@test isa_tfunc(Any, Type{Union{}}) === Const(false)
@test isa_tfunc(Union{Int64, Float64}, Type{Real}) === Const(true)
@test isa_tfunc(Union{Int64, Float64}, Type{Integer}) === Bool
Expand Down Expand Up @@ -1245,7 +1245,7 @@ let subtype_tfunc = Core.Compiler.T_FFUNC_VAL[
@test subtype_tfunc(Type{Union{}}, Union{Type{Int64}, Type{Float64}}) === Const(true)
@test subtype_tfunc(Type{Union{}}, Union{Type{T}, Type{Float64}} where T) === Const(true)
let c = Conditional(Core.SlotNumber(0), Const(Union{}), Const(Union{}))
@test subtype_tfunc(c, Const(Bool)) === Bool # any result is ok
@test subtype_tfunc(c, Const(Bool)) === Const(true) # any result is ok
end
@test subtype_tfunc(Type{Val{1}}, Type{Val{T}} where T) === Bool
@test subtype_tfunc(Type{Val{1}}, DataType) === Bool
Expand Down Expand Up @@ -1717,3 +1717,8 @@ Base.iterate(i::Iterator27434, ::Val{2}) = i.z, Val(3)
Base.iterate(::Iterator27434, ::Any) = nothing
@test @inferred splat27434(Iterator27434(1, 2, 3)) == (1, 2, 3)
@test Core.Compiler.return_type(splat27434, Tuple{typeof(Iterators.repeated(1))}) == Union{}

# issue #27078
f27078(T::Type{S}) where {S} = isa(T, UnionAll) ? f27078(T.body) : T
T27078 = Vector{Vector{T}} where T
@test f27078(T27078) === T27078.body
3 changes: 2 additions & 1 deletion test/sets.jl
Original file line number Diff line number Diff line change
Expand Up @@ -557,7 +557,8 @@ end
x = @inferred replace(x -> x > 1, [1, 2], missing)
@test isequal(x, [1, missing]) && x isa Vector{Union{Int, Missing}}

x = @inferred replace([1, missing], missing=>2)
@test_broken @inferred replace([1, missing], missing=>2)
x = replace([1, missing], missing=>2)
@test x == [1, 2] && x isa Vector{Int}
x = @inferred replace([1, missing], missing=>2, count=1)
@test x == [1, 2] && x isa Vector{Union{Int, Missing}}
Expand Down