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

REPL: allow switching contextual module #33872

Merged
merged 26 commits into from
Jun 6, 2022
Merged

Conversation

rfourquet
Copy link
Member

Main remains the default module in which REPL expressions
are evaled, but it's possible to switch to a module Mod via
activate_module(Mod). The default prompt then indicates this,
e.g. (Mod) julia> .

When not using Revise, this can be quite useful to work on a module (e.g. a workflow is redefine function f from a module, but first a bunch of import Mod: ... and/or using Mod: ... have to be typed for internal functions/constants used in f. With this change, copy-pasting the function from the module definitions into the REPL works out of the box).

@rfourquet rfourquet added the stdlib:REPL Julia's REPL (Read Eval Print Loop) label Nov 17, 2019
@andyferris
Copy link
Member

This seems like a good idea. When I read how Juno did this (before Revise even existed) I remember wondering why we don't do this at the REPL.

The default prompt then indicates this, e.g. (Mod) julia>.

It makes me wonder if it could just be Mod>. (As a only semi-serious idea, then rename Main to Julia or something so it's not a special case... :) )

@rfourquet
Copy link
Member Author

It makes me wonder if it could just be Mod>.

I thought about it too, and I believe I would definitely customize it like that in my startup file. But (Mod) julia> looks more aligned with what is done for pkg mode. And as this "module-switch" applies also to help mode, the only way to distinguish between these two modes would then be the color, which likely has potential for being confusing.

base/client.jl Outdated
both for evaluating expressions and printing them.
"""
function activate_module(mod::Module=Main)
global active_repl
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

This is redundant, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah proably, as it's not modified.

@rfourquet rfourquet force-pushed the rf/repl/switch-module branch 2 times, most recently from e88bb02 to f14ea54 Compare October 4, 2020 11:15
@rfourquet
Copy link
Member Author

rfourquet commented Oct 4, 2020

Bump; would be cool to make a decision here.

Copy link
Sponsor Member

@KristofferC KristofferC left a comment

Choose a reason for hiding this comment

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

I was just thinking about this and I agree, this would be very nice!

I guess prompt paste wouldn't work in this mode, like if you try to paste

(Base) julia> throwto

Theoretically, the module prefix could be detected and the result evaluated into the module but that can be done in another PR.

From what I understand the following has to be done:

  • Add some tests
  • Add something to the docs about this

base/client.jl Outdated
Set `mod` as the default contextual module in the REPL,
both for evaluating expressions and printing them.
"""
function activate_module(mod::Module=Main)
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Maybe we can make this a bit shorter? Just for convenience if you want to swap module often. Could be just activate since the module is implicit with the argument type (would also mirror Pkg.activate?

Copy link
Member Author

Choose a reason for hiding this comment

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

activate sounds fine but then it might be preferable to not export it (i.e. call it Base.activate) as activate is quite a generic verb?

Copy link
Sponsor Member

@KristofferC KristofferC Oct 4, 2020

Choose a reason for hiding this comment

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

Oh, I was sure this was in the REPL stdlib but it is not. I really think it should be REPL.activate(_module).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I also thought REPL.activate would be nice after reading your comment, but it updates Base.active_repl ... But sure it's easy enough to call it REPL.activate :)

Copy link
Sponsor Member

@KristofferC KristofferC Oct 5, 2020

Choose a reason for hiding this comment

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

Yeah, but that is arguably irrelevant from a user's point of view.

@rfourquet
Copy link
Member Author

Theoretically, the module prefix could be detected and the result evaluated into the module but that can be done in another PR.

Yes good point. I will look into that eventually. And yes this needs docs, tests and news, but first approval ;-)

@rfourquet
Copy link
Member Author

There is another use-case that I forgot to mention: with the proper definitions in your startup.jl (that I will put here if merged), you can kinda emulate the old workspace() function: I have a keyboard shortuct to create a new "root module" (like Main) on the fly and switch to it, so you effectively end up with a clean space (what you don't get is deletion of variables in your original Main, which can take-up space), and another couple shortcuts to navigate between all these created modules.

@KristofferC
Copy link
Sponsor Member

you can kinda emulate the old workspace() function: I

Yeah, that's nice! I miss that. It also makes sense that it now is a REPL feature instead of before where the actual Main module was replaced (which is much more intrusive).

so you effectively end up with a clean space (what you don't get is deletion of variables in your original Main, which can take-up space), and another couple shortcuts to navigate between all these created modules.

FWIW, the original workspace() didn't do this either:

julia> a = 3
3

julia> workspace()

julia> LastMain.a
3

I don't think it is a big deal.

@KristofferC
Copy link
Sponsor Member

KristofferC commented Oct 4, 2020

And yes this needs docs, tests and news, but first approval ;-)

Well, I approve. So now we are two. Should be enough? 😄

@rfourquet
Copy link
Member Author

Alright, I will finish it up then :)

@KristofferC
Copy link
Sponsor Member

KristofferC commented Oct 7, 2020

One question, you activate Base, now how can you go back since you don't have access to REPL anymore?

@rfourquet
Copy link
Member Author

One question, you activate Base, now how can you go back since you don't have access to REPL anymore?

Yes, having played a bit with moving the activate function in the REPL module, it's a bit annoying. I have a startup.jl shortcut (which import REPL) to go back, but otherwise switching back and forth is unconvenient, I certainly had a couple warning that REPL is not a dependency when using it, and ended up using Base.active_repl.mistate.active_module = ... which kinda defeats the purpose of REPL.activate which is supposed to abstract this away...

@rfourquet
Copy link
Member Author

I updated with few tests, news and docs. I put back activate_module as an exported function in Base, but (temporarily) kept the alias REPL.activate if people want to play with both alternatives.

I also changed the prompt to Mod> as suggested by @andyferris , but only in the Julia mode; in help mode etc., it would still be (Mod) help> . The idea being that (Mod) julia> is verbose and doesn't add much value over simply Mod>. (Just experimenting here, feedback welcome).

More seriously, It would be good if at least a pair of eyes go through the change, which touches part of Julia that I'm not familiar with. Also, I didn't even attempt to update some parts which contain a hard-coded Main, like in the Distributed stdlib, when I really don't know the code nor have experience with using it. Maybe this activate_module should be labelled as experimental, as it's hard (above my capacity right now) to understand and test all implications of this change.

@KristofferC
Copy link
Sponsor Member

I don't get the __module__ === Base.active_module() checks. It seems active_module() will set the currently active module to Main and return nothing. So this will never be true and also has a side effect of setting the module.

@rfourquet
Copy link
Member Author

It seems active_module() will set the currently active module to Main and return nothing

No, you are describing activate_module(), whereas Base.active_module() is just a glorified shortcut for Base.active_repl.mistate.active_module, and is not exported (cf. https://github.com/JuliaLang/julia/pull/33872/files#diff-b4267643a238b5a375b3e055817ee6caR444). Admitedly, the names are not different enough.

@KristofferC
Copy link
Sponsor Member

Ah, I see now. My bad,

@@ -20,12 +20,12 @@ using Unicode: normalize
## Help mode ##

# This is split into helpmode and _helpmode to easier unittest _helpmode
helpmode(io::IO, line::AbstractString) = :($REPL.insert_hlines($io, $(REPL._helpmode(io, line))))
helpmode(line::AbstractString) = helpmode(stdout, line)
helpmode(io::IO, line::AbstractString, mod::Module) = :($REPL.insert_hlines($io, $(REPL._helpmode(io, line, mod))))
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

The tests in REPL calls these functions explicitly so either these need to be given default arguments for mod or just change the tests.

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 right thank you, I had only run the repl.jl tests locally.

@rfourquet
Copy link
Member Author

Yesterday's triage was in favor, but before merging this, @JeffBezanson might want to fix some instances of hard-coded references to Main in Base, like in the "show.jl" file (which can be regarded as a bug, code should depend on the :module attribute of the IOContext).

Also, as Base is moving towards having less global state and in particular needs to be independent of the REPL module, having a Base.activate_module function is not acceptable. As using REPL.activate is not convenient enough (as discussed above), a more interactive feature should be used. @KristofferC proposed to use a similar functionality as what allows you to @edit a function from a stacktrace with ^Q^Q after typing the number. Another idea could be to have a TerminalMenu showing the list of modules reachable from the current active module.

I added support for using the "Alt-mm" ("Alt-m" followed by "m") keybinding to activate the module whose name (possibly with qualification, e.g. "Main.Base") is written under the cursor.
I think just "Alt-m" would be too easy to type accidentally, but also there could be a handful of functionalities related to switching modules which could be bound to a keybinding starting with "Alt-m" (then "Alt-m" must be reserved for being a keybinding prefix).

isempty(word) && return beep(s)
try
REPL.activate(Base.Core.eval(Base.active_module(),
Base.Meta.parse(word)))
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: add a note that Core.eval is used, because Mod.eval(e) would work fine in general but not when Mod is Core.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

mod.eval is also (nearly) always wrong, so I'm not sure it makes sense to comment about that in particular

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, will not add a comment :) But I thought that mod.eval is nearly always correct, except for bare modules (hence incorrrect in the general case).

@rfourquet rfourquet force-pushed the rf/repl/switch-module branch 2 times, most recently from 4670c0f to 92e3d27 Compare October 18, 2020 09:38
@StefanKarpinski
Copy link
Sponsor Member

Would be great to get this feature at some point.

@rfourquet
Copy link
Member Author

Would be great to get this feature at some point.

Agreed! Maybe we could merge it early in the 1.7 cycle to have time to play with it before releasing it (or just release it as experimental)

@rfourquet
Copy link
Member Author

I just realized that this feature might help alleviate the long-standing Revise limitation that structs can't be modifed. The following seems to work:

  1. includet(path) (in what I tried, path is a path to the main source file "Package.jl" of package Package)
  2. update a struct (then Revise warns)
  3. make Revise untrack path (I don't know of the proper way, so I just did empty!(Revise.watched_files) and empty!(Revise.queue_errors)
  4. create a new module Main2, and activate it with this PR
  5. back to step 1 (includet(Main2, path)), now the updated struct is tracked properly

I don't know enough about the internals, and am not sure this is sound: @timholy would Revise be able to adjust to this workflow in principle?
Of course, the user would then get an empty workspace and would have to recreate a desired state, but in some cases that could be preferable compared to restarting the REPL (e.g. if your state is restored by loading file). It would also be easy enough to add a function e.g. to reload the last n entries of the REPL, or something like that.

@timholy
Copy link
Sponsor Member

timholy commented Nov 30, 2020

@timholy would Revise be able to adjust to this workflow in principle?

Yeah, I think that should be OK. While this is only part of the puzzle (interacting packages are still going to struggle with struct updates), in practice it should help a lot of cases.

Let me know if want a review and I'll look it over.

@rfourquet
Copy link
Member Author

Let me know if want a review and I'll look it over.

Thank you for your feedback and review offer! But reviewing would not be required now, as there is no Revise-specific bit in this PR, and anyway it won't be merged now .

@KristofferC
Copy link
Sponsor Member

I rebased (pretty sure I messed up some commits but it will get squashed in the end anyway) this (and made a few tweaks to it). I think this is a really great piece of functionality. While the extra statefulness in Base is slightly annoying, it isn't really worse than hardcoding Main everywhere in my opinion. So I think we can live with that until the whole special case of Main (or now the "active module") goes away.

@KristofferC KristofferC changed the title [WIP] REPL: allow switching contextual module REPL: allow switching contextual module Jun 2, 2022
@KristofferC
Copy link
Sponsor Member

I think this is good to go. I've been playing around with it locally for quite a bit and it is very nice. I aim to merge this in a couple of days unless anything new comes up. Comments welcome.

@KristofferC KristofferC merged commit 803f90d into master Jun 6, 2022
@KristofferC KristofferC deleted the rf/repl/switch-module branch June 6, 2022 09:30
@davidanthoff
Copy link
Contributor

Is there a way that the VS Code extension can be informed if this happens, i.e. some sort of hook? It would be nice if we could sync the existing UI in VS Code for module selection with the state in the REPL.

CC @pfitzseb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib:REPL Julia's REPL (Read Eval Print Loop)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants