这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@liam923
Copy link
Contributor

@liam923 liam923 commented Jul 8, 2025

Completion.completion_order is responsible for defining the order in which Merlin presents autocompletion options, dependent on the context the cursor is in. This PR is meant to open a conversation about modifying this function, as it doesn't seem to currently be ideal.

Specifically, this PR prioritizes modules over types when completing in an expression context. I think this makes sense because it is common to want to type an expression there (ex: you want to type Deferred.t, so Deferred is a good completion suggestion). Jimmy Zhao has suggested removing types as a completion option altogether when completing in an expression context. But I'm concerned that users might actually sometimes want types here.

@liam923
Copy link
Contributor Author

liam923 commented Jul 8, 2025

There's two things I'm concerned about with removing types altogether:

  1. In a syntactically invalid file, Merlin might incorrectly infer an expression context when a user wants to write a type. (To be clear, I'm saying "I suspect Merlin will sometimes do this", not "I've observed Merlin do this.")
  2. Users might actually sometimes want to write a type in an expression context. One example that comes to mind is in [%sexp_of: t] (t is actually an expression, not a type). Or maybe the user will make the following series of edits: let foo = f in -> let foo = f t in -> let foo = f (x : t) in.

@goldfirere
Copy link
Contributor

Is there a test that witnesses the behavior change?

@JimmyZJX
Copy link

JimmyZJX commented Jul 9, 2025

The motivation to tweak the behavior is to improve the quality of fuzzy search behavior.

For example, you have a type string and a module String available at the cursor, and typing an expression should only show the module String rather than the type string. In many languages, typing str yields auto-completion to the class/module name, and we want to replicate that behavior for ocaml lsp. If the type shows up in the list before the module name, it can be a bit annoying.

I'm not sure tweaking the priority is the best approach. I still think we should remove types from the list of completions on normal expressions.

@liam923 pointed out that this might behave worse in ppxes (good catch!). I think we should provide more items only for ppxes given its flexibility.

@liam923
Copy link
Contributor Author

liam923 commented Jul 9, 2025

@JimmyZJX I don't think there's an easy way for Merlin to be aware that it's in a location that a ppx treats like a type, so that would be difficult to do.

@JimmyZJX
Copy link

JimmyZJX commented Jul 9, 2025

@liam923 Ack. I'm fine with just changing the order and hope that clients would sort as expected.

@liam923
Copy link
Contributor Author

liam923 commented Jul 10, 2025

@goldfirere I've added tests - see the "Completions in an expression context" one. Without the change in completion.ml, the type foobar comes before the module Foobar.

@liam923 liam923 requested a review from goldfirere July 10, 2025 14:46
@goldfirere goldfirere merged commit 15b6c9a into main Jul 16, 2025
1 check passed
@liam923 liam923 deleted the adjust-completion-order branch July 16, 2025 19:41
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