Skip to content

repl: add a bunch of types to structs and methods #22377

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jun 19, 2017
Merged

repl: add a bunch of types to structs and methods #22377

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8566385
Terminals.jl: minor style tweaks
StefanKarpinski Jun 15, 2017
5431b5e
Terminals.jl: add AbstractTerminal supertype above TextTerminal
StefanKarpinski Jun 15, 2017
ce44871
base/repl: collect all repl-related file under one directory
StefanKarpinski Jun 15, 2017
f9a4b39
base/repl: make some mutable structs immutable
StefanKarpinski Jun 15, 2017
8181394
base/repl: add some types to structures
StefanKarpinski Jun 15, 2017
b3e6589
base/repl: add types on various fields and method arguments
StefanKarpinski Jun 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 33 additions & 36 deletions base/LineEdit.jl → base/repl/LineEdit.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,32 @@ abstract type ModeState end
export run_interface, Prompt, ModalInterface, transition, reset_state, edit_insert, keymap

struct ModalInterface <: TextInterface
modes
modes::Array{Base.LineEdit.TextInterface,1}
end

mutable struct Prompt <: TextInterface
prompt::String
# A string or function to be printed before the prompt. May not change the length of the prompt.
# This may be used for changing the color, issuing other terminal escape codes, etc.
prompt_prefix::Union{String,Function}
# Same as prefix except after the prompt
prompt_suffix::Union{String,Function}
keymap_dict::Dict{Char}
keymap_func_data # ::AbstractREPL
complete # ::REPLCompletionProvider
on_enter::Function
on_done::Function
hist # ::REPLHistoryProvider
sticky::Bool
end

show(io::IO, x::Prompt) = show(io, string("Prompt(\"", x.prompt, "\",...)"))

mutable struct MIState
interface::ModalInterface
current_mode
current_mode::TextInterface
aborted::Bool
mode_state
mode_state::Dict
kill_buffer::String
previous_key::Array{Char,1}
key_repeats::Int
Expand All @@ -33,31 +51,13 @@ function show(io::IO, s::MIState)
print(io, "MI State (", s.current_mode, " active)")
end

mutable struct Prompt <: TextInterface
prompt
# A string or function to be printed before the prompt. May not change the length of the prompt.
# This may be used for changing the color, issuing other terminal escape codes, etc.
prompt_prefix
# Same as prefix except after the prompt
prompt_suffix
keymap_dict
keymap_func_data
complete
on_enter
on_done
hist
sticky::Bool
end

show(io::IO, x::Prompt) = show(io, string("Prompt(\"", x.prompt, "\",...)"))

struct InputAreaState
num_rows::Int64
curs_row::Int64
end

mutable struct PromptState <: ModeState
terminal
terminal::AbstractTerminal
p::Prompt
input_buffer::IOBuffer
ias::InputAreaState
Expand All @@ -77,11 +77,8 @@ end
abstract type HistoryProvider end
abstract type CompletionProvider end

mutable struct EmptyCompletionProvider <: CompletionProvider
end

mutable struct EmptyHistoryProvider <: HistoryProvider
end
struct EmptyCompletionProvider <: CompletionProvider end
struct EmptyHistoryProvider <: HistoryProvider end

reset_state(::EmptyHistoryProvider) = nothing

Expand Down Expand Up @@ -970,15 +967,15 @@ function write_response_buffer(s::PromptState, data)
end

mutable struct SearchState <: ModeState
terminal
histprompt
terminal::AbstractTerminal
histprompt # ::HistoryPrompt
Copy link
Member

Choose a reason for hiding this comment

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

Why comment the type annotation here? (similar question in other places, e.g. with CompletionProvider)

Copy link
Member Author

Choose a reason for hiding this comment

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

Because that's the type in practice, but HistoryPrompt doesn't exist yet when this is defined. The reverse dependency is unfortunate and something that should be fixed, but that kind of refactoring is beyond the scope of this PR.

#rsearch (true) or ssearch (false)
backward::Bool
query_buffer::IOBuffer
response_buffer::IOBuffer
ias::InputAreaState
#The prompt whose input will be replaced by the matched history
parent
parent::Prompt
SearchState(terminal, histprompt, backward, query_buffer, response_buffer) =
new(terminal, histprompt, backward, query_buffer, response_buffer, InputAreaState(0,0))
end
Expand Down Expand Up @@ -1015,7 +1012,7 @@ end

mutable struct HistoryPrompt{T<:HistoryProvider} <: TextInterface
hp::T
complete
complete # ::CompletionProvider
keymap_dict::Dict{Char,Any}
HistoryPrompt{T}(hp) where T<:HistoryProvider = new(hp, EmptyCompletionProvider())
end
Expand All @@ -1024,16 +1021,16 @@ HistoryPrompt(hp::T) where T<:HistoryProvider = HistoryPrompt{T}(hp)
init_state(terminal, p::HistoryPrompt) = SearchState(terminal, p, true, IOBuffer(), IOBuffer())

mutable struct PrefixSearchState <: ModeState
terminal
histprompt
terminal::AbstractTerminal
histprompt # ::HistoryPrompt
prefix::String
response_buffer::IOBuffer
ias::InputAreaState
indent::Int
# The modal interface state, if present
mi
mi::MIState
#The prompt whose input will be replaced by the matched history
parent
parent::Prompt
PrefixSearchState(terminal, histprompt, prefix, response_buffer) =
new(terminal, histprompt, prefix, response_buffer, InputAreaState(0,0), 0)
end
Expand All @@ -1055,7 +1052,7 @@ input_string(s::PrefixSearchState) = String(s.response_buffer)
mutable struct PrefixHistoryPrompt{T<:HistoryProvider} <: TextInterface
hp::T
parent_prompt::Prompt
complete
complete # ::CompletionProvider
keymap_dict::Dict{Char,Any}
PrefixHistoryPrompt{T}(hp, parent_prompt) where T<:HistoryProvider =
new(hp, parent_prompt, EmptyCompletionProvider())
Expand Down
57 changes: 31 additions & 26 deletions base/REPL.jl → base/repl/REPL.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ using ..LineEdit
using ..REPLCompletions

export
AbstractREPL,
BasicREPL,
LineEditREPL,
StreamREPL
Expand Down Expand Up @@ -172,7 +173,7 @@ struct REPLBackendRef
response_channel::Channel
end

function run_repl(repl::AbstractREPL, consumer = x->nothing)
function run_repl(repl::AbstractREPL, consumer::Function = x->nothing)
repl_channel = Channel(1)
response_channel = Channel(1)
backend = start_repl_backend(repl_channel, response_channel)
Expand Down Expand Up @@ -254,8 +255,8 @@ mutable struct LineEditREPL <: AbstractREPL
in_help::Bool
envcolors::Bool
waserror::Bool
specialdisplay
interface
specialdisplay::Union{Void,Display}
interface::ModalInterface
backendref::REPLBackendRef
LineEditREPL(t,hascolor,prompt_color,input_color,answer_color,shell_color,help_color,history_file,in_shell,in_help,envcolors) =
new(t,true,prompt_color,input_color,answer_color,shell_color,help_color,history_file,in_shell,
Expand All @@ -266,20 +267,19 @@ specialdisplay(r::LineEditREPL) = r.specialdisplay
specialdisplay(r::AbstractREPL) = nothing
terminal(r::LineEditREPL) = r.t

LineEditREPL(t::TextTerminal, envcolors = false) = LineEditREPL(t,
true,
Base.text_colors[:green],
Base.input_color(),
Base.answer_color(),
Base.text_colors[:red],
Base.text_colors[:yellow],
false, false, false, envcolors)

mutable struct REPLCompletionProvider <: CompletionProvider; end

mutable struct ShellCompletionProvider <: CompletionProvider; end
LineEditREPL(t::TextTerminal, envcolors::Bool=false) =
LineEditREPL(t, true,
Base.text_colors[:green],
Base.input_color(),
Base.answer_color(),
Base.text_colors[:red],
Base.text_colors[:yellow],
false, false, false, envcolors
)
Copy link
Member

Choose a reason for hiding this comment

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

Odd indentation? Perhaps better the ) simply terminate the preceding line?

Copy link
Member Author

@StefanKarpinski StefanKarpinski Jun 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This is my preferred style for writing this kind of code. Clearly not universally agreed upon, but until we have an official style guide for this, I'm going to write it the way I prefer 😬

Copy link
Contributor

Choose a reason for hiding this comment

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

Also happens to be my and Invenia's preferred style :)

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough :).


struct LatexCompletions <: CompletionProvider; end
mutable struct REPLCompletionProvider <: CompletionProvider end
mutable struct ShellCompletionProvider <: CompletionProvider end
Copy link
Member

Choose a reason for hiding this comment

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

Seems you can remove the mutable here as you did for other types.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried that – they have finalizers, so it doesn't work.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok thanks

Copy link
Member

Choose a reason for hiding this comment

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

Need these two structs continue to be mutable? Above (around old lines 80-84) you changed a couple similar mutable structs to structs, so thought I might ask :).

struct LatexCompletions <: CompletionProvider end

beforecursor(buf::IOBuffer) = String(buf.data[1:buf.ptr-1])

Expand All @@ -305,16 +305,15 @@ function complete_line(c::LatexCompletions, s)
return ret, partial[range], should_complete
end


mutable struct REPLHistoryProvider <: HistoryProvider
history::Array{String,1}
history_file
history_file::Union{Void,IO}
start_idx::Int
cur_idx::Int
last_idx::Int
last_buffer::IOBuffer
last_mode
mode_mapping
last_mode::Union{Void,Prompt}
mode_mapping::Dict
modes::Array{Symbol,1}
end
REPLHistoryProvider(mode_mapping) =
Expand Down Expand Up @@ -619,7 +618,9 @@ end

backend(r::AbstractREPL) = r.backendref

send_to_backend(ast, backend::REPLBackendRef) = send_to_backend(ast, backend.repl_channel, backend.response_channel)
send_to_backend(ast, backend::REPLBackendRef) =
send_to_backend(ast, backend.repl_channel, backend.response_channel)

function send_to_backend(ast, req, rep)
put!(req, (ast, 1))
return take!(rep) # (val, bt)
Expand Down Expand Up @@ -661,7 +662,7 @@ function prepare_next(repl::LineEditREPL)
println(terminal(repl))
end

function mode_keymap(julia_prompt)
function mode_keymap(julia_prompt::Prompt)
AnyDict(
'\b' => function (s,o...)
if isempty(s) || position(LineEdit.buffer(s)) == 0
Expand Down Expand Up @@ -689,7 +690,11 @@ repl_filename(repl, hp) = "REPL"
const JL_PROMPT_PASTE = Ref(true)
enable_promptpaste(v::Bool) = JL_PROMPT_PASTE[] = v

function setup_interface(repl::LineEditREPL; hascolor = repl.hascolor, extra_repl_keymap = Dict{Any,Any}[])
function setup_interface(
repl::LineEditREPL;
hascolor::Bool = repl.hascolor,
extra_repl_keymap::Vector{<:Dict} = Dict{Any,Any}[]
Copy link
Member

Choose a reason for hiding this comment

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

did you mean ::Vector{<:Associative} instead of ::Vector{<:Dict} ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I could do that, but in practice only Dicts get stuck in there

Copy link
Member

Choose a reason for hiding this comment

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

Then why not Vector{Dict} ? The subtype operator on a concrete type looks confusing

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, I see what you're saying. Yes, that makes sense, not sure what I was thinking here.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's funny.

Pedantic question: do we say Dict is concrete without its type parameters? (asDict != Dict{Any,Any} - and if concrete Dict{Any,Any} always finds its way here in practice, I suppose there may be a minor performance difference too).

Copy link
Member

Choose a reason for hiding this comment

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

Oh I got confused, no 100% familiar yet with this {<:T} notation ;-)

)
Copy link
Member

