WIP: allow changing context module in REPL #33102

aviatesk · 2019-08-28T12:16:30Z

UPDATED

After a discussion, I decided to add a functionality to change the REPL context module for execution, completion, and docs, in this PR

How works

The context module can be set via calling REPL.set_context_module(mod), and then execution/completion/show docs/display are all done in the context of mod

TODO

add tests
- change module in the context of execution
- change module in the context of completion
- change module in the context of help
update docs ?: the method signature of eval_user_input has been changed and it appears in some docs like stack trace, and profiling.

[The original description]

Purpose

Allow REPL [tab] completions in a module context other than Main.

Description

Currently REPL's [tab] completions work only in the context of the Main module, but there are cases when we want to get completions in a context of the other specific module:
E.g.: Juno can change REPL's module so that an entered code gets evaluated in a context of that module, and in this case we may want to get completions in the context of the module as well.

With this PR, each REPL provider could call REPL.set_context_module(mod) hooking on its module change, and then can get the corresponding completions.

If this idea is acceptable, I would really like to add tests for that, of course.

/cc @pfitzseb

KristofferC · 2019-08-28T12:21:53Z

This is already implemented by calling

REPLCompletions.completions(string, pos, context_module=Main)

julia/stdlib/REPL/src/REPLCompletions.jl

Line 573 in 8093a7c

function completions(string, pos, context_module=Main)::Completions

See it in use in IJulia:

https://github.com/JuliaLang/IJulia.jl/blob/92b01a291519ecf68fbb58019c29b7747044584d/src/handlers.jl#L125

But you mean that you want to be able to set that module globally for a REPL?

aviatesk · 2019-08-28T12:26:49Z

Thanks for the quick reply.
Yeah, I know, but that's not enough since tab completions are hooked at https://github.com/JuliaLang/julia/blob/master/stdlib/REPL/src/LineEdit.jl#L1826 and can't be changed outside of base as far as I understand.

pfitzseb · 2019-08-28T12:32:13Z

It would generally be useful to be able to set the REPL module (which Juno already allows you to do), both for execution and completions.
Only changing the context for completions on its own doesn't make too much sense, I think.

aviatesk · 2019-08-28T12:40:30Z

It would generally be useful to be able to set the REPL module (which Juno already allows you to do), both for execution and completions.

It would make more sense, and I'm willing to implement that if it sounds good to the other maintainers as well.

AFAIU the current Juno's approach to change module context for REPL execution couldn't be applied to REPL completions (or could be done in a dirty way with lots of copy and paste in a same way as https://github.com/JunoLab/Atom.jl/blob/master/src/Atom.jl#L22), thus I think it would be better to be implemented here.

pfitzseb · 2019-08-28T13:08:07Z

AFAICT you only need to change this line plus some display logic below to use context_module.
That would still be a somewhat hacky solution though, and I think it would be better to pipe the current module through the backend channel instead, so that each request is associated with the correct module.

aviatesk · 2019-08-28T14:42:16Z

@pfitzseb

That would still be a somewhat hacky solution though, and I think it would be better to pipe the current module through the backend channel instead, so that each request is associated with the correct module.

Thanks for the pointers. Piping modules through REPLBackend sounds better.

@KristofferC
So adding the functionality to change the module context for execution and completions (and maybe for helps) would be the preferred, after all ?
If so I'm going to change the PR title and get to the work.

aviatesk · 2019-08-29T19:22:20Z

Before proceeding to write tests, I would really like to hear your comments on those updates: #33102 (comment)
(for the general purpose of this PR and the current implementation approach or something :))

@KristofferC @pfitzseb

aviatesk · 2019-08-31T18:48:18Z

@pfitzseb @KristofferC @StefanKarpinski
I finished up the work for now, and would like to hear your comments. The build failures seems to be irrelevant to the changes I made.

aviatesk · 2019-08-31T19:06:10Z

stdlib/REPL/src/docview.jl

@@ -229,7 +229,8 @@ repl_search(s) = repl_search(stdout, s)

 function repl_corrections(io::IO, s)
    print(io, "Couldn't find ")
-    printstyled(io, s, '\n', color=:cyan)
+    pre = context_module == Main ? [] : [context_module, '.']


Here I tried to mimic the behaviour introduced in #23806. It's not exactly same, but kind of consistent IMO.

aviatesk · 2019-08-31T19:08:07Z

stdlib/REPL/test/repl.jl

@@ -1032,3 +1032,50 @@ fake_repl() do stdin_write, stdout_read, repl
    wait(repltask)
    @test istaskdone(repltask)
 end
+
+# context_module changing #33102


I'm feeling like I want to add more tests here, and would really appreciate if you give me your ideas to make this test more effective.

pfitzseb

IMHO this looks good in general, but a couple of things to think about:

The whole context_module global is a bit messy as is -- I think it would be a bit cleaner (and much more complex) to only use the global in the frontend and put the current module into REPLDisplay and REPLCompletionProvider, so that those don't rely on any globals. Haven't thought this through in detail, so it might a) not be much better and b) infeasible ;)
It would be cool to have a minimal REPL mode that allows changing the current module without weird Main.Base.set_context_module invocations (which would be necessary in a baremodule). That mode could also display the current module.
Should we change the julia> prompt to show the current module? Or change its color when you're not in Main? I feel like people are going to be confused without some indication that you're not in the default module.

pfitzseb · 2019-09-04T08:56:54Z

stdlib/REPL/src/REPL.jl

@@ -59,6 +59,11 @@ answer_color(::AbstractREPL) = ""

 const JULIA_PROMPT = "julia> "

+context_module = Main


const context_module = Ref{Module}(Main)

seems better here.

Definitely, I just pushed the commit to use Ref for context_module :)

stdlib/REPL/src/REPL.jl

KristofferC · 2019-09-04T09:28:12Z

Note that we have this global object to hold options:

julia/stdlib/REPL/src/REPL.jl

Lines 263 to 311 in a9d4eac

    
           mutable struct Options 
        
               hascolor::Bool 
        
               extra_keymap::Union{Dict,Vector{<:Dict}} 
        
               # controls the presumed tab width of code pasted into the REPL. 
        
               # Must satisfy `0 < tabwidth <= 16`. 
        
               tabwidth::Int 
        
               # Maximum number of entries in the kill ring queue. 
        
               # Beyond this number, oldest entries are discarded first. 
        
               kill_ring_max::Int 
        
               region_animation_duration::Float64 
        
               beep_duration::Float64 
        
               beep_blink::Float64 
        
               beep_maxduration::Float64 
        
               beep_colors::Vector{String} 
        
               beep_use_current::Bool 
        
               backspace_align::Bool 
        
               backspace_adjust::Bool 
        
               confirm_exit::Bool # ^D must be repeated to confirm exit 
        
               auto_indent::Bool # indent a newline like line above 
        
               auto_indent_tmp_off::Bool # switch auto_indent temporarily off if copy&paste 
        
               auto_indent_bracketed_paste::Bool # set to true if terminal knows paste mode 
        
               # cancel auto-indent when next character is entered within this time frame : 
        
               auto_indent_time_threshold::Float64 
        
           end 
        
           Options(; 
        
                   hascolor = true, 
        
                   extra_keymap = AnyDict[], 
        
                   tabwidth = 8, 
        
                   kill_ring_max = 100, 
        
                   region_animation_duration = 0.2, 
        
                   beep_duration = 0.2, beep_blink = 0.2, beep_maxduration = 1.0, 
        
                   beep_colors = ["\e[90m"], # gray (text_colors not yet available) 
        
                   beep_use_current = true, 
        
                   backspace_align = true, backspace_adjust = backspace_align, 
        
                   confirm_exit = false, 
        
                   auto_indent = true, 
        
                   auto_indent_tmp_off = false, 
        
                   auto_indent_bracketed_paste = false, 
        
                   auto_indent_time_threshold = 0.005) = 
        
                       Options(hascolor, extra_keymap, tabwidth, 
        
                               kill_ring_max, region_animation_duration, 
        
                               beep_duration, beep_blink, beep_maxduration, 
        
                               beep_colors, beep_use_current, 
        
                               backspace_align, backspace_adjust, confirm_exit, 
        
                               auto_indent, auto_indent_tmp_off, auto_indent_bracketed_paste, auto_indent_time_threshold) 
        
           # for use by REPLs not having an options field 
        
           const GlobalOptions = Options()

Perhaps the context_module would fit in there? Not sure.

pfitzseb · 2019-09-04T09:32:42Z

So the reason why it's dangerous to have context_module as a global is that it can change at any time (e.g. when you switch the module in Juno), even between the backend evaluating a request and the result of that being displayed. To get this fully correct we can only access the global when initiating the request in the frontend, and then everything else (eval, completions, display) needs to use that, regardless of what value the global has now.

aviatesk · 2019-09-12T08:59:30Z

@pfitzseb, thanks for your thoughtful review !

So the reason why it's dangerous to have context_module as a global is that it can change at any time (e.g. when you switch the module in Juno), even between the backend evaluating a request and the result of that being displayed. To get this fully correct we can only access the global when initiating the request in the frontend, and then everything else (eval, completions, display) needs to use that, regardless of what value the global has now.

Really agree with this idea, but

The whole context_module global is a bit messy as is -- I think it would be a bit cleaner (and much more complex) to only use the global in the frontend and put the current module into REPLDisplay and REPLCompletionProvider, so that those don't rely on any globals. Haven't thought this through in detail, so it might a) not be much better and b) infeasible ;)

So "only using the global in the frontend" would mean to handle context_module in respond and pass it to eval_with_backend, and print_response, right ?
Actually I don't have a solid understanding of what does the "frontend" here mean.

It would be cool to have a minimal REPL mode that allows changing the current module without weird Main.Base.set_context_module invocations (which would be necessary in a baremodule). That mode could also display the current module.

Yeah, sounds nice to me.
And more, the mode (I gonna call it as "Module mode") would allow an user to change context_module even for modules that are not imported to the current module, with the same logic as we change the module for workspace in Juno.

Should we change the julia> prompt to show the current module? Or change its color when you're not in Main? I feel like people are going to be confused without some indication that you're not in the default module.

Would be cool as well.

aviatesk · 2019-09-12T09:02:53Z

Note that we have this global object to hold options:

julia/stdlib/REPL/src/REPL.jl

Lines 263 to 311 in a9d4eac

mutable struct Options

hascolor::Bool

extra_keymap::Union{Dict,Vector{<:Dict}}

# controls the presumed tab width of code pasted into the REPL.

# Must satisfy `0 < tabwidth <= 16`.

tabwidth::Int

# Maximum number of entries in the kill ring queue.

# Beyond this number, oldest entries are discarded first.

kill_ring_max::Int

region_animation_duration::Float64

beep_duration::Float64

beep_blink::Float64

beep_maxduration::Float64

beep_colors::Vector{String}

beep_use_current::Bool

backspace_align::Bool

backspace_adjust::Bool

confirm_exit::Bool # ^D must be repeated to confirm exit

auto_indent::Bool # indent a newline like line above

auto_indent_tmp_off::Bool # switch auto_indent temporarily off if copy&paste

auto_indent_bracketed_paste::Bool # set to true if terminal knows paste mode

# cancel auto-indent when next character is entered within this time frame :

auto_indent_time_threshold::Float64

end

Options(;

hascolor = true,

extra_keymap = AnyDict[],

tabwidth = 8,

kill_ring_max = 100,

region_animation_duration = 0.2,

beep_duration = 0.2, beep_blink = 0.2, beep_maxduration = 1.0,

beep_colors = ["\e[90m"], # gray (text_colors not yet available)

beep_use_current = true,

backspace_align = true, backspace_adjust = backspace_align,

confirm_exit = false,

auto_indent = true,

auto_indent_tmp_off = false,

auto_indent_bracketed_paste = false,

auto_indent_time_threshold = 0.005) =

Options(hascolor, extra_keymap, tabwidth,

kill_ring_max, region_animation_duration,

beep_duration, beep_blink, beep_maxduration,

beep_colors, beep_use_current,

backspace_align, backspace_adjust, confirm_exit,

auto_indent, auto_indent_tmp_off, auto_indent_bracketed_paste, auto_indent_time_threshold)

# for use by REPLs not having an options field

const GlobalOptions = Options()

Perhaps the context_module would fit in there? Not sure.

If we are okay with disabling the context module changing within BasicREPL and StreamREPL, I think we can make Options hold context_module.

aviatesk · 2022-06-06T10:31:12Z

I think this is superseded by #33872.

rfourquet · 2023-01-17T12:34:39Z

Sorry for having started #33872 without checking (or finding) that there was already another attempt...

aviatesk force-pushed the context-module-for-tab-completion branch from 1f1e0b2 to bb6c268 Compare August 28, 2019 12:19

aviatesk force-pushed the context-module-for-tab-completion branch from bb6c268 to 705a4f2 Compare August 29, 2019 18:55

aviatesk changed the title ~~allow context module in REPL tab completions~~ allow changing context module in REPL Aug 29, 2019

