Omit module prefix when printing types which are reachable from Main #23806

nalimilan · 2017-09-21T11:38:58Z

Only add the module prefix when it is actually required to access the type from Main.

As discussed on Slack some time ago, this is an attempt at improving the printing of types defined outside Base. I'm really not familiar with this, so there might be better ways of doing this. Also, I'm not sure how to test this given that tests do not run in Main.

The improvement is the most visible for types defined in packages for which the package name is just the plural of the type name. Before:

julia> using CategoricalArrays

julia> CategoricalArray(1:2)
2-element CategoricalArrays.CategoricalArray{Int64,1,UInt32}:
...

After:

julia> using CategoricalArrays

julia> CategoricalArray(1:2)
2-element CategoricalArray{Int64,1,UInt32}:
...

StefanKarpinski · 2017-09-21T14:22:15Z

I'm onboard with the intention here, but I don't think that making Main more special here is the right way to go. Instead, this should be handled by doing IOContext(io, :module => Main) from the REPL and then adding generic logic to only print as much as is required to reconstruct a name with respect to the get(io, :module, Empty) where const Empty = Module(:Empty). I also think that searches for aliases could be done per context module so that we know how to print a short version of an object in the context of some module, which will often be Main but not always.

nalimilan · 2017-09-24T19:02:33Z

Good idea. I've updated the PR to add an IOContext mechanism and used it for Main from the REPL.

with respect to the get(io, :module, Empty) where const Empty = Module(:Empty).

I've tried using that dummy module as a default, but that prints lots of warnings like "importing deprecated binding Base.airy into Empty" when calling sprint(dump, Any) (which happens in a test). I don't understand why this happens, so I've used get(io, :module, nothing) instead.

I also think that searches for aliases could be done per context module so that we know how to print a short version of an object in the context of some module, which will often be Main but not always.

What do you mean by "context module"? In general it's hard to know what's the relevant module, as the string generated by show can be passed to different functions and eventually be shown to the user in a very different context.

nalimilan · 2017-09-24T19:04:10Z

test/repl.jl

@@ -167,6 +167,21 @@ fake_repl() do stdin_write, stdout_read, repl
    close(t)
    readuntil(stdout_read, "\n\n")

+    # Test that module prefix is omitted when type is reachable from Main


I couldn't find a simpler way to test display(::REPLDisplay, ...). If there's a cleaner solution, I'm all ears.

Well, as I said, you can

eval [...] in Main

But eval what? :-)

Whatever you need to be run in Main?

Instead of write(stdin_write, "module TestShowTypeREPL; export TypeA; struct TypeA end; end\n") and write(stdin_write, "using .TestShowTypeREPL\n") just do eval instead?

@eval Main module TestShowTypeREPL; export TypeA; struct TypeA end; end and @eval Main using .TestShowTypeREPL

Thanks, that makes the code simpler. Though I haven't found a way to get the result without the whole readline stuff.

StefanKarpinski · 2017-09-25T14:52:48Z

Using get(io, :module, nothing) is probably better anyway!

What do you mean by "context module"?

I just meant whatever the value of get(io, :module) is.

KristofferC · 2017-10-08T21:51:52Z

Cross-reffing JuliaDocs/Documenter.jl#574 (comment) since it is a bit related.

KristofferC · 2017-10-09T09:54:54Z

Another cross ref: #24059.

rfourquet · 2017-10-09T13:18:07Z

base/show.jl

@@ -1992,7 +2007,7 @@ function showarray(io::IO, X::AbstractArray, repr::Bool = true; header = true)
        # override usual show method for Vector{Method}: don't abbreviate long lists
        io = IOContext(io, :limit => false)
    end
-    (!repr && header) && print(io, summary(X))


This change seems unrelated?

Right, but probably not worth a PR?

Oh, and it's not completely unrelated since it's needed so that the module context is passed to summary via io. So it's useful on its own, but needed for this PR.

rfourquet · 2017-10-09T13:28:53Z

base/show.jl

+    isdefined(from, sym) || return false
+    getfield(from, sym) === getfield(parent, sym)
+end
+is_reachable(sym::Symbol, parent::Module, from::Void) = false


as isdefined(nothing, :anysymbol) == false, the 2 methods could be simplified to is_reachable(sym::Symbol, parent::Module, from::Union{Module,Void}) = isdefined(from, sym) && getfield(from, sym) === getfield(parent, sym). Not saying it's necessarily an improvement!

Yeah, at least the current definition is quite clear, if a bit verbose.

I find @rfourquet's formulation clearer, but people's brains are different, so YMMV. This isn't a public function so the name is not crucial, but this might be better called isvisible; isreachable sounds like there's some kind of recursive search going on, which there isn't – it's just an immediate visibility check.

nalimilan · 2017-11-14T20:39:20Z

Bump. I think this is good for a review.

StefanKarpinski · 2017-11-14T23:08:40Z

I will review soon!

StefanKarpinski

Generally good, my major comment is about the criteria for when to print prefixes. I think it we should only consider visibility with respect to the given module. If no module is given, always print a fully qualified name.

StefanKarpinski · 2017-11-22T12:14:06Z

base/repl/REPL.jl

@@ -122,7 +122,7 @@ end
 function display(d::REPLDisplay, mime::MIME"text/plain", x)
    io = outstream(d.repl)
    Base.have_color && write(io, answer_color(d.repl))
-    show(IOContext(io, :limit => true), mime, x)
+    show(IOContext(io, :limit => true, :module => Main), mime, x)


We may want to allow giving the module via a keyword, but that can be added later.

StefanKarpinski · 2017-11-22T12:16:10Z

base/show.jl

+    isdefined(from, sym) || return false
+    getfield(from, sym) === getfield(parent, sym)
+end
+is_reachable(sym::Symbol, parent::Module, from::Void) = false


I find @rfourquet's formulation clearer, but people's brains are different, so YMMV. This isn't a public function so the name is not crucial, but this might be better called isvisible; isreachable sounds like there's some kind of recursive search going on, which there isn't – it's just an immediate visibility check.

StefanKarpinski · 2017-11-22T12:18:15Z

base/show.jl

@@ -266,7 +274,14 @@ function show_type_name(io::IO, tn::TypeName)
    elseif globfunc
        print(io, "typeof(")
    end
-    if isdefined(tn, :module) && !(is_exported_from_stdlib(sym, tn.module) || (tn.module === Main && !hidden))
+    # Print module prefix unless type is:


Wasn't immediately clear to me without reading the code if this was a conjunction or disjunction of conditions, so maybe change the comment to this:

# Print module prefix unless type satisfies one of the following:

But see comment below which would change this to just check visibility.

StefanKarpinski · 2017-11-22T12:28:10Z

base/show.jl

+    if isdefined(tn, :module) &&
+        !(is_exported_from_stdlib(sym, tn.module) ||
+          is_reachable(sym, tn.module, get(io, :module, nothing)) ||
+          (tn.module === Main && !hidden))


I feel like it might be better to only consider visibility. Doesn't that cover the other cases in the typical situations where we are displaying from Main and the Base has been imported? If the user asks to display something with respect to another module, wouldn't we want to only shorten names that are actually visible in that module? Why are Main or stdlib relevant there?

I've just tried it, and it triggers a lot of test failures all over the place. That made me realize the IOContext approach implies hardcoding :module => Main in many places. For example, stringmime calls _textreprmime(m::MIME, x) = sprint(verbose_show, m, x). To omit the module prefix, we need to create the IOBuffer by hand to pass it an adequate IOContext. There are many examples like that.

Do we really want to add this kind of thing all over the place? I'm asking before spending time to find them all. The problem is that in the end many functions do not take a IO object, so they will hardcode Main, which kind of defeats the use of IOContext. Is that still worth it, rather than hardcoding Main in a single central place?

Actually, a good intermediate solution would be to default to Main (i.e. use get(io, :module, Main) instead of get(io, :module, nothing)), but still allow passing a different :module. That way by default we get reasonably short type names without hardcoding Main in many places, but it can still be customized.

That sounds like a good approach: Main as the default nothing as a value that can be passed to have no context module. Another thing that occurs to me is allowing an array of modules to look in and having [] indicate not to look anywhere, defaulting to [Main]. But it would also make sense to allow a single module as a shortcut in that case. Maybe too complicated as a starting point.

nalimilan · 2017-11-24T18:48:28Z

I've implemented the new approach which falls back to Main by default.

@StefanKarpinski You haven't commented on the fact that functions are always printed with the full prefix (i.e. not change). Does that sound reasonable to you? That's kind of inconsistent, but in practice I think you're generally interested in knowing where a function is define, notably to add methods to it. But I could be convinced otherwise.

StefanKarpinski · 2017-11-25T20:50:28Z

I do think that knowing where a function comes from is useful.

StefanKarpinski · 2017-11-25T20:59:25Z

base/show.jl

-    if isdefined(tn, :module) && !(is_exported_from_stdlib(sym, tn.module) || (tn.module === Main && !hidden))
+    # Print module prefix unless type is visible from module passed to IOContext
+    # (defaulting to Main)
+    if isdefined(tn, :module) && !isvisible(sym, tn.module, get(io, :module, Main))


What about pulling out get(io, :module, Main) and checking if it's nothing instead of adding the Void method to isvisible? That method seems a bit iffy to me.

Only add the module prefix when it is actually required to access the type from the module passed to IOContext. By default, take Main as a reference.

fredrikekre · 2018-01-30T07:19:58Z

I agree with Alex here: #25798 (comment), I liked the idea at first, but after some experience with it I don't really like it anymore. What do other people think? I agree that printing CategoricalArrays in CategoricalArrays.CategoricalArray from OP seem rather superfluous when the type directly matches the package name like that, but consider e.g.

julia> A'
2×2 LinearAlgebra.Adjoint{Float64,Array{Float64,2}}:
 0.628495  0.0341545
 0.693558  0.787898

and

julia> A'
2×2 Adjoint{Float64,Array{Float64,2}}:
 0.742562  0.773093
 0.875984  0.206749

Here I think it is informative to see the LinearAlgebra. prefix. Perhaps it would be better if this change was opt-in somehow. Thoughts?

(Unrelated, but LinearAlgebra.Adjoint looks so much nicer than Base.LinAlg.Adjoint 😄 )

quinnj · 2018-01-30T12:17:33Z

Personally, I've really liked the new printing because it provides that extra piece of information of: what you see is what you can access. I.e. if it's prequalified w/ LinearAlgebra.Adjoint, then I know to access that type, I need to do LinearAlgebra.Adjoint. Before, we never had this information; you had to keep a mental tally of, "oh, even though it prints like Base.LinearAlgebra.Adjoint, I did using LinearAlgebra, so I don't need to fully qualify the type in my own code".

StefanKarpinski · 2018-01-30T14:44:51Z

I've found this to be incredibly pleasant to use and consider it a big success at making the way we print things more concise and intuitive. Note that I've added repr(obj, :module => nothing) and repr(obj, :module => Base) and other IOContext argument pairs so that if you want to know how to print something with respect to another module, it's easy enough. We do probably want to figure out a scheme for configuring the IOContext that the REPL uses for printing. That would allow doing something like REPL.context[:module] = Base to make the REPL print from the context of Base instead of Main or REPL.context[:module] = nothing to make it print every name in full. That would also allow REPL.context[:float_digits] = 6 or something like that to control how significant many digits floating-point numbers are printed to.

stevengj · 2018-02-27T14:08:08Z

This PR gives a way to force the printing of a module (by setting :module to nothing), but not a way to forcibly suppress it.

I would suggest instead using IOContext(io, :module => true) to force printing of the module and IOContext(io, :module => false) to force it to be suppressed.

This came up for me in JuliaLang/IJulia.jl#632 where we need a very compact version of a type for display in a pop-op menu. Another possibility would be to support :compact => true to suppress type parameters as well if they get too long.

nalimilan · 2018-02-27T14:27:50Z

:module => false makes sense to me. Though you can pass the result of parentmodule to get the same result, at least when printing a single type.

Using :compact => true to drop type parameters is an interesting idea, but it could be misleading as it really removes information and it could be more problematic than rounding numbers.

stevengj · 2018-02-27T21:02:06Z

You could print Foo{...} to indicate that parameters are elided.

stevengj · 2018-02-27T21:04:03Z

Passing parentmodule is a pain when printing multiple types, and impossible for nested type parameters

nalimilan added display and printing Aesthetics and correctness of printed representations of objects. needs tests Unit tests are required for this change labels Sep 21, 2017