Choose a reason for hiding this comment

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

The ) placement seems a bit unusual here as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is my preferred style, although obviously not everyone agrees.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also happens to be my and Invenia's preferred style :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Glad to know I'm not the only one 😁

###
#
# This function returns the main interface that describes the REPL
Expand Down Expand Up @@ -932,7 +937,7 @@ function setup_interface(repl::LineEditREPL; hascolor = repl.hascolor, extra_rep
ModalInterface([julia_prompt, shell_mode, help_mode, search_prompt, prefix_prompt])
end

function run_frontend(repl::LineEditREPL, backend)
function run_frontend(repl::LineEditREPL, backend::REPLBackendRef)
d = REPLDisplay(repl)
dopushdisplay = repl.specialdisplay === nothing && !in(d,Base.Multimedia.displays)
dopushdisplay && pushdisplay(d)
Expand Down Expand Up @@ -975,7 +980,7 @@ input_color(r::StreamREPL) = r.input_color

# heuristic function to decide if the presence of a semicolon
# at the end of the expression was intended for suppressing output
function ends_with_semicolon(line)
function ends_with_semicolon(line::AbstractString)
match = rsearch(line, ';')
if match != 0
# state for comment parser, assuming that the `;` isn't in a string or comment
Expand Down Expand Up @@ -1057,7 +1062,7 @@ function run_frontend(repl::StreamREPL, backend::REPLBackendRef)
dopushdisplay && popdisplay(d)
end

function start_repl_server(port)
function start_repl_server(port::Int)
Copy link
Contributor

Choose a reason for hiding this comment

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

::Integer ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's an internal function and if the value is too large, this won't work anyway, so Int is ok.

listen(port) do server, status
client = accept(server)
run_repl(client)
Expand Down
0 base/REPLCompletions.jl → base/repl/REPLCompletions.jl
View file
Open in desktop
File renamed without changes.
28 changes: 13 additions & 15 deletions base/Terminals.jl → base/repl/Terminals.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
module Terminals

export
AbstractTerminal,
TextTerminal,
UnixTerminal,
TerminalBuffer,
Expand Down Expand Up @@ -33,11 +34,15 @@ import Base:
read,
readuntil

## AbstractTerminal: abstract supertype of all terminals ##

abstract type AbstractTerminal <: Base.AbstractPipe end

## TextTerminal ##

abstract type TextTerminal <: Base.AbstractPipe end
abstract type TextTerminal <: AbstractTerminal end

# INTERFACE
# Terminal interface:
pipe_reader(::TextTerminal) = error("Unimplemented")
pipe_writer(::TextTerminal) = error("Unimplemented")
displaysize(::TextTerminal) = error("Unimplemented")
Expand Down Expand Up @@ -95,7 +100,7 @@ pipe_reader(t::UnixTerminal) = t.in_stream
pipe_writer(t::UnixTerminal) = t.out_stream

mutable struct TerminalBuffer <: UnixTerminal
out_stream::Base.IO
out_stream::IO
end

mutable struct TTYTerminal <: UnixTerminal
Expand All @@ -113,22 +118,17 @@ cmove_right(t::UnixTerminal, n) = write(t.out_stream, "$(CSI)$(n)C")
cmove_left(t::UnixTerminal, n) = write(t.out_stream, "$(CSI)$(n)D")
cmove_line_up(t::UnixTerminal, n) = (cmove_up(t, n); cmove_col(t, 1))
cmove_line_down(t::UnixTerminal, n) = (cmove_down(t, n); cmove_col(t, 1))
cmove_col(t::UnixTerminal, n) = (write(t.out_stream, '\r'); n > 1 && cmove_right(t, n - 1))
cmove_col(t::UnixTerminal, n) = (write(t.out_stream, '\r'); n > 1 && cmove_right(t, n-1))
Copy link
Member

Choose a reason for hiding this comment

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

Why change this?

Copy link
Member Author

Choose a reason for hiding this comment

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

To be consistent with the formatting in the rest of the file, which doesn't have spaces in small arithmetic expressions anywhere else.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, thanks for clarifying


if is_windows()
function raw!(t::TTYTerminal,raw::Bool)
check_open(t.in_stream)
if Base.ispty(t.in_stream)
run(if raw
`stty raw -echo onlcr -ocrnl opost`
else
`stty sane`
end,t.in_stream,t.out_stream,t.err_stream)
run((raw ? `stty raw -echo onlcr -ocrnl opost` : `stty sane`),
t.in_stream, t.out_stream, t.err_stream)
Copy link
Member

Choose a reason for hiding this comment

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

👍 :)

true
else
ccall(:jl_tty_set_mode,
Int32, (Ptr{Void},Int32),
t.in_stream.handle, raw) != -1
ccall(:jl_tty_set_mode, Int32, (Ptr{Void},Int32), t.in_stream.handle, raw) != -1
Copy link
Member

Choose a reason for hiding this comment

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

Missing space separating tuple arguments?

end
end
else
Expand All @@ -148,9 +148,7 @@ end
@eval clear_line(t::UnixTerminal) = write(t.out_stream, $"\r$(CSI)0K")
#beep(t::UnixTerminal) = write(t.err_stream,"\x7")

function Base.displaysize(t::UnixTerminal)
return displaysize(t.out_stream)
end
Base.displaysize(t::UnixTerminal) = displaysize(t.out_stream)

if is_windows()
hascolor(t::TTYTerminal) = true
Expand Down
0 base/emoji_symbols.jl → base/repl/emoji_symbols.jl
View file
Open in desktop
File renamed without changes.
0 base/latex_symbols.jl → base/repl/latex_symbols.jl
View file
Open in desktop
File renamed without changes.
8 changes: 4 additions & 4 deletions base/sysimg.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,10 +360,10 @@ using .I18n

# frontend
include("initdefs.jl")
include("Terminals.jl")
include("LineEdit.jl")
include("REPLCompletions.jl")
include("REPL.jl")
include("repl/Terminals.jl")
Copy link
Contributor

Choose a reason for hiding this comment

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

using joinpath if possible may avoid mixed forward and backslashes in backtraces on windows

Copy link
Member

Choose a reason for hiding this comment

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

I finally understand why I see people use joinpath in otherwise trivial cases! Wow.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd kind of prefer if we used forward slashes for joinpath and everything else even on Windows but that might be unlikely. Most Windows programs can handle forward slashes in paths just fine, but maybe not all.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should probably introduce Path types and have path"repl/Terminals.jl" produce whatever the native path syntax is. That would also allow path"C:\some\where" to work on Windows systems without double backslashes.

Copy link
Member Author

@StefanKarpinski StefanKarpinski Jun 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This matches the style of all other includes in sysimg.jl, so I'm going to leave it. Normalizing path names to included files would probably make sense. I suspect this is not simply a style issue since joinpath is not defined when much of this file runs.

include("repl/LineEdit.jl")
include("repl/REPLCompletions.jl")
include("repl/REPL.jl")
include("client.jl")

# Stack frames and traces
Expand Down
2 changes: 1 addition & 1 deletion doc/src/manual/interacting-with-julia.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ end
atreplinit(customize_keys)
```

Users should refer to `base/LineEdit.jl` to discover the available actions on key input.
Users should refer to `LineEdit.jl` to discover the available actions on key input.

## Tab completion

Expand Down