fix some bugs in diagonal subtyping #31833
Conversation
JeffBezanson
added
breaking
| @@ -28,11 +28,8 @@ end | |||
| @test convert(Tuple{Int, Int, Float64}, (1, 2, 3)) === (1, 2, 3.0) | |||
|
|
|||
| @test convert(Tuple{Float64, Int, UInt8}, (1.0, 2, 0x3)) === (1.0, 2, 0x3) | |||
| @test convert(NTuple, (1.0, 2, 0x3)) === (1.0, 2, 0x3) | |||
There was a problem hiding this comment.
change to NTuple{N, Real} where N or something similar?
| @test convert(Tuple{Vararg{Int}}, (1.0, 2, 0x3)) === (1, 2, 3) | ||
| @test convert(Tuple{Int, Vararg{Int}}, (1.0, 2, 0x3)) === (1, 2, 3) | ||
| @test convert(Tuple{Vararg{T}} where T<:Integer, (1.0, 2, 0x3)) === (1, 2, 0x3) |
There was a problem hiding this comment.
Good point. In fact we need a new method to handle that now.
There was a problem hiding this comment.
So, one downside to making the element type inside Vararg diagonal is that Tuple{Vararg{Integer}} doesn't match Tuple{Vararg{T}} where T. In fact I can't find a type that includes all such vararg types without also including NTuple, which you can't sensibly convert to. AFAICT, the best we can do is adding a type assertion to the conversion method to make sure the result is valid.
There was a problem hiding this comment.
Arguably, I'm not convinced the following normalization is correct:
julia> Tuple{Vararg{T where T<:S}} where S
Tuple{Vararg{S,N} where N} where S
There was a problem hiding this comment.
Fair point; I would put all the blame on the diagonal rule however --- it's too structure-based, so potentially all sorts of rearrangements get in its way.
JeffBezanson
force-pushed
the
jb/diagfix
branch
4 times, most recently
from
8960c6c to
7113fec
Compare
test/tuple.jl
Outdated
| # issue #31824 | ||
| # there is no generic way to convert an arbitrary tuple to a homogeneous tuple | ||
| @test_throws MethodError convert(NTuple, (1, 1.0)) | ||
| @test_throws MethodError convert(Tuple{Vararg{T}} where T<:Integer, (1.0, 2, 0x3)) === (1, 2, 0x3) |
There was a problem hiding this comment.
| @test_throws MethodError convert(Tuple{Vararg{T}} where T<:Integer, (1.0, 2, 0x3)) === (1, 2, 0x3) | |
| let T = Tuple{Vararg{T}} where T<:Integer, v = (1.0, 2, 0x3) | |
| @test_throws MethodError(convert, (T, v)) convert(T, v) | |
| end |
JeffBezanson
force-pushed
the
jb/diagfix
branch
from
48fe2ef to
dd079a2
Compare
|
@nanosoldier |
Ah, that's a bug. I'll fix it and re-trigger. |
|
@nanosoldier |
|
Your test job has completed - possible new issues were detected. A full report can be found here. cc @maleadt |
|
Ok, this has some legit regressions. Can wait for 1.4.1 to see if those can be avoided. |
fixes #31824, fixes #24166
This is slightly breaking, since
convert(NTuple, (1,""))used to work but now gives a MethodError. I'm convinced it's correct though, since NTuple means "homogeneous tuple".