fix same file documentation bug by ulugbekna · Pull Request #1265 · ocaml/merlin

ulugbekna · 2021-02-09T15:53:59Z

Hi!

tl;dr Ocamldoc.associate_comment needs a list of doc-comments from the start to the end of file (non-decreasing positions of comments), but Locate.get_doc returned the comments from the end to the start of the file (non-increasing positions of comments). The last doc-comment was available because the last elem of non-decr list = head of non-incr list.

More detailed:

Ocamldoc.associate_comment takes a list of doc-comments (with their positions) from a file and compares positions of those comments with the position p of the cursor. If a doc-comment d with position p1 has p1 > p (ie documentation occurs later in the file than p), it considered there the list doesn't contain documentation for position p. (see code)

Since Locate.get_doc was returning List.rev comments, it returned doc-comments in a non-decreasing order, which meant that head of the list would have position "larger" than p causing the associate_comment return None.

for lookups of documentation for values defined in the same file, merlin will not return documentation for any but the last documented definition.

in the same file are now available

voodoos · 2021-03-15T10:24:02Z

Thanks, these changes look good to me.
I wonder why this List.rev was used in the first place, @trefis ?

It might just be a mistake; or the comments list was previously in the wrong order ? Or Ocamldoc.associate_comment was too aggressively optimized at some point...

I am also curious about the role of File_switching.where_am_i () ?

ulugbekna · 2021-04-09T15:20:49Z

Thanks Ulysse! In retrospective, I should've probably added tests to show that there are no regressions. I have tested manually however if that counts :p

…

On Mon, Mar 15, 2021, 15:24 Ulysse ***@***.***> wrote: Thanks, these changes look good to me. I wonder why these List.rev was used in the first place, @trefis <https://github.com/trefis> ? It might just be a mistake; or the comments list was previously in the wrong order ? Or Ocamldoc.associate_comment was too aggressively optimized at some point... I am also curious about the role of File_switching.where_am_i () ? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1265 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD4YR656UC3RRMJ6MIB5CALTDXN5LANCNFSM4XLH2TPQ> .

voodoos · 2021-04-09T16:28:44Z

It's never to late to add tests 🙂

* Merge pull request #1265 from ulugbekna/fix-same-file-comments * Add more doc-related tests

@ulugbekna

CHANGES: Tue Apr 12 11:44:22 AM CET 2021 + merlin binary - external configuration reading: + use relative paths to communicate with Dune when possible. This solves issues related to symlinks on Unix and improve Windows support (ocaml/merlin#1271, fixes ocaml/merlin#1288) + make the `workdir` configuration value when using the `dune ocaml-merlin` configuration provider the same as when using `dot-merlin-reader` so that ppxes behaves in the same way as before (ocaml/merlin#1284, fixes ocaml/dune#4479, discussion in ocaml/merlin#1292) - destruct: + improve prefixing of generated constructors in Destruct by filtering opened modules (ocaml/merlin#1277) + make the destruct command more resilient to ill-typed expressions and when called without nodes (ocaml/merlin#1304, fixes ocaml/merlin#1300) - reintroduce some record recovery and improve completion (ocaml/merlin#1276) - introduce a new AST node for holes (`_`), allow correct typing of these holes and add a new `holes` command that returns the locations of all holes in the current file along with their types (ocaml/merlin#1242, ocaml/merlin#1289) - Mppx: don't restore cookies after invocation. Ppx are invoked only once so there is no need to manage cookies. This small change should increase performance and should not change any other behavior (ocaml/merlin#1309) - Windows: system command variant: do not open a window console when launching a ppx (ocaml/merlin#1270, fixes ocaml/merlin#714) - fix same file documentation bug (ocaml/merlin#1265 by @ulugbekna, fixes ocaml/merlin#1261) + editor modes - vim: Add `MerlinNextHole` and `MerlinPreviousHole` commands to navigate between holes. Jump to the first hole after destruct (ocaml/merlin#1287, ocaml/merlin#1303) - emacs: Add `merlin-next-hole` and `merlin-previous-hole` commands to navigate holes. Jump to the first hole after calling destruct. (ocaml/merlin#1291) - emacs: modernization of the elisp code and conformance with coding guidelines (ocaml/merlin#1247, ocaml/merlin#1310 by Steve Purcell ) - vim & emacs : new client-side "merlin use package" commands, restoring previous behavior (ocaml/merlin#1272, fixes ocaml/merlin#1191) + test suite - cover constructor disambiguation and record fields (ocaml/merlin#1276) - cover the new `holes` command and AST node (ocaml/merlin#1242, ocaml/merlin#1289) - cover the document fix (ocaml/merlin#1265, ocaml/merlin#1315)

@ulugbekna

CHANGES: Tue Apr 12 11:44:22 AM CET 2021 + merlin binary - external configuration reading: + use relative paths to communicate with Dune when possible. This solves issues related to symlinks on Unix and improve Windows support (ocaml/merlin#1271, fixes ocaml/merlin#1288) + make the `workdir` configuration value when using the `dune ocaml-merlin` configuration provider the same as when using `dot-merlin-reader` so that ppxes behaves in the same way as before (ocaml/merlin#1284, fixes ocaml/dune#4479, discussion in ocaml/merlin#1292) - destruct: + improve prefixing of generated constructors in Destruct by filtering opened modules (ocaml/merlin#1277) + make the destruct command more resilient to ill-typed expressions and when called without nodes (ocaml/merlin#1304, fixes ocaml/merlin#1300) - reintroduce some record recovery and improve completion (ocaml/merlin#1276) - introduce a new AST node for holes (`_`), allow correct typing of these holes and add a new `holes` command that returns the locations of all holes in the current file along with their types (ocaml/merlin#1242, ocaml/merlin#1289) - Mppx: don't restore cookies after invocation. Ppx are invoked only once so there is no need to manage cookies. This small change should increase performance and should not change any other behavior (ocaml/merlin#1309) - Windows: system command variant: do not open a window console when launching a ppx (ocaml/merlin#1270, fixes ocaml/merlin#714) - fix same file documentation bug (ocaml/merlin#1265 by @ulugbekna, fixes ocaml/merlin#1261) + editor modes - vim: Add `MerlinNextHole` and `MerlinPreviousHole` commands to navigate between holes. Jump to the first hole after destruct (ocaml/merlin#1287, ocaml/merlin#1303) - emacs: Add `merlin-next-hole` and `merlin-previous-hole` commands to navigate holes. Jump to the first hole after calling destruct. (ocaml/merlin#1291) - emacs: modernization of the elisp code and conformance with coding guidelines (ocaml/merlin#1247, ocaml/merlin#1310 by Steve Purcell ) - vim & emacs : new client-side "merlin use package" commands, restoring previous behavior (ocaml/merlin#1272, fixes ocaml/merlin#1191) + test suite - cover constructor disambiguation and record fields (ocaml/merlin#1276) - cover the new `holes` command and AST node (ocaml/merlin#1242, ocaml/merlin#1289) - cover the document fix (ocaml/merlin#1265, ocaml/merlin#1315)

@ulugbekna

CHANGES: Tue Apr 12 11:44:22 AM CET 2021 + merlin binary - external configuration reading: + use relative paths to communicate with Dune when possible. This solves issues related to symlinks on Unix and improve Windows support (ocaml/merlin#1271, fixes ocaml/merlin#1288) + make the `workdir` configuration value when using the `dune ocaml-merlin` configuration provider the same as when using `dot-merlin-reader` so that ppxes behaves in the same way as before (ocaml/merlin#1284, fixes ocaml/dune#4479, discussion in ocaml/merlin#1292) - destruct: + improve prefixing of generated constructors in Destruct by filtering opened modules (ocaml/merlin#1277) + make the destruct command more resilient to ill-typed expressions and when called without nodes (ocaml/merlin#1304, fixes ocaml/merlin#1300) - reintroduce some record recovery and improve completion (ocaml/merlin#1276) - introduce a new AST node for holes (`_`), allow correct typing of these holes and add a new `holes` command that returns the locations of all holes in the current file along with their types (ocaml/merlin#1242, ocaml/merlin#1289) - Mppx: don't restore cookies after invocation. Ppx are invoked only once so there is no need to manage cookies. This small change should increase performance and should not change any other behavior (ocaml/merlin#1309) - Windows: system command variant: do not open a window console when launching a ppx (ocaml/merlin#1270, fixes ocaml/merlin#714) - fix same file documentation bug (ocaml/merlin#1265 by @ulugbekna, fixes ocaml/merlin#1261) + editor modes - vim: Add `MerlinNextHole` and `MerlinPreviousHole` commands to navigate between holes. Jump to the first hole after destruct (ocaml/merlin#1287, ocaml/merlin#1303) - emacs: Add `merlin-next-hole` and `merlin-previous-hole` commands to navigate holes. Jump to the first hole after calling destruct. (ocaml/merlin#1291) - emacs: modernization of the elisp code and conformance with coding guidelines (ocaml/merlin#1247, ocaml/merlin#1310 by Steve Purcell ) - vim & emacs : new client-side "merlin use package" commands, restoring previous behavior (ocaml/merlin#1272, fixes ocaml/merlin#1191) + test suite - cover constructor disambiguation and record fields (ocaml/merlin#1276) - cover the new `holes` command and AST node (ocaml/merlin#1242, ocaml/merlin#1289) - cover the document fix (ocaml/merlin#1265, ocaml/merlin#1315)

@ulugbekna

CHANGES: Tue Apr 12 11:44:22 AM CET 2021 + merlin binary - external configuration reading: + use relative paths to communicate with Dune when possible. This solves issues related to symlinks on Unix and improve Windows support (ocaml/merlin#1271, fixes ocaml/merlin#1288) + make the `workdir` configuration value when using the `dune ocaml-merlin` configuration provider the same as when using `dot-merlin-reader` so that ppxes behaves in the same way as before (ocaml/merlin#1284, fixes ocaml/dune#4479, discussion in ocaml/merlin#1292) - destruct: + improve prefixing of generated constructors in Destruct by filtering opened modules (ocaml/merlin#1277) + make the destruct command more resilient to ill-typed expressions and when called without nodes (ocaml/merlin#1304, fixes ocaml/merlin#1300) - reintroduce some record recovery and improve completion (ocaml/merlin#1276) - introduce a new AST node for holes (`_`), allow correct typing of these holes and add a new `holes` command that returns the locations of all holes in the current file along with their types (ocaml/merlin#1242, ocaml/merlin#1289) - Mppx: don't restore cookies after invocation. Ppx are invoked only once so there is no need to manage cookies. This small change should increase performance and should not change any other behavior (ocaml/merlin#1309) - Windows: system command variant: do not open a window console when launching a ppx (ocaml/merlin#1270, fixes ocaml/merlin#714) - fix same file documentation bug (ocaml/merlin#1265 by @ulugbekna, fixes ocaml/merlin#1261) + editor modes - vim: Add `MerlinNextHole` and `MerlinPreviousHole` commands to navigate between holes. Jump to the first hole after destruct (ocaml/merlin#1287, ocaml/merlin#1303) - emacs: Add `merlin-next-hole` and `merlin-previous-hole` commands to navigate holes. Jump to the first hole after calling destruct. (ocaml/merlin#1291) - emacs: modernization of the elisp code and conformance with coding guidelines (ocaml/merlin#1247, ocaml/merlin#1310 by Steve Purcell ) - vim & emacs : new client-side "merlin use package" commands, restoring previous behavior (ocaml/merlin#1272, fixes ocaml/merlin#1191) + test suite - cover constructor disambiguation and record fields (ocaml/merlin#1276) - cover the new `holes` command and AST node (ocaml/merlin#1242, ocaml/merlin#1289) - cover the document fix (ocaml/merlin#1265, ocaml/merlin#1315)

@ulugbekna

CHANGES: Tue Apr 12 11:44:22 AM CET 2021 + merlin binary - external configuration reading: + use relative paths to communicate with Dune when possible. This solves issues related to symlinks on Unix and improve Windows support (ocaml/merlin#1271, fixes ocaml/merlin#1288) + make the `workdir` configuration value when using the `dune ocaml-merlin` configuration provider the same as when using `dot-merlin-reader` so that ppxes behaves in the same way as before (ocaml/merlin#1284, fixes ocaml/dune#4479, discussion in ocaml/merlin#1292) - destruct: make the destruct command more resilient to ill-typed expressions and when called without nodes (ocaml/merlin#1304, fixes ocaml/merlin#1300) - Mppx: don't restore cookies after invocation. Ppx are invoked only once so there is no need to manage cookies. This small change should increase performance and should not change any other behavior (ocaml/merlin#1309) - windows: + system command variant: do not open a window console when launching a ppx (ocaml/merlin#1270, fixes ocaml/merlin#714) + fix Emacs hanging when starting Merlin (ocaml/merlin#1263) + fix path canonicalization (ocaml/merlin#1254) - fix same file documentation bug (ocaml/merlin#1265 by @ulugbekna, fixes ocaml/merlin#1261) + editor modes - emacs: + modernization of the elisp code and conformance with coding guidelines (ocaml/merlin#1247, ocaml/merlin#1310 by Steve Purcell ) + use opam var where applicable (ocaml/merlin#1310) + fix "wrong number of argument" (ocaml/merlin#1250 by @atharvashukla, fixes ocaml/merlin#1234) + fix for Neovim's CursorMoved semantics (ocaml/merlin#1213 by @ddickstein) - vim & emacs : new client-side "merlin use package" commands, restoring previous behavior (ocaml/merlin#1272, fixes ocaml/merlin#1191) + test suite - cover the document fix (ocaml/merlin#1265, ocaml/merlin#1315)

ulugbekna added 2 commits March 2, 2021 12:48

add a test to show (ocaml#1261):

efac724

for lookups of documentation for values defined in the same file, merlin will not return documentation for any but the last documented definition.

fix ocaml#1261: documentation for all values

8fe7ec2

in the same file are now available

ulugbekna force-pushed the fix-same-file-comments branch from befd23e to 8fe7ec2 Compare March 2, 2021 07:57

voodoos merged commit cb1094e into ocaml:master Apr 9, 2021

voodoos mentioned this pull request Apr 9, 2021

Prepare for 4.2 release #1299

Merged

32 tasks

voodoos mentioned this pull request Apr 12, 2021

Add more doc-related tests #1315

Merged

voodoos added a commit that referenced this pull request Apr 12, 2021

[backport] fix same file documentation bug (#1265)

59d5b7a

* Merge pull request #1265 from ulugbekna/fix-same-file-comments * Add more doc-related tests

voodoos added a commit that referenced this pull request Apr 12, 2021

[backport] fix same file documentation bug (#1265)

25058bf

* Merge pull request #1265 from ulugbekna/fix-same-file-comments * Add more doc-related tests

This was linked to issues Apr 12, 2021

merlin does not work (4.1-412 / dune 2.8.2): "Path is not in dune workspace" #1288

Closed

Assertion failure when calling destruct inside a pattern #1300

Closed

This was unlinked from issues Apr 12, 2021

merlin does not work (4.1-412 / dune 2.8.2): "Path is not in dune workspace" #1288

Closed

Assertion failure when calling destruct inside a pattern #1300

Closed

ulugbekna deleted the fix-same-file-comments branch May 3, 2021 11:25

ulugbekna mentioned this pull request Jul 17, 2021

Hover doesn't include odoc content for definitions in the same file ocamllabs/vscode-ocaml-platform#519

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix same file documentation bug#1265

fix same file documentation bug#1265
voodoos merged 2 commits intoocaml:masterfrom
ulugbekna:fix-same-file-comments

ulugbekna commented Feb 9, 2021 •

edited

Loading

Uh oh!

voodoos commented Mar 15, 2021 •

edited

Loading

Uh oh!

ulugbekna commented Apr 9, 2021 via email

Uh oh!

voodoos commented Apr 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ulugbekna commented Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

voodoos commented Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ulugbekna commented Apr 9, 2021 via email

Uh oh!

voodoos commented Apr 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ulugbekna commented Feb 9, 2021 •

edited

Loading

voodoos commented Mar 15, 2021 •

edited

Loading