Skip to content

Conversation

@stev47
Copy link
Contributor

@stev47 stev47 commented May 3, 2020

fixes #34887

typevars with gensym symbols will now be printed inlined for UnionAll type arguments if their type bounds allow to do so (i.e. <:UB or >:LB).
typevars with named symbols are printed as before since they are assumed to carry meaning and thus might be helpful.

Before:

julia> Ref{<:Any}
Ref{var"#s2"} where var"#s2"
julia> Ref{<:Ref{<:Ref}}
Ref{var"#s1"} where var"#s1"<:(Ref{var"#s2"} where var"#s2"<:Ref)

After:

julia> Ref{<:Any}
Ref
julia> Ref{<:Ref{<:Ref}}
Ref{<:Ref{<:Ref}}

@JeffBezanson JeffBezanson added the display and printing Aesthetics and correctness of printed representations of objects. label May 3, 2020
@JeffBezanson
Copy link
Member

JeffBezanson commented May 3, 2020

This is a great improvement, thanks for working on this. The difficulty is that the short-form syntax only works for one level of nesting. For example this PR gives

julia> T = TypeVar(gensym(),Integer);

julia> UnionAll(T,Ref{Ref{T}})
Ref{Ref{<:Integer}}

which is not correct; in that case the current output needs to be used.

@stev47
Copy link
Contributor Author

stev47 commented May 4, 2020 via email

@JeffBezanson
Copy link
Member

Yes, that's a good point. Pair{<:Any,<:Any} creates the structure Pair{A,B} where B where A, so it has to match that. The variable also can only occur once, for example Pair{T,T} where T (where T is a gensym) can't use the special printing.

Anyway you can probably tell why this hasn't been implemented already :)

@stev47 stev47 force-pushed the feature/print-unionall branch from e43f192 to 35962a6 Compare May 5, 2020 19:18
@stev47
Copy link
Contributor Author

stev47 commented May 5, 2020

next try:

  • unionall printing now populates a dict to manage duplicates and named typevars
  • fixed misuse of :unionall_env => tv for just displaying the typevar plainly in a few places
  • :compact => true now also shortens named typevars

@stev47 stev47 force-pushed the feature/print-unionall branch from 35962a6 to ea51a06 Compare May 5, 2020 22:51
@stev47
Copy link
Contributor Author

stev47 commented May 7, 2020

tester_macos64 failure seems unrelated

@JeffBezanson
Copy link
Member

Wow, this is really impressive! As far as I can tell it works correctly. Just amazing how difficult this is to implement.

  • It would be good to document the meanings of the associated IOContext keys. When there was just one it was pretty clear, but with 3 it will be significantly harder for maintainers to see what's going on and which one they should use.
  • It's cool that this detects that Pair{<:Any,<:Any} is equal to Pair and displays it like that, but I wonder if it might be confusing. Would be good to get a couple opinions on that.

@StefanKarpinski
Copy link
Member

  • It's cool that this detects that Pair{<:Any,<:Any} is equal to Pair and displays it like that, but I wonder if it might be confusing. Would be good to get a couple opinions on that.

FWIW, I like it.

@stev47 stev47 force-pushed the feature/print-unionall branch from ea51a06 to f5998d0 Compare May 14, 2020 22:44
@stev47
Copy link
Contributor Author

stev47 commented May 14, 2020

I added some comments explaining the keys. If one was to touch the method printing stuff too, simplifying the logic a bit might be possible, but otherwise there are just too many cornercases making it difficult.

Does isgensym(::Symbol) qualify for public api? I was surprised it wasn't available.

@JeffBezanson
Copy link
Member

JeffBezanson commented May 28, 2020

Here is a small bug:

julia> struct P{A,B<:Integer} end

julia> P{Int,<:Any}
P{Int64}

julia> P{Int64}
P{Int64,B} where B<:Integer

Similarly this quasi-bug where the input and output forms are swapped:

julia> show(IOContext(stdout,:compact=>true), Pair{Int64,<:Any})
Pair{Int64}
julia> show(IOContext(stdout,:compact=>true), Pair{Int64})
Pair{Int64,<:Any}

It would be ok to print both as Pair{Int64} or as Pair{Int64,<:Any}. But to use T{Int64}, we have to check that the bounds of the second typevar match the original typevar in T, or to make it simpler we could just check that it's the same === typevar as in the original.

I also think this doesn't need to depend on :compact. I would just use the most compact form all the time.

A future improvement would be to add support for Unions:

julia> Union{Int,<:Any}
Union{Int64, var"#s2"} where var"#s2"

but that can be done in a later PR.

@stev47 stev47 force-pushed the feature/print-unionall branch from f5998d0 to 861438c Compare May 30, 2020 11:10
@stev47
Copy link
Contributor Author

stev47 commented May 30, 2020

Changes:

  • fixed typevar elision with non-trivial typevar constraints
  • fixed dependent typevar printing Pair{A,<:A} where A
  • typevar printing is now independent of :compact

Since e.g. Complex != Complex{<:Any} because of the typevar constraint, I'm still hesitant to print them the same way though, since that would violate reparsability to equal typevars.
With this PR you would currently have:

julia> Complex{<:Any}
Complex{<:Any}
julia> Complex{<:Real}
Complex
julia> Complex{<:Integer}
Complex{<:Integer}

which is fully reparsable.

Parametric type aliases might still be somewhat ugly:

julia> NTuple{2,<:Int}
Tuple{var"#s141",var"#s141"} where var"#s141"<:Int64

But finding the shortest alias representation in general is probably over-the-top, so that is left for the future.

fixes JuliaLang#34887

typevars in unionall type arguments can now be printed inlined if their
type bounds allow to do so (e.g. `<:UB` or `>:LB`) and they are not
referenced from elsewhere in the type hierarchy.

Before:

  julia> Ref{<:Any}
  Ref{var"#s2"} where var"#s2"
  julia> Ref{<:Ref{<:Ref}}
  Ref{var"#s1"} where var"#s1"<:(Ref{var"#s2"} where var"#s2"<:Ref)

After:

  julia> Ref{<:Any}
  Ref
  julia> Ref{<:Ref{<:Ref}}
  Ref{<:Ref{<:Ref}}
@stev47 stev47 force-pushed the feature/print-unionall branch from 861438c to ddd6afb Compare May 30, 2020 14:20
@JeffBezanson
Copy link
Member

I'm still hesitant to print them the same way though, since that would violate reparsability to equal typevars.

But finding the shortest alias representation in general is probably over-the-top

Fully agree with both; the first priority is to print everything correctly, and we don't need every possible improvement at once.

@JeffBezanson JeffBezanson self-assigned this Oct 21, 2020
@JeffBezanson
Copy link
Member

I tried to resolve the conflicts but I'm not sure it will work. Let's see what happens.

@JeffBezanson
Copy link
Member

Ok, show_datatype needs to be re-indented with an extra end removed, and the new typealias-showing code needs to be updated to use the new IOContext keys. For example

julia> Vector{Vector{<:Any}}
Vector{Vector{<:Any} where var"#s1"} (alias for Array{Array{<:Any, 1}, 1})

@stev47
Copy link
Contributor Author

stev47 commented Oct 24, 2020

Thanks, I almost forgot about this.
Unfortunately it is a bit more complicated now since type aliases can mess around with typevar elision by reordering, consider:

struct A{T,S} end
const B{T,S} = A{S,T}
show(B) # previously showed `A{<:Any,T} where T`, now needs to know about `B` to elide typevars when printing

This small example is trivial to fix but the proper solution would need to build the fully recursed typealias representation before printing (another IOContext key?), then apply my hierarchic typevar counting/elision and only after that print recursively.

@JeffBezanson
Copy link
Member

Yes, you're right. Sorry to do that to you. This PR is a great piece of work. We merged the typealias printing first since in some common cases it reduces the amount of output very drastically.

vtjnash added a commit that referenced this pull request Jan 23, 2021
vtjnash added a commit that referenced this pull request Jan 23, 2021
Inspired by #35710

Co-authored-by: Stephan Hilb <[email protected]>
@stev47
Copy link
Contributor Author

stev47 commented Feb 28, 2021

superseeded by #39395

@stev47 stev47 closed this Feb 28, 2021
vtjnash added a commit that referenced this pull request Mar 3, 2021
Inspired by #35710

Co-authored-by: Stephan Hilb <[email protected]>
vtjnash added a commit that referenced this pull request Mar 18, 2021
Inspired by #35710

Co-authored-by: Stephan Hilb <[email protected]>
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
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.

show for UnionAll types prints non-syntactic identifiers for unnamed type variables

4 participants