Apply syntax highlighting to Exprs in REPL

[tecosaur]

As discussed on slack a while ago, it's now near trivial to make inspecting large Exprs much more pleasant with JuliaSyntax highlighting.

See the commit message for more details.

Screenshots

[tecosaur]

If anyone's up for it, I think this should be a pretty quick and easy review & merge 🙂

[fredrikekre]

This is type-piracy so I guess this should be caught higher up in the REPLDisplay code somewhere?

[tecosaur]

I suppose line 120 doesn't look that high up, but it is just after the using and exports lines.

[KristofferC]

Type piracy means that one defines a method where one neither owns the function or any of the types (which is the case here).

With "higher up" it doesn't mean literally higher up in the file but higher up in the "dependency chain" (here this means in Base since Base owns show and Expr).

[fredrikekre]

The REPL stdlib has it's own display and I guess this is something that should be implemented there, i.e. how REPL display Exprs and not how they are showd.

[tecosaur]

Yea, doing this in Base isn't really an option since we're using two stdlibs here.

[tecosaur]

I'll just switch from implementing a show method to a display method then :)

[KristofferC]

Hehe, display is also a Base function. Instead of

 show(io, mime, x[]) 

in the REPL you could add something like

...
if x[] is a Expr
   print(io, JuliaSyntaxHighlighting.highlight(sprint(show, ex)))
else
  show(io, mime, x[]) 
end
...

to

julia/stdlib/REPL/src/REPL.jl

Line 383 in 8742d3c

show(io, mime, x[])

[fredrikekre]

The display type is owned by REPL though, but I think perhaps in this case it is cleaner to change

julia/stdlib/REPL/src/REPL.jl

Line 383 in 8742d3c

show(io, mime, x[])

to

show_repl(io, mime, x[])

with implementations

show_repl(io::IO, mime::MIME, x) = show(io, mime, x)
show_repl(io::IO, mime::MIME, x::Expr) = ...

?

[KristofferC]

The display type is owned by REPL though

Yes, it's the function I am suggesting to edit here heh, but it sets up various iocontexts etc which is also why adding a new overload to it isn't that good.

It would also be good to check that #38028 doesn't regress.

[tecosaur]

I've just pushed the change that @fredrikekre suggested 🙂

[KristofferC]

This should be done without type piracy and respect the REPL IOContext:

Before:

julia> :(quote 123.32132132132 end)
:($(Expr(:quote, quote
    #= REPL[1]:1 =#
    123.32132132132
end)))

julia> Base.active_repl.options.iocontext[:compact] = true
true

julia> :(quote 123.32132132132 end)
:($(Expr(:quote, quote
    #= REPL[3]:1 =#
    123.321
end)))

After

julia> :(quote 123.32132132132 end)
:($(Expr(:quote, quote
    #= REPL[1]:1 =#
    123.32132132132
end)))

julia> Base.active_repl.options.iocontext[:compact] = true
true

julia> :(quote 123.32132132132 end)
:($(Expr(:quote, quote
    #= REPL[3]:1 =#
    123.32132132132
end)))

[tecosaur]

@fredrikekre / @KristofferC any thoughts on the current version of the code?

[KristofferC]

Is this really needed?

[tecosaur]

I'm actually not sure, there was some conversation I had (on Slack) which concluded with the import StyledStrings line being needed in #51869 even though it's not used in precompile.jl, so I've just "followed the pattern" here.

I could just try dropping the line and seeing what happens?

[KristofferC]

I don't see a mechanism for how it should be needed so it's probably good to try to remove it and see what happens.

[tecosaur]

Trying... 🤞

Updated

[KristofferC]

The current version seems ok to me at least. Does it still obey --color=no?

[tecosaur]

Yep, since it's just printing via. StyledStrings now, and StyledStrings respects the colour property as expected.

[fingolfin]

This looks fine to me now, but it'd be great if @KristofferC and/or @fredrikekre could have a final look (and then perhaps directly merge it if they are happy)