aviatesk changed the title ~~allow changing context module in REPL~~ WIP: allow changing context module in REPL Aug 29, 2019

fredrikekre added the REPL Julia's REPL (Read Eval Print Loop) label Aug 29, 2019

aviatesk force-pushed the context-module-for-tab-completion branch 4 times, most recently from 203abc1 to 01419e1 Compare August 31, 2019 14:27

aviatesk changed the title ~~WIP: allow changing context module in REPL~~ RFC: allow changing context module in REPL Aug 31, 2019

aviatesk commented Aug 31, 2019

View reviewed changes

aviatesk force-pushed the context-module-for-tab-completion branch from 4d1d9aa to 57fe646 Compare September 3, 2019 19:23

pfitzseb reviewed Sep 4, 2019

View reviewed changes

aviatesk changed the title ~~RFC: allow changing context module in REPL~~ WIP: allow changing context module in REPL Sep 5, 2019

aviatesk added 5 commits September 11, 2019 18:14

allow context module in REPL execution/completions

4941491

add context module for REPL help/docs

835b1f0

add tests for REPL context module change

7f12cee

change signature of eval_user_input in profile/stacktraces docs

f430581

use Ref for global context_module

0cccfcf

aviatesk force-pushed the context-module-for-tab-completion branch from 57fe646 to 0cccfcf Compare September 11, 2019 11:16

aviatesk mentioned this pull request Sep 17, 2019

enhance completions within the integrated REPL JunoLab/Atom.jl#180

Merged

aviatesk closed this Jun 6, 2022

aviatesk deleted the context-module-for-tab-completion branch June 6, 2022 10:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: allow changing context module in REPL #33102

WIP: allow changing context module in REPL #33102

aviatesk commented Aug 28, 2019 •

edited

Loading

KristofferC commented Aug 28, 2019 •

edited

Loading

aviatesk commented Aug 28, 2019 •

edited

Loading

pfitzseb commented Aug 28, 2019

aviatesk commented Aug 28, 2019 •

edited

Loading

pfitzseb commented Aug 28, 2019

aviatesk commented Aug 28, 2019 •

edited

Loading

aviatesk commented Aug 29, 2019

aviatesk commented Aug 31, 2019

aviatesk Aug 31, 2019

aviatesk Aug 31, 2019

pfitzseb left a comment

pfitzseb Sep 4, 2019

aviatesk Sep 11, 2019

KristofferC commented Sep 4, 2019

pfitzseb commented Sep 4, 2019

aviatesk commented Sep 12, 2019

aviatesk commented Sep 12, 2019

aviatesk commented Jun 6, 2022

rfourquet commented Jan 17, 2023

		@@ -59,6 +59,11 @@ answer_color(::AbstractREPL) = ""

		const JULIA_PROMPT = "julia> "

		context_module = Main

WIP: allow changing context module in REPL #33102

WIP: allow changing context module in REPL #33102

Conversation

aviatesk commented Aug 28, 2019 • edited Loading

UPDATED

How works

TODO

Purpose

Description

KristofferC commented Aug 28, 2019 • edited Loading

aviatesk commented Aug 28, 2019 • edited Loading

pfitzseb commented Aug 28, 2019

aviatesk commented Aug 28, 2019 • edited Loading

pfitzseb commented Aug 28, 2019

aviatesk commented Aug 28, 2019 • edited Loading

aviatesk commented Aug 29, 2019

aviatesk commented Aug 31, 2019

aviatesk Aug 31, 2019

Choose a reason for hiding this comment

aviatesk Aug 31, 2019

Choose a reason for hiding this comment

pfitzseb left a comment

Choose a reason for hiding this comment

pfitzseb Sep 4, 2019

Choose a reason for hiding this comment

aviatesk Sep 11, 2019

Choose a reason for hiding this comment

KristofferC commented Sep 4, 2019

pfitzseb commented Sep 4, 2019

aviatesk commented Sep 12, 2019

aviatesk commented Sep 12, 2019

aviatesk commented Jun 6, 2022

rfourquet commented Jan 17, 2023

aviatesk commented Aug 28, 2019 •

edited

Loading

KristofferC commented Aug 28, 2019 •

edited

Loading

aviatesk commented Aug 28, 2019 •

edited

Loading

aviatesk commented Aug 28, 2019 •

edited

Loading

aviatesk commented Aug 28, 2019 •

edited

Loading