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

Implement signature help request #324

Closed
wants to merge 6 commits into from
Closed

Conversation

mnxn
Copy link
Collaborator

@mnxn mnxn commented Dec 1, 2020

Partially completes #312

Depends on rgrinberg/merlin#8

Currently supports showing signature help when an expression is detected as having a function type or is being applied. So it will show signature help for these two scenarios:

let f x = x
let _  = f |

f : 'a -> 'a

let f x = x
let _  = f 1|

f : int -> int

The second one will underline the specific parameter that is currently being applied. Operators also show signature help.


Notes:

  • I haven't figured what the best way to provide function/parameter documentation is. The function could just return positions for function documentation, but I feel like there should be a better way to do it.

  • Showing signature help for operators or after entering a space after a function-type value might be annoying to users. Perhaps the functionality should be configurable? I couldn't find a built-in VSCode setting to disable signature help.

  • The current implementation seems to depend on having correctly typed arguments. This doesn't show any signature help:

let _ = string_of_int true|
  • Providing too many arguments just shows the function type without any parameters highlighted, despite the type error.
let _ = string_of_int 1 2|

string_of_int : int -> string


Images:
Space after function-typed value:
image

Providing a function argument:
image

Anonymous function:
image

Operator:
image

@mnxn mnxn requested a review from rgrinberg December 1, 2020 08:28
@mnxn
Copy link
Collaborator Author

mnxn commented Dec 1, 2020

The first non-labelled parameter is now underlined right after a function-typed value:

image

Typing a ~ will underline the f:('a -> 'b) instead.

Copy link
Member

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

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

Looks good. I'll merge both this and the merlin PR they are both ready.

@rgrinberg
Copy link
Member

Worth adding a CHANGES entry as well.

@rgrinberg
Copy link
Member

Awesome work. Merged manually.

@rgrinberg rgrinberg closed this Dec 1, 2020
@mnxn
Copy link
Collaborator Author

mnxn commented Dec 1, 2020

Awesome work. Merged manually.

Thanks, I was having trouble figuring out why Github was detecting conflicts...

@tmattio
Copy link
Collaborator

tmattio commented Dec 2, 2020

Thanks for the great work on this @mnxn, the feature is amazing 🎉

I noticed two things when testing:

  • The highlighted argument seems to always be the first one

image
(I added a space after the first 0)

  • The highlighted argument does not switch to the first labeled with only ~, it needs the name of the labeled argument as well:

image
then
image

@mnxn
Copy link
Collaborator Author

mnxn commented Dec 2, 2020

The highlighted argument seems to always be the first one

The highlighted argument won't always be the first, but it won't detect that you've moved onto the next parameter until you start a new expression. I'm not sure it's possible to detect you've moved on to the next function argument with the expression tree that Merlin provides. It's harder to do in a function-application-by-juxtaposition language like OCaml since it lacks the comma argument delimiters that C-like languages have.

The highlighted argument does not switch to the first labeled with only ~, it needs the name of the labeled argument as well:

The language server actually isn't providing an activeParameter index in this example, but VSCode is defaulting to the first one. We could fix this by always returning an activeParameter based on the triggerCharacter (~, ?, or other).

@mnxn
Copy link
Collaborator Author

mnxn commented Dec 3, 2020

  • The highlighted argument does not switch to the first labeled with only ~, it needs the name of the labeled argument as well:

@tmattio: your second issue should be fixed in #328

rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Dec 18, 2020
CHANGES:

## Features

- Support cancellation notifications when possible. (ocaml/ocaml-lsp#323)

- Implement signature help request for functions (ocaml/ocaml-lsp#324)

- Server LSP requests & notifications concurrently. Requests that require merlin
  are still serialized. (ocaml/ocaml-lsp#330)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants