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

Fix edge cases that prevent completion #1181

Closed
wants to merge 25 commits into from
Closed

Conversation

faldor20
Copy link
Contributor

I encountered a few cases where completion doesn't work correctly. Most obviously:

let a=[1]|>Li

This will not complete properly.
This was because the position_prefix code had a bunch of edge cases it didn't account for. I have added a whole collection of tests for these edge cases, and included two different proposed replacements for this prefix code, plus a benchmark of the difference between the old and new solutions.

I'm quite new to ocaml overall so i wanted to just run this by you all to get some feedback before i clean it up and settle on a solution.

After mucking around with this for many hours I think that the best solution would be to actually run the lexer over the tiny snippet and then use that token information to make an informed decision about what to feed back to merlin.

One key thing that occured to me is that currently none of these solution covers very odd edge cases like:

[1]|>List.
ma
[1]|> List. ma

but use lexer would solve this problem. though I'm honestly not sure if merlin would even deal with such a thing.

The solution here is to change all whitespace to spaces for ease of regex matching(all whitespace is equivelent semantically) and then remove all spaces from the prefix that's passed to merlin.
@faldor20
Copy link
Contributor Author

faldor20 commented Sep 20, 2023

┌─────────────────┬────────────┬─────────┬────────────┐
│ Name            │   Time/Run │ mWd/Run │ Percentage │
├─────────────────┼────────────┼─────────┼────────────┤
│ regex           │   916.98ns │  90.00w │     44.62% │
│ regex_long      │ 1_168.30ns │  97.00w │     56.85% │
│ regex_only      │   392.91ns │  51.00w │     19.12% │
│ regex_only_long │ 2_055.10ns │  50.99w │    100.00% │
│ old             │   269.50ns │  22.00w │     13.11% │
│ old_long        │   260.33ns │  22.00w │     12.67% │
└─────────────────┴────────────┴─────────┴────────────┘

these are the results of running it on my laptop.
This could get a bit if we are happy to reverse the list still. but it does make the regex a little weird, because it's backwards.
This is that reversed version:

┌─────────────────┬──────────┬─────────┬────────────┐
│ Name            │ Time/Run │ mWd/Run │ Percentage │
├─────────────────┼──────────┼─────────┼────────────┤
│ regex           │ 652.22ns │  96.00w │     72.12% │
│ regex_long      │ 904.39ns │ 103.00w │    100.00% │
│ regex_only      │ 128.21ns │  51.00w │     14.18% │
│ regex_only_long │ 169.68ns │  52.00w │     18.76% │
│ old             │ 236.27ns │  22.00w │     26.12% │
│ old_long        │ 235.92ns │  22.00w │     26.09% │
└─────────────────┴──────────┴─────────┴────────────┘

I think it's worth it to use the reversed approach

@@ -91,6 +91,395 @@ describe_opt("textDocument/completion", () => {
expect(items).toMatchObject([{ label: "StringLabels" }]);
});

it("can start completion after operator with space", async () => {
Copy link
Member

Choose a reason for hiding this comment

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

The typescript test suite is deprecated. Look into e2e-new for the new OCaml tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't really want to migrate the whole test suite in this PR and i didn't see any existing completion tests.

Would it be okay to just merge this with the new tests in the old suite and then bundle them in with a larger effort to convert all the completion tests to the new e2e suite?

Copy link
Member

@rgrinberg rgrinberg Sep 23, 2023

Choose a reason for hiding this comment

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

If you don't have time, I can do it myself. I'm afraid that the larger effort to convert everything to the new test suite is never going to come. So for now, we're just not going to let the typescript test suite grow.

@rgrinberg
Copy link
Member

I'm curious for why you added the benchmarks, anything in particular you expect to be slow here?


(** matches let%lwt and let* style expressions. See here:https://v2.ocaml.org/manual/bindingops.html *)
let monadic_bind =
Re.compile @@ Re.Posix.re {|(let|and)([$&*+\-/=>@^|.]|(%[a-zA-Z0-9_']*))$|}
Copy link
Member

Choose a reason for hiding this comment

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

Can you use the combinators from Re instead of Re.Posix please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How set are you on this? I far far prefer to use standard regex because
a) it's something universal
b) it means it's easy to test and debug using tools like https://regexr.com/
But if you think there are good reasons to use the combinators I can convert it

Copy link
Member

Choose a reason for hiding this comment

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

I think the main issue with the way you've written it is that you're using capturing groups that you don't actually need. That's a little confusing and also slows down the matching.

Copy link
Member

Choose a reason for hiding this comment

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

Also, I think that if you used the combinators, constructing the "reverse" regex would be much simpler.

@faldor20
Copy link
Contributor Author

faldor20 commented Sep 21, 2023

@rgrinberg Well, no not particularly. But for code that runs almost every keypress I think speed is key. I wanted to validate that I wasn't causing any meaningful slowdown

@faldor20
Copy link
Contributor Author

Also I've switched to the reversed implementation because its performance is more predictable and writing some regexes backwards isn't that onerous

@faldor20
Copy link
Contributor Author

As for the benchmarks, I understand they may not make sense in this PR and you may want a different benchmarking lib etc. But I do think it makes sense to have something setup at some point. Even just for testing little things to ensure that new changes aren't causing perf regressions. I'm sure some people out there can always accurately guess the performance effect of a change just by looking at it, but I think microbenchmarks are a very good thing to have around.

An example of this is that i find this server mostly very performant(kudos, the startup time is super impressive) but every now and again it gets into a weird state or i open a particular file that causes major slowdown. I'd love to find out why

@rgrinberg
Copy link
Member

As for the benchmarks, I understand they may not make sense in this PR and you may want a different benchmarking lib etc. But I do think it makes sense to have something setup at some point. Even just for testing little things to ensure that new changes aren't causing perf regressions. I'm sure some people out there can always accurately guess the performance effect of a change just by looking at it, but I think microbenchmarks are a very good thing to have around.

I don't mind the benchmarking library you chose. It's just that there's so much overhead to getting completions that I doubt that running a regex is going to make any difference. Anyway, you can keep them.

An example of this is that i find this server mostly very performant(kudos, the startup time is super impressive) but every now and again it gets into a weird state or i open a particular file that causes major slowdown. I'd love to find out why

Unfortunately most of the those performance problems are inherited from merlin itself.

@faldor20 faldor20 marked this pull request as ready for review October 9, 2023 16:26
@faldor20
Copy link
Contributor Author

Okay this is all good to go, all the old and new completion tests have been migrated and the ground work for migrating the others has been established

I used chatgpt for a lot of the migration, I think with a subscription and gpt4 it could be done very very quickly

@rgrinberg rgrinberg added this to the 1.17.0 milestone Oct 16, 2023
@faldor20
Copy link
Contributor Author

Okay, I have no idea why the formatter decided that it would be a good idea to format the result of that expect test, should be fixed now. Sorry this has been a bit of a mess, I'm actually traveling right now, so my work on this has been a bit erratic and distracted.

@faldor20
Copy link
Contributor Author

Okay I actually have no idea why the build failed, I didn't change the doc_to_md file, at all.
I'll try to check more tomorrow when I at my laptop.

@faldor20
Copy link
Contributor Author

Ahh I see, looks like I was hit with the issue fixed here #1204 could I get a rerun of CI @rgrinberg?

@@ -59,6 +59,7 @@ possible and does not make any assumptions about IO.
(odoc-parser (and (>= 2.0.0) (< 2.3.0)))
(ppx_expect (and (>= v0.15.0) :with-test))
(ocamlformat (and :with-test (= 0.24.1)))
(ppx_bench (and :with-test (>= 0.16.0)))
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this for the tests?

@rgrinberg
Copy link
Member

Thanks for the thorough PR and apologies for such a late response. I've merged it in #1212 with a minor tweaks.

The benchmarks had to be disabled in CI because there's no good way to report their results and they just slow things down with the unnecessary dependencies otherwise.

@rgrinberg rgrinberg closed this Nov 27, 2023
@faldor20
Copy link
Contributor Author

All good, sorry I didn't quite have the time and expertise to finish that last little bit. Glad it got done :)

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

## Fixes

- Fix missing super & subscripts in markdown documentation. (ocaml/ocaml-lsp#1170)

- Do not invoke dune at all if `--fallback-read-dot-merlin` flag is on. (ocaml/ocaml-lsp#1173)

- Fix semantic highlighting of infix operators that contain '.'. (ocaml/ocaml-lsp#1186)

- Disable highlighting unit as an enum member to fix comment highlighting bug. (ocaml/ocaml-lsp#1185)

- Improve type-on-hover and type-annotate efficiency by only formatting the type
  of the first enclosing. (ocaml/ocaml-lsp#1191, ocaml/ocaml-lsp#1196)

- Fix the encoding of URI's to match how vscode does it (ocaml/ocaml-lsp#1197)

- Fix parsing of completion prefixes (ocaml/ocaml-lsp#1181)

## Features

- Compatibility with Odoc 2.3.0, with support for the introduced syntax: tables,
  and "codeblock output" (ocaml/ocaml-lsp#1184)

- Display text of references in doc strings (ocaml/ocaml-lsp#1166)

- Add mark/remove unused actions for open, types, for loop indexes, modules,
  match cases, rec, and constructors (ocaml/ocaml-lsp#1141)

- Offer auto-completion for the keyword `in` (ocaml/ocaml-lsp#1217)
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
CHANGES:

## Fixes

- Fix missing super & subscripts in markdown documentation. (ocaml/ocaml-lsp#1170)

- Do not invoke dune at all if `--fallback-read-dot-merlin` flag is on. (ocaml/ocaml-lsp#1173)

- Fix semantic highlighting of infix operators that contain '.'. (ocaml/ocaml-lsp#1186)

- Disable highlighting unit as an enum member to fix comment highlighting bug. (ocaml/ocaml-lsp#1185)

- Improve type-on-hover and type-annotate efficiency by only formatting the type
  of the first enclosing. (ocaml/ocaml-lsp#1191, ocaml/ocaml-lsp#1196)

- Fix the encoding of URI's to match how vscode does it (ocaml/ocaml-lsp#1197)

- Fix parsing of completion prefixes (ocaml/ocaml-lsp#1181)

## Features

- Compatibility with Odoc 2.3.0, with support for the introduced syntax: tables,
  and "codeblock output" (ocaml/ocaml-lsp#1184)

- Display text of references in doc strings (ocaml/ocaml-lsp#1166)

- Add mark/remove unused actions for open, types, for loop indexes, modules,
  match cases, rec, and constructors (ocaml/ocaml-lsp#1141)

- Offer auto-completion for the keyword `in` (ocaml/ocaml-lsp#1217)
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