Improve how some kind of Expr are displayed by the show method

[ChiaffarelliMarco]

The improvements concerns the following Expr:

• typed bracket expressions as
  T[a b] T[a b; c d]
• quote blocks + unquoting
  quote $a+$b end
  -"special macros"
  `a b c` r"foo" foo"bar"zot
  -"long" numeric literals, which are implemented as macros
  11111111111111111111 0xfffffffffffffffff 11111111111111111111111111111111111111111111111111111111111111

Also, now the function test_repr in test/show.jl can (and indeed does) directly and successfully compare
Meta.parse(x)
with
eval(Meta.parse(repr(Meta.parse(x))))
Notice that I'm not to be credited with the latter achievement, since master branch also could have
(but did not) directly and successfully use the same comparison. I suppose the person to be credited
with solving the issue preventing this way of testing to succeed failed to actually update the test routine.

[JeffBezanson]

This is awesome, thanks! Welcome.

[ChiaffarelliMarco]

I see that having merged with #32412 fixed only the buildbot/tester_win32 run, but the buildbot/tester_win64 still fails, even though with different symptoms: this time it doesn't shows any error(but it shows some warnings), it simply times out at some point.
As I cannot test this stuff on a win64 machine, I'm a little unsure about what should I do now.

[musm]

That failure simply looks like a fluke: it timed out. We see this from time to time, so I wouldn't worry about it.

[ChiaffarelliMarco]

Thanks @musm for the reply. Is there something specific I should do, or the failing test will at some point be subject to a rebuilt? Or could I forget about it and leave it in this failed state?

[JeffBezanson]

Could you rebase this now that #32412 is merged?

[c42f]

This is a really great first contribution. Thanks!

I'm a little worried about the change to the printing of $. Does it make sense for nested quoted code?

[c42f]

Just to link up some threads, here's some related (complementary) improvements to show I've been toying with: #32408, #32201 (comment)

[ChiaffarelliMarco]

Rebased as requested to accomodate for #32412 having been merged.

[ChiaffarelliMarco]

I've realized that printing unquotes as $x do not work if they are not properly enclosed in a quote, that is,
for this way of printing to work it is necessary for the number of nested quote to at least match the number of nested unquotes. If this is not the case, it seams to me that the only way forward is to fallback to the "unhandled" case, printing it like $(Expr(:$, whatever)).
I've thus added a mechanism that keep track of how many quotes/unquotes has been encountered descending down an Expr tree, triggering the fallback mechanism if it is required, otherwise displaying stuff like $x as is.

[c42f]

It looks to me that you're on the right track to sorting out the problems with $ using quote_level.

Does quote_level need to interact with the interpolation syntax used to print unusual Exprs in the unhandled section and inside show_unquoted(io::IO, ex::QuoteNode, indent::Int, prec::Int) ?

[ChiaffarelliMarco]

Regarding @c42f question about the need for interactions between quote_level and the unhandled and QuoteNode case, I think that @c42f is right, as it seams I've missed to pass quote_level around in those situations, but preliminary tests show that the 'QuoteNode` case works just fine, contrary to what I had expected. This fact confuse me. I have to look closer, I suspect.

[c42f]

QuoteNode is for literal symbols and ASTs; it doesn't support interpolation.

julia> eval(Expr(:quote, (Expr(:$, :x))))
ERROR: UndefVarError: x not defined
Stacktrace:
 [1] eval at ./boot.jl:328 [inlined]
 [2] eval(::Expr) at ./client.jl:404
 [3] top-level scope at none:0

julia> eval(QuoteNode((Expr(:$, :x))))
:($(Expr(:$, :x)))

I mention it because it's one of the expressions which is itself printed as an interpolated expression when it's shown and I wondered if that mattered.

[c42f]

Do you have more time to push this forward? It's such a big improvement it would be a pity to not finish it off. It would be good to rebase to fix the conflicts, and maybe beneficial to squash the commits while you're at it.

After that, there's one remaining question in my mind: does this printing handle all cases where exotic Expr heads are mixed in with nested quoting and interpolation? I need to try this branch out, but I'm thinking of things vaguely like

Expr(:block, Expr(:exotic_head, Expr(:$, :x)))

[ChiaffarelliMarco]

I apologies for the long absence and for the late reply: in these past moths i was really busy in completing my master degree, and this will continue until the end of October. After that I think I should be able to find some time to reprise this PR.

[c42f]

No problem at all!

[JeffBezanson]

var"@cmd" parses @cmd as if it were a normal identifier, so that would be like writing Core.x "a b c". So it should be Core.@cmd to trigger macro call parsing.

[c42f]

which contexts var"@cmd" should be used instead of @cmd

For more on this, see

julia/base/docs/basedocs.jl

Lines 990 to 1004 in f0772c5

	var
	The syntax `var"#example#"` refers to a variable named `Symbol("#example#")`,
	even though `#example#` is not a valid Julia identifier name.
	This can be useful for interoperability with programming languages which have
	different rules for the construction of valid identifiers. For example, to
	refer to the `R` variable `draw.segments`, you can use `var"draw.segments"` in
	your Julia code.
	It is also used to `show` julia source code which has gone through macro
	hygiene or otherwise contains variable names which can't be parsed normally.
	Note that this syntax requires parser support so it is expanded directly by the
	parser rather than being implemented as a normal string macro `@var_str`.

[ChiaffarelliMarco]

I have handled macrocall expressions whose macroname argument is "qualified", as in
Expr(:macrocall, Expr(:(.), :A, Expr(:quote, Symbol("@m"))), LineNumberNode(0, :none), :a, :b: )
or
Expr(:macrocall, Expr(:(.), :A, Quotenode(Symbol("@m"))), LineNumberNode(0, :none), :a, :b: )
This fixes for example the repr of the aforementioned Expr(:macrocall, Expr(:(.), :Core, Expr(:quote, Symbol("@cmd"))), LineNumberNode(0, :none), "a b c")
to be
":(#= none:0 =# Core.@cmd \"a b c\")"

[JeffBezanson]

I don't think this is correct, since when these are parsed there will not be a :block expr. I think this whole branch can just be removed.

[ChiaffarelliMarco]

On the countrary, tests suggest that this branch is needed to correctly handle cases like

macro m(a, b)
    quote
        \$a + \$b
    end
end

In fact, without the branch

julia> repr(Meta.parse("""
       macro m(a, b)
           quote
               \$a + \$b
           end
       end"""))
":(macro m(a, b)\n      #= none:2 =#\n      :(begin\n            #= none:3 =#\n            \$a + \$b\n        end)\n  end)"

while with it in place, more satisfactorily the result is

julia> repr(Meta.parse("""
       macro m(a, b)
           quote
               \$a + \$b
           end
       end"""))
":(macro m(a, b)\n      #= none:2 =#\n      quote\n          #= none:3 =#\n          \$a + \$b\n      end\n  end)"

Another case that fails without the branch is

quote
    quote
        \$\$x
    end
end

with both quotes turned into :(begin ... end) upon repr(Meta.parse("..."))ing it:

julia> repr(Meta.parse("""
       quote
           quote
               \$\$x
           end
       end
       """))
":(:(begin\n        #= none:2 =#\n        :(begin\n              #= none:3 =#\n              \$\$x\n          end)\n    end))"

[ChiaffarelliMarco]

In fact I've checked now, and actually "quote \$x end" is parsed as a quote Expr containing a block Expr:

julia> dump(Meta.parse("quote \$x end"))
Expr
  head: Symbol quote
  args: Array{Any}((1,))
    1: Expr
      head: Symbol block
      args: Array{Any}((2,))
        1: LineNumberNode
          line: Int64 1
          file: Symbol none
        2: Expr
          head: Symbol $
          args: Array{Any}((1,))
            1: Symbol x

[JeffBezanson]

Yes, those cases are fine. The problem is when the block Expr has one element, e.g. it prints the value of Expr(:quote, Expr(:block, :(a+b))) as :(:(a + b)).

[ChiaffarelliMarco]

After some thinking, I must say that I very much prefer having

julia> show(Expr(:quote, Expr(:block, :(a + b))))
:(:(a + b))

than

julia> show(Expr(:quote, Expr(:block, :(a + b))))
:(:(begin
        a + b
    end))

since the former is coherent with the case in which the block has more than one argument, where, for reasons mentioned in my last two comments, the block itself is actually hidden upon showing:

julia> show(Expr(:quote, Expr(:block, :(a + b), :(c + d))))
:(quote
      a + b
      c + d
  end)

Nevertheless, the final word is of course yours.

[c42f]

Sounds good to me. Another option could be :(:(a + b;)) which parses as a block with a single entry and no line number node. What I'm not entirely sure about is whether that works in all cases?

[ChiaffarelliMarco]

Now I see your point. After some experimenting I think my preference is for @JeffBezanson 's suggestion. With that, we get a nicely consistent picture:

julia> show(Expr(:quote, Expr(:block)))
:(quote
  end)

julia> show(Expr(:quote, Expr(:block, :(a + b))))
:(quote
      a + b
  end)

julia> show(Expr(:quote, Expr(:block, :(a + b), :(c + d))))
:(quote
      a + b
      c + d
  end)

@c42f 's suggestion is somewhat cleaner, as there are no line numbers appearing in the call Meta.parse(repr(Expr(:quote, Expr(:block, :(a + b)))))
but I find :(:(a + b;)) and in particular :(:(a;)) to be a little confusing, and I definitely dislike it if instead of a + b or a there is some more complicated expression, possibly spread over more than one line: compare

julia> show(Expr(:quote, Expr(:block, Expr(:if, :(a == b), :(a + b), :(a - b)))))
:(:(if a == b
        a + b
    else
        a - b
    end;))

with

julia> show(Expr(:quote, Expr(:block, Expr(:if, :(a == b), :(a + b), :(a - b)))))
:(quote
      if a == b
          a + b
      else
          a - b
      end
  end)

By the way, this is my opinion, and I must admit that I may dislike semicolons more than they possibly deserve.

[ChiaffarelliMarco]

I have updated the test_repr macro, adding a stronger form of the previous condition which have to be satisfied for the test to pass. The current solution, which neglect the one-argument inner block, fails the stronger test, while both of the suggestions due to @JeffBezanson and @c42f pass it. Now it is only a matter of deciding which solution to endorse.

[ChiaffarelliMarco]

Once this is settled, with a cleaner local history I will merge with the upstream and fix conflicts.

[JeffBezanson]

Let's use the quote ... end syntax as you prefer. I don't think there is any particular promise that :(x;) will not include a line number; in fact many people would probably prefer if it did include one.

[ChiaffarelliMarco]

