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

Demystifying OCaml Syntax with Merlin and OCaml-LSP #1218

Merged
merged 5 commits into from
May 14, 2024

Conversation

PizieDust
Copy link
Contributor

@PizieDust PizieDust commented Dec 11, 2023

FEATURE: Add Syntax Documentation to Hover request

Introduction

This PR introduces a new feature to OCaml-LSP for both beginners and also more experienced programmers. No more searching for obscure keywords or symbols on Google trying to find documentation.
With this PR, you can hover over OCaml code in your editor, and instantly see a clear and summarized explanation of its syntax along with a link to the exact portion of OCaml's documentation for it.

Benefits:

  • Faster understanding: At a glance you can get a quick description of what the syntax is about.
  • Improved learning: Beginners can easily understand how their code works while writing the code.
  • Enhanced productivity: Very useful for understanding codebases and what a syntax is supposed to do.

How it works:

  • Hover your cursor over the OCaml code.
  • The hover request will call the new Syntax Documentation command from Merlin which returns a JSON output containing a name, some description and a url pointing to the documentation.
  • This information is then parsed and displayed on your editors hover UI.

Scope

Currently the command covers only specific sections of OCaml's syntax such as Language Extensions. Subsequently, more syntax will be covered.

Code Example

Example: Hovering over type definitions in VS code:
Code example:

(*  Variant type *)
type p = 
  | A (** comment for A*)
  | B (** comment for B*)

(* Record type *)
type point = {x: int; y: int}

Current Behaviour

Before Syntax document command: UI will display only type definition

image
image

Or display comments if they are available

image

New Feature

The UI now includes a short description about the syntax highlighted under the cursor.

image

UI will also display comments if they are available

image

New Syntax Document command can differentiate different kinds of syntax and return appropriate documentation

image

WIP/Dependencies

This PR relies on a new Merlin command found in this PR (ocaml/merlin#1706) which is currently unmerged. To test this feature out, add a pin dependency of Merlin to https://github.com/PizieDust/merlin/tree/syntax_documentation.

This PR is related to ocaml/merlin#1692

Update

@jfeser made a great comment about displaying the urls as markdown links instead of full string versions which greatly improves on the UX both functionally and aesthetically.

@pitag-ha suggested using JSON instead of one blob string for the Syntax Document command which works great for this purpose.

cc @voodoos @pitag-ha

@jfeser
Copy link
Collaborator

jfeser commented Dec 12, 2023

I suspect that users will want this to be configurable. I could see how novice users might find this information helpful, but it's noisy.

#1198 suggests go-to links for types. We could consider a go-to link for documentation, instead of inlining the documentation sentence into the hover.

@PizieDust
Copy link
Contributor Author

PizieDust commented Dec 13, 2023

#1198 suggests go-to links for types. We could consider a go-to link for documentation, instead of inlining the documentation sentence into the hover.

This is certainly a good idea. I like the Go to Documentation format. It definitely trims is down a bunch.

@PizieDust PizieDust changed the title Add more info to Document command Demystifying OCaml's Code with Merlin and OCaml-LSP Dec 15, 2023
@PizieDust PizieDust changed the title Demystifying OCaml's Code with Merlin and OCaml-LSP Demystifying OCaml Syntax with Merlin and OCaml-LSP Dec 15, 2023
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.

Made a few comments, you are doing good progress !

To make the feature opt-in (or opt-out ?) can take inspiration from other configurable features of ocaml-lsp like "extended hover".

ocaml-lsp-server/src/document.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/hover_req.ml Show resolved Hide resolved
ocaml-lsp-server/src/hover_req.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/hover_req.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/docs/ocamllsp/config.md Outdated Show resolved Hide resolved
ocaml-lsp-server/src/config_data.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/custom_requests/req_hover_extended.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/document.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/hover_req.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/hover_req.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/hover_req.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/hover_req.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/hover_req.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/hover_req.ml Outdated Show resolved Hide resolved
@PizieDust PizieDust requested a review from voodoos January 5, 2024 09:22
@smorimoto
Copy link
Member

Changes for this PR on the VSCode extension side have just been merged.

@PizieDust PizieDust requested a review from rgrinberg February 2, 2024 06:17
ocaml-lsp-server/src/hover_req.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/document.ml Show resolved Hide resolved
@PizieDust PizieDust requested a review from rgrinberg February 12, 2024 08:20
@rgrinberg
Copy link
Member

I'll let @voodoos merge

@rgrinberg rgrinberg added this to the 1.18.0 milestone Feb 12, 2024
@voodoos
Copy link
Collaborator

voodoos commented Feb 12, 2024

I'll let @voodoos merge

ack, I will try to release Merlin so that we can properly run the testsuite before merging.

@coveralls
Copy link

coveralls commented Feb 27, 2024

Pull Request Test Coverage Report for Build 4211

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 33 of 59 (55.93%) changed or added relevant lines in 3 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.09%) to 21.266%

Changes Missing Coverage Covered Lines Changed/Added Lines %
ocaml-lsp-server/src/document.ml 6 7 85.71%
ocaml-lsp-server/src/hover_req.ml 9 16 56.25%
ocaml-lsp-server/src/config_data.ml 18 36 50.0%
Files with Coverage Reduction New Missed Lines %
ocaml-lsp-server/src/hover_req.ml 1 42.93%
Totals Coverage Status
Change from base Build 4187: 0.09%
Covered Lines: 5137
Relevant Lines: 24156

💛 - Coveralls

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.

Sorry for the late review. Nice job Pizie, it looks like it's almost ready !

I think you changed the formatting of the on_hover responses slightly which breaks other tests. (The 5.0/5.1 CI only pass because it doesn't run the tests.) Did you run them on your machine ? Did they pass on your machine ?

For example, the following diff is clearly linked to your new printing of the dividers:

    - ---
    +
    +  --- 
    +

There is no reason to change the results of other tests in this PR, unless the new feature is activated. You can run the make test target to run the test on your machine and check that the result is correct.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
ocaml-lsp-server/src/document.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/document.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/document.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/hover_req.ml Show resolved Hide resolved
@@ -12,29 +12,40 @@ let environment_mode =
| Some true -> Extended_variable
| Some false | None -> Default

let format_contents ~syntax ~markdown ~typ ~doc =
let print_dividers sections = String.concat ~sep:"\n\n --- \n\n" sections
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think in the orginal printer there is only one \n before and after the --- and no spaced. Does that matter ?

It would be great to have the commit with the test moved at the first position in this PR so that it first show the printing before the changes in that PR and then how this PR alters the test. (@pitag-ha can probably help you with the rebase 🤞)

@PizieDust
Copy link
Contributor Author

PizieDust commented Mar 8, 2024

Sorry for the late review. Nice job Pizie, it looks like it's almost ready !

I think you changed the formatting of the on_hover responses slightly which breaks other tests. (The 5.0/5.1 CI only pass because it doesn't run the tests.) Did you run them on your machine ? Did they pass on your machine ?

For example, the following diff is clearly linked to your new printing of the dividers:

    - ---
    +
    +  --- 
    +

There is no reason to change the results of other tests in this PR, unless the new feature is activated. You can run the make test target to run the test on your machine and check that the result is correct.

Ahh yeah, I was using dune build @runtest and I think that's why I didn't notice that the tests were failing. It's all sorted now.

image

@PizieDust PizieDust requested a review from voodoos March 8, 2024 05:24
@PizieDust PizieDust force-pushed the add_syntax_doc branch 3 times, most recently from 192f59a to c5f37c9 Compare March 14, 2024 07:57
add syntax doc command

change syntax_doc output from string to record type

parse record to extract and display different fields correctly

lint code

refactor code add formatting functions and check if syn_doc is activated

check if syntax_doc is activated in ocamllsp env variables

refactor for proper types

include syntax documentation in configuation data

check if feature is activated from ocaml.server

remove redundant code

lint

add version

refactor code and lint

refactor code and lint

let merlin work only if syntax doc is activated

lint

change from italics to code pill

lint

update changelog

update documentation

remove configuration via environment variables

update docs

syntax highlighting for code block

make syntax highlighter required

add constraint for most recent version of merlin-lib

upgrade merlin-lib to latest version

add syntax doc to ppx expect

Write tests for syntax doc command

linting

helper function for positions

Update README.md

Co-authored-by: Ulysse <5031221+voodoos@users.noreply.github.com>

Update README.md

Co-authored-by: Ulysse <5031221+voodoos@users.noreply.github.com>

Update ocaml-lsp-server/src/document.ml

Co-authored-by: Ulysse <5031221+voodoos@users.noreply.github.com>

Update ocaml-lsp-server/src/document.ml

Co-authored-by: Ulysse <5031221+voodoos@users.noreply.github.com>

80 char per line limit

use better descriptive function name

adjust formatting

fix bug

fix bug
@voodoos
Copy link
Collaborator

voodoos commented May 14, 2024

Thanks @PizieDust, I will merge once the checks finish.

@voodoos voodoos merged commit 97bd236 into ocaml:master May 14, 2024
9 checks passed
voodoos added a commit to voodoos/opam-repository that referenced this pull request Jul 5, 2024
CHANGES:

## Features

- Introduce a configuration option to control dune diagnostics. The option is
  called `duneDiganostics` and it may be set to `{ enable: false }` to disable
  diagnostics. (ocaml/ocaml-lsp#1221)

- Support folding of `ifthenelse` expressions (ocaml/ocaml-lsp#1031)

- Improve hover behavior (ocaml/ocaml-lsp#1245)

  Hovers are no longer displaye on useless parsetree nodes such as keywords,
  comments, etc.

  Multiline hovers are now filtered away.

  Display expanded ppx's in the hover window.

- Improve document symbols (ocaml/ocaml-lsp#1247)

  Use the parse tree instead of the typed tree. This means that document
  symbols will work even if the source code doesn't type check.

  Include symbols at arbitrary depth.

  Differentiate functions / types / variants / etc.

  This now includes PPXs like `let%expect_test` or `let%bench` in the outline.

- Introduce a `destruct-line` code action. This is an improved version of the
  old `destruct` code action. (ocaml/ocaml-lsp#1283)

- Improve signature inference to only include types for elements that were
  absent from the signature. Previously, all signature items would always be
  inserted. (ocaml/ocaml-lsp#1289)

- Add an `update-signature` code action to update the types of elements that
  were already present in the signature (ocaml/ocaml-lsp#1289)

- Add custom
  [`ocamllsp/merlinCallCompatible`](https://github.com/ocaml/ocaml-lsp/blob/e165f6a3962c356adc7364b9ca71788e93489dd0/ocaml-lsp-server/docs/ocamllsp/merlinCallCompatible-spec.md)
  request (ocaml/ocaml-lsp#1265)

- Add custom [`ocamllsp/typeEnclosing`](https://github.com/ocaml/ocaml-lsp/blob/109801e56f2060caf4487427bede28b824f4f1fe/ocaml-lsp-server/docs/ocamllsp/typeEnclosing-spec.md) request (ocaml/ocaml-lsp#1304)

## Fixes

- Detect document kind by looking at merlin's `suffixes` config.

  This enables more lsp features for non-.ml/.mli files. Though it still
  depends on merlin's support. (ocaml/ocaml-lsp#1237)

- Correctly accept the `--clientProcessId` flag. (ocaml/ocaml-lsp#1242)

- Disable automatic completion and signature help inside comments (ocaml/ocaml-lsp#1246)

- Includes a new optional/configurable option to toggle syntax documentation. If
  toggled on, allows display of syntax documentation on hover tooltips. Can be
  controlled via environment variables and by GUI for VS code. (ocaml/ocaml-lsp#1218)

- For completions on labels that the LSP gets from merlin, take into account
  whether the prefix being completed starts with `~` or `?`. Change the label
  completions that start with `?` to start with `~` when the prefix being
  completed starts with `~`. (ocaml/ocaml-lsp#1277)

- Fix document syncing (ocaml/ocaml-lsp#1278, ocaml/ocaml-lsp#1280, fixes ocaml/ocaml-lsp#1207)

- Stop generating inlay hints on generated code (ocaml/ocaml-lsp#1290)

- Fix parenthesizing of function types in `SignatureHelp` (ocaml/ocaml-lsp#1296)

- Fix syntax documentation rendering (ocaml/ocaml-lsp#1318)
avsm pushed a commit to avsm/opam-repository that referenced this pull request Sep 5, 2024
CHANGES:

## Features

- Introduce a configuration option to control dune diagnostics. The option is
  called `duneDiganostics` and it may be set to `{ enable: false }` to disable
  diagnostics. (ocaml/ocaml-lsp#1221)

- Support folding of `ifthenelse` expressions (ocaml/ocaml-lsp#1031)

- Improve hover behavior (ocaml/ocaml-lsp#1245)

  Hovers are no longer displaye on useless parsetree nodes such as keywords,
  comments, etc.

  Multiline hovers are now filtered away.

  Display expanded ppx's in the hover window.

- Improve document symbols (ocaml/ocaml-lsp#1247)

  Use the parse tree instead of the typed tree. This means that document
  symbols will work even if the source code doesn't type check.

  Include symbols at arbitrary depth.

  Differentiate functions / types / variants / etc.

  This now includes PPXs like `let%expect_test` or `let%bench` in the outline.

- Introduce a `destruct-line` code action. This is an improved version of the
  old `destruct` code action. (ocaml/ocaml-lsp#1283)

- Improve signature inference to only include types for elements that were
  absent from the signature. Previously, all signature items would always be
  inserted. (ocaml/ocaml-lsp#1289)

- Add an `update-signature` code action to update the types of elements that
  were already present in the signature (ocaml/ocaml-lsp#1289)

- Add custom
  [`ocamllsp/merlinCallCompatible`](https://github.com/ocaml/ocaml-lsp/blob/e165f6a3962c356adc7364b9ca71788e93489dd0/ocaml-lsp-server/docs/ocamllsp/merlinCallCompatible-spec.md)
  request (ocaml/ocaml-lsp#1265)

- Add custom [`ocamllsp/typeEnclosing`](https://github.com/ocaml/ocaml-lsp/blob/109801e56f2060caf4487427bede28b824f4f1fe/ocaml-lsp-server/docs/ocamllsp/typeEnclosing-spec.md) request (ocaml/ocaml-lsp#1304)

## Fixes

- Detect document kind by looking at merlin's `suffixes` config.

  This enables more lsp features for non-.ml/.mli files. Though it still
  depends on merlin's support. (ocaml/ocaml-lsp#1237)

- Correctly accept the `--clientProcessId` flag. (ocaml/ocaml-lsp#1242)

- Disable automatic completion and signature help inside comments (ocaml/ocaml-lsp#1246)

- Includes a new optional/configurable option to toggle syntax documentation. If
  toggled on, allows display of syntax documentation on hover tooltips. Can be
  controlled via environment variables and by GUI for VS code. (ocaml/ocaml-lsp#1218)

- For completions on labels that the LSP gets from merlin, take into account
  whether the prefix being completed starts with `~` or `?`. Change the label
  completions that start with `?` to start with `~` when the prefix being
  completed starts with `~`. (ocaml/ocaml-lsp#1277)

- Fix document syncing (ocaml/ocaml-lsp#1278, ocaml/ocaml-lsp#1280, fixes ocaml/ocaml-lsp#1207)

- Stop generating inlay hints on generated code (ocaml/ocaml-lsp#1290)

- Fix parenthesizing of function types in `SignatureHelp` (ocaml/ocaml-lsp#1296)

- Fix syntax documentation rendering (ocaml/ocaml-lsp#1318)
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.

6 participants