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

missing sub2ind ambiguity detection confuses inference #18307

Closed
vtjnash opened this issue Aug 31, 2016 · 5 comments
Closed

missing sub2ind ambiguity detection confuses inference #18307

vtjnash opened this issue Aug 31, 2016 · 5 comments
Labels
help wanted Indicates that a maintainer wants help on an issue or pull request needs tests Unit tests are required for this change types and dispatch Types, subtyping and method dispatch
Milestone

Comments

@vtjnash
Copy link
Member

vtjnash commented Aug 31, 2016

came across this while investigating why the inference of isassigned is bad. it seems the sub2ind definitions cause several problems for type intersection. for example:

julia> @which sub2ind((1,))
ERROR: no method found for the specified argument types
 in which(::Any, ::Any) at ./reflection.jl:466

julia> sub2ind((1,)) # supposed to throw a MethodError (ambiguity)
1

this is caused by the intersection of the following 3 methods:

sub2ind{N,T<:Integer}(inds:: Union{Base.Indices{N}, Dims{N}}, I::AbstractArray{T,1}...) at abstractarray.jl:1482
sub2ind(::Base.DimsInteger) at abstractarray.jl:1429
sub2ind(dims::Base.DimsInteger, I::Integer...) at abstractarray.jl:1433

testing with:

julia> g{N,T<:Integer}(inds::Dims{N}, I::AbstractArray{T,1}...) = 1
julia> g(::Base.DimsInteger) = 2
julia> g(dims::Base.DimsInteger, I::Integer...) = 3

with all 3, @which thinks none will be called, but actually 2 gets (incorrectly) called
with just method 1 and 2, @which thinks that 1 will definitely be called (but 2 is actually – correctly – called)
with just 1 and 3, it fails correctly
with just 2 and 3, it works correctly

@vtjnash vtjnash added bug Indicates an unexpected problem or unintended behavior backport pending 0.5 labels Aug 31, 2016
@tkelman tkelman added this to the 0.5.x milestone Aug 31, 2016
@tkelman
Copy link
Contributor

tkelman commented Aug 31, 2016

use milestones, not backport pending label if there isn't a PR yet

@yuyichao
Copy link
Contributor

yuyichao commented Sep 3, 2016

Another missing ambiguity from #17003 that might be related

julia> abstract A{T}

julia> type B{T} <: A{T}
       end

julia> f1(::A{Int}) = 1
f1 (generic function with 1 method)

julia> f1(::B) = 2
f1 (generic function with 2 methods)

julia> f1(B{Int}())
2

Here B isn't more specific than A{Int} even though it is more specific than A.

timholy added a commit that referenced this issue Sep 4, 2016
Also adds a broken test for #18307
timholy added a commit that referenced this issue Sep 5, 2016
Also adds a broken test for #18307
timholy added a commit that referenced this issue Sep 5, 2016
Also adds a broken test for #18307
vtjnash added a commit that referenced this issue Sep 13, 2016
This reverts commit eb57536.

This is a workaround for failed ambiguity detection in #18307.
vtjnash added a commit that referenced this issue Sep 13, 2016
This reverts commit eb57536.

This is a workaround for failed ambiguity detection in #18307.
vtjnash added a commit that referenced this issue Sep 14, 2016
This reverts commit eb57536.

This is a workaround for failed ambiguity detection in #18307.
@vtjnash
Copy link
Member Author

vtjnash commented Sep 15, 2016

probably will get fixed by #18457?

tkelman pushed a commit that referenced this issue Sep 16, 2016
Also adds a broken test for #18307

(cherry picked from commit e242d46)
ref #18352
vtjnash added a commit that referenced this issue Sep 21, 2016
This reverts commit eb57536.

This is a workaround for failed ambiguity detection in #18307.
@StefanKarpinski StefanKarpinski added help wanted Indicates that a maintainer wants help on an issue or pull request and removed help wanted Indicates that a maintainer wants help on an issue or pull request labels Oct 27, 2016
@pabloferz
Copy link
Contributor

The OP example seems to be working now.

@tkelman tkelman added the needs tests Unit tests are required for this change label Feb 7, 2017
@vtjnash vtjnash mentioned this issue Feb 27, 2017
53 tasks
@mbauman mbauman removed the bug Indicates an unexpected problem or unintended behavior label Aug 30, 2017
@JeffBezanson JeffBezanson added the types and dispatch Types, subtyping and method dispatch label Jan 15, 2018
@JeffBezanson
Copy link
Member

This has a test in test/ambiguous that claims the methods should be ambiguous, but aren't currently. I'm not sure they really are ambiguous though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Indicates that a maintainer wants help on an issue or pull request needs tests Unit tests are required for this change types and dispatch Types, subtyping and method dispatch
Projects
None yet
Development

No branches or pull requests

7 participants