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

RFC: print all types using equivalent exported aliases #36107

Merged
merged 1 commit into from
Jul 2, 2020

Conversation

vtjnash
Copy link
Sponsor Member

@vtjnash vtjnash commented Jun 1, 2020

Given triage's interest in #36032, this works to generalize those printing improvements to all types. While type-intersection failures can be rather unexpected (i.e. handling difficult cases, but not simplifications of them), it often does much better than master anyways:

julia> T = Union{Nothing, StridedVector{S}} where S
Union{Nothing, StridedArray{S,1}} where S

julia> T = Union{Nothing, StridedVecOrMat{S}} where S<:Integer
Union{Nothing, StridedMatrix{S}, StridedVector{S}} where S<:Integer

julia> T = Union{Nothing, StridedVecOrMat{S}, S} where S<:Integer
Union{Nothing, S, StridedMatrix{S}, StridedVector{S}} where S<:Integer

julia> StridedVecOrMat
StridedVecOrMat{T} where T = Union{DenseArray{T,1}, DenseArray{T,2}, Base.ReinterpretArray{T,1,S,A} where S where A<:Union{SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray}, Base.ReinterpretArray{T,2,S,A} where S where A<:Union{SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray}, Base.ReshapedArray{T,1,A,MI} where MI<:Tuple{Vararg{Base.MultiplicativeInverses.SignedMultiplicativeInverse{Int64},N} where N} where A<:Union{Base.ReinterpretArray{T,N,S,A} where S where A<:Union{SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray} where N where T, SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray}, Base.ReshapedArray{T,2,A,MI} where MI<:Tuple{Vararg{Base.MultiplicativeInverses.SignedMultiplicativeInverse{Int64},N} where N} where A<:Union{Base.ReinterpretArray{T,N,S,A} where S where A<:Union{SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray} where N where T, SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray}, SubArray{T,1,A,I,L} where L where I<:Tuple{Vararg{Union{Int64, AbstractRange{Int64}, Base.AbstractCartesianIndex, Base.ReshapedArray{T,N,A,Tuple{}} where A<:AbstractUnitRange where N where T},N} where N} where A<:Union{Base.ReinterpretArray{T,N,S,A} where S where A<:Union{SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray} where N where T, Base.ReshapedArray{T,N,A,MI} where MI<:Tuple{Vararg{Base.MultiplicativeInverses.SignedMultiplicativeInverse{Int64},N} where N} where A<:Union{Base.ReinterpretArray{T,N,S,A} where S where A<:Union{SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray} where N where T, SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray} where N where T, DenseArray}, SubArray{T,2,A,I,L} where L where I<:Tuple{Vararg{Union{Int64, AbstractRange{Int64}, Base.AbstractCartesianIndex, Base.ReshapedArray{T,N,A,Tuple{}} where A<:AbstractUnitRange where N where T},N} where N} where A<:Union{Base.ReinterpretArray{T,N,S,A} where S where A<:Union{SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray} where N where T, Base.ReshapedArray{T,N,A,MI} where MI<:Tuple{Vararg{Base.MultiplicativeInverses.SignedMultiplicativeInverse{Int64},N} where N} where A<:Union{Base.ReinterpretArray{T,N,S,A} where S where A<:Union{SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray} where N where T, SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray} where N where T, DenseArray}} where T

Note: all relevant changes are in base/show.jl

first = true
for typ in uniontypes(x)
if !isa(typ, TypeVar) && rewrap_unionall(typ, properx) <: applied
continue
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

We should avoid changing the print order of the union elements.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

Hm, that'd require some additional complexity, since in full generality it's not possible. Is it that bad to split it into two independently partially-sorted sets in this way?

base/show.jl Outdated Show resolved Hide resolved
@JeffBezanson JeffBezanson added the display and printing Aesthetics and correctness of printed representations of objects. label Jun 1, 2020
@vtjnash vtjnash added the triage This should be discussed on a triage call label Jun 15, 2020
@JeffBezanson
Copy link
Sponsor Member

Why is there both make_typealias and make_typealiases? Could they share some code?

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Jun 18, 2020

I thought it might harm readability to attempt to reduce their duplication any more. One uses subtyping while one uses intersection, so the later is a bit more general while the former is a bit more straightforward.

@JeffBezanson
Copy link
Sponsor Member

From triage: will fix the doctests in Statistics, then add a commit here to bump the version of Statistics used.

@JeffBezanson JeffBezanson removed the triage This should be discussed on a triage call label Jun 18, 2020
For example, this prints Array{T,1} as Vector{T} and Array{T,2} as
Matrix{T} and Base.GenericIOBuffer{Array{UInt8,1}} as IOBuffer,
and generalizes some existing code.

Co-authored-by: Kristoffer Carlsson <kristoffer.carlsson@chalmers.se>
@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Jul 1, 2020

Since this tends to get stale pretty fast, I'm hoping to merge it as soon as CI is green! (be aware though that other PR doctests may need to be updated/rerun for the next week or two when merging, thx)

@vtjnash vtjnash merged commit ab05173 into master Jul 2, 2020
@vtjnash vtjnash deleted the jn/show-typealiases branch July 2, 2020 01:52
@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Jul 2, 2020

Let's see how this goes! As we discover and collect feedback, we can further refine the concept (and possibly the implementation).

@StefanKarpinski
Copy link
Sponsor Member

Very exciting!

@kimikage
Copy link
Contributor

kimikage commented Jul 4, 2020

Wow! 😄

julia> using ColorTypes, FixedPointNumbers

julia> RGB{Fractional} # == RGB{Union{AbstractFloat, FixedPoint}}
RGB{Union{AbstractFloat, FixedPoint, N0f16, N0f32, N0f64, N0f8, N10f22, N10f54, N10f6, N11f21, N11f5, N11f53, N12f20, N12f4, N12f52, N13f19, N13f3, N13f51, N14f18, N14f2, N14f50, N15f1, N15f17, N15f49, N16f16, N16f48, N17f15, N17f47, N18f14, N18f46, N19f13, N19f45, N1f15, N1f31, N1f63, N1f7, N20f12, N20f44, N21f11, N21f43, N22f10, N22f42, N23f41, N23f9, N24f40, N24f8, N25f39, N25f7, N26f38, N26f6, N27f37, N27f5, N28f36, N28f4, N29f3, N29f35, N2f14, N2f30, N2f6, N2f62, N30f2, N30f34, N31f1, N31f33, N32f32, N33f31, N34f30, N35f29, N36f28, N37f27, N38f26, N39f25, N3f13, N3f29, N3f5, N3f61, N40f24, N41f23, N42f22, N43f21, N44f20, N45f19, N46f18, N47f17, N48f16, N49f15, N4f12, N4f28, N4f4, N4f60, N50f14, N51f13, N52f12, N53f11, N54f10, N55f9, N56f8, N57f7, N58f6, N59f5, N5f11, N5f27, N5f3, N5f59, N60f4, N61f3, N62f2, N63f1, N6f10, N6f2, N6f26, N6f58, N7f1, N7f25, N7f57, N7f9, N8f24, N8f56, N8f8, N9f23, N9f55, N9f7, Q0f15, Q0f31, Q0f63, Q0f7, Q10f21, Q10f5, Q10f53, Q11f20, Q11f4, Q11f52, Q12f19, Q12f3, Q12f51, Q13f18, Q13f2, Q13f50, Q14f1, Q14f17, Q14f49, Q15f0, Q15f16, Q15f48, Q16f15, Q16f47, Q17f14, Q17f46, Q18f13, Q18f45, Q19f12, Q19f44, Q1f14, Q1f30, Q1f6, Q1f62, Q20f11, Q20f43, Q21f10, Q21f42, Q22f41, Q22f9, Q23f40, Q23f8, Q24f39, Q24f7, Q25f38, Q25f6, Q26f37, Q26f5, Q27f36, Q27f4, Q28f3, Q28f35, Q29f2, Q29f34, Q2f13, Q2f29, Q2f5, Q2f61, Q30f1, Q30f33, Q31f0, Q31f32, Q32f31, Q33f30, Q34f29, Q35f28, Q36f27, Q37f26, Q38f25, Q39f24, Q3f12, Q3f28, Q3f4, Q3f60, Q40f23, Q41f22, Q42f21, Q43f20, Q44f19, Q45f18, Q46f17, Q47f16, Q48f15, Q49f14, Q4f11, Q4f27, Q4f3, Q4f59, Q50f13, Q51f12, Q52f11, Q53f10, Q54f9, Q55f8, Q56f7, Q57f6, Q58f5, Q59f4, Q5f10, Q5f2, Q5f26, Q5f58, Q60f3, Q61f2, Q62f1, Q63f0, Q6f1, Q6f25, Q6f57, Q6f9, Q7f0, Q7f24, Q7f56, Q7f8, Q8f23, Q8f55, Q8f7, Q9f22, Q9f54, Q9f6}}

This is an edge case and is not a problem in practice.
However, I think this affects the practice of making decisions of alias exporting.

omus added a commit to JuliaTime/TimeZones.jl that referenced this pull request Jul 10, 2020
* Revise tests to work with printing type aliases

Introduced in JuliaLang/julia#36107

* Address doctest failures with alias printing
simeonschaub pushed a commit to simeonschaub/julia that referenced this pull request Aug 11, 2020
For example, this prints Array{T,1} as Vector{T} and Array{T,2} as
Matrix{T} and Base.GenericIOBuffer{Array{UInt8,1}} as IOBuffer,
and thus generalizes some existing code.

Co-authored-by: Kristoffer Carlsson <kristoffer.carlsson@chalmers.se>
@Jutho
Copy link
Contributor

Jutho commented Aug 12, 2020

This causes printing errors in a package of mine (TensorKit.jl). make_alias tries to construct things which are not allowed.

Probably part of the cause is that I defined Base.show(io::IO, ::Type{some_type_of_mine_that_has_an_alias}) rather than Base.show(io::IO, ::MIME{Symbol("text/plain")}, ::Type{...}) but still.

The type is rather simple, namely

struct ZNIrrep{N} <: AbelianIrrep
    n::Int8
end

with alias

const ℤ₂ = ZNIrrep{2}

and it ends up trying to construct a completely unrelated parametric type, with ℤ₂ as a type parameter where it is not allowed, hence throwing the error.

@Jutho
Copy link
Contributor

Jutho commented Aug 12, 2020

I can confirm that Base.show(io::IO, ::MIME{Symbol("text/plain")}, ::Type{...}) instead does fix the bug.

@Jutho
Copy link
Contributor

Jutho commented Aug 12, 2020

In fact, it does not; it fixes it for display on the REPL, but there are cases where show(io, ℤ₂) is called which now fail.

@mhauru
Copy link
Contributor

mhauru commented Aug 12, 2020

I took @Jutho's TensorKit.jl that he mentions above, and distilled it to a minimal case that exhibits the problem:

struct ZNIrrep{N} end
abstract type VectorSpace end
struct ProductSpace{S<:VectorSpace} end
const TensorSpace{S} = Union{S, ProductSpace{S}}
show(ZNIrrep{2})

On the latest master, this spits out

ERROR: TypeError: in ProductSpace, in S, expected S<:VectorSpace, got Type{
Stacktrace:
 [1] make_typealias(x::Type)
   @ Base ./show.jl:563
...

@KristofferC
Copy link
Sponsor Member

Would be good to open an issue with this.

@Jutho
Copy link
Contributor

Jutho commented Aug 12, 2020

I think I traced it down to this definition:

const TensorSpace{S} = Union{S, ProductSpace{S}}

where S is unconstrained in the first part of the union, and thus matches basically any type, but then it is constrained as a type parameter in ProductSpace. In make_alias, type intersection of TensorSpace basically matches with any type S, but make_alias then tries to construct ProductSpace{S} with a type S not matching the constraint, the error occurs. So the fix on my side is easy, and this counts as a bug in my code.

But still, good to know about unexpected side results.

kpamnany pushed a commit to RelationalAI-oss/TimeZones.jl that referenced this pull request May 5, 2023
* Revise tests to work with printing type aliases

Introduced in JuliaLang/julia#36107

* Address doctest failures with alias printing
Keno pushed a commit that referenced this pull request Jun 5, 2024
For example, this prints Array{T,1} as Vector{T} and Array{T,2} as
Matrix{T} and Base.GenericIOBuffer{Array{UInt8,1}} as IOBuffer,
and thus generalizes some existing code.

Co-authored-by: Kristoffer Carlsson <kristoffer.carlsson@chalmers.se>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
display and printing Aesthetics and correctness of printed representations of objects.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants