Skip to content

Commit

Permalink
fix #30114, specificity transitivity errors in convert methods (#30160)
Browse files Browse the repository at this point in the history
  • Loading branch information
@JeffBezanson
JeffBezanson authored Dec 5, 2018
1 parent 49c4e09 commit 6594acc
Show file tree
Hide file tree
Showing 4 changed files with 125 additions and 39 deletions.
4 changes: 4 additions & 0 deletions NEWS.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ Language changes
to the `Core` module ([#29968]).
* Using the same name for both a local variable and a static parameter is now an error instead
of a warning ([#29429]).
* Method signatures such as
`f(::Type{T}, ::T) where {T <: X}` and
`f(::Type{X}, ::Any)`
are now considered ambiguous. Previously a bug caused the first one to be considered more specific ([#30160]).

Command-line option changes
---------------------------
Expand Down
1 change: 1 addition & 0 deletions base/bitarray.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,7 @@ reinterpret(B::BitArray, dims::NTuple{N,Int}) where {N} = reshape(B, dims)

if nameof(@__MODULE__) === :Base # avoid method overwrite
(::Type{T})(x::T) where {T<:BitArray} = copy(x)
BitArray(x::BitArray) = copy(x)
end

"""
Expand Down
99 changes: 61 additions & 38 deletions src/subtype.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ static int obviously_disjoint(jl_value_t *a, jl_value_t *b, int specificity)
for(i=0; i < np; i++) {
jl_value_t *ai = jl_tparam(ad,i);
jl_value_t *bi = jl_tparam(bd,i);
if (!istuple && specificity) {
if (!istuple && specificity && jl_has_free_typevars(ai)) {
// X{<:SomeDataType} and X{Union{Y,Z,...}} need to be disjoint to
// avoid this transitivity problem:
// A = Tuple{Type{LinearIndices{N,R}}, LinearIndices{N}} where {N,R}
Expand Down Expand Up @@ -2585,9 +2585,8 @@ static int tuple_morespecific(jl_datatype_t *cdt, jl_datatype_t *pdt, int invari
return 1;

int cms = type_morespecific_(ce, pe, invariant, env);
int eqv = !cms && eq_msp(ce, pe, env);

if (!cms && !eqv && !sub_msp(ce, pe, env)) {
if (!cms && !sub_msp(ce, pe, env)) {
/*
A bound vararg tuple can be more specific despite disjoint elements in order to
preserve transitivity. For example in
Expand All @@ -2600,7 +2599,8 @@ static int tuple_morespecific(jl_datatype_t *cdt, jl_datatype_t *pdt, int invari
}

// Tuple{..., T} not more specific than Tuple{..., Vararg{S}} if S is diagonal
if (eqv && i == clen-1 && clen == plen && !cva && pva && jl_is_typevar(ce) && jl_is_typevar(pe) && !cdiag && pdiag)
if (!cms && i == clen-1 && clen == plen && !cva && pva && eq_msp(ce, pe, env) &&
jl_is_typevar(ce) && jl_is_typevar(pe) && !cdiag && pdiag)
return 0;

if (cms) some_morespecific = 1;
Expand Down Expand Up @@ -2692,24 +2692,23 @@ static int num_occurs(jl_tvar_t *v, jl_typeenv_t *env)
return 0;
}

#define HANDLE_UNIONALL_A \
jl_unionall_t *ua = (jl_unionall_t*)a; \
jl_typeenv_t newenv = { ua->var, 0x0, env }; \
newenv.val = (jl_value_t*)(intptr_t)count_occurs(ua->body, ua->var); \
return type_morespecific_(ua->body, b, invariant, &newenv)

#define HANDLE_UNIONALL_B \
jl_unionall_t *ub = (jl_unionall_t*)b; \
jl_typeenv_t newenv = { ub->var, 0x0, env }; \
newenv.val = (jl_value_t*)(intptr_t)count_occurs(ub->body, ub->var); \
return type_morespecific_(a, ub->body, invariant, &newenv)

static int type_morespecific_(jl_value_t *a, jl_value_t *b, int invariant, jl_typeenv_t *env)
{
if (a == b)
return 0;

if (jl_is_unionall(a)) {
jl_unionall_t *ua = (jl_unionall_t*)a;
jl_typeenv_t newenv = { ua->var, 0x0, env };
newenv.val = (jl_value_t*)(intptr_t)count_occurs(ua->body, ua->var);
return type_morespecific_(ua->body, b, invariant, &newenv);
}
if (jl_is_unionall(b)) {
jl_unionall_t *ub = (jl_unionall_t*)b;
jl_typeenv_t newenv = { ub->var, 0x0, env };
newenv.val = (jl_value_t*)(intptr_t)count_occurs(ub->body, ub->var);
return type_morespecific_(a, ub->body, invariant, &newenv);
}

if (jl_is_tuple_type(a) && jl_is_tuple_type(b)) {
// When one is JL_VARARG_BOUND and the other has fixed length,
// allow the argument length to fix the tvar
Expand All @@ -2727,7 +2726,15 @@ static int type_morespecific_(jl_value_t *a, jl_value_t *b, int invariant, jl_ty
return tuple_morespecific((jl_datatype_t*)a, (jl_datatype_t*)b, invariant, env);
}

if (!invariant) {
if ((jl_datatype_t*)a == jl_any_type) return 0;
if ((jl_datatype_t*)b == jl_any_type && !jl_is_typevar(a)) return 1;
}

if (jl_is_uniontype(a)) {
if (jl_is_unionall(b)) {
HANDLE_UNIONALL_B;
}
// Union a is more specific than b if some element of a is more specific than b, but
// not vice-versa.
if (sub_msp(b, a, env))
Expand Down Expand Up @@ -2764,17 +2771,15 @@ static int type_morespecific_(jl_value_t *a, jl_value_t *b, int invariant, jl_ty
}

if (jl_is_uniontype(b)) {
if (jl_is_unionall(a)) {
HANDLE_UNIONALL_A;
}
jl_uniontype_t *u = (jl_uniontype_t*)b;
if (type_morespecific_(a, u->a, invariant, env) || type_morespecific_(a, u->b, invariant, env))
return !type_morespecific_(b, a, invariant, env);
return 0;
}

if (!invariant) {
if ((jl_datatype_t*)a == jl_any_type) return 0;
if ((jl_datatype_t*)b == jl_any_type) return 1;
}

if (jl_is_datatype(a) && jl_is_datatype(b)) {
jl_datatype_t *tta = (jl_datatype_t*)a, *ttb = (jl_datatype_t*)b;
// Type{Union{}} is more specific than other types, so TypeofBottom must be too
Expand All @@ -2796,13 +2801,20 @@ static int type_morespecific_(jl_value_t *a, jl_value_t *b, int invariant, jl_ty
for(size_t i=0; i < jl_nparams(tta); i++) {
jl_value_t *apara = jl_tparam(tta,i);
jl_value_t *bpara = jl_tparam(ttb,i);
if (!jl_has_free_typevars(apara) && !jl_has_free_typevars(bpara) &&
!jl_types_equal(apara, bpara))
int afree = jl_has_free_typevars(apara);
int bfree = jl_has_free_typevars(bpara);
if (!afree && !bfree && !jl_types_equal(apara, bpara))
return 0;
if (type_morespecific_(apara, bpara, 1, env))
if (type_morespecific_(apara, bpara, 1, env) && (jl_is_typevar(apara) || !afree || bfree))
ascore += 1;
else if (type_morespecific_(bpara, apara, 1, env))
else if (type_morespecific_(bpara, apara, 1, env) && (jl_is_typevar(bpara) || !bfree || afree))
bscore += 1;
else if (eq_msp(apara, bpara, env)) {
if (!afree && bfree)
ascore += 1;
else if (afree && !bfree)
bscore += 1;
}
if (jl_is_typevar(bpara) && !jl_is_typevar(apara) && !jl_is_type(apara))
ascore1 = 1;
else if (jl_is_typevar(apara) && !jl_is_typevar(bpara) && !jl_is_type(bpara))
Expand All @@ -2828,9 +2840,6 @@ static int type_morespecific_(jl_value_t *a, jl_value_t *b, int invariant, jl_ty
return 0;
return ascore > bscore || adiag > bdiag;
}
else if (invariant) {
return 0;
}
tta = tta->super; super = 1;
}
return 0;
Expand All @@ -2852,16 +2861,18 @@ static int type_morespecific_(jl_value_t *a, jl_value_t *b, int invariant, jl_ty
if (invariant) {
if (((jl_tvar_t*)a)->ub == jl_bottom_type)
return 1;
if (jl_has_free_typevars(b)) {
if (type_morespecific_(((jl_tvar_t*)a)->ub, b, 0, env) ||
eq_msp(((jl_tvar_t*)a)->ub, b, env))
return num_occurs((jl_tvar_t*)a, env) >= 2;
}
else {
if (!jl_has_free_typevars(b))
return 0;
}
if (eq_msp(((jl_tvar_t*)a)->ub, b, env))
return num_occurs((jl_tvar_t*)a, env) >= 2;
}
else {
// need `{T,T} where T` more specific than `{Any, Any}`
if (b == (jl_value_t*)jl_any_type && ((jl_tvar_t*)a)->ub == (jl_value_t*)jl_any_type &&
num_occurs((jl_tvar_t*)a, env) >= 2)
return 1;
}
return type_morespecific_((jl_value_t*)((jl_tvar_t*)a)->ub, b, 0, env);
return type_morespecific_(((jl_tvar_t*)a)->ub, b, 0, env);
}
if (jl_is_typevar(b)) {
if (!jl_is_type(a))
Expand All @@ -2873,12 +2884,24 @@ static int type_morespecific_(jl_value_t *a, jl_value_t *b, int invariant, jl_ty
if (type_morespecific_(a, ((jl_tvar_t*)b)->ub, 0, env) ||
eq_msp(a, ((jl_tvar_t*)b)->ub, env))
return num_occurs((jl_tvar_t*)b, env) < 2;
return 0;
}
else {
if (obviously_disjoint(a, ((jl_tvar_t*)b)->ub, 1))
return 0;
if (type_morespecific_(((jl_tvar_t*)b)->ub, a, 0, env))
return 0;
return 1;
}
}
return type_morespecific_(a, (jl_value_t*)((jl_tvar_t*)b)->ub, 0, env);
return type_morespecific_(a, ((jl_tvar_t*)b)->ub, 0, env);
}

if (jl_is_unionall(a)) {
HANDLE_UNIONALL_A;
}
if (jl_is_unionall(b)) {
HANDLE_UNIONALL_B;
}

return 0;
Expand Down
60 changes: 59 additions & 1 deletion test/specificity.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ f17016(f, t::T_17016) = 0
f17016(f, t1::Tuple) = 1
@test f17016(0, (1,2,3)) == 0

@test args_morespecific(Tuple{Type{Any}, Any}, Tuple{Type{T}, Any} where T<:VecElement)
@test !args_morespecific(Tuple{Type{Any}, Any}, Tuple{Type{T}, Any} where T<:VecElement)
@test !args_morespecific((Tuple{Type{T}, Any} where T<:VecElement), Tuple{Type{Any}, Any})

@test !args_morespecific(Tuple{Type{T}, Tuple{Any, Vararg{Any, N} where N}} where T<:Tuple{Any, Vararg{Any, N} where N},
Expand Down Expand Up @@ -240,3 +240,61 @@ end
@test args_morespecific(Tuple{Array,Int64}, Tuple{Array,Vararg{Int64,N}} where N)
@test args_morespecific(Tuple{Array,Int64}, Tuple{Array,Vararg{Int64,N} where N})
@test !args_morespecific(Tuple{Array,Int64}, Tuple{AbstractArray, Array})

# issue #30114
let T1 = Tuple{Type{Tuple{Vararg{AbstractUnitRange{Int64},N} where N}},CartesianIndices{N,R} where R<:Tuple{Vararg{AbstractUnitRange{Int64},N}}} where N
T2 = Tuple{Type{T},T} where T<:AbstractArray
T3 = Tuple{Type{AbstractArray{T,N} where N},AbstractArray} where T
T4 = Tuple{Type{AbstractArray{T,N}},AbstractArray{s57,N} where s57} where N where T
@test !args_morespecific(T1, T2)
@test !args_morespecific(T1, T3)
@test !args_morespecific(T1, T4)
@test args_morespecific(T2, T3)
@test args_morespecific(T2, T4)
end

@test !args_morespecific(Tuple{Type{Tuple{Vararg{AbstractUnitRange{Int64},N}}},} where N,
Tuple{Type{Tuple{Vararg{AbstractUnitRange,N} where N}},})

@test args_morespecific(Tuple{Type{SubArray{T,2,P} where T}, Array{T}} where T where P,
Tuple{Type{AbstractArray{T,N} where N},AbstractArray} where T)

# these are ambiguous
@test !args_morespecific(Tuple{Type{T},T} where T<:BitArray,
Tuple{Type{BitArray},Any})
@test !args_morespecific(Tuple{Type{BitArray},Any},
Tuple{Type{T},T} where T<:BitArray)

abstract type Domain{T} end

abstract type AbstractInterval{T} <: Domain{T} end

struct Interval{L,R,T} <: AbstractInterval{T}
end

let A = Tuple{Type{Interval{:closed,:closed,T} where T}, Interval{:closed,:closed,T} where T},
B = Tuple{Type{II}, AbstractInterval} where II<:(Interval{:closed,:closed,T} where T),
C = Tuple{Type{AbstractInterval}, AbstractInterval}
@test args_morespecific(A, B)
@test !args_morespecific(B, C)
@test !args_morespecific(A, C)
end

let A = Tuple{Type{Domain}, Interval{L,R,T} where T} where R where L,
B = Tuple{Type{II}, AbstractInterval} where II<:(Interval{:closed,:closed,T} where T),
C = Tuple{Type{AbstractInterval{T}}, AbstractInterval{T}} where T
@test !args_morespecific(A, B)
@test args_morespecific(B, C)
@test !args_morespecific(A, C)
end

let A = Tuple{Type{AbstractInterval}, Interval{L,R,T} where T} where R where L,
B = Tuple{Type{II}, AbstractInterval} where II<:(Interval{:closed,:closed,T} where T),
C = Tuple{Type{AbstractInterval{T}}, AbstractInterval{T}} where T
@test !args_morespecific(A, B)
@test args_morespecific(B, C)
@test args_morespecific(A, C)
end

@test args_morespecific(Tuple{Type{Missing},Any},
Tuple{Type{Union{Nothing, T}},Any} where T)

2 comments on commit 6594acc

@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 benchmark build, I will reply here when finished:

@nanosoldier runbenchmarks(ALL, 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.

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

Please sign in to comment.