Skip to content

Commit

Permalink
Fix some overly optimistic fastpaths in subtyping (#25796)
Browse files Browse the repository at this point in the history
  • Loading branch information
martinholters authored Jul 17, 2020
1 parent e36194a commit bd52a95
Show file tree
Hide file tree
Showing 7 changed files with 62 additions and 27 deletions.
1 change: 1 addition & 0 deletions src/datatype.c
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ jl_datatype_t *jl_new_uninitialized_datatype(void)
t->zeroinit = 0;
t->isinlinealloc = 0;
t->has_concrete_subtype = 1;
t->cached_by_hash = 0;
t->layout = NULL;
t->names = NULL;
t->types = NULL;
Expand Down
4 changes: 3 additions & 1 deletion src/dump.c
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,8 @@ static void jl_serialize_datatype(jl_serializer_state *s, jl_datatype_t *dt) JL_
| (dt->isbitstype << 3)
| (dt->zeroinit << 4)
| (dt->isinlinealloc << 5)
| (dt->has_concrete_subtype << 6));
| (dt->has_concrete_subtype << 6)
| (dt->cached_by_hash << 7));
if (!dt->abstract) {
write_uint16(s->s, dt->ninitialized);
}
Expand Down Expand Up @@ -1174,6 +1175,7 @@ static jl_value_t *jl_deserialize_datatype(jl_serializer_state *s, int pos, jl_v
dt->zeroinit = (memflags >> 4) & 1;
dt->isinlinealloc = (memflags >> 5) & 1;
dt->has_concrete_subtype = (memflags >> 6) & 1;
dt->cached_by_hash = (memflags >> 7) & 1;
dt->types = NULL;
dt->parameters = NULL;
dt->name = NULL;
Expand Down
27 changes: 22 additions & 5 deletions src/jltypes.c
Original file line number Diff line number Diff line change
Expand Up @@ -568,7 +568,8 @@ static int typekey_eq(jl_datatype_t *tt, jl_value_t **key, size_t n)
// require exact same Type{T}. see e.g. issue #22842
if (jl_is_type_type(tj) || jl_is_type_type(kj))
return 0;
if (jl_is_concrete_type(tj) || jl_is_concrete_type(kj))
if ((jl_is_concrete_type(tj) || jl_is_concrete_type(kj)) &&
jl_type_equality_is_identity(tj, kj))
return 0;
if (!jl_types_equal(tj, kj))
return 0;
Expand Down Expand Up @@ -860,6 +861,17 @@ jl_datatype_t *jl_lookup_cache_type_(jl_datatype_t *type)
return (jl_datatype_t*)lookup_type(type->name, key, n);
}

int jl_type_equality_is_identity(jl_value_t *t1, jl_value_t *t2)
{
if (t1 == t2)
return 1;
if (!jl_is_datatype(t1) || !jl_is_datatype(t2))
return 0;
jl_datatype_t *dt1 = (jl_datatype_t *) t1;
jl_datatype_t *dt2 = (jl_datatype_t *) t2;

return dt1->cached_by_hash == dt2->cached_by_hash;
}

// type instantiation

Expand Down Expand Up @@ -1155,6 +1167,7 @@ void jl_precompute_memoized_dt(jl_datatype_t *dt, int cacheable)
if (dt->name == jl_type_typename)
cacheable = 0; // the cache for Type ignores parameter normalization, so it can't be used as a regular hash
dt->hash = typekey_hash(dt->name, jl_svec_data(dt->parameters), l, cacheable);
dt->cached_by_hash = cacheable ? (typekey_hash(dt->name, jl_svec_data(dt->parameters), l, 0) != 0) : (dt->hash != 0);
}

static void check_datatype_parameters(jl_typename_t *tn, jl_value_t **params, size_t np)
Expand Down Expand Up @@ -1880,7 +1893,7 @@ void jl_init_types(void) JL_GC_DISABLED
jl_datatype_type->name->wrapper = (jl_value_t*)jl_datatype_type;
jl_datatype_type->super = (jl_datatype_t*)jl_type_type;
jl_datatype_type->parameters = jl_emptysvec;
jl_datatype_type->name->names = jl_perm_symsvec(19,
jl_datatype_type->name->names = jl_perm_symsvec(20,
"name",
"super",
"parameters",
Expand All @@ -1899,8 +1912,9 @@ void jl_init_types(void) JL_GC_DISABLED
"isbitstype",
"zeroinit",
"isinlinealloc",
"has_concrete_subtype");
jl_datatype_type->types = jl_svec(19,
"has_concrete_subtype",
"cached_by_hash");
jl_datatype_type->types = jl_svec(20,
jl_typename_type,
jl_datatype_type,
jl_simplevector_type,
Expand All @@ -1909,7 +1923,7 @@ void jl_init_types(void) JL_GC_DISABLED
jl_any_type, jl_any_type, jl_any_type, jl_any_type, // properties
jl_any_type, jl_any_type, jl_any_type, jl_any_type,
jl_any_type, jl_any_type, jl_any_type, jl_any_type,
jl_any_type);
jl_any_type, jl_any_type);
jl_datatype_type->abstract = 0;
// NOTE: types are not actually mutable, but we want to ensure they are heap-allocated with stable addresses
jl_datatype_type->mutabl = 1;
Expand Down Expand Up @@ -2019,10 +2033,12 @@ void jl_init_types(void) JL_GC_DISABLED
jl_anytuple_type->isconcretetype = 0;
jl_anytuple_type->layout = NULL;
jl_anytuple_type->size = 0;
jl_anytuple_type->cached_by_hash = 0;

