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

(::Type)(args...) constructors with Union args mess up method sorting #30794

Closed
timholy opened this issue Jan 22, 2019 · 5 comments
Closed

(::Type)(args...) constructors with Union args mess up method sorting #30794

timholy opened this issue Jan 22, 2019 · 5 comments
Labels
regression Regression in behavior compared to a previous version types and dispatch Types, subtyping and method dispatch

Comments

@timholy
Copy link
Member

timholy commented Jan 22, 2019

Defining a method (::Type)(x::U, ...) where U is a Union seems to mess up method sorting, in a manner that's dependent upon precompilation:

module Demo

struct OffsetArray{T,N,AA<:AbstractArray} <: AbstractArray{T,N}
    parent::AA
    offsets::NTuple{N,Int}
end

OffsetArray(A::AbstractArray{T,N}, offsets::Vararg{Int,N}) where {T,N} =
    OffsetArray(A, offsets)

const ArrayInitializer = Union{UndefInitializer, Missing, Nothing}
OffsetArray{T,N}(init::ArrayInitializer, inds::Vararg{AbstractUnitRange,N}) where {T,N} = OffsetArray{T,N}(init, inds)

OffsetArray(A::AbstractArray{T,0}) where {T} = OffsetArray{T,0,typeof(A)}(A, ())

OffsetArray(A::AbstractArray{T,N}, axs::Vararg{AbstractUnitRange,N}) where {T,N} =
    OffsetArray(A, map(first, axs))

end

Session:

julia> push!(LOAD_PATH, "/tmp/pkgs")
4-element Array{String,1}:
 "@"        
 "@v#.#"    
 "@stdlib"  
 "/tmp/pkgs"

julia> using Demo
[ Info: Recompiling stale cache file /home/tim/.julia/compiled/v1.1/Demo.ji for Demo [top-level]

julia> methods(Demo.OffsetArray)
# 3 methods for generic function "(::Type)":
[1] (::Type{Demo.OffsetArray})(A::AbstractArray{T,N}, offsets::Vararg{Int64,N}) where {T, N} in Demo at /tmp/pkgs/Demo.jl:8
[2] (::Type{Demo.OffsetArray})(A::AbstractArray{T,N}, axs::Vararg{AbstractUnitRange,N}) where {T, N} in Demo at /tmp/pkgs/Demo.jl:16
[3] (::Type{Demo.OffsetArray})(A::AbstractArray{T,0}) where T in Demo at /tmp/pkgs/Demo.jl:14

Note that the ::AbstractArray{T,0} method is of too-low specificity. There are lots of things you can do to fix this:

  • move that method prior to the ArrayInitializer method in the source file
  • change ArrayInitializer to a concrete type, e.g., Missing.

This appears to be the root cause of timholy/Revise.jl#234.

@c42f
Copy link
Member

c42f commented Jan 24, 2019

Could this be an unintended side effect of #30095 (see #29923) ?

@timholy timholy added the regression Regression in behavior compared to a previous version label Jan 31, 2019
@timholy
Copy link
Member Author

timholy commented Jan 31, 2019

git bisect blames #30160; I have verified this directly by comparing it with the previous commit. CC @JeffBezanson (sorry).

@JeffBezanson
Copy link
Member

No need to apologize :) We can expect bugs like this to appear and disappear as we mess with specificity, since it is still not a well-behaved relation.

@ajkeller34
Copy link
Contributor

ajkeller34 commented Apr 15, 2019

Parsers.jl, used by CSV.jl, seems to define several methods of this variety which are causing problems in other packages. As an example, some incorrect output:

julia> using UniqueVectors, Parsers

julia> @which UniqueVector(["aaa", "bbb"])
(::Type{UniqueVector})(items) in UniqueVectors at /Users/andrew.keller/.julia/dev/UniqueVectors/src/UniqueVectors.jl:32

The output should actually be:

julia> using UniqueVectors

julia> @which UniqueVector(["aaa", "bbb"])
(::Type{UniqueVector})(items::Array{T,1}) where T in UniqueVectors at /Users/andrew.keller/.julia/dev/UniqueVectors/src/UniqueVectors.jl:30

I can work around the issue for several of the methods in Parsers.jl but there was a method or two that was not functionally equivalent after my fix. This issue in particular is actually worse than it looks because the incorrect method being called here leads to a stack overflow which sometimes is not caught, resulting in Julia crashing...

My version info:

julia> versioninfo()
Julia Version 1.1.0
Commit 80516ca202 (2019-01-21 21:24 UTC)
Platform Info:
  OS: macOS (x86_64-apple-darwin14.5.0)
  CPU: Intel(R) Core(TM) i7-7820HQ CPU @ 2.90GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-6.0.1 (ORCJIT, skylake)

@vtjnash
Copy link
Member

vtjnash commented Jul 8, 2019

works now:

julia> methods(Demo.OffsetArray)
# 3 methods for type constructor:
[1] (::Type{Main.Demo.OffsetArray})(A::AbstractArray{T,0}) where T in Main.Demo at REPL[2]:14
[2] (::Type{Main.Demo.OffsetArray})(A::AbstractArray{T,N}, offsets::Vararg{Int64,N}) where {T, N} in Main.Demo at REPL[2]:8
[3] (::Type{Main.Demo.OffsetArray})(A::AbstractArray{T,N}, axs::Vararg{AbstractUnitRange,N}) where {T, N} in Main.Demo at REPL[2]:16

I'm updating the method sorter logic soon, and will add a test then that'll cover this case.

@vtjnash vtjnash closed this as completed Jul 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression Regression in behavior compared to a previous version types and dispatch Types, subtyping and method dispatch
Projects
None yet
Development

No branches or pull requests

5 participants