-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Add REPL-completion for keyword arguments #43536
Conversation
f6b804b
to
52ad27f
Compare
It's a bit tricky to follow logic by code review, so I played with this PR myself. What I found so far is: julia> foo(a; foo=0) = println(a, foo);
julia> obj = (; foo = 42);
julia> foo(1; obj.| # => completes to `foo(1; foo`, which doesn't seem intended Also, how about suggesting keyword completions even if we haven't pressed any preceding characters? julia> foo(1; | # => suggests `key=` ? |
52ad27f
to
b25b696
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
b25b696
to
c7a7c7c
Compare
I just want to point out that the feature implemented by this PR cover 100% my initial request! Thanks @Liozou for the implementation :) |
c7a7c7c
to
fbc57f7
Compare
Rebased on top of #43577 to fix a conflicting test |
fbc57f7
to
5ed2042
Compare
e4b1136
to
5fedd87
Compare
With #43577 and #43865 merged, most of the previous conversation is moot, and I updated the PR since. So here is the current (hopefully final) version of this PR: Commits
All three main changes are implemented in three separate commits, for easier review. These also include tests. ExampleAn example to illustrate some of these changes. With the current PR: julia> foo(a::Int, b, args...; kwarg) = nothing;
julia> foo(3, 4, 5, 6, kw #TAB
julia> foo(3, 4, 5, 6, kwarg= # (replaced inline). Same behaviour with a semicolon.
julia> foo(1; #TAB -> nothing whereas the current state is: julia> foo(a::Int, b, args...; kwarg) = nothing;
julia> foo(3, 4, 5, 6; kw #TAB -> nothing, no keyword argument completion
julia> foo(1; #TAB
foo(a::Int64, b, args...; kwarg) in Main at REPL[1]:1 # should not appear because foo requires at least two arguments CI failure looks unrelated. Could I bother you @aviatesk for another review? |
sure, I will try to have another review before 1.8 feature freeze (next Monday). |
5fedd87
to
d46fff5
Compare
Rebased on top of #44166 to fix a merge conflict |
45eccad
to
5f5ba00
Compare
This comment was marked as outdated.
This comment was marked as outdated.
There is an issue with the current state of this PR because of a poor interplay between optional and keyword arguments, for instance in the following example: julia> foo(a, bar=12, qux=""; kwarg) = nothing
foo (generic function with 3 methods)
julia> foo(3; kwa#TAB ---> no completion! The reason is that there is no method that appears to match both the presence of keyword arguments and the absence of optional arguments in the following list: julia> foo( #TAB
foo(a) in Main at REPL[1]:1
foo(a, bar) in Main at REPL[1]:1
foo(a, bar, qux; kwarg) in Main at REPL[1]:1 Of course the call would be valid because it would be dispatched to the appropriate method through the kwsorter, but there is no way to match the methods of the kwsorter with the appropriate keyword arguments (I think). For instance, there is no mention of julia> methods(methods(foo).mt.kwsorter)
# 3 methods for anonymous function "foo##kw":
[1] (var"#s25"::var"#foo##kw")(::Any, ::typeof(foo), a) in Main at REPL[1]:1
[2] (var"#s25"::var"#foo##kw")(::Any, ::typeof(foo), a, bar) in Main at REPL[1]:1
[3] (var"#s25"::var"#foo##kw")(::Any, ::typeof(foo), a, bar, qux) in Main at REPL[1]:1 So the solution I found at this moment is #44434, I'll wait on comments on that PR before updating the current one. |
what's the status of this PR? |
Very interested in this PR |
As mentioned in the comment before, I believe #44434 is a prerequisite for this because many kwarg completions will unexpectedly fail otherwise, which is arguably worse than having no completion at all. So I'm still waiting on that PR to be discussed before updating this one (with both a rebase on master and taking into account the review comments above). |
to decouple those 2 PR efforts, I think here we might be better to treat any |
Sure. I'd rather avoid having to roll back and forth between rebases and such so of course it would be ideal to know whether #44434 will go through to plan ahead, but since there is not much traction there I'll try to do that in the meantime (hopefully this week-end). |
5f5ba00
to
878305d
Compare
Rebase done! So, I removed all the stuff that prevented kwarg completion when the call signature did not correspond to the method, because it would lead to false-negatives (i.e. some completions missing, see comment above). As a consequence, we now have false-positives (i.e. some completions provided when they should not) but no false-negatives, for example: julia> foo(a::Int, bar=12, qux=""; kwarg) = nothing
foo (generic function with 3 methods)
julia> foo(387; kwa#TAB
julia> foo(387; kwarg= # expected completion, great!
julia> foo("a"; kwa#TAB
julia> foo("a"; kwarg= # false-positive, this should not complete. For safe-keeping, the previous state of the PR is available at master...Liozou:julia:replcompletekwargs_save, including additional handling of splats that should come in a separate PR. I added the tests that passed there but not here as comments with a "TODO:" instead of |
# First, check that the last punctuation is either ',', ';' or '(' | ||
idx_last_punct = something(findprev(x -> ispunct(x) && x != '_' && x != '!', partial, last_idx), 0)::Int | ||
idx_last_punct == 0 && return fail | ||
last_punct = partial[idx_last_punct] | ||
last_punct == ',' || last_punct == ';' || last_punct == '(' || return fail | ||
|
||
# Then, check that `last_punct` is only followed by an identifier or nothing | ||
before_last_word_start = something(findprev(in(non_identifier_chars), partial, last_idx), 0) | ||
before_last_word_start == 0 && return fail | ||
all(isspace, @view partial[nextind(partial, idx_last_punct):before_last_word_start]) || return fail | ||
|
||
# Check that `last_punct` is either the last '(' or placed after a previous '(' | ||
frange, method_name_end = find_start_brace(@view partial[1:idx_last_punct]) | ||
method_name_end ∈ frange || return fail | ||
|
||
# Strip the preceding ! operators, if any, and close the expression with a ')' | ||
s = replace(partial[frange], r"\G\!+([^=\(]+)" => s"\1"; count=1) * ')' | ||
ex = Meta.parse(s, raise=false, depwarn=false) | ||
isa(ex, Expr) || return fail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems unfortunate we need all this code, since it seems like a slightly less complete version of
julia/stdlib/REPL/src/REPLCompletions.jl
Lines 905 to 926 in 9bdaabd
while 0 < i | |
c = string[i] | |
if c in (')', ']') | |
if c == ')' | |
c_start = '(' | |
c_end = ')' | |
elseif c == ']' | |
c_start = '[' | |
c_end = ']' | |
end | |
frange, end_of_identifier = find_start_brace(string[1:prevind(string, i)], c_start=c_start, c_end=c_end) | |
isempty(frange) && break # unbalanced parens | |
startpos = first(frange) | |
i = prevind(string, startpos) | |
elseif c in ('\'', '\"', '\`') | |
s = "$c$c"*string[startpos:pos] | |
break | |
else | |
break | |
end | |
s = string[startpos:pos] | |
end |
followed by a copy of
julia/stdlib/REPL/src/REPLCompletions.jl
Lines 831 to 834 in 9bdaabd
frange, method_name_end = find_start_brace(partial) | |
# strip preceding ! operator | |
s = replace(partial[frange], r"\!+([^=\(]+)" => s"\1") | |
ex = Meta.parse(s * ")", raise=false, depwarn=false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, I see the purpose is a bit different, since we are just looking for whether there is something quite trivial after the last punctuation, and the other code (and should_method_complete) is moved here now
complete_methods!(methods, funct, Any[Vararg{Any}], kwargs_ex, -1, kwargs_flag == 1) | ||
# TODO: use args_ex instead of Any[Vararg{Any}] and only provide kwarg completion for | ||
# method calls compatible with the current arguments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory, this is a pretty bad choice of methods to consider, since it is specifically only looking at methods without kwargs. And you never want to pass -1
here, as you can accidentally break interactivity for several minutes. So without those bad trailing args, and looking only at methods with kwargs, we want something like this:
# get the list of possible kw method table
kwt = try; Core.kwftype(funct); catch ex; ex isa ErrorException || rethrow(); return fail; end
ml = Completion[]
complete_methods!(ml, kwt, Any[Any, funct, args_ex...])
ml = Method[m.method for m in ml if m isa MethodCompletion]
# decide if we might have a `...` in the list
have_kwarg = any(isa(TextCompletion), ml)
for m in ml
kwli = m.method
slotnames = ccall(:jl_uncompress_argnames, Vector{Symbol}, (Any,), kwli.slot_syms)
have_kwarg = any(endswith("..."), slotnames)
have_kwarg && break
end
if have_kwarg
# if we have a `kwarg...`, instead return the union of all possible method kwargs for this function
methods = Base.MethodList(kwt.name.mt)
end
# now get the kwarg names, and filter them to just the matching ones
kwargs = Set{String}()
for m in ml
kwli = m.method
slotnames = ccall(:jl_uncompress_argnames, Vector{Symbol}, (Any,), kwli.slot_syms)
union!(kwargs, filter!(slotnames))
end
(kwargs_ex should not be passed here either, as that is sort of just an historical accident of past implementation problems)
I just stumbled across this in 1.9.0-beta4, awesome. But I don't see it Edit: My bad, it is in the docs. Created a PR (my first to JuliaLang/julia!) to add to the NEWS file. |
this was introduced in JuliaLang/julia#43536
Closes #43437 by implementing the feature.
The feature
I think it's a convenient non-disruptive feature to have, thank you @ronisbr for thinking of it! The behaviour in this branch:
As you can see, all suggested keyword arguments have a finishing
=
, both because it's useful to be able to distinguish them, and because that's probably what you want anyway (feel free to comment if you disagree!). If you want to use thefoo(; bar)
syntax, meaningfoo(; bar=bar)
, then it's fine as well since for that you needbar
to exist, so it will be part of the suggestions:The intentionally not-implemented
In the cases like the previous example, I thought about restricting the REPL suggestions to only those keyword arguments, since any argument following a semicolon or another keyword argument must itself be a keyword argument, and thus, be of the form
bar=barvalue
or justbar
ifbar
is both a known name and a keyword argument. But there is the special case of splat keyword arguments, of the formfoo(; kwargs...)
which is an exception sincekwargs
can be about anything. So I kept all the usual names for suggestions, I thought I'd mention the reasoning here for reference.Update
See #43536 (comment) below for a slightly more detailed view on what this PR implements.
(EDIT: I removed parts of this message which were not relevant anymore, mostly thanks to the PRs referenced below and some updates of this PR.)