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

WIP: controlling dispatch with varargs of defined length (rebased) #10911

Closed
wants to merge 11 commits into from

Conversation

timholy
Copy link
Member

@timholy timholy commented Apr 20, 2015

#10691 needed to be reworked in the wake of the merger of #10380. I decided the easiest approach would be to start fresh. Compared to that PR, this

  • avoids picking any special syntax. Declare such functions via foo(x::Vararg{Any,3}).
  • fixes the printing of method tables.

Aside from those, it's basically the same as #10691.

@timholy
Copy link
Member Author

timholy commented Apr 20, 2015

Passing all tests. Since this went through review once already, I'll merge soon if I don't hear anything to the contrary.

@JeffBezanson
Copy link
Member

I think I found a bug:

julia> N=TypeVar(:N,true);

julia> typeintersect(Tuple{Array{Int,N},Vararg{Int,N}}, Tuple{Vector{Int},Real,Real,Real})
Tuple{Array{Int64,1},Int64,Int64,Int64}

The new vararg types seem to have the interesting property of being "epsilon" larger than some concrete type:

julia> Tuple{Int8,Vararg{Int,2}} <: Tuple{Int8,Int,Int}
false

julia> Tuple{Int8,Int,Int} <: Tuple{Int8,Vararg{Int,2}}
true

I think we will eventually have to bite the bullet and make these type-equal. Making vararg types always abstract is kind of clever, since it sidesteps various issues, but I wonder if it will have unintended consequences.

@JeffBezanson
Copy link
Member

To get this working well (at least as well as NTuples do now) it might be easiest to reuse/replace all the existing NTuple code with this, and make NTuple a typealias like I suggested in the last PR. Just search for jl_ntuple_type.

@JeffBezanson
Copy link
Member

Also typejoin should be updated.

@timholy
Copy link
Member Author

timholy commented Apr 21, 2015

Thanks for the review!

I'll wager that both of those issues are due to the fact that I broke the symmetry of jltypes.c: intersect_tuple. For example, the first example gives Union() if you reverse the order of the arguments. I wondered if that was going to be a problem, but it seemed to always be called in order args, signature.

I confess I'm not sure I understand what you mean about the NTuple unification, but I will study it and ask again if I can't figure it out.

@JeffBezanson
Copy link
Member

I mean that since Tuple{Vararg{T,N}} is equivalent to NTuple{N,T}, it should use the exact same code paths. Then NTuple would not need to be built-in at all anymore, and could just be an alias.

@timholy
Copy link
Member Author

timholy commented Apr 21, 2015

Got it. I was concerned that since we don't allow "interior" varargs this couldn't really be what you meant, but I guess since they're already "packed" it will be OK.

@timholy
Copy link
Member Author

timholy commented Apr 21, 2015

Now I'm beginning to get a better appreciation for the kinds of things that keep you up at night. For example, what should typejoin(Tuple{Vararg{Int,2}}, Tuple{Int,Int,Int}) be? I'm guessing the correct answer is Tuple{Int,Int,Vararg{Int}}, but we can't have that until your "epsilon larger" problem is fixed---currently we'd be stuck at Tuple{Vararg{Int,2},Vararg{Int}}, which introduces Varargs in two slots. (While your bug was fixed by making intersect_tuple symmetric, the "epsilon larger" characteristic still holds.)

I suspect this needs to be fixed before we can really implement typejoin properly and make NTuple equivalent to Tuple{Vararg}. I'll start poking around jl_tuple_subtype_ and friends, presuming that's where this needs to be fixed. EDIT: and see how much the ntuple code helps!

@pao
Copy link
Member

pao commented Apr 21, 2015

Per discussion in #10925, would it be possible to PR/push 76b2b26 separately as a pure bugfix while the rest of the design gets sorted out?

@JeffBezanson
Copy link
Member

Wow, it's great that there's a separate commit for that. I didn't notice that at first. Nicely done @timholy !

This is at least enough to get the build working
The block of tests focused on subtype and intersect all pass now.
@timholy
Copy link
Member Author

timholy commented Apr 28, 2015

I'm making progress: replacing NTuple{N,T} with Tuple{Vararg{T,N}}, I've got julia building successfully and through the first couple hundred tests in core.jl. But I came across something that worries me: on current master

julia> N = TypeVar(:N,true)
N

julia> Tuple{} <: NTuple{N,Int}
true

julia> Tuple{Array{Int,2}} <: Tuple{Array{Int,N}}
true

julia> Tuple{Array{Int,2}} <: Tuple{Array{Int,N}, Vararg{Int}}
true

In the new branch the first should be translated to Tuple{} <: Tuple{Vararg{Int,N}}. So doesn't that imply that

julia> Tuple{Array{Int,0}} <: Tuple{Array{Int,N},Vararg{Int,N}}
true

but that we'd also presumably want

julia> Tuple{Array{Int,2}} <: Tuple{Array{Int,N},Vararg{Int,N}}
false

However, the whole jl_subtype_le framework doesn't seem to look into whether TypeVar constraints can be solved. I assume that's deliberate, but I don't understand why.

@timholy
Copy link
Member Author

timholy commented Apr 28, 2015

Ah, this has nothing to do with varargs/ntuples:

julia> Tuple{Array{Int,2},Array{Int,3}} <: Tuple{Array{Int,N},Array{Int,N}}
true

So I guess T1 <: T2 does not imply that typeintersect(T1,T2) != Bottom?

@toivoh
Copy link
Contributor

toivoh commented Apr 28, 2015

I guess that T1 <: T2 should imply

typeintersect(T1,T2) == T1

@timholy
Copy link
Member Author

timholy commented Apr 28, 2015

It doesn't, though. (You can check on master using my last example.) I think <: must be a "speculative" subtype relationship?

@Jutho
Copy link
Contributor

Jutho commented Apr 28, 2015

Maybe (hopefully) this will change with #8974 , when it will become easier to create such types as

@UnionAll N Tuple{Array{Int,N},Array{Int,N}}

?

@timholy timholy changed the title Controlling dispatch with varargs of defined length (rebased) WIP: controlling dispatch with varargs of defined length (rebased) Apr 28, 2015
@timholy
Copy link
Member Author

timholy commented Apr 28, 2015

OK, now this is getting much further: 1955 lines into test/core.jl (72% of the file) before crashing, so I pushed my WIP. There's a lot of code that I've just commented out rather than deleting (since I still find it useful to refer to it), but I'll delete before this becomes final. As you can see, the tuple type intersection rules are quite a bit more complicated now (which isn't surprising), but eventually we will get to delete the NTuple code.

I vote we rename this PR "tupoca:lips:"

@carnaval
Copy link
Contributor

Most computations on the type lattice are only required to overapproximate the result for correctness. That is <: must only answer true when it is guaranteed and typeintersect must only answer an upper bound for the actual meet.

The failure mode for inexactness should only be spurious ambiguity warnings where you would be forced to implement the dispatch rules by hand because the method table can't figure it out by itself (and sub-optimal type inference).

Typevars are another business however and I'm not sure they are handled everywhere as one would expect.

@timholy
Copy link
Member Author

timholy commented Apr 28, 2015

The difference re typevars is simply that jl_type_intersect takes the cenv_t typevar constraints as arguments, and jl_subtype_le does not.

@JeffBezanson
Copy link
Member

In the future, the only reason for the typevar arguments to jl_type_intersect will be to request values for particular variables, i.e. compute the intersection plus tell me what constraints it implies about the specified variables.

@timholy
Copy link
Member Author

timholy commented Apr 28, 2015

Meaning, you'd rather see the intersection code return something that's not jl_bottom, but also insert incompatible constraints into eqc?

@JeffBezanson
Copy link
Member

No, no, if intersection knows there are incompatible constraints, it should absolutely return Bottom. The returned environment in that case should generally not be used. In inexact cases the returned environment will have overly broad constraints like T=T.

This uses a more systematic approach, and should be more maintainable.
Also adds lots of documentation about the strategy.
@timholy
Copy link
Member Author

timholy commented May 4, 2015

@JeffBezanson, I'm finding some differences in subtyping behavior with NTuples and Tuple{Vararg}s. Since the plan is to make the latter a typealias for the former, we have to resolve the differences. Do you have strong feelings about which of the following should be dropped? (I've written these as tests that document current behavior on master---all of these test pass as of 3f2ed27.) If not, I'll just pick whatever seems reasonable and see how much stuff breaks 😄.

let N = TypeVar(:N,true), V = TypeVar(:V, NTuple{N,Int}), T = TypeVar(:T,true)
    @test Tuple{} <: NTuple
    @test Tuple{} <: NTuple{N,Int}
    @test Tuple{} <: NTuple{N,T}
    @test !(Array{Tuple{}} <: Array{NTuple})    # is the difference...
    @test Array{Tuple{}} <: Array{NTuple{N}}    # ...here desirable?
    @test Array{Tuple{}} <: Array{NTuple{N,Int}}
    @test Array{Tuple{}} <: Array{NTuple{N,T}}
    @test Array{Tuple{Int}} <: Array{NTuple{N,Int}}
    @test Array{Tuple{}} <: Array{NTuple{TypeVar(:N),Int}}    # these two...
    @test !(Array{Tuple{Int}} <: Array{Tuple{Vararg{Int}}})   # ...conflict
    @test !(Array{Tuple{Int}} <: Array{Tuple{Int,Vararg{Int}}})
    @test Tuple{Type{Int8}, Tuple{}} <: Tuple{Type{Int8}, NTuple{N,Int}}
    @test Tuple{Type{Int8}, Tuple{Int,Int}} <: Tuple{Type{Int8}, NTuple{N,Int}}
    @test V <: NTuple             # and...
    @test !(V <: Tuple{Vararg})   # ...here
end

If it helps, I can pull out the lines of source that causes these differences; the last one (the V::TypeVar case), I remember correctly, is here (after passing through the a::TypeVar portion), in that it doesn't check what might be a reasonable alternative.

(Note: small edits applied to the above after initial posting.)

@timholy
Copy link
Member Author

timholy commented May 10, 2015

I may have this about ready (it's not pushed yet, just on my own machine). But I'm running into a snafu I should have anticipated much earlier. I'd like to make the typealias look like this:

#typealias NTuple{N,T} Tuple{Vararg{T,N}}
const NTuple = Tuple{Vararg{Any}}
const NTuple{N} = Tuple{Vararg{Any,N}}
const NTuple{N,T} = Tuple{Vararg{T,N}}

but of course this doesn't work.

Unfortunately, the commented-out typealias (which is permissible syntax) causes NTuple to expand to Tuple{Vararg{T,N}} rather than Tuple{Vararg{Any,N}}, which obviously causes invariance problems for things like

@test !(issubtype(Array{Tuple{Vararg{Int}}},Array{NTuple}))

EDIT: hmm, maybe that test simply has to go. After all, we have

julia> Array{Tuple{Int}} <: Array{NTuple}
true

so NTuple is indeed defaulting to NTuple{N,T} rather than NTuple{N,Any}. It seems to be a specific test for behavior that I've come to regard as dubious.

A secondary point is that I wonder if there are any negatives to having this typealias in Base, whereas previously NTuple was defined in Core.

@timholy
Copy link
Member Author

timholy commented May 13, 2015

Superseded by #11242

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants