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: Improve tuple subtyping in invariant position #18451

Closed
wants to merge 1 commit into from

Conversation

TotalVerb
Copy link
Contributor

@TotalVerb TotalVerb commented Sep 12, 2016

When computing tuple subtype in invariant position, fail early when encountering an unbound typevar. As far as I can tell, this added logic is similar to that for jl_subtype_le for DataType above. (Perhaps this logic should be put into a helper function.)

Fix #18450.

Currently not working; it seems to have broken Array{Tuple} <: Array{NTuple}.


# issue #18450
f18450() = ifelse(true, Tuple{Vararg{Int}}, Tuple{Vararg})
@test f18450() = Tuple{Vararg{Int}}
Copy link
Contributor

@yuyichao yuyichao Sep 12, 2016

Choose a reason for hiding this comment

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

==

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. I should run tests locally first.

@TotalVerb TotalVerb changed the title Improve tuple subtyping in invariant position WIP: Improve tuple subtyping in invariant position Sep 12, 2016
@TotalVerb
Copy link
Contributor Author

Well, this is awkward. The broken test (core.jl) is hard to fix because it seems to be testing precisely the behaviour that I thought was buggy. Indeed, currently

julia> Array{Tuple} <: Array{NTuple}  # tested in core.jl
true

julia> Array{NTuple} <: Array{Tuple}
false

which is a little weird. Should I just remove the test?

@TotalVerb
Copy link
Contributor Author

TotalVerb commented Sep 12, 2016

The problem here, I suppose, is that since Tuple and NTuple are type equal (both are subtypes of the other), then we would like Array{Tuple} and Array{NTuple} to be type equal. But the same argument does not apply to Type{Tuple} and Type{NTuple}, which might be better kept distinct, or otherwise Type's singleton nature is violated.

As far as I can tell, the only solutions to this are:

  • Loosen the singleton nature of Type, so that isa(Tuple, Type{NTuple}) (as is the case currently) and isa(NTuple, Type{Tuple}) (as is not currently the case). This will probably require substantial changes to inference to avoid problems like Vararg.hastypevars is false, but issubtype behaves otherwise #18450 .
  • Give up the current typeseq-preserving invariance of Array and other DataTypes in favour of an stronger form of invariance based off type identity and not type equality. This is effectively the change being made in this PR, and would require deleting the two failing tests in core.jl.
  • Handle Type separately from other DataTypes; ensure that !isa(Tuple, Type{NTuple}) but also that isa(Vector{NTuple}(), Array{Tuple}), both of which are not yet the case.

@timholy
Copy link
Member

timholy commented Sep 12, 2016

You've probably followed the breadcrumbs here, but this seems to originate in the fix for #6561. What puzzles me is that the resulting behavior seems to contradict the consensus of the discussion.

CC @JeffBezanson.

@JeffBezanson
Copy link
Member

I would not bother working on this now, as I'm actively working on #8974 which will replace most of this code and provide a lot of clarity on these kinds of issues. I think with the new algorithm NTuple will be a strict subtype of Tuple, since a tuple can only match Tuple{Vararg{T}} if all elements are the same type.

@StefanKarpinski
Copy link
Member

Should we close this as superseded then?

@StefanKarpinski StefanKarpinski added this to the 0.6.0 milestone Sep 12, 2016
@timholy
Copy link
Member

timholy commented Sep 12, 2016

Nice, Jeff.

Since the new type system won't be backported to 0.5, presumably the issue of "is this worth working on?" depends on whether @TotalVerb needs this for some package to be used with Julia 0.5. I fixed at least one type-system bug precisely for this reason (#18355).

But I'd say deleting a test to fix the behavior is breaking and hence can't go into 0.5.x (unless it's a true "emergency," but I don't think that applies).

@TotalVerb
Copy link
Contributor Author

TotalVerb commented Sep 12, 2016

I don't need this for v0.5. It would be nice (though not an "emergency") if we could work around the inference bug in the meantime, though. Maybe we could have an inference special case for Vararg until #18457 is merged? I'll see if I can come up with a PR.

@TotalVerb TotalVerb closed this Sep 12, 2016
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.

5 participants