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

make printing of typealias env shorter using {<:T} syntax, when possible #39395

Merged
merged 3 commits into from
Apr 3, 2021

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Jan 25, 2021

Inspired by #35710, including taking the tests (though not any of the implementation) from there. Unlike the original PR, this also prefers the short form (eliding the TypeVar name), even in case where it is not a gensym.

julia> struct A{B,C,D,E,F,G} end

julia> A{U, <:Any, T, Int, S, <:T} where {S, T<:S, U<:Integer}
A{<:Integer, <:Any, T, Int64, S, <:T} where {S, T<:S}

Github: this closes #35710 and fixes #34887

@vtjnash vtjnash added the display and printing Aesthetics and correctness of printed representations of objects. label Jan 25, 2021
Copy link
Contributor

@stev47 stev47 left a comment

Choose a reason for hiding this comment

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

Nice, I must have missed the has_typevar intrinsic back when I tried to do this. Avoiding additional io context keys using the mutable wheres is probably good too. It seems solid from my quick tests.

It would be nice if Ref{T} where T and Ref{X} where X both printed consistently, though I guess this requires non-trivial changes in how you build orig for show_typeparams.

elide = length(wheres)
for i = n:-1:1
p = env[i]
if p isa TypeVar
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe reduce nesting: p isa TypeVar || continue?

Copy link
Member Author

Choose a reason for hiding this comment

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

there is an else clause, so this is the right structure

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, I see this is the other loop. The reason is still the same though (keeping the general structure of the two loops in common)

base/show.jl Outdated
elide -= 1
elseif p.lb === Union{} && show_can_elide(p, wheres, elide, env, i)
elide -= 1
elseif p.ub === Any && show_can_elide(p, wheres, elide, env, i)
Copy link
Contributor

Choose a reason for hiding this comment

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

can merge with Union{} branch

@vtjnash
Copy link
Member Author

vtjnash commented Mar 3, 2021

I agree those should do the same (as did CI). I've used the new jl_types_egal function to handle that now, so let's see how CI fares now.

@vtjnash vtjnash requested a review from JeffBezanson March 9, 2021 21:40
@JeffBezanson
Copy link
Member

I think we should put back the gensym heuristic; i.e. still print where name for non-gensyms. It's nice to print something egal to the original input when possible, and less likely to break tests that might exist out there.

vtjnash and others added 2 commits March 18, 2021 14:55
@vtjnash vtjnash force-pushed the jn/feature/print-unionall branch from 6ae1bb2 to a2e7b46 Compare March 18, 2021 19:27
@vtjnash
Copy link
Member Author

vtjnash commented Mar 18, 2021

Okay, top example is now:

julia> struct A{B,C,D,E,F,G} end

julia> A{U, <:Any, T, Int, S, <:T} where {S, T<:S, U<:Integer}
A{U, <:Any, T, Int64, S, <:T} where {S, T<:S, U<:Integer}

with added gensym (#) test.

@vtjnash vtjnash force-pushed the jn/feature/print-unionall branch from a2e7b46 to e593839 Compare March 18, 2021 21:16
@vtjnash
Copy link
Member Author

vtjnash commented Apr 1, 2021

Thinking to merge this tomorrow, since I haven't heard any concerns with this now

@StefanKarpinski
Copy link
Member

I actually prefer the more aggressively normalized original form. It's easier to see what's happening, IMO. Parameter names aren't significant after all, so eliminating where possible seems good.

@vtjnash
Copy link
Member Author

vtjnash commented Apr 2, 2021

There's some cases were readability was slightly lower. Like hiding parameter names to functions too–they are not a significant part of the call, but they can be useful. However, as Jeff mentioned, it is also slightly more preferable to print an egal copy when feasible. I still think we should merge this now, but partly because we can still easily make that choice later after getting some experience with this.

@StefanKarpinski
Copy link
Member

Fair enough, although I think we should probably make a decision before 1.7 to minimize the number of disruptive changes to printing. Although otoh maybe it's good to get people to stop testing exact printing of types.

@vtjnash
Copy link
Member Author

vtjnash commented Apr 3, 2021

We've made more drastic changes than this is in 1.5 and 1.6 already, so hopefully that's already having an influence 😬

@vtjnash vtjnash merged commit c0d2e1a into master Apr 3, 2021
@vtjnash vtjnash deleted the jn/feature/print-unionall branch April 3, 2021 02:19
@NHDaly
Copy link
Member

NHDaly commented Apr 3, 2021

Cool, thanks! :)

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
5 participants