I have added to the test_repr macro a check regarding "Rule 1", mentioned in a summary comment regarding AST printing, in base/show.jl around line 780. This is the rule stating that an Expr ex should satisfy Meta.parse(string(ex)) == ex.
The test I have added actually check if
Meta.parse(string(ex)) == ex
with ex=Meta.parse(s) and s a String, because, in order to keep things simple and avoiding having to add lots of test by hand, I have incorporated the check in the test_repr macro, which take a string as an argument.
Furthermore, in a small number of test cases the weaker form
Base.remove_linenums!(Meta.parse(string(ex))) == ex
is used, because calling Meta.parse(string(ex) results in line numbers metadata to be added to the AST, thus causing the stronger form to fail.

Having said that, "Rule 1" (or its weaker form) was satisfied for free in all cases bar for Exprs outside quotes with :$ as head: handling this case requires differentiating between show/repr and print/string. I provided a simple mechanism that does just that. The way it works is by making show_unquoted never using the fallback "unhandled" case for printing the said Exprs when it is invoked by print or string, while for show and repr it works as before.
For example we now have

julia> repr(Expr(:$, :y))
":(\$(Expr(:\$, :y)))"

julia> string(Expr(:$, :y))
"\$y"

[ChiaffarelliMarco]

@JeffBezanson @c42f
I have cleaned up some stuff, and polished the quote_level mechanism taking advantage of IOContext. I think that now the implementation is much cleaner, as it avoids passing around quote_level everywhere. See if you like it.

With this I think I have reached satisfaction with this PR. Let me know if you think there is more to do, or if you want me to squash some commits.

[vtjnash]

It seems strongly unlikely to me that quote_level "belongs" in IOContext. I know it seems like a simpler way to pass around arguments, but it's actually dynamic scope and so has a different use case (although one can usually be used to emulate the other, poorly).

[c42f]

@vtjnash I don't disagree, but I'd like to better understand your objection to using IOContext here. As far as I can tell this is not so much about dynamic scope, and more about what constitutes the public API? In particular,

• IOContext is generally used to provide options to show which may be set by the caller when showing a type T, and therefore constitute part of the public API of show(::IO, ::T).
• Contrast this with quote_level, which is an internal implementation detail of show(::IO, ::Expr).

I suspect variables like quote_level and indent would be fairly suited to a dynamic scope based implementation if julia provided a natural and efficient implementation of dynamic scope (it doesn't). They just shouldn't be considered part of the public API which makes IOContext a bit ill suited to the job. Additionally, attaching them to IOContext means they will be inferred as Any which is an unfortunate performance pessimization.

Regarding an alternative to using IOContext, perhaps show_unquoted and related functions could just be enhanced with a ShowASTContext or some such; this could be used to also bundle up the indent and prec arguments which currently need to be passed through in many places? Personally I found the way these are sometimes-but-not-always passed around confusing last time I worked on this code.

[vtjnash]

IOContext is generally used to provide options to show which may be set by the caller when showing a type T, and therefore constitute part of the public API of show(::IO, ::T).

This is subtle, but it's exactly backwards: the IOContext is for setting options when using IO. It's the public API of show(::IO, ::Any) for when T is unknown.

By contrast, that means indent could realistically be an IOContext parameter, and I've even taken a decent crack at implementing that in #27430 (which needs a pretty solid rebase at this point). But since show is usually not supposed to print newlines, it's usually a moot point and sufficient just to pass it around as an argument here.

Yes, they could be paired into a ShowASTContext struct, and that's often a nice design. I don't think it'll end up enhancing this code much though, or could even be worse, since the values change so often.

edit: fixed to say indent instead of quote_level for the contrast statement

[c42f]

Yes, they could be paired into a ShowASTContext struct, and that's often a nice design. I don't think it'll end up enhancing this code much though, or could even be worse, since the values change so often.

Sure, I'd say it might or might not be helpful; a bit hard to judge without trying it out. It might turn out cleanly if paired with a utility function like indent(::ShowASTContext) to increment the indent.

[JeffBezanson]

Thanks @ChiaffarelliMarco ! Let's revert the quote_level change and then merge this. I don't want to hold back this great work with a debate on that design detail.

[c42f]

I don't want to hold back this great work with a debate on that design detail.

Completely agreed 💯. The benefit of merging this soon is high compared to getting some minor implementation details "just right".

Further cleanup can definitely happen in a separate PR. Doing it separately makes sense anyway if combined with a similar refactor for the other AST-showing context parameters.

[ChiaffarelliMarco]

Well, thanks for the feedback. I have reverted to the old quote_level mechanism.
I now understand why IOContext is not the correct way of doing things, and I second @c42f suggestion of introducing a ShowASTContext object, paired with some mechanism for cleanly changing it.
Still, the current implementation uses IOContext to distinguish between a call to show_unquote coming from print and a call coming from show. For the moment I think its cleaner this way, rather then having to pass around an another parameter, in particular if soon a ShowASTContext will come to light.

[JeffBezanson]

Thanks for seeing this through @ChiaffarelliMarco ! It's taken quite a while, but we got there!

[c42f]

Yes, great work @ChiaffarelliMarco, I look forward to having these improvements 🎉

[stevengj]

Rather than adding a new quote_level::Int that needs to get passed around everywhere, it might be nicer to change this (and maybe some of the other context data) to use IOContext(io, quote_level=>...) instead in the future.

[c42f]

@stevengj this was done previously but removed; see discussion starting at #32423 (comment)