Skip to content
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

more complete ambiguity detection #16255

Merged
merged 2 commits into from
May 11, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion base/abstractarray.jl
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ similar{T}(a::AbstractArray{T}, dims::DimsInteger) = similar(a, T, dims)
similar{T}(a::AbstractArray{T}, dims::Integer...) = similar(a, T, dims)
similar( a::AbstractArray, T::Type, dims::Integer...) = similar(a, T, dims)
# similar creates an Array by default
similar( a::AbstractArray, T::Type, dims::DimsInteger) = Array(T, dims...)
similar( a::AbstractArray, T::Type, dims::DimsInteger) = similar(a, T, convert(Dims, dims))
similar( a::AbstractArray, T::Type, dims::Dims) = Array(T, dims)

## from general iterable to any array
Expand Down
18 changes: 9 additions & 9 deletions base/array.jl
Original file line number Diff line number Diff line change
Expand Up @@ -117,15 +117,15 @@ end

## Constructors ##

similar(a::Array, T, dims::Dims) = Array(T, dims)
similar{T}(a::Array{T,1}) = Array(T, size(a,1))
similar{T}(a::Array{T,2}) = Array(T, size(a,1), size(a,2))
similar{T}(a::Array{T,1}, dims::Dims) = Array(T, dims)
similar{T}(a::Array{T,1}, m::Int) = Array(T, m)
similar{T}(a::Array{T,1}, S::Type) = Array(S, size(a,1))
similar{T}(a::Array{T,2}, dims::Dims) = Array(T, dims)
similar{T}(a::Array{T,2}, m::Int) = Array(T, m)
similar{T}(a::Array{T,2}, S::Type) = Array(S, size(a,1), size(a,2))
similar(a::Array, T::Type, dims::Dims) = Array(T, dims)
similar{T}(a::Array{T,1}) = Array(T, size(a,1))
similar{T}(a::Array{T,2}) = Array(T, size(a,1), size(a,2))
similar{T}(a::Array{T,1}, dims::Dims) = Array(T, dims)
similar{T}(a::Array{T,1}, m::Int) = Array(T, m)
similar{T}(a::Array{T,1}, S::Type) = Array(S, size(a,1))
similar{T}(a::Array{T,2}, dims::Dims) = Array(T, dims)
similar{T}(a::Array{T,2}, m::Int) = Array(T, m)
similar{T}(a::Array{T,2}, S::Type) = Array(S, size(a,1), size(a,2))

# T[x...] constructs Array{T,1}
function getindex(T::Type, vals...)
Expand Down
1 change: 1 addition & 0 deletions base/bool.jl
Original file line number Diff line number Diff line change
Expand Up @@ -61,4 +61,5 @@ rem(x::Bool, y::Bool) = y ? false : throw(DivideError())
mod(x::Bool, y::Bool) = rem(x,y)

promote_op(op, ::Type{Bool}, ::Type{Bool}) = typeof(op(true, true))
promote_op(::typeof(^), ::Type{Bool}, ::Type{Bool}) = Bool
promote_op{T<:Integer}(::typeof(^), ::Type{Bool}, ::Type{T}) = Bool
2 changes: 1 addition & 1 deletion base/irrationals.jl
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ for T in (Range, BitArray, StridedArray, AbstractArray)
end

log(::Irrational{:e}) = 1 # use 1 to correctly promote expressions like log(x)/log(e)
log(::Irrational{:e}, x) = log(x)
log(::Irrational{:e}, x::Number) = log(x)

# align along = for nice Array printing
function alignment(io::IO, x::Irrational)
Expand Down
3 changes: 2 additions & 1 deletion base/linalg/hessenberg.jl
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ immutable HessenbergQ{T,S<:AbstractMatrix} <: AbstractMatrix{T}
end
HessenbergQ{T}(factors::AbstractMatrix{T}, τ::Vector{T}) = HessenbergQ{T,typeof(factors)}(factors, τ)
HessenbergQ(A::Hessenberg) = HessenbergQ(A.factors, A.τ)
size(A::HessenbergQ, args...) = size(A.factors, args...)
size(A::HessenbergQ, d) = size(A.factors, d)
size(A::HessenbergQ) = size(A.factors)

