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.

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

Jump to bottom

Signature Help #1720

Merged
merged 2 commits into from
Sep 19, 2024
Merged

Signature Help #1720

merged 2 commits into from
Sep 19, 2024

Conversation

3Rafal
Copy link
Collaborator

@3Rafal 3Rafal commented Jan 4, 2024

The goal of this PR is to upstream SignatureHelp functionality from ocaml-lsp to merlin. Similarly to #1286.

@3Rafal 3Rafal marked this pull request as ready for review January 8, 2024 07:53
@3Rafal 3Rafal requested a review from voodoos January 8, 2024 07:54
@3Rafal 3Rafal force-pushed the signature-help branch 2 times, most recently from b7fd76b to 3419ac6 Compare April 26, 2024 08:08
@3Rafal 3Rafal changed the title WIP: Signature Help Signature Help Apr 26, 2024
@3Rafal
Copy link
Collaborator Author

3Rafal commented Apr 26, 2024

@voodoos, I've rebased this branch and it should be good for review

@voodoos
Copy link
Collaborator

voodoos commented Apr 26, 2024

Thanks Rafal !

xvw
xvw reviewed Apr 30, 2024
View reviewed changes
Copy link
Collaborator

@xvw xvw left a comment

Choose a reason for hiding this comment

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

Nice!
I made a first round of review (with maybe some nitpicking. Feel free to discard comments if you find it not relevant)

src/analysis/signature_help.ml Show resolved Hide resolved
src/analysis/signature_help.ml Outdated Show resolved Hide resolved
src/analysis/signature_help.ml Outdated Show resolved Hide resolved
src/analysis/signature_help.ml Show resolved Hide resolved
src/analysis/signature_help.mli Outdated Show resolved Hide resolved
voodoos
voodoos reviewed May 2, 2024
View reviewed changes
Copy link
Collaborator

@voodoos voodoos left a comment

Choose a reason for hiding this comment

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

Thanks Rafal, I think this covers well the first step which was to bring the code from ocaml-lsp to merlin-lib.

You should also document the new command in the protocol file: https://github.com/ocaml/merlin/blob/master/doc/dev/PROTOCOL.md

Aditionnaly, it looks like the LSP protocol also provides additional optional parameters to help the server provide a correct answer: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_signatureHelp

May be we should also accept these even if they are not used right now ?

@voodoos voodoos mentioned this pull request Jun 5, 2024
Merged
3Rafal
3Rafal commented Sep 3, 2024
View reviewed changes
tests/test-dirs/signature-help/sh-mix.t Outdated
> let _ = f 0
> EOF

FIXME: After we expect the next non-labelled parameter to be active:
Copy link
Collaborator Author

@3Rafal 3Rafal Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@voodoos
even simpler case for this would be

It can make a third non-labelled parameter active.

  $ $MERLIN single signature-help -position 2:14 <<EOF
  > let f x y z = ignore (x, y, z)
  > let _ = f 0 1

result will be activeParameter equal one. As I understand it, the reason is that we search by arguments and when you just put the space after an argument, there are no new arguments in typed tree.

3Rafal and others added 2 commits September 19, 2024 10:39
@3Rafal @voodoos
Signature Help
c6a8943 
Move String functions to Std

Move comments to signature_help.mli

Upstream parenthesizing fix
@voodoos
Illustrate and fix issues when entering whitespace
41557ce 
Also expose additional parameters in the API that
might be useful at some point.
@voodoos voodoos merged commit 03b5cb5 into ocaml:main Sep 19, 2024
1 check was pending
voodoos added a commit to voodoos/merlin that referenced this pull request Sep 25, 2024
@voodoos
[B] ocaml#1720 Signature Help
6e75142
voodoos added a commit to voodoos/merlin that referenced this pull request Sep 25, 2024
@voodoos
[B] ocaml#1720 Signature Help
eee2736
voodoos added a commit to voodoos/merlin that referenced this pull request Sep 25, 2024
@voodoos
[B] ocaml#1720 Signature Help
db0b37e
voodoos added a commit that referenced this pull request Sep 26, 2024
@voodoos
[B] #1720 Signature Help
4b6c52f
voodoos added a commit that referenced this pull request Sep 26, 2024
@voodoos
[B] #1720 Signature Help
cdda0ec
voodoos added a commit to voodoos/opam-repository that referenced this pull request Sep 26, 2024
@voodoos
[new release] merlin (3 packages) (4.17-501)
942c2c4 
CHANGES:

Thu Sep 26 18:48:42 CEST 2024

  + merlin binary
    - A new `WRAPPING_PREFIX` configuration directive that can be used to tell Merlin
      what to append to the current unit name in the presence of wrapping (ocaml/merlin#1788)
    - Add `-unboxed-types` and `-no-unboxed-types` as ocaml ignored flags (ocaml/merlin#1795, fixes ocaml/merlin#1794)
    - destruct: Refinement in the presence of optional arguments (ocaml/merlin#1800 ocaml/merlin#1807, fixes ocaml/merlin#1770)
    - Implement new expand-node command for expanding PPX annotations (ocaml/merlin#1745)
    - Implement new inlay-hints command for adding hints on a sourcetree (ocaml/merlin#1812)
    - Add `signature-help` command (ocaml/merlin#1720)
    - Implement new search-by-type command for searching values by types (ocaml/merlin#1828)
    - Fix dot-merlin-reader ignoring `SOURCE_ROOT` and `STDLIB` directives
      (ocaml/merlin#1839, ocaml/merlin#1803)
  + editor modes
    - vim: fix python-3.12 syntax warnings in merlin.py (ocaml/merlin#1798)
    - vim: Dead code / doc removal for previously deleted MerlinPhrase command (ocaml/merlin#1804)
    - emacs: Improve the way that result of polarity search is displayed (ocaml/merlin#1814)
    - emacs: Add `merlin-search-by-type`, `merlin-search-by-polarity` and change the
	  behaviour of `merlin-search` to switch between `by-type` or `by-polarity`
	  depending on the query (ocaml/merlin#1828)
@voodoos voodoos mentioned this pull request Sep 26, 2024
Closed
voodoos added a commit to voodoos/opam-repository that referenced this pull request Sep 26, 2024
@voodoos
[new release] merlin (3 packages) (4.17-414)
a44866d 
CHANGES:

Thu Sep 26 18:48:42 CEST 2024

  + merlin binary
    - A new `WRAPPING_PREFIX` configuration directive that can be used to tell Merlin
      what to append to the current unit name in the presence of wrapping (ocaml/merlin#1788)
    - Add `-unboxed-types` and `-no-unboxed-types` as ocaml ignored flags (ocaml/merlin#1795, fixes ocaml/merlin#1794)
    - destruct: Refinement in the presence of optional arguments (ocaml/merlin#1800 ocaml/merlin#1807, fixes ocaml/merlin#1770)
    - Implement new expand-node command for expanding PPX annotations (ocaml/merlin#1745)
    - Implement new inlay-hints command for adding hints on a sourcetree (ocaml/merlin#1812)
    - Add `signature-help` command (ocaml/merlin#1720)
    - Implement new search-by-type command for searching values by types (ocaml/merlin#1828)
    - Fix dot-merlin-reader ignoring `SOURCE_ROOT` and `STDLIB` directives
      (ocaml/merlin#1839, ocaml/merlin#1803)
  + editor modes
    - vim: fix python-3.12 syntax warnings in merlin.py (ocaml/merlin#1798)
    - vim: Dead code / doc removal for previously deleted MerlinPhrase command (ocaml/merlin#1804)
    - emacs: Improve the way that result of polarity search is displayed (ocaml/merlin#1814)
    - emacs: Add `merlin-search-by-type`, `merlin-search-by-polarity` and change the
	  behaviour of `merlin-search` to switch between `by-type` or `by-polarity`
	  depending on the query (ocaml/merlin#1828)
voodoos added a commit to voodoos/opam-repository that referenced this pull request Sep 27, 2024
@voodoos
[new release] merlin (3 packages) (4.17.1-501)
7ebb9c0 
CHANGES:

Fri Sep 27 12:02:42 CEST 2024

  + merlin binary
    - A new `WRAPPING_PREFIX` configuration directive that can be used to tell Merlin
      what to append to the current unit name in the presence of wrapping (ocaml/merlin#1788)
    - Add `-unboxed-types` and `-no-unboxed-types` as ocaml ignored flags (ocaml/merlin#1795, fixes ocaml/merlin#1794)
    - destruct: Refinement in the presence of optional arguments (ocaml/merlin#1800 ocaml/merlin#1807, fixes ocaml/merlin#1770)
    - Implement new expand-node command for expanding PPX annotations (ocaml/merlin#1745)
    - Implement new inlay-hints command for adding hints on a sourcetree (ocaml/merlin#1812)
    - Add `signature-help` command (ocaml/merlin#1720)
    - Implement new search-by-type command for searching values by types (ocaml/merlin#1828)
    - Fix dot-merlin-reader ignoring `SOURCE_ROOT` and `STDLIB` directives
      (ocaml/merlin#1839, ocaml/merlin#1803)
  + editor modes
    - vim: fix python-3.12 syntax warnings in merlin.py (ocaml/merlin#1798)
    - vim: Dead code / doc removal for previously deleted MerlinPhrase command (ocaml/merlin#1804)
    - emacs: Improve the way that result of polarity search is displayed (ocaml/merlin#1814)
    - emacs: Add `merlin-search-by-type`, `merlin-search-by-polarity` and change the
	  behaviour of `merlin-search` to switch between `by-type` or `by-polarity`
	  depending on the query (ocaml/merlin#1828)
@voodoos voodoos mentioned this pull request Sep 27, 2024
Closed
voodoos added a commit to voodoos/opam-repository that referenced this pull request Sep 27, 2024
@voodoos
[new release] merlin (3 packages) (4.17.1-414)
390722d 
CHANGES:

Fri Sep 27 12:02:42 CEST 2024

  + merlin binary
    - A new `WRAPPING_PREFIX` configuration directive that can be used to tell Merlin
      what to append to the current unit name in the presence of wrapping (ocaml/merlin#1788)
    - Add `-unboxed-types` and `-no-unboxed-types` as ocaml ignored flags (ocaml/merlin#1795, fixes ocaml/merlin#1794)
    - destruct: Refinement in the presence of optional arguments (ocaml/merlin#1800 ocaml/merlin#1807, fixes ocaml/merlin#1770)
    - Implement new expand-node command for expanding PPX annotations (ocaml/merlin#1745)
    - Implement new inlay-hints command for adding hints on a sourcetree (ocaml/merlin#1812)
    - Add `signature-help` command (ocaml/merlin#1720)
    - Implement new search-by-type command for searching values by types (ocaml/merlin#1828)
    - Fix dot-merlin-reader ignoring `SOURCE_ROOT` and `STDLIB` directives
      (ocaml/merlin#1839, ocaml/merlin#1803)
  + editor modes
    - vim: fix python-3.12 syntax warnings in merlin.py (ocaml/merlin#1798)
    - vim: Dead code / doc removal for previously deleted MerlinPhrase command (ocaml/merlin#1804)
    - emacs: Improve the way that result of polarity search is displayed (ocaml/merlin#1814)
    - emacs: Add `merlin-search-by-type`, `merlin-search-by-polarity` and change the
	  behaviour of `merlin-search` to switch between `by-type` or `by-polarity`
	  depending on the query (ocaml/merlin#1828)
voodoos added a commit to voodoos/opam-repository that referenced this pull request Sep 27, 2024
@voodoos
[new release] merlin (4 packages) (5.2.1-502)
4241be6 
CHANGES:

Fri Sep 27 12:02:42 CEST 2024

  + merlin binary
    - A new `WRAPPING_PREFIX` configuration directive that can be used to tell Merlin
      what to append to the current unit name in the presence of wrapping (ocaml/merlin#1788)
    - Add `-unboxed-types` and `-no-unboxed-types` as ocaml ignored flags (ocaml/merlin#1795, fixes ocaml/merlin#1794)
    - destruct: Refinement in the presence of optional arguments (ocaml/merlin#1800 ocaml/merlin#1807, fixes ocaml/merlin#1770)
    - Implement new expand-node command for expanding PPX annotations (ocaml/merlin#1745)
    - Implement new inlay-hints command for adding hints on a sourcetree (ocaml/merlin#1812)
    - Implement new search-by-type command for searching values by types (ocaml/merlin#1828)
    - Canonicalize paths in occurrences. This helps deduplicate the results and
      show more user-friendly paths. (ocaml/merlin#1840)
    - Fix dot-merlin-reader ignoring `SOURCE_ROOT` and `STDLIB` directives
      (ocaml/merlin#1839, ocaml/merlin#1803)
  + editor modes
    - vim: fix python-3.12 syntax warnings in merlin.py (ocaml/merlin#1798)
    - vim: Dead code / doc removal for previously deleted MerlinPhrase command (ocaml/merlin#1804)
    - emacs: Improve the way that result of polarity search is displayed (ocaml/merlin#1814)
    - emacs: Add `merlin-search-by-type`, `merlin-search-by-polarity` and change the
	  behaviour of `merlin-search` to switch between `by-type` or `by-polarity`
	  depending on the query (ocaml/merlin#1828)

[new release] merlin (3 packages) (4.17.1-501)

CHANGES:

Fri Sep 27 12:02:42 CEST 2024

  + merlin binary
    - A new `WRAPPING_PREFIX` configuration directive that can be used to tell Merlin
      what to append to the current unit name in the presence of wrapping (ocaml/merlin#1788)
    - Add `-unboxed-types` and `-no-unboxed-types` as ocaml ignored flags (ocaml/merlin#1795, fixes ocaml/merlin#1794)
    - destruct: Refinement in the presence of optional arguments (ocaml/merlin#1800 ocaml/merlin#1807, fixes ocaml/merlin#1770)
    - Implement new expand-node command for expanding PPX annotations (ocaml/merlin#1745)
    - Implement new inlay-hints command for adding hints on a sourcetree (ocaml/merlin#1812)
    - Add `signature-help` command (ocaml/merlin#1720)
    - Implement new search-by-type command for searching values by types (ocaml/merlin#1828)
    - Fix dot-merlin-reader ignoring `SOURCE_ROOT` and `STDLIB` directives
      (ocaml/merlin#1839, ocaml/merlin#1803)
  + editor modes
    - vim: fix python-3.12 syntax warnings in merlin.py (ocaml/merlin#1798)
    - vim: Dead code / doc removal for previously deleted MerlinPhrase command (ocaml/merlin#1804)
    - emacs: Improve the way that result of polarity search is displayed (ocaml/merlin#1814)
    - emacs: Add `merlin-search-by-type`, `merlin-search-by-polarity` and change the
	  behaviour of `merlin-search` to switch between `by-type` or `by-polarity`
	  depending on the query (ocaml/merlin#1828)

[new release] merlin (3 packages) (4.17.1-414)

CHANGES:

Fri Sep 27 12:02:42 CEST 2024

  + merlin binary
    - A new `WRAPPING_PREFIX` configuration directive that can be used to tell Merlin
      what to append to the current unit name in the presence of wrapping (ocaml/merlin#1788)
    - Add `-unboxed-types` and `-no-unboxed-types` as ocaml ignored flags (ocaml/merlin#1795, fixes ocaml/merlin#1794)
    - destruct: Refinement in the presence of optional arguments (ocaml/merlin#1800 ocaml/merlin#1807, fixes ocaml/merlin#1770)
    - Implement new expand-node command for expanding PPX annotations (ocaml/merlin#1745)
    - Implement new inlay-hints command for adding hints on a sourcetree (ocaml/merlin#1812)
    - Add `signature-help` command (ocaml/merlin#1720)
    - Implement new search-by-type command for searching values by types (ocaml/merlin#1828)
    - Fix dot-merlin-reader ignoring `SOURCE_ROOT` and `STDLIB` directives
      (ocaml/merlin#1839, ocaml/merlin#1803)
  + editor modes
    - vim: fix python-3.12 syntax warnings in merlin.py (ocaml/merlin#1798)
    - vim: Dead code / doc removal for previously deleted MerlinPhrase command (ocaml/merlin#1804)
    - emacs: Improve the way that result of polarity search is displayed (ocaml/merlin#1814)
    - emacs: Add `merlin-search-by-type`, `merlin-search-by-polarity` and change the
	  behaviour of `merlin-search` to switch between `by-type` or `by-polarity`
	  depending on the query (ocaml/merlin#1828)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@voodoos voodoos voodoos left review comments

@xvw xvw xvw left review comments

Assignees
No one assigned
Labels
None yet
Projects
Backport to 414
Status: Ready for backporting
Backport to 501
Status: 4.17-501
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@3Rafal @voodoos @xvw