jl_tvar_t *tttvar = tvar("T");
((jl_datatype_t*)jl_type_type)->parameters = jl_svec(1, tttvar);
((jl_datatype_t*)jl_type_type)->hasfreetypevars = 1;
((jl_datatype_t*)jl_type_type)->cached_by_hash = 0;
jl_type_typename->wrapper = jl_new_struct(jl_unionall_type, tttvar, (jl_value_t*)jl_type_type);
jl_type_type = (jl_unionall_t*)jl_type_typename->wrapper;

Expand Down Expand Up @@ -2482,6 +2498,7 @@ void jl_init_types(void) JL_GC_DISABLED
jl_svecset(jl_datatype_type->types, 16, jl_bool_type);
jl_svecset(jl_datatype_type->types, 17, jl_bool_type);
jl_svecset(jl_datatype_type->types, 18, jl_bool_type);
jl_svecset(jl_datatype_type->types, 19, jl_bool_type);
jl_svecset(jl_typename_type->types, 1, jl_module_type);
jl_svecset(jl_typename_type->types, 6, jl_long_type);
jl_svecset(jl_typename_type->types, 3, jl_type_type);
Expand Down
2 changes: 2 additions & 0 deletions src/julia.h
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,7 @@ typedef struct _jl_datatype_t {
uint8_t zeroinit; // if one or more fields requires zero-initialization
uint8_t isinlinealloc; // if this is allocated inline
uint8_t has_concrete_subtype; // If clear, no value will have this datatype
uint8_t cached_by_hash; // stored in hash-based set cache (instead of linear cache)
} jl_datatype_t;

typedef struct {
Expand Down Expand Up @@ -1216,6 +1217,7 @@ JL_DLLEXPORT int jl_egal(jl_value_t *a JL_MAYBE_UNROOTED, jl_value_t *b JL_MAYBE
JL_DLLEXPORT uintptr_t jl_object_id(jl_value_t *v) JL_NOTSAFEPOINT;

// type predicates and basic operations
JL_DLLEXPORT int jl_type_equality_is_identity(jl_value_t *t1, jl_value_t *t2) JL_NOTSAFEPOINT;
JL_DLLEXPORT int jl_has_free_typevars(jl_value_t *v) JL_NOTSAFEPOINT;
JL_DLLEXPORT int jl_has_typevar(jl_value_t *t, jl_tvar_t *v) JL_NOTSAFEPOINT;
JL_DLLEXPORT int jl_has_typevar_from_unionall(jl_value_t *t, jl_unionall_t *ua);
Expand Down
17 changes: 8 additions & 9 deletions src/subtype.c
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,8 @@ static int obviously_unequal(jl_value_t *a, jl_value_t *b)
if (ad->name != bd->name)
return 1;
int istuple = (ad->name == jl_tuple_typename);
if (jl_is_concrete_type(a) || jl_is_concrete_type(b)) {
if ((jl_is_concrete_type(a) || jl_is_concrete_type(b)) &&
jl_type_equality_is_identity(a, b)) {
if (!istuple && ad->name != jl_type_typename) // HACK: can't properly normalize Tuple{Float64} == Tuple{<:Float64} like types or Type{T} types
return 1;
}
Expand Down Expand Up @@ -313,13 +314,11 @@ static int obviously_disjoint(jl_value_t *a, jl_value_t *b, int specificity)
return 0;
if (specificity && a == (jl_value_t*)jl_typeofbottom_type)
return 0;
// TODO: this would be a nice fast-path to have, unfortuanately,
// datatype allocation fails to correctly hash-cons them
// and the subtyping tests include tests for this case
//if (jl_is_concrete_type(a) && jl_is_concrete_type(b) &&
// (((jl_datatype_t*)a)->name != jl_tuple_typename ||
// ((jl_datatype_t*)b)->name != jl_tuple_typename))
// return 1;
if (jl_is_concrete_type(a) && jl_is_concrete_type(b) &&
jl_type_equality_is_identity(a, b) &&
(((jl_datatype_t*)a)->name != jl_tuple_typename ||
((jl_datatype_t*)b)->name != jl_tuple_typename))
return 1;
if (jl_is_unionall(a)) a = jl_unwrap_unionall(a);
if (jl_is_unionall(b)) b = jl_unwrap_unionall(b);
if (jl_is_datatype(a) && jl_is_datatype(b)) {
Expand Down Expand Up @@ -2115,7 +2114,7 @@ JL_DLLEXPORT int jl_isa(jl_value_t *x, jl_value_t *t)
return 0;
}
}
if (jl_is_concrete_type(t))
if (jl_is_concrete_type(t) && jl_type_equality_is_identity(jl_typeof(x), t))
return 0;
return jl_subtype(jl_typeof(x), t);
}
Expand Down
11 changes: 11 additions & 0 deletions test/core.jl
Original file line number Diff line number Diff line change
Expand Up @@ -7239,6 +7239,17 @@ struct AVL35416{K,V}
end
@test AVL35416(Node35416{AVL35416{Integer,AbstractString},Int,String}()) isa AVL35416{Integer,AbstractString}

# issue #31696
foo31696(x::Int8, y::Int8) = 1
foo31696(x::T, y::T) where {T <: Int8} = 2
@test length(methods(foo31696)) == 1
let T1 = Tuple{Int8}, T2 = Tuple{T} where T<:Int8, a = T1[(1,)], b = T2[(1,)]
b .= a
@test b[1] == (1,)
a .= b
@test a[1] == (1,)
end

# issue #36104
module M36104
struct T36104
Expand Down
27 changes: 15 additions & 12 deletions test/subtype.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1358,6 +1358,18 @@ let (t, e) = intersection_env(Tuple{Union{Int,Int8}}, Tuple{T} where T)
@test e[1] isa TypeVar
end

# issue #25430
@test Vector{Tuple{Any}}() isa Vector{Tuple{>:Int}}
@test Vector{Tuple{>:Int}}() isa Vector{Tuple{Any}}
@test Vector{Tuple{Any}} == Vector{Tuple{>:Int}}
@test Vector{Vector{Tuple{Any}}} == Vector{Vector{Tuple{>:Int}}}
f25430(t::Vector{Tuple{Any}}) = true
g25430(t::Vector{Tuple{>:Int}}) = true
@test f25430(Vector{Tuple{>:Int}}())
@test g25430(Vector{Tuple{Any}}())
@testintersect(Vector{Tuple{>:Int}}, Vector{Tuple{Any}}, Vector{Tuple{Any}})
@testintersect(Vector{Vector{Tuple{>:Int}}}, Vector{Vector{Tuple{Any}}}, Vector{Vector{Tuple{Any}}})

# issue #24521
g24521(::T, ::T) where {T} = T
@test_throws MethodError g24521(Tuple{Any}, Tuple{T} where T)
Expand Down Expand Up @@ -1627,18 +1639,9 @@ end
@testintersect(Tuple{Type{<:AbstractVector{T}}, Int} where T,
Tuple{Type{Vector}, Any},
Union{})
#@testintersect(Tuple{Type{<:AbstractVector{T}}, Int} where T,
# Tuple{Type{Vector{T} where Int<:T<:Int}, Any},
# Tuple{Type{Vector{Int}}, Int})
@test_broken isequal(_type_intersect(Tuple{Type{<:AbstractVector{T}}, Int} where T,
Tuple{Type{Vector{T} where Int<:T<:Int}, Any}),
Tuple{Type{Vector{Int}}, Int})
@test isequal_type(_type_intersect(Tuple{Type{<:AbstractVector{T}}, Int} where T,
Tuple{Type{Vector{T} where Int<:T<:Int}, Any}),
Tuple{Type{Vector{Int}}, Int})
@test isequal_type(_type_intersect(Tuple{Type{Vector{T} where Int<:T<:Int}, Any},
Tuple{Type{<:AbstractVector{T}}, Int} where T),
Tuple{Type{Vector{Int}}, Int})
@testintersect(Tuple{Type{<:AbstractVector{T}}, Int} where T,
Tuple{Type{Vector{T} where Int<:T<:Int}, Any},
Tuple{Type{Vector{Int}}, Int})
let X = LinearAlgebra.Symmetric{T, S} where S<:(AbstractArray{U, 2} where U<:T) where T,
Y = Union{LinearAlgebra.Hermitian{T, S} where S<:(AbstractArray{U, 2} where U<:T) where T,
LinearAlgebra.Symmetric{T, S} where S<:(AbstractArray{U, 2} where U<:T) where T}
Expand Down

0 comments on commit bd52a95

Please sign in to comment.