function getindex(A::Hessenberg, d::Symbol)
d == :Q && return HessenbergQ(A)
Expand Down
3 changes: 2 additions & 1 deletion base/linalg/symmetric.jl
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ end
typealias HermOrSym{T,S} Union{Hermitian{T,S}, Symmetric{T,S}}
typealias RealHermSymComplexHerm{T<:Real,S} Union{Hermitian{T,S}, Symmetric{T,S}, Hermitian{Complex{T},S}}

size(A::HermOrSym, args...) = size(A.data, args...)
size(A::HermOrSym, d) = size(A.data, d)
size(A::HermOrSym) = size(A.data)
@inline function getindex(A::Symmetric, i::Integer, j::Integer)
@boundscheck checkbounds(A, i, j)
@inbounds r = (A.uplo == 'U') == (i < j) ? A.data[i, j] : A.data[j, i]
Expand Down
3 changes: 2 additions & 1 deletion base/linalg/triangular.jl
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ for t in (:LowerTriangular, :UnitLowerTriangular, :UpperTriangular,
return $t{eltype(A), typeof(A)}(A)
end

size(A::$t, args...) = size(A.data, args...)
size(A::$t, d) = size(A.data, d)
size(A::$t) = size(A.data)

convert{T,S}(::Type{$t{T}}, A::$t{T,S}) = A
convert{Tnew,Told,S}(::Type{$t{Tnew}}, A::$t{Told,S}) = (Anew = convert(AbstractMatrix{Tnew}, A.data); $t(Anew))
Expand Down
8 changes: 5 additions & 3 deletions base/multidimensional.jl
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,11 @@ using .IteratorsMD
# Bounds-checking specialization
# Specializing for a fixed number of arguments provides a ~25%
# improvement over the general definitions in abstractarray.jl

# This is annoying, but we must first define logical indexing to avoid ambiguities
_internal_checkbounds(A::AbstractVector, I::AbstractVector{Bool}) = length(A) == length(I) || throw_boundserror(A, I)
_internal_checkbounds(A::AbstractVector, I::AbstractArray{Bool}) = size(A) == size(I) || throw_boundserror(A, I)

for N = 1:5
args = [:($(Symbol(:I, d))) for d = 1:N]
targs = [:($(Symbol(:I, d))::Union{Colon,Number,AbstractArray}) for d = 1:N] # prevent co-opting the CartesianIndex version
Expand All @@ -215,9 +220,6 @@ for N = 1:5
end
end
end
# This is annoying, but we must also define logical indexing to avoid ambiguities
_internal_checkbounds(A::AbstractVector, I::AbstractArray{Bool}) = size(A) == size(I) || throw_boundserror(A, I)
_internal_checkbounds(A::AbstractVector, I::AbstractVector{Bool}) = length(A) == length(I) || throw_boundserror(A, I)

# Bounds-checking with CartesianIndex
@inline function checkbounds(::Type{Bool}, ::Tuple{}, I1::CartesianIndex)
Expand Down
3 changes: 3 additions & 0 deletions base/rational.jl
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,9 @@ function round{T}(::Type{T}, x::Rational{Bool})
convert(T, x)
end

round{T}(::Type{T}, x::Rational{Bool}, ::RoundingMode{:Nearest}) = round(T, x)
round{T}(::Type{T}, x::Rational{Bool}, ::RoundingMode{:NearestTiesAway}) = round(T, x)
round{T}(::Type{T}, x::Rational{Bool}, ::RoundingMode{:NearestTiesUp}) = round(T, x)
Copy link
Contributor

Choose a reason for hiding this comment

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

why are these needed?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

they resolve the ambiguity of the method that follows

Copy link
Contributor

@tkelman tkelman May 11, 2016

Choose a reason for hiding this comment

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

what's the ambiguity though, aren't these 3 subtypes of the 4th? nevermind I see it higher up in the file now

round{T}(::Type{T}, x::Rational{Bool}, ::RoundingMode) = round(T, x)

trunc{T}(x::Rational{T}) = Rational(trunc(T,x))
Expand Down
2 changes: 1 addition & 1 deletion base/reshapedarray.jl
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ size_strides(out::Tuple) = out
size(A::ReshapedArray) = A.dims
size(A::ReshapedArray, d) = d <= ndims(A) ? A.dims[d] : 1
similar(A::ReshapedArray, eltype::Type) = similar(parent(A), eltype, size(A))
similar(A::ReshapedArray, eltype::Type, dims...) = similar(parent(A), eltype, dims...)
similar(A::ReshapedArray, eltype::Type, dims::Dims) = similar(parent(A), eltype, dims...)
linearindexing{R<:ReshapedArrayLF}(::Type{R}) = LinearFast()
parent(A::ReshapedArray) = A.parent
parentindexes(A::ReshapedArray) = map(s->1:s, size(parent(A)))
Expand Down
4 changes: 2 additions & 2 deletions base/sharedarray.jl
Original file line number Diff line number Diff line change
Expand Up @@ -430,8 +430,8 @@ function shmem_randn(dims; kwargs...)
end
shmem_randn(I::Int...; kwargs...) = shmem_randn(I; kwargs...)

similar(S::SharedArray, T, dims::Dims) = similar(S.s, T, dims)
similar(S::SharedArray, T) = similar(S.s, T, size(S))
similar(S::SharedArray, T::Type, dims::Dims) = similar(S.s, T, dims)
similar(S::SharedArray, T::Type) = similar(S.s, T, size(S))
similar(S::SharedArray, dims::Dims) = similar(S.s, eltype(S), dims)
similar(S::SharedArray) = similar(S.s, eltype(S), size(S))

Expand Down
2 changes: 1 addition & 1 deletion base/sparse/sparsematrix.jl
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ end

similar(S::SparseMatrixCSC, Tv::Type=eltype(S)) = SparseMatrixCSC(S.m, S.n, copy(S.colptr), copy(S.rowval), Array(Tv, length(S.nzval)))
similar{Tv,Ti,TvNew,TiNew}(S::SparseMatrixCSC{Tv,Ti}, ::Type{TvNew}, ::Type{TiNew}) = SparseMatrixCSC(S.m, S.n, convert(Array{TiNew},S.colptr), convert(Array{TiNew}, S.rowval), Array(TvNew, length(S.nzval)))
similar{Tv, N}(S::SparseMatrixCSC, ::Type{Tv}, d::NTuple{N, Integer}) = spzeros(Tv, d...)
similar{Tv}(S::SparseMatrixCSC, ::Type{Tv}, d::Dims) = spzeros(Tv, d...)

function convert{Tv,Ti,TvS,TiS}(::Type{SparseMatrixCSC{Tv,Ti}}, S::SparseMatrixCSC{TvS,TiS})
if Tv == TvS && Ti == TiS
Expand Down
2 changes: 1 addition & 1 deletion base/subarray.jl
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ viewindexing(I::Tuple{AbstractArray, Vararg{Any}}) = LinearSlow()
size(V::SubArray) = V.dims
length(V::SubArray) = prod(V.dims)

similar(V::SubArray, T, dims::Dims) = similar(V.parent, T, dims)
similar(V::SubArray, T::Type, dims::Dims) = similar(V.parent, T, dims)

parent(V::SubArray) = V.parent
parentindexes(V::SubArray) = V.indexes
Expand Down
99 changes: 70 additions & 29 deletions src/gf.c
Original file line number Diff line number Diff line change
Expand Up @@ -744,36 +744,40 @@ struct ambiguous_matches_env {
struct typemap_intersection_env match;
union jl_typemap_t defs;
jl_typemap_entry_t *newentry;
jl_array_t *shadowed;
int after;
};
const int eager_ambiguity_printing = 0;
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Do we now reliably pick up ambiguous methods Core? If not, would it help to turn this on during Travis builds?

static int check_ambiguous_visitor(jl_typemap_entry_t *oldentry, struct typemap_intersection_env *closure0)
{
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

if (oldentry == closure->newentry) {
closure->after = 1;
return 1;
}
union jl_typemap_t map = closure->defs;
jl_tupletype_t *type = (jl_tupletype_t*)closure->match.type;
jl_method_t *m = closure->newentry->func.method;
jl_tupletype_t *sig = oldentry->sig;
jl_value_t *isect = closure->match.ti;
if (sigs_eq(isect, (jl_value_t*)(closure->after ? sig : type), 1)) {
// we're ok if the new definition is actually the one we just
// inferred to be required (see issue #3609). ideally this would
// never happen, since if New ⊓ Old == New then we should have
// considered New more specific, but jl_args_morespecific is not
// perfect, so this is a useful fallback.
return 1;
}

// we know type ∩ sig != Union{} and
// we know !jl_args_morespecific(type, sig)
// we know !jl_args_morespecific(type, sig) [before]
// or !jl_args_morespecific(sig, type) [after]
// now we are checking that the reverse is true
if (!jl_args_morespecific((jl_value_t*)sig, (jl_value_t*)type)) {
if (sigs_eq(isect, (jl_value_t*)type, 1)) {
// we're ok if the new definition is actually the one we just
// inferred to be required (see issue #3609). ideally this would
// never happen, since if New ⊓ Old == New then we should have
// considered New more specific, but jl_args_morespecific is not
// perfect, so this is a useful fallback.
return 1;
}
if (!jl_args_morespecific((jl_value_t*)(closure->after ? type : sig),
(jl_value_t*)(closure->after ? sig : type))) {
jl_typemap_entry_t *l = jl_typemap_assoc_by_type(map, (jl_tupletype_t*)isect, NULL, 0, 0, 0);
if (l) {
// ok, intersection is covered
if (l != NULL) // ok, intersection is covered
return 1;
}
jl_method_t *mambig = oldentry->func.method;
if (m->ambig == jl_nothing) {
m->ambig = (jl_value_t*) jl_alloc_cell_1d(0);
Expand All @@ -785,13 +789,32 @@ static int check_ambiguous_visitor(jl_typemap_entry_t *oldentry, struct typemap_
}
jl_cell_1d_push((jl_array_t*) m->ambig, (jl_value_t*) mambig);
jl_cell_1d_push((jl_array_t*) mambig->ambig, (jl_value_t*) m);
if (eager_ambiguity_printing) {
JL_STREAM *s = JL_STDERR;
jl_printf(s, "WARNING: New definition \n ");
jl_static_show_func_sig(s, (jl_value_t*)type);
print_func_loc(s, m);
jl_printf(s, "\nis ambiguous with: \n ");
jl_static_show_func_sig(s, (jl_value_t*)sig);
print_func_loc(s, oldentry->func.method);
jl_printf(s, ".\nTo fix, define \n ");
jl_static_show_func_sig(s, isect);
jl_printf(s, "\nbefore the new definition.\n");
}
return 1; // there may be multiple ambiguities, keep going
}
else if (closure->after) {
// record that this method definition is being partially replaced
if (closure->shadowed == NULL) {
closure->shadowed = jl_alloc_cell_1d(0);
}
jl_cell_1d_push(closure->shadowed, oldentry->func.value);
}
return 1;
}

static void check_ambiguous_matches(union jl_typemap_t defs,
jl_typemap_entry_t *newentry)
static jl_array_t *check_ambiguous_matches(union jl_typemap_t defs,
jl_typemap_entry_t *newentry)
{
jl_tupletype_t *type = newentry->sig;
size_t l = jl_svec_len(type->parameters);
Expand All @@ -811,9 +834,12 @@ static void check_ambiguous_matches(union jl_typemap_t defs,
env.match.env = NULL;
env.defs = defs;
env.newentry = newentry;
JL_GC_PUSH2(&env.match.env, &env.match.ti);
env.shadowed = NULL;
env.after = 0;
JL_GC_PUSH3(&env.match.env, &env.match.ti, &env.shadowed);
jl_typemap_intersection_visitor(defs, 0, &env.match);
JL_GC_POP();
return env.shadowed;
}

static void method_overwrite(jl_typemap_entry_t *newentry, jl_method_t *oldvalue) {
Expand All @@ -834,7 +860,7 @@ static void method_overwrite(jl_typemap_entry_t *newentry, jl_method_t *oldvalue
}

// invalidate cached methods that overlap this definition
static void invalidate_conflicting(union jl_typemap_t *pml, jl_value_t *type, jl_value_t *parent)
static void invalidate_conflicting(union jl_typemap_t *pml, jl_value_t *type, jl_value_t *parent, jl_array_t *shadowed)
{
jl_typemap_entry_t **pl;
if (jl_typeof(pml->unknown) == (jl_value_t*)jl_typemap_level_type) {
Expand All @@ -843,15 +869,15 @@ static void invalidate_conflicting(union jl_typemap_t *pml, jl_value_t *type, jl
for(int i=0; i < jl_array_len(cache->arg1); i++) {
union jl_typemap_t *pl = &((union jl_typemap_t*)jl_array_data(cache->arg1))[i];
if (pl->unknown && pl->unknown != jl_nothing) {
invalidate_conflicting(pl, type, (jl_value_t*)cache->arg1);
invalidate_conflicting(pl, type, (jl_value_t*)cache->arg1, shadowed);
}
}
}
if (cache->targ != (void*)jl_nothing) {
for(int i=0; i < jl_array_len(cache->targ); i++) {
union jl_typemap_t *pl = &((union jl_typemap_t*)jl_array_data(cache->targ))[i];
if (pl->unknown && pl->unknown != jl_nothing) {
invalidate_conflicting(pl, type, (jl_value_t*)cache->targ);
invalidate_conflicting(pl, type, (jl_value_t*)cache->targ, shadowed);
}
}
}
Expand All @@ -862,9 +888,17 @@ static void invalidate_conflicting(union jl_typemap_t *pml, jl_value_t *type, jl
pl = &pml->leaf;
}
jl_typemap_entry_t *l = *pl;
size_t i, n = jl_array_len(shadowed);
jl_value_t **d = jl_cell_data(shadowed);
while (l != (void*)jl_nothing) {
if (jl_type_intersection(type, (jl_value_t*)l->sig) !=
(jl_value_t*)jl_bottom_type) {
int replaced = 0;
for (i = 0; i < n; i++) {
if (d[i] == (jl_value_t*)l->func.linfo->def) {
replaced = jl_type_intersection(type, (jl_value_t*)l->sig) != (jl_value_t*)jl_bottom_type;
break;
}
}
if (replaced) {
*pl = l->next;
jl_gc_wb(parent, *pl);
}
Expand Down Expand Up @@ -893,16 +927,23 @@ void jl_method_table_insert(jl_methtable_t *mt, jl_method_t *method, jl_tupletyp
jl_svec_t *tvars = method->tvars;
assert(jl_is_tuple_type(type));
JL_SIGATOMIC_BEGIN();
jl_value_t *oldvalue;
jl_value_t *oldvalue = NULL;
JL_GC_PUSH1(&oldvalue);
jl_typemap_entry_t *newentry = jl_typemap_insert(&mt->defs, (jl_value_t*)mt,
type, tvars, simpletype, jl_emptysvec, (jl_value_t*)method, 0, &method_defs, &oldvalue);
if (oldvalue) {
method->ambig = ((jl_method_t*)oldvalue)->ambig;
method_overwrite(newentry, (jl_method_t*)oldvalue);
jl_array_t *shadowed = jl_alloc_cell_1d(1);
jl_cellset(shadowed, 0, oldvalue);
oldvalue = (jl_value_t*)shadowed;
}
else
check_ambiguous_matches(mt->defs, newentry);
invalidate_conflicting(&mt->cache, (jl_value_t*)type, (jl_value_t*)mt);
else {
oldvalue = (jl_value_t*)check_ambiguous_matches(mt->defs, newentry);
}
if (oldvalue)
invalidate_conflicting(&mt->cache, (jl_value_t*)type, (jl_value_t*)mt, (jl_array_t*)oldvalue);
JL_GC_POP();
update_max_args(mt, type);
JL_SIGATOMIC_END();
}
Expand Down
18 changes: 9 additions & 9 deletions test/ambiguous.jl
Original file line number Diff line number Diff line change
Expand Up @@ -60,52 +60,52 @@ ambig(x, y::Integer) = 3

# Automatic detection of ambiguities
module Ambig1

ambig(x, y) = 1
ambig(x::Integer, y) = 2
ambig(x, y::Integer) = 3

end

ambs = detect_ambiguities(Ambig1)
@test length(ambs) == 1

module Ambig2

ambig(x, y) = 1
ambig(x::Integer, y) = 2
ambig(x, y::Integer) = 3
ambig(x::Number, y) = 4

end

ambs = detect_ambiguities(Ambig2)
@test length(ambs) == 2

module Ambig3

ambig(x, y) = 1
ambig(x::Integer, y) = 2
ambig(x, y::Integer) = 3
ambig(x::Int, y::Int) = 4

end

ambs = detect_ambiguities(Ambig3)
@test length(ambs) == 1

module Ambig4

ambig(x, y) = 1
ambig(x::Int, y) = 2
ambig(x, y::Int) = 3
ambig(x::Int, y::Int) = 4

end

ambs = detect_ambiguities(Ambig4)
@test length(ambs) == 0

module Ambig5
ambig(x::Int8, y) = 1
ambig(x::Integer, y) = 2
ambig(x, y::Int) = 3
end

ambs = detect_ambiguities(Ambig5)
@test length(ambs) == 2

# Test that Core and Base are free of ambiguities
@test isempty(detect_ambiguities(Core, Base; imported=true))

Expand Down