Implement signature help

[mlechu]

Shows method signatures when the user is typing out a function call.

Examples

Emacs	Visual Studio Code

Implemented

• Parameter highlighting for positional and keyword arguments
• Filtering of signatures (no type info used yet, just by number of positional arguments and names of keyword arguments)
• Support for keyword args before the semicolon in a call
• Tolerance of some partially-incorrect syntax, e.g. foo(|, foo(1,,|)

Todo

• dotcall support (simple to implement, I just need to decide what would go in the UI)

One day

• Provenance would be nice here to get the original signature from source instead of the output of show(::Method)

Misc griping

Ran into lots of flakiness in the clients. I estimate half of these are my own bugs and I've yet to figure it out, but I figured I should write them down somewhere.

• emacs: Specifying param labels using ranges is off by one (see above)
• emacs: Highlighting params doesn't work on the first signature (see above)
  • This works with other language servers, so worth investigating
• vscode: The signature popup closes when typing . or ; (and others)
  • We just need to add everything to the trigger character list, I guess
• protocol (vscode): You can't not highlight your parameters. From the LSP docs:

  /**
   * The active parameter of the active signature. If omitted or the value
   * lies outside the range of `signatures[activeSignature].parameters`
   * defaults to 0 if the active signature has parameters. If
   * the active signature has no parameters it is ignored.
   * In future version of the protocol this property might become
   * mandatory to better express the active parameter if the
   * active signature does have any.
   */
  

  but it doesn't always make sense to highlight a parameter, e.g. f(1,2,a...|). Eglot does the reasonable thing in my opinion, and vscode will fall back to highlighting the first.
• vscode: It is possible to break the param highlighter completely if it doesn't like the characters in a string parameter label, which at least fixes the issue above

[codecov]

Codecov Report

Attention: Patch coverage is 89.12134% with 26 lines in your changes missing coverage. Please review.

Project coverage is 75.79%. Comparing base (4150686) to head (110225e).
Report is 8 commits behind head on master.

Files with missing lines	Patch %	Lines
src/signature-help.jl	88.69%	19 Missing ⚠️
src/JETLS.jl	75.00%	4 Missing ⚠️
src/LSP/language-features/signature-help.jl	50.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #34      +/-   ##
==========================================
+ Coverage   68.55%   75.79%   +7.23%     
==========================================
  Files          21       23       +2     
  Lines        1218     1417     +199     
==========================================
+ Hits          835     1074     +239     
+ Misses        383      343      -40     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[aviatesk]

I apologize for the delay in the review.
I was focused on submitting several other PRs that were pending.

I'm about to start the review on dtails, but I'd like to ask a question about the basic approach.
I believe the approach taken in this PR is based on JL, but could you explain why
a JL-based implementation is necessary, rather than the approach being attempted in
JuliaLang/julia#57767 (an advanced JS-based approach with setting up a custom node)?

Are there cases where scope resolution using JL is advantageous for implementing this feature?
I'm not opposed to using JL at all - in fact, I support it - but I'd like to deepen my understanding of the overall approach, while seeking for a possibly simpler alternative approach.

[mlechu]

I apologize for the delay in the review

No worries, review whenever you have the time.

I believe the approach taken in this PR is based on JL, but could you explain why a JL-based implementation is necessary

I do use JuliaLowering's syntax tree structure for parsed code, but nothing is lowered, and we could switch to JS.SyntaxNode if we wanted. I used JL.SyntaxTree for consistency with completions, and also because some comments in the JuliaLowering readme made me think JL.SyntaxTree is meant to be an updated version of JS.SyntaxNode.

rather than the approach being attempted in JuliaLang/julia#57767 (an advanced JS-based approach with setting up a custom node)?

We don't use custom nodes currently, but SyntaxTree would actually make an approach like this much easier, because we could add new attributes (e.g. :parent, :cursor_offset) to any subset of nodes by doing a pass over an existing tree instead of defining a new type of node and translating.

Our current solution to finding the nodes containing the cursor is byte_ancestors, which is messier, but flexible, and works well after lowering when byte ranges of parents don't necessarily contain their children.

Are there cases where scope resolution using JL is advantageous for implementing this feature?

No local scope resolution happens right now (our signatures are just methods() from the cursor's current module) but lowering would be necessary if we were to try.

[aviatesk]

Since I made quite large-scale changes to master, I have added the updates here myself.

[aviatesk]

As you mention in the docstring, the better approach to do this might be to do something like
https://github.com/JuliaLang/julia/blob/1ea959a8b12f06391c305d600fb6942cb25905f6/stdlib/REPL/src/REPLCompletions.jl#L640-L674

It should allow us to handle cases like (print("hi"); Base).print.
Additionally, it might also allow us to account for argument types to some extent (by inferring the entire method call expression, and inspect types of IR elements corresponding to arguments).

However since it's a somewhat advanced approach, it's fine to tackle it in a separate PR. Maybe I'm better fit to write ConcreteEvaluator at least.

Also, since it's questionable whether that approach can handle incomplete code, it might be necessary to keep this resolve_property as a fallback, but I'm somewhat reluctant to provide two routines to do the same thing.

[aviatesk]

This enhancement will be tackled in another PR.

[aviatesk]

There is a way for doing this, but I agree with that the signature help don't need to show it.

[aviatesk]

I do use JuliaLowering's syntax tree structure for parsed code, but nothing is lowered, and we could switch to JS.SyntaxNode if we wanted. I used JL.SyntaxTree for consistency with completions, and also because some comments in the JuliaLowering readme made me think JL.SyntaxTree is meant to be an updated version of JS.SyntaxNode.

Okay, got it. From you comment now I think SyntaxTree can essentially do the same things as JS+custom node. I re-read the code, and I think most of it can be done with SyntaxTree, but since we can use JL, let's use it. Also, if we extend this feature in the future to include type information, using JL will be essential.

[aviatesk]

I feel that parsing the output of sprint(show, m::Method) and analyzing candidate methods based on it seems a bit hacky.
Is there a reason for taking this approach instead of using Base.arg_decl_parts/Base.kwarg_decl which are internally used by Base.show_method?
https://github.com/JuliaLang/julia/blob/9108dd08a572e394854d85aa0b2b680cc6a591c3/base/methodshow.jl#L217-L259

[mlechu]

(data race with the comment below) Discussed in meeting; definitely hacky, but since we want the provenance of parameters in the string, this might be the approach with the least amount of new code. Changing this later wouldn't be hard, just tedious (we'd copy the body of show(::Method) here and either use JuliaSyntax on that, or join strings together while keeping a counter)

[aviatesk]

As we discussed in the meeting, since we're using the output of mstr = sprint(show, m::Method) as SignatureHelpInformation.label, we need position information within mstr for parameter highlighting, which is why the current implementation is designed this way.

For future improvements, I think we could consider the following options:

• Adjust Base.show_method to add an option that removes the @ /path/to/method part from the output (for making the parsing part safe)
• For Method objects that JETLS analyzes, maintain the original JS/JL definitions and use them for signature help as well

Anyway, I think we can proceed with this PR as it is for now.

[aviatesk]

Suggested change

	triggerCharacters = ["(", ",", ";"],
	triggerCharacters = ["(", ",", ";", "="],

Maybe we should add = too as it allows us to get sighelp on cases like

?

[aviatesk]

Or, maybe we should register "all" characters (is that overkill?). We could consider cases where a user cancels signature help midway and then starts typing again, and it's a bit annoying that signature help is cancelled the moment " is typed right now. I feel like performance tuning handle_SignatureHelpRequest might be necessary though.

[aviatesk]

By the way, as a hack that is only possible with VSCode, when experimenting with various triggerCharacters, the hack explained at
https://github.com/aviatesk/JETLS.jl?tab=readme-ov-file#dynamic-registration
might be useful.

[mlechu]

I think it would make sense to put = (and maybe more) in retriggerCharacters instead.

Agree the default vscode behaviour is annoying...for reference, eglot (in addition to trigger chars) just sends a signature help request whenever the cursor stops moving for a second or so. What does Zed do?

hack that is only possible with VSCode

Cool, will take a look

[aviatesk]

I think it would make sense to put = (and maybe more) in retriggerCharacters instead.

The reason I think triggerCharacters might be better than retriggerCharacters is that currently, as per #34 (comment), when typing up to func(arg1, arg2; k|), the signature help gets canceled, so unless we trigger fresh signature help, it might not be retriggered.
At least in VSCode, once signature help disappears, it doesn't seem to be retriggered. However, eglot might handle this case better.

So, if #34 (comment) can be resolved, it might be fine to include = in retriggerCharacter. But registering it into triggerCharacters seems to be more robust and inefficient solution.

What does Zed do?

The situation with Zed is even worse...
The implementation of signature help in Zed is very primitive, and it doesn't seem to respect triggerCharacters at all. I've submitted this as an issue.
zed-industries/zed#31846

[mlechu]

if #34 (comment) can be resolved

Yup, I'm going to try and exclude k from the filtering logic here, as it doesn't make sense to filter on partially-typed kwargs. I'll test if = works well in retriggerCharacters then.

The situation with Zed is even worse

nooooo. Thanks for filing the issue haha

[mlechu]

I ended up liking = in triggerCharacters better, since typing = after a misspelled keyword would otherwise close the signature help for good

[aviatesk]

Yeah, I agree that that is a sensible choice.

[aviatesk]

Quick question, is it possible to show signature help in this case? Specifically, when the text typed after ; matches one of the kwargs. Right now, signature help stops here.

[aviatesk]

I think this PR is in a mergeable state, so let's move forward for now!
While there are still various things that could be improved, we'll enhance it iteratively in the future.
Thank you for producing excellent progress with this prototyping.