ararslan requested a review from JeffBezanson September 21, 2017 18:01

nalimilan force-pushed the nl/typename branch from a61c91e to b8c5d19 Compare September 23, 2017 19:52

nalimilan mentioned this pull request Sep 24, 2017

fix #17997, don't load packages in Main #23579

Merged

nalimilan force-pushed the nl/typename branch from b8c5d19 to ded2da1 Compare September 24, 2017 17:31

nalimilan commented Sep 24, 2017

View reviewed changes

nalimilan requested a review from StefanKarpinski October 9, 2017 12:13

nalimilan mentioned this pull request Oct 9, 2017

Possible regression in printing of getfield with anonymous functions #24059

Closed

nalimilan force-pushed the nl/typename branch from ded2da1 to 167f9ee Compare October 9, 2017 13:10

rfourquet reviewed Oct 9, 2017

View reviewed changes

mortenpi mentioned this pull request Oct 10, 2017

fix deprecations on 0.7 JuliaDocs/Documenter.jl#574

Merged

nalimilan removed the needs tests Unit tests are required for this change label Nov 14, 2017

StefanKarpinski requested changes Nov 22, 2017

View reviewed changes

nalimilan force-pushed the nl/typename branch from 9ee1255 to 66a31de Compare November 24, 2017 13:01

nalimilan mentioned this pull request Nov 24, 2017

IOContext: introduce the :typeinfo property #24651

Merged

4 tasks

nalimilan force-pushed the nl/typename branch from 66a31de to a0b430b Compare November 24, 2017 15:51

StefanKarpinski reviewed Nov 25, 2017

View reviewed changes

Omit module prefix when printing types which are reachable without it

b211a8a

Only add the module prefix when it is actually required to access the type from the module passed to IOContext. By default, take Main as a reference.

nalimilan force-pushed the nl/typename branch from a0b430b to b211a8a Compare November 26, 2017 11:33

StefanKarpinski approved these changes Nov 27, 2017

View reviewed changes

StefanKarpinski merged commit 690f4bf into master Nov 27, 2017

StefanKarpinski deleted the nl/typename branch November 27, 2017 02:44

vtjnash mentioned this pull request Dec 11, 2017

bad show of anonymous function names in Main #25031

Closed

KristofferC mentioned this pull request Jan 27, 2018

LinearAlgebra tests failing when run directly #25779

Closed

fredrikekre mentioned this pull request Jan 29, 2018

fix some show test when run directly in the repl, #25798

Merged

nalimilan mentioned this pull request Jul 4, 2018

RFC: don't show module prefix in :compact printing mode #27925

Merged

aviatesk mentioned this pull request Aug 31, 2019

WIP: allow changing context module in REPL #33102

Closed

1 task

timholy mentioned this pull request Jan 3, 2020

doc why julia sometimes can't save inference results timholy/SnoopCompile.jl#40

Closed

fredrikekre mentioned this pull request Feb 4, 2020

Dates/io tests fail on master when networking is unavailable #34655

Open

Omit module prefix when printing types which are reachable from Main #23806

Omit module prefix when printing types which are reachable from Main #23806

Conversation

nalimilan commented Sep 21, 2017

StefanKarpinski commented Sep 21, 2017 • edited Loading

nalimilan commented Sep 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefanKarpinski commented Sep 25, 2017

KristofferC commented Oct 8, 2017 • edited Loading

KristofferC commented Oct 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan commented Nov 14, 2017

StefanKarpinski commented Nov 14, 2017

StefanKarpinski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan commented Nov 24, 2017

StefanKarpinski commented Nov 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredrikekre commented Jan 30, 2018 • edited Loading

quinnj commented Jan 30, 2018

StefanKarpinski commented Jan 30, 2018

stevengj commented Feb 27, 2018 • edited Loading

nalimilan commented Feb 27, 2018

stevengj commented Feb 27, 2018

stevengj commented Feb 27, 2018

StefanKarpinski commented Sep 21, 2017 •

edited

Loading

KristofferC commented Oct 8, 2017 •

edited

Loading

fredrikekre commented Jan 30, 2018 •

edited

Loading

stevengj commented Feb 27, 2018 •

edited

Loading