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: correctly use merlin's pipeline #904

Merged
merged 1 commit into from
Nov 5, 2022

Conversation

rgrinberg
Copy link
Member

@rgrinberg rgrinberg commented Nov 4, 2022

do not reuse pipelines unless we are using the same pipeline multiple
times in a row

@ulugbekna let's get a quick fix in and work on optimizing in #901 parallel. This bug is likely very annoying to users.

Copy link
Collaborator

@ulugbekna ulugbekna left a comment

Choose a reason for hiding this comment

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

Looks correct. Indeed fixes the go-to-definition for me.

Noteworthy: there's Mpipeline.make used in action_type_annotate.ml. Could this trip up Single_pipeline? I couldn't reproduce the initial bug anyway.

CHANGES.md Outdated Show resolved Hide resolved
@rgrinberg
Copy link
Member Author

there's Mpipeline.make used in action_type_annotate.ml. Could this trip up Single_pipeline? I couldn't reproduce the initial bug anyway.

It should be fine b/c it creates the pipeline inside the merlin thread.

@rgrinberg rgrinberg force-pushed the ps/rr/fix__correctly_use_merlin_s_pipeline branch from 74226d2 to 8680493 Compare November 4, 2022 16:38
@rgrinberg rgrinberg added this to the 1.14.2 milestone Nov 5, 2022
@rgrinberg rgrinberg force-pushed the ps/rr/fix__correctly_use_merlin_s_pipeline branch from 8680493 to fa6b616 Compare November 5, 2022 16:13
@ulugbekna
Copy link
Collaborator

Shall we merge this?

@rgrinberg
Copy link
Member Author

rgrinberg commented Nov 5, 2022 via email

@rgrinberg rgrinberg force-pushed the ps/rr/fix__correctly_use_merlin_s_pipeline branch from fa6b616 to 5be7e81 Compare November 5, 2022 20:21
do not reuse pipelines unless we are using the same pipeline multiple
times in a row

Signed-off-by: Rudi Grinberg <[email protected]>

ps-id: 383efc92-8d86-4d09-b01a-16fcf8c0a0d0
@rgrinberg rgrinberg force-pushed the ps/rr/fix__correctly_use_merlin_s_pipeline branch from 5be7e81 to d1507c6 Compare November 5, 2022 20:48
@rgrinberg rgrinberg merged commit dbf854e into master Nov 5, 2022
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Nov 6, 2022
CHANGES:

## Fixes

- Fix random requests failing after switching documents (ocaml/ocaml-lsp#904, fixes ocaml/ocaml-lsp#898)

- Do not offer related diagnostic information unless the user enables in client
  capabilities (ocaml/ocaml-lsp#905)

- Do not offer diagnostic tags unless the client supports them (ocaml/ocaml-lsp#909)

- Do not attach extra data to diagnostics unless the client supports this
  (ocaml/ocaml-lsp#910)

- Use /bin/sh instead of /bin/bash. This fixes ocamllsp on NixOS
Khady added a commit to Khady/ocaml-lsp that referenced this pull request Nov 7, 2022
* master:
  chore(nix): update flakes (ocaml#915)
  chore(merlin): subrepo to submodule (ocaml#914)
  refactor: add signature_help mli (ocaml#913)
  fix: correctly use merlin's pipeline (ocaml#904)
  feature(lsp): add workspace/diagnostic/refresh (ocaml#910)
  refactor: add mli's to test helpers (ocaml#912)
  fix: client capabilities and diagnostics (ocaml#908)
  fix(lsp): respect diagnostic tag client capabilities (ocaml#909)
  refactor: related information in diagnostics (ocaml#907)
  test: update to show related information (ocaml#906)
  fix: offer related information only when supported (ocaml#905)
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.

2 participants