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

ocaml-lsp reports bogus erros on large files when editor autosaving is enabled #1003

Closed
copy opened this issue Jan 9, 2023 · 12 comments · Fixed by #1004
Closed

ocaml-lsp reports bogus erros on large files when editor autosaving is enabled #1003

copy opened this issue Jan 9, 2023 · 12 comments · Fixed by #1004
Milestone

Comments

@copy
Copy link

copy commented Jan 9, 2023

I can reproduce the following issue on OCaml 4.14 with ocaml-lsp-server 1.13.1 and 1.14.2, and OCaml 5.0 with ocaml-lsp-server pinned to current #500 (176d387).

It's a bit tricky to reproduce, but the following seems to work relatively reliably.

% opam source core
Successfully extracted to /tmp/core.v0.15.1
% dune build -w core/src/core.cmxs

Make some quick changes in large file with auto-saving enabled. I use the following vimrc:

command! AutoWrite autocmd TextChanged,InsertLeave <buffer> try | undojoin | silent! update | catch /^Vim\%((\a\+)\)\=:E790/ | finally | silent! update | endtry

augroup autowrite
    autocmd!
    autocmd Filetype ocaml AutoWrite
augroup END

Edit core/src/command.ml and repeatedly paste open Sys at the top. After a few pastes, lsp reports a bogus syntax error, while the dune watch command doesn't print anything of the sort.

Here is a recording: https://asciinema.org/a/gQqsK784tKJYwPxtB7wBn6I61
And the lsp logfile: lsp.log

@rgrinberg
Copy link
Member

Are you sure you need watch mode to reproduce this? If lsp reports a false syntax error, the issue is likely due to document synchronization.

@copy
Copy link
Author

copy commented Jan 10, 2023

Yeah, I'm pretty sure the issue only happens when watch mode is running.

@rgrinberg
Copy link
Member

What about when you do the following:

  1. Get to the state where the server is showing you a false syntax error
  2. Save the document - :w

Will you still see the syntax error?

@copy
Copy link
Author

copy commented Jan 10, 2023

Yes, the syntax error is still present after :w. The only way to get rid of it is to delete the offending line or restart the lsp.

@rgrinberg
Copy link
Member

Okay, then do you mind trying this PR? #1004

This sounds like a syncing issue.

@copy
Copy link
Author

copy commented Jan 10, 2023

Unfortunately, the issue still happens with #1004 applied.

@rgrinberg
Copy link
Member

I'll have a closer look. Thanks for testing.

@rgrinberg
Copy link
Member

I was unable to reproduce the issue on master after a lot of trying. @ulugbekna have you ran into issues like this?

@rgrinberg
Copy link
Member

Fixed by #1004

@ulugbekna
Copy link
Collaborator

  1. You probably meant refactor: simplify merlin diagnostics #1005
  2. I can't reproduce the problem with refactor: simplify merlin diagnostics #1005 indeed, but let's also wait for @copy's feedback

@rgrinberg
Copy link
Member

Yes, I meant the other PR. @copy please re-open if you can still repro the bug.

@copy
Copy link
Author

copy commented Jan 13, 2023

I can confirm that this is fixed, cheers!

ulugbekna added a commit to ulugbekna/opam-repository that referenced this issue Jan 19, 2023
CHANGES:

## Fixes

- Fix race condition when a document was being edited and dune in watch mode was
  running ([ocaml/ocaml-lsp#1005](ocaml/ocaml-lsp#1005), fixes
  [ocaml/ocaml-lsp#941](ocaml/ocaml-lsp#941),
  [ocaml/ocaml-lsp#1003](ocaml/ocaml-lsp#1003))
ulugbekna added a commit to ulugbekna/opam-repository that referenced this issue Jan 19, 2023
CHANGES:

## Fixes

- Fix race condition when a document was being edited and dune in watch mode was
  running ([ocaml/ocaml-lsp#1005](ocaml/ocaml-lsp#1005), fixes
  [ocaml/ocaml-lsp#941](ocaml/ocaml-lsp#941),
  [ocaml/ocaml-lsp#1003](ocaml/ocaml-lsp#1003))
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 a pull request may close this issue.

3 participants