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

JuliaSyntax parser-based REPL completions overhaul #57767

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

xal-0
Copy link
Member

@xal-0 xal-0 commented Mar 13, 2025

Overview

As we add REPL features, bugs related to the ad-hoc parsing done by
REPLCompletions.completions have crept in. This pull request replaces most of
the manual parsing (regex, find_start_brace) with a new approach that parses
the entire input buffer once, before and after the cursor, using JuliaSyntax.
We then query the parsed syntax tree to determine the kind of completion
to be done.

Changes

Future work

  • Method completions could be changed to look for methods with exactly the given
    number of arguments if the closing ) is present, and search for signatures
    with the right prefix otherwise.

    • It would be nice to be able to search by type as well as value (perhaps
      by putting ::T in place of arguments).
  • Other REPL features could benefit from JuliaSyntax, so it might be worth
    sharing the parse tree between completions and other features:

    • Emacs-style sexpr navigation: C-M-f/C-M-b/C-M-u, etc.
    • Improved auto-indent.
  • It would be nice if hints worked even when the cursor is between text.

  • CursorNode is a slightly tweaked copy of SyntaxNode from JuliaSyntax that
    tracks the parent node but includes all trivia. It is used with seek_pos,
    which navigates to the innermost node at a given position so we can examine
    nearby nodes and the parent. This could probably duplicate less code from
    JuliaSyntax.

@xal-0 xal-0 added bugfix This change fixes an existing bug completions Tab and autocompletion in the repl labels Mar 13, 2025
@xal-0 xal-0 requested a review from vtjnash March 13, 2025 23:38
@xal-0 xal-0 force-pushed the juliasyntax-repl branch from 385cc7f to b62b012 Compare March 13, 2025 23:40
xal-0 added 2 commits March 13, 2025 16:49
Adds another permitted return type for complete_line, where the second element
of the tuple is a Region (a Pair{Int, Int}) describing the region of text to be
replaced.  This is useful for making completions work consistently when the
closing delimiter may or may not be present: the cursor can be made to "jump"
out of the delimiters regardless of whether it is there already.

  "exam|    =TAB=>   "example.jl"|
  "exam|"   =TAB=>   "example.jl"|
This commit replaces the heuristic parsing done by REPLCompletions.completions
with a new approach that parses the entire input buffer once with JuliaSyntax.
In addition to fixing bugs, the more precise parsing should allow for new
features in the future.

Some features now work in more situations "for free", like dictionary key
completion (the expression evaluated to find the keys is now more precise) and
method suggestions (arguments beyond the cursor can be used to narrow the list).

The tests have been updated to reflect slightly differing behaviour for string
and Cmd-string completion: the new code returns a character range encompassing
the entire string when completing paths (not observable by the user), and the
behaviour of '~'-expansion has be tweaked to be consistent across all places
where paths can be completed.  Some escaping issues have also been fixed.

Fixes: JuliaLang#55420, JuliaLang#55518, JuliaLang#55520, JuliaLang#55842, JuliaLang#56389, JuliaLang#57611
@xal-0 xal-0 force-pushed the juliasyntax-repl branch from b62b012 to c539ec4 Compare March 13, 2025 23:49
@xal-0 xal-0 added the don't squash Don't squash merge label Mar 14, 2025
@IanButterworth
Copy link
Member

This sounds great! I think we should backport it to at least 1.12 given it fixes so many issues.

@IanButterworth IanButterworth added the backport 1.12 Change should be backported to release-1.12 label Mar 14, 2025
Copy link
Contributor

@giordano giordano left a comment

Choose a reason for hiding this comment

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

@xal-0 first of all, speaking as the author of most of the issues linked to this PR, thank you for tackling this!

Also, can you please add a test for #57772 as well? I verified this works already (which is amazing!)

Comment on lines 2518 to 2522
# #55518
let s = "CompletionFoo.@barfoo kwtest"
c, r = test_complete(s)
@test isempty(c)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this quite cover all the broken cases reported in #55518 (there are a few scattered in the discussion)

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've added a few more from the thread

@StefanKarpinski
Copy link
Member

Bravo. Truly excellent quality-of-life PR 👏🏻

@xal-0 xal-0 removed the don't squash Don't squash merge label Mar 14, 2025
@giordano
Copy link
Contributor

Bravo. Truly excellent quality-of-life PR 👏🏻

Best part is that this PR has a negative diff, depsite the fact it added many tests.

@@ -158,6 +158,10 @@ let ex =
export exported_symbol
exported_symbol(::WeirdNames) = nothing

macro ignoremacro(e...)
:nothing
Copy link
Contributor

@giordano giordano Mar 14, 2025

Choose a reason for hiding this comment

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

Should this be

Suggested change
:nothing
nothing

?

julia> macro ignoremacro(e...)
           :nothing
       end
@ignoremacro (macro with 1 method)

julia> @macroexpand @ignoremacro
:(Main.nothing)

julia> macro ignoremacro(e...)
           nothing
       end
@ignoremacro (macro with 1 method)

julia> @macroexpand @ignoremacro

julia> 

Edit: anyway this is for a test, so not particularly important to get perfectly right.

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 was actually unaware that so many things were self-quoting!

xal-0 added 2 commits March 14, 2025 13:09
Since we parse the entire input now, we need to avoid triggering method
completion when the cursor is on the function name of a valid call.
Comment on lines +2513 to +2518
let s = "begin\n using Linear"
c, r = test_complete(s)
@test "LinearAlgebra" in c
@test r == 15:20
@test s[r] == "Linear"
end
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't pass when run via Pkg.test("REPL") because Base.runtests puts all stdlibs on the load path, whereas Pkg.test only does REPL deps and test deps.

Suggested change
let s = "begin\n using Linear"
c, r = test_complete(s)
@test "LinearAlgebra" in c
@test r == 15:20
@test s[r] == "Linear"
end
let s = "begin\n using Ran"
c, r = test_complete(s)
@test "Random" in c
@test r == 15:17
@test s[r] == "Ran"
end


# Expand '~' if the user hits TAB after exhausting completions (either
# because we have found an existing file, or there is no such file).
full_path = ispath(path) || isempty(paths)
Copy link
Member

@IanButterworth IanButterworth Mar 15, 2025

Choose a reason for hiding this comment

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

I found two cases that can cause ispath to error, so I think we should make a safe_ispath

  1. ArgumentError: embedded NULs are not allowed in C strings: "invalid\0"
    for let name = \"valid.txt\", name⁺ = \"invalid\\0.txt\"

  2. IOError name too long (ENAMETOOLONG) for strings that are too long.

Something like this? Or just never throw and just return false?

function safe_ispath(path::AbstractString)
    try
        return ispath(path)
    catch err
        if err isa Base.IOError # i.e. name too long (ENAMETOOLONG)
            return false
        elseif err isa Base.ArgumentError && occursin("embedded NULs", err.msg)
            return false
        else
            rethrow()
        end
    end
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment