Skip to content
New issue

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

#

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

Already on GitHub? # to your account

some fixes to method ambiguity reflection #16220

Merged
merged 1 commit into from
May 6, 2016
Merged

some fixes to method ambiguity reflection #16220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 18 additions & 11 deletions base/reflection.jl
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Copy link
Member

Choose a reason for hiding this comment

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

+1

alternatively, I've been wondering if we can use the work done here to create a fast-path for invalidate_conflicting where we only test lambdas that were derived from methods that check_ambiguous deemed to have an intersection

Copy link
Member Author

Choose a reason for hiding this comment

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

This was motivated by the following example:

f(::Int8, x)
f(::Integer, x)
f(x, ::Int)

Here we fail to detect that Int8, Int is ambiguous.

Yes, that sounds promising.


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
Open in desktop
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))
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the expected result of these now then?

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 felt code_* were separate features that didn't need to be tested here. But they have the same length as the result of methods.

@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