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

confusing dispatch when method parameter bounds are wider than a type parameter's declared bounds #6383

Closed
carlobaldassi opened this issue Apr 2, 2014 · 9 comments
Assignees
Labels
bug Indicates an unexpected problem or unintended behavior types and dispatch Types, subtyping and method dispatch

Comments

@carlobaldassi
Copy link
Member

See a85476b and 005197a and comments therein.

In summary, dispatch on a parametric type whose type is restricted in the declaration (type A{T<:Integer} etc.) make a difference between A, A{T} and A{T<:Integer}. In one case, A{T} seems to be taken as more general than A; in another case, A seems to be taken as more general than A{T<:Integer} - see examples below.

Example: this shows the ambiguity

julia> type A{T<:Integer} <: Real
           a::T
       end

julia> type B <: FloatingPoint
       end

julia> tst{T<:FloatingPoint,S}(::Type{T}, x::A{S}) = println(1)
tst (generic function with 1 method)

julia> tst(::Type{B}, x::A) = println(2) # this should disambiguate between the one above and the one below
tst (generic function with 2 methods)

julia> tst(::Type{B}, x::Real) = println(2)
Warning: New definition 
    tst(Type{B},Real) at none:1
is ambiguous with: 
    tst(Type{T<:FloatingPoint},A{S}) at none:1.
To fix, define 
    tst(Type{B},A{S})
before the new definition.
tst (generic function with 3 methods)

julia> tst(B, A(1)) # works
2

Currently, the workarounds for the example above are: defining the disambiguation method with an otherwise useless parameter

julia> type A{T<:Integer} <: Real
           a::T
       end


julia> type B <: FloatingPoint
       end

julia> tst{T<:FloatingPoint,S}(::Type{T}, x::A{S}) = println(1)
tst (generic function with 1 method)

julia> tst{S}(::Type{B}, x::A{S}) = println(2) # useless parameter S
tst (generic function with 2 methods)

julia> tst(::Type{B}, x::Real) = println(2) # no warning though
tst (generic function with 3 methods)

julia> tst(B, A(1))
2

Or restricting the first method to S<:Integer:

julia> type A{T<:Integer} <: Real
           a::T
       end

julia> type B <: FloatingPoint
       end

julia> tst{T<:FloatingPoint,S<:Integer}(::Type{T}, x::A{S}) = println(1) # the <:Integer part should be implicit
tst (generic function with 1 method)

julia> tst(::Type{B}, x::A) = println(2)
tst (generic function with 2 methods)

julia> tst(::Type{B}, x::Real) = println(2)
tst (generic function with 3 methods)

julia> tst(B, A(1))
2

One more problem: from the last example, continue like this:

julia> tst(::Type{FloatingPoint}, x::A) = println(3) # this should be more specific then the one which prints 1
tst (generic function with 4 methods)

julia> tst(FloatingPoint, A(1)) # calls the wrong version
1

Again, to fix it restrict the type parameter in the above definition:

julia> tst{S<:Integer}(::Type{FloatingPoint}, x::A{S}) = println(3)
tst (generic function with 4 methods)

julia> tst(FloatingPoint, A(1))
3
@carlobaldassi
Copy link
Member Author

Note: changes which can be reverted when this is fixed:

  • part of 005197a (the change in rational.jl)
  • part of 0715b88 (the change in linalg/ldlt.jl)

@mbauman
Copy link
Member

mbauman commented Feb 19, 2015

This seems more serious than just a spurious ambiguity warning. It can manifest itself as a method sorting bug. Here's a simpler example:

julia> type Foo{A<:Real}; end

julia> f{A}(x::Foo{A}) = 1
f (generic function with 1 method)

julia> f(x::Foo, y...) = 2
f (generic function with 2 methods)

julia> f(Foo{Int}())
2

If Foo is changed such that A is no longer restricted to be <: Real, then dispatch works as expected.

@vtjnash
Copy link
Member

vtjnash commented Dec 22, 2016

Should be taken care of by jb/subtypes. But not as much of an issue now that we don't print ambiguity warnings early

@JeffBezanson JeffBezanson changed the title Spurious ambiguity warning (type parameter constraint not taken into account by dispatch) confusing dispatch when method parameter bounds are wider than a type's declared bounds Sep 22, 2017
@JeffBezanson JeffBezanson changed the title confusing dispatch when method parameter bounds are wider than a type's declared bounds confusing dispatch when method parameter bounds are wider than a type parameter's declared bounds Sep 22, 2017
@iamed2
Copy link
Contributor

iamed2 commented Sep 22, 2017

My comment from #23823:

julia> struct Foo{A<:Real} end

julia> f(x::Foo{A}) where A = 1
f (generic function with 1 method)

julia> f(x::Foo, y...) = 2
f (generic function with 2 methods)

julia> f(Foo{Int}())
2

The second method's 1-arg form appears (unless you're an Expert) as if it is less specific than the first. However, Foo is treated as Foo{T} where T<:Real, while the compiler puts a type bound of Any on A.

I think it would make sense if Foo{A} where A was treated as Foo{A} where A<:Real, at least in method definitions.

@timholy
Copy link
Member

timholy commented Jan 2, 2018

Easy test cases in #25361.

@timholy
Copy link
Member

timholy commented Dec 19, 2018

Still an issue:

abstract type StaticArray{S <: Tuple, T, N} <: AbstractArray{T, N} end

# Like `getindex` but with short method tables                                                                                                                                                                                                                                 
gtindex(a::StaticArray{<:Any,<:Any,N}, inds::Vararg{Int,N}) where N = 1
gtindex(a::StaticArray, inds::Union{Int, StaticArray{<:Any, Int}, Colon}...) = 2

julia> methods(gtindex)
# 2 methods for generic function "gtindex":
[1] gtindex(a::StaticArray, inds::Union{Colon, Int64, StaticArray{#s4,Int64,N} where N where #s4}...) in Main at /tmp/tim/dispatch.jl:5
[2] gtindex(a::StaticArray{#s3,#s4,N} where #s4 where #s3, inds::Vararg{Int64,N}) where N in Main at /tmp/tim/dispatch.jl:4

This is wrong, since the method returning 1 is much more specific.

If I change the first to

gtindex(a::StaticArray{<:Tuple,<:Any,N}, inds::Vararg{Int,N}) where N = 1

then the sorting is correct.

timholy added a commit to JuliaArrays/StaticArrays.jl that referenced this issue Dec 19, 2018
@bgroenks96
Copy link

Is there any progress on this? If there is no fix in the pipeline, we should at least add a section in the documentation for "Types" to make this quirk explicitly known. As a computer scientist and fairly recent newcomer to Julia, this issue was very confusing and surprising to discover by accident...

@bgroenks96
Copy link

This does relate to a similar aesthetic issue I have noticed, namely that the current requirements for inner constructors create redundancy in many, if not most, scenarios:

struct Foo{A<:AbstractArray}
    data::A
    Foo(data::A) where {A<:AbstractArray} = new{A}(data)
end

Now of course, given that this bug still exists, we actually have to remove the bound from Foo's generic type argument to avoid erroneous dispatch. This inadvertently solves the redundancy issue but comes at the cost of code clarity (in my opinion, at least) because one cannot tell from the signature of Foo what the bounds are on the type arguments. You have to read the constructor which is sometimes painful depending on how complex the type is.

This begs the question: why aren't type arguments visible to inner-constructors? I think I saw this discussed somewhere else at some point, and I assume there is some technical limitation, but it would be nice if the compiler could, by default, assume that the type constraints on Foo are the same for inner constructors and copy-paste them for you. You would still need to be able to specify independent constraints, of course, since the inner constructor signature could differ from new.

@brenhinkeller
Copy link
Contributor

Hmm, I actually can't reproduce the original example with current (1.8.3) syntax

julia> struct A{T<:Integer} <: Real
           a::T
       end

julia> struct B <: AbstractFloat
       end

julia> tst(::Type{T}, x::A{S}) where {T<:AbstractFloat,S} = println(1)
tst (generic function with 1 method)

julia> tst(::Type{B}, x::A) = println(2) # this should disambiguate between the one above and the one below
tst (generic function with 2 methods)

julia> tst(::Type{B}, x::Real) = println(3)
tst (generic function with 3 methods)

julia> tst(Float64, A(1)) # Should print 1
1

julia> tst(B, A(1)) # Should print 2
2

julia> tst(B, 1.0) # Should print 3
3

I won't close the issue just yet in case I am misunderstanding, but this all looks like it's working properly now as far as I can tell

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior types and dispatch Types, subtyping and method dispatch
Projects
None yet
Development

No branches or pull requests

9 participants