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

Revert verbosity back to 0 #433

Merged
merged 1 commit into from
May 3, 2021
Merged

Conversation

rgrinberg
Copy link
Member

This matches merlin's default behavior and prevents stack overflows in some
situations.

@rgrinberg rgrinberg requested a review from voodoos May 3, 2021 19:07
This matches merlin's default behavior

Signed-off-by: Rudi Grinberg <[email protected]>
@rgrinberg rgrinberg force-pushed the revert-verbosity branch from bafaf03 to c4f2525 Compare May 3, 2021 19:08
@ulugbekna
Copy link
Collaborator

I understand usefulness of having verbosity set to 0, but I think this change needs to come along with a way to display a module's type in a useful way.

Going from this

image

to this

image

would be a regression in hover functionality, don't you think?

What to do

  1. Maybe we could detect the ast node in merlin and if it's a module, use verbosity = 1
  2. OR look at the ident being hovered over in ocaml-lsp and send a request with an apt verbosity?

I think we should aim to have the next release to have both this PR and a fix for modules. What do you think?

@rgrinberg
Copy link
Member Author

rgrinberg commented May 3, 2021 via email

@rgrinberg rgrinberg merged commit bb8075d into ocaml:master May 3, 2021
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request May 18, 2021
CHANGES:

## Fixes

- Switch `verbosity` from 1 to 0. This is the same default that merlin uses.
  The old value for verbosity (ocaml/ocaml-lsp#433)

- Get fresh diagnostics (warning and error messages) on a file save (ocaml/ocaml-lsp#438)

  Note: If you want the fresh diagnostics to take into account changes in other
  files, you likely need to rebuild your project. An easy way to get automatic
  rebuilds is to run `dune` in a watching mode, e.g.,[dune build --watch].
@voodoos
Copy link
Collaborator

voodoos commented Jul 1, 2021

@ulugbekna :

I think we should aim to have the next release to have both this PR and a fix for modules. What do you think?

Using the master version of OCaml-LSP is indeed less useful since it does not gives the signature of module.
Have you had time to look into that issue ?

@ulugbekna
Copy link
Collaborator

@voodoos I completely forgot about this. I will look into it and report back.

@ulugbekna
Copy link
Collaborator

@voodoos (cc @rgrinberg)

An interesting side note: you see, when we hover over a module pulled from a library, say, Fiber in ocaml-lsp or Mvar in Fiber.Mvar.create, we see the expanded type with the module's items, but if we hover over a module that's pulled from a local library or a same-directory file, you get (module <modulename>). It gets even more interesting with Stdlib.Hashtbl.create, where Stdlib is expanded, but not Hashtbl. Do you happen to know how merlin treats differently "local" modules vs modules from other libs?

The simplest solution to the problem we have I see (without fixing the problem I described in the side note above): we reconstruct the (shortest) current identifier at the given position; here, by shortest I mean Mvar in Fiber.M<|>var.create where <|> is the cursor. If identifier starts with a capital letter, we send verbosity=1 request; otherwise, the default. Of course, in this solution we do not differentiate between a module and a value constructor, but

  1. how bad is that we use verbosity 1 vs 0 for value constructors?
  2. fixing this problem seems to dive quite deep into merlin source (I think merlin does this in type_in_env using Context?)

Do you have better ideas?

Other unrelated questions I have:

I) can merlin offer auto-complete for polymorphic variants?
II) Shouldn't Compl.prefix_of_position allow a back tick in the prefix?

@voodoos
Copy link
Collaborator

voodoos commented Jul 2, 2021

I guess some input from @trefis might be profitable here.

@trefis
Copy link

trefis commented Jul 2, 2021

Maybe we could detect the ast node in merlin and if it's a module, use verbosity = 1

It'd be easier if merlin did that itself. We could add a -verbosity smart flag.

OR look at the ident being hovered over in ocaml-lsp and send a request with an apt verbosity?

Not sure what exactly you mean. But the intended behavior for verbosity is that the editor would first set it to 0, and then if the user issues the same query at the same position (with no other query in between) increase the verbosity by 1.

Do you happen to know how merlin treats differently "local" modules vs modules from other libs?

I think you got your diagnostic slightly wrong.
Merlin will print you the type of the module, sometimes that type is a signature, sometimes it's just an alias to something else.

A higher value of verbosity asks merlin to traverse the alias.

@trefis
Copy link

trefis commented Jul 2, 2021

can merlin offer auto-complete for polymorphic variants?

It used to work, at least in the context of function applications: if the current argument is supposed to be a polymorphic variant constructor, then completion would offer you the choice between the accepted ones.
Apparently that's broken, I don't know since when.

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.

4 participants