Skip to content

Commit

Permalink
some fixes to method ambiguity reflection
Browse files Browse the repository at this point in the history 
`methods` now doesn't return ambiguous matches by default

some fixes to `isambiguous` and more tests
  • Loading branch information
@JeffBezanson
JeffBezanson committed May 5, 2016
1 parent 59cc3b4 commit 0fa8bb5
Show file tree
Hide file tree
Showing 4 changed files with 121 additions and 64 deletions.
29 changes: 18 additions & 11 deletions base/reflection.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,11 +176,13 @@ code_lowered(f, t::ANY=Tuple) = map(m -> (m::Method).lambda_template, methods(f,

function _methods(f::ANY,t::ANY,lim)
ft = isa(f,Type) ? Type{f} : typeof(f)
if isa(t,Type)
return _methods_by_ftype(Tuple{ft, t.parameters...}, lim)
else
return _methods_by_ftype(Tuple{ft, t...}, lim)
end
tt = isa(t,Type) ? Tuple{ft, t.parameters...} : Tuple{ft, t...}
return _methods_by_ftype(tt, lim)
end
function methods_including_ambiguous(f::ANY, t::ANY)
ft = isa(f,Type) ? Type{f} : typeof(f)
tt = isa(t,Type) ? Tuple{ft, t.parameters...} : Tuple{ft, t...}
return ccall(:jl_matching_methods, Any, (Any,Cint,Cint), tt, -1, 1)
end
function _methods_by_ftype(t::ANY, lim)
tp = t.parameters
Expand All @@ -194,11 +196,11 @@ function _methods_by_ftype(t::ANY, lim)
return _methods(Any[tp...], length(tp), lim, [])
end
# TODO: the following can return incorrect answers that the above branch would have corrected
return ccall(:jl_matching_methods, Any, (Any,Int32), t, lim)
return ccall(:jl_matching_methods, Any, (Any,Cint,Cint), t, lim, 0)
end
function _methods(t::Array,i,lim::Integer,matching::Array{Any,1})
if i == 0
new = ccall(:jl_matching_methods, Any, (Any,Int32), Tuple{t...}, lim)
new = ccall(:jl_matching_methods, Any, (Any,Cint,Cint), Tuple{t...}, lim, 0)
new === false && return false
append!(matching, new::Array{Any,1})
else
Expand Down Expand Up @@ -437,8 +439,13 @@ end

function isambiguous(m1::Method, m2::Method)
ti = typeintersect(m1.sig, m2.sig)
ml = _methods_by_ftype(ti, 1)
ml == false && return true
m = ml[1][3]
!(m.sig <: ti)
ti === Bottom && return false
ml = _methods_by_ftype(ti, -1)
isempty(ml) && return true
for m in ml
if ti <: m[3].sig
return false
end
end
return true
end
2 changes: 1 addition & 1 deletion base/replutil.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ function showerror(io::IO, ex::MethodError)
is_arg_types = isa(ex.args, DataType)
arg_types = is_arg_types ? ex.args : typesof(ex.args...)
f = ex.f
meth = methods(f, arg_types)
meth = methods_including_ambiguous(f, arg_types)
if length(meth) > 1
return showerror_ambiguous(io, meth, f, arg_types)
end
Expand Down
117 changes: 68 additions & 49 deletions src/gf.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,7 @@ static jl_tupletype_t *join_tsig(jl_tupletype_t *tt, jl_tupletype_t *sig)
}

static jl_value_t *ml_matches(union jl_typemap_t ml, int offs,
jl_tupletype_t *type, int lim);
jl_tupletype_t *type, int lim, int include_ambiguous);

static jl_lambda_info_t *cache_method(jl_methtable_t *mt, union jl_typemap_t *cache, jl_value_t *parent,
jl_tupletype_t *type, jl_tupletype_t *origtype,
Expand Down Expand Up @@ -560,7 +560,7 @@ static jl_lambda_info_t *cache_method(jl_methtable_t *mt, union jl_typemap_t *ca
temp2 = (jl_value_t*)type;
}
if (need_guard_entries) {
temp = ml_matches(mt->defs, 0, type, -1); // TODO: use MAX_UNSPECIALIZED_CONFLICTS?
temp = ml_matches(mt->defs, 0, type, -1, 0); // TODO: use MAX_UNSPECIALIZED_CONFLICTS?
int guards = 0;
if (temp == jl_false) {
cache_with_orig = 1;
Expand Down Expand Up @@ -749,6 +749,7 @@ static int check_ambiguous_visitor(jl_typemap_entry_t *oldentry, struct typemap_
struct ambiguous_matches_env *closure = container_of(closure0, struct ambiguous_matches_env, match);
if (oldentry == closure->newentry)
return 0; // finished once it finds the method that was just inserted
// TODO: instead, maybe stop once we hit something newentry is definitely more specific than

union jl_typemap_t map = closure->defs;
jl_tupletype_t *type = (jl_tupletype_t*)closure->match.type;
Expand Down Expand Up @@ -1006,7 +1007,7 @@ jl_lambda_info_t *jl_method_lookup(jl_methtable_t *mt, jl_value_t **args, size_t
return sf;
}

JL_DLLEXPORT jl_value_t *jl_matching_methods(jl_tupletype_t *types, int lim);
JL_DLLEXPORT jl_value_t *jl_matching_methods(jl_tupletype_t *types, int lim, int include_ambiguous);

// compile-time method lookup
jl_lambda_info_t *jl_get_specialization1(jl_tupletype_t *types)
Expand All @@ -1025,7 +1026,7 @@ jl_lambda_info_t *jl_get_specialization1(jl_tupletype_t *types)
// might match. also be conservative with tuples rather than trying
// to analyze them in detail.
if (ti == (jl_value_t*)jl_datatype_type || jl_is_tuple_type(ti)) {
jl_value_t *matches = jl_matching_methods(types, 1);
jl_value_t *matches = jl_matching_methods(types, 1, 0);
if (matches == jl_false)
return NULL;
break;
Expand Down Expand Up @@ -1717,6 +1718,7 @@ struct ml_matches_env {
jl_value_t *t; // results: array of svec(argtypes, params, Method)
jl_svec_t *matc; // current working svec
int lim;
int include_ambiguous; // whether ambiguous matches should be included
};
static int ml_matches_visitor(jl_typemap_entry_t *ml, struct typemap_intersection_env *closure0)
{
Expand All @@ -1732,18 +1734,11 @@ static int ml_matches_visitor(jl_typemap_entry_t *ml, struct typemap_intersectio
assert(meth);
int skip = 0;
size_t len = jl_array_len(closure->t);
// skip over any previously-added or less specific methods
// (this method might have been added previously through meth->ambig)
for (i = 0; i < len; i++) {
jl_method_t *priormeth = (jl_method_t*)jl_svecref(jl_cellref(closure->t, i), 2);
if (priormeth == meth) {
skip = 1;
break;
}
if (closure->lim >= 0) {
// we can skip this match if the types are already covered
// by a prior (more specific) match. but only do this in
// the "limited" mode used by type inference.
if (closure->lim >= 0) {
// we can skip this match if the types are already covered
// by a prior (more specific) match. but only do this in
// the "limited" mode used by type inference.
for (i = 0; i < len; i++) {
jl_value_t *prior_ti = jl_svecref(jl_cellref(closure->t, i), 0);
// in issue #13007 we incorrectly set skip=1 here, due to
// Type{_<:T} ∩ (UnionAll S Type{T{S}}) = Type{T{S}}
Expand All @@ -1768,35 +1763,6 @@ static int ml_matches_visitor(jl_typemap_entry_t *ml, struct typemap_intersectio
*/
}
if (!skip) {
if (closure->lim >= 0 && len >= closure->lim) {
closure->t = (jl_value_t*)jl_false;
return 0; // terminate search
}
closure->matc = jl_svec(3, closure->match.ti, closure->match.env, meth);
if (len == 0) {
closure->t = (jl_value_t*)jl_alloc_cell_1d(1);
jl_cellset(closure->t, 0, (jl_value_t*)closure->matc);
}
else {
jl_cell_1d_push((jl_array_t*)closure->t, (jl_value_t*)closure->matc);
}
// Add ambiguous calls
if (meth->ambig != jl_nothing) {
jl_svec_t *env = NULL;
JL_GC_PUSH1(&env);
for (size_t j = 0; j < jl_array_len(meth->ambig); j++) {
jl_method_t *mambig = (jl_method_t*)jl_cellref(meth->ambig, j);
env = jl_emptysvec;
jl_value_t *mti = jl_type_intersection_matching((jl_value_t*)mambig->sig,
(jl_value_t*)closure->match.type,
&env, jl_emptysvec);
if (mti != (jl_value_t*)jl_bottom_type) {
jl_cell_1d_push((jl_array_t*)closure->t,
(jl_value_t*)jl_svec(3, mti, env, mambig));
}
}
JL_GC_POP();
}
/*
Check whether all static parameters matched. If not, then we
have an argument type like Vector{T{Int,_}}, and a signature like
Expand All @@ -1820,13 +1786,65 @@ static int ml_matches_visitor(jl_typemap_entry_t *ml, struct typemap_intersectio
break;
}
}
int done = 0, return_this_match = 1;
// (type ∩ ml->sig == type) ⇒ (type ⊆ ml->sig)
// NOTE: jl_subtype check added in case the intersection is
// over-approximated.
if (matched_all_typevars && jl_types_equal(closure->match.ti, closure->match.type) &&
jl_subtype(closure->match.type, (jl_value_t*)ml->sig, 0)) {
return 0; // terminate visiting method list
done = 1; // terminate visiting method list
// here we have reached a definition that fully covers the arguments.
// however, if there are ambiguities this method might not actually
// match, so we shouldn't add it to the results.
if (meth->ambig != jl_nothing) {
jl_svec_t *env = NULL;
JL_GC_PUSH1(&env);
for (size_t j = 0; j < jl_array_len(meth->ambig); j++) {
jl_method_t *mambig = (jl_method_t*)jl_cellref(meth->ambig, j);
env = jl_emptysvec;
jl_value_t *mti = jl_type_intersection_matching((jl_value_t*)closure->match.type,
(jl_value_t*)mambig->sig,
&env, mambig->tvars);
if (mti != (jl_value_t*)jl_bottom_type) {
if (closure->include_ambiguous) {
int k;
for(k=0; k < len; k++) {
if ((jl_value_t*)mambig == jl_svecref(jl_cellref(closure->t, k), 2))
break;
}
if (k >= len) {
if (len == 0) {
closure->t = (jl_value_t*)jl_alloc_cell_1d(0);
}
jl_cell_1d_push((jl_array_t*)closure->t,
(jl_value_t*)jl_svec(3, mti, env, mambig));
len++;
}
}
else {
return_this_match = 0;
break;
}
}
}
JL_GC_POP();
}
}
if (return_this_match) {
if (closure->lim >= 0 && len >= closure->lim) {
closure->t = (jl_value_t*)jl_false;
return 0; // terminate search
}
closure->matc = jl_svec(3, closure->match.ti, closure->match.env, meth);
if (len == 0) {
closure->t = (jl_value_t*)jl_alloc_cell_1d(1);
jl_cellset(closure->t, 0, (jl_value_t*)closure->matc);
}
else {
jl_cell_1d_push((jl_array_t*)closure->t, (jl_value_t*)closure->matc);
}
}
if (done) return 0;
}
return 1;
}
Expand All @@ -1837,7 +1855,7 @@ static int ml_matches_visitor(jl_typemap_entry_t *ml, struct typemap_intersectio
// Returns a match as an array of svec(argtypes, static_params, Method).
// See below for the meaning of lim.
static jl_value_t *ml_matches(union jl_typemap_t defs, int offs,
jl_tupletype_t *type, int lim)
jl_tupletype_t *type, int lim, int include_ambiguous)
{
size_t l = jl_svec_len(type->parameters);
jl_value_t *va = NULL;
Expand All @@ -1857,6 +1875,7 @@ static jl_value_t *ml_matches(union jl_typemap_t defs, int offs,
env.t = jl_an_empty_cell;
env.matc = NULL;
env.lim = lim;
env.include_ambiguous = include_ambiguous;
JL_GC_PUSH4(&env.t, &env.matc, &env.match.env, &env.match.ti);
jl_typemap_intersection_visitor(defs, offs, &env.match);
JL_GC_POP();
Expand All @@ -1870,7 +1889,7 @@ static jl_value_t *ml_matches(union jl_typemap_t defs, int offs,
//
// lim is the max # of methods to return. if there are more, returns jl_false.
// -1 for no limit.
JL_DLLEXPORT jl_value_t *jl_matching_methods(jl_tupletype_t *types, int lim)
JL_DLLEXPORT jl_value_t *jl_matching_methods(jl_tupletype_t *types, int lim, int include_ambiguous)
{
assert(jl_nparams(types) > 0);
if (jl_tparam0(types) == jl_bottom_type)
Expand All @@ -1879,7 +1898,7 @@ JL_DLLEXPORT jl_value_t *jl_matching_methods(jl_tupletype_t *types, int lim)
jl_methtable_t *mt = ((jl_datatype_t*)jl_tparam0(types))->name->mt;
if (mt == NULL)
return (jl_value_t*)jl_alloc_cell_1d(0);
return ml_matches(mt->defs, 0, types, lim);
return ml_matches(mt->defs, 0, types, lim, include_ambiguous);
}

#ifdef __cplusplus
Expand Down
37 changes: 34 additions & 3 deletions test/ambiguous.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ for m in mt
end

@test length(methods(ambig, (Int, Int))) == 1
@test length(methods(ambig, (UInt8, Int))) == 2
@test length(methods(ambig, (UInt8, Int))) == 0
@test length(Base.methods_including_ambiguous(ambig, (UInt8, Int))) == 2

@test ambig("hi", "there") == 1
@test ambig(3.1, 3.2) == 5
Expand All @@ -49,8 +50,6 @@ code_native(io, ambig, (Int, Int))
# Test that ambiguous cases fail appropriately
@test precompile(ambig, (UInt8, Int)) == false
cfunction(ambig, Int, (UInt8, Int)) # test for a crash (doesn't throw an error)
@test length(code_lowered(ambig, (UInt8, Int))) == 2
@test length(code_typed(ambig, (UInt8, Int))) == 2
@test_throws ErrorException code_llvm(io, ambig, (UInt8, Int))
@test_throws ErrorException code_native(io, ambig, (UInt8, Int))

Expand Down Expand Up @@ -110,4 +109,36 @@ ambs = detect_ambiguities(Ambig4)
# Test that Core and Base are free of ambiguities
@test isempty(detect_ambiguities(Core, Base; imported=true))

amb_1(::Int8, ::Int) = 1
amb_1(::Integer, x) = 2
amb_1(x, ::Int) = 3
# if there is an ambiguity with some methods and not others, `methods`
# should return just the non-ambiguous ones, i.e. the ones that could actually
# be called.
@test length(methods(amb_1, Tuple{Integer, Int})) == 1

amb_2(::Int, y) = 1
amb_2(x, ::Int) = 2
amb_2(::Int8, y) = 3
@test length(methods(amb_2)) == 3 # make sure no duplicates

amb_3(::Int8, ::Int8) = 1
amb_3(::Int16, ::Int16) = 2
amb_3(::Integer, ::Integer) = 3
amb_3(::Integer, x) = 4
amb_3(x, ::Integer) = 5
# ambiguous definitions exist, but are covered by multiple more specific definitions
let ms = methods(amb_3).ms
@test !Base.isambiguous(ms[4], ms[5])
end

amb_4(::Int8, ::Int8) = 1
amb_4(::Int16, ::Int16) = 2
amb_4(::Integer, x) = 4
amb_4(x, ::Integer) = 5
# as above, but without sufficient definition coverage
let ms = methods(amb_4).ms
@test Base.isambiguous(ms[3], ms[4])
end

nothing

0 comments on commit 0fa8bb5

Please sign in to comment.