Skip to content

Conversation

lessp
Copy link

@lessp lessp commented Oct 14, 2025

This adds auto-completion for labelled arguments. I decided to disable them for primitives like int or bool as it felt a bit iffy, but it'd work for those too. For int merlin seems to suggest 0 etc.

Examples

Here's an example with variants:

CleanShot.2025-10-14.at.13.30.03.mp4

And records:

CleanShot.2025-10-14.at.13.35.27.mp4

match context with
| `Application { Query_protocol.Compl.labels = _; argument_type }
when not (is_polymorphic_type argument_type)
&& not (is_primitive_type argument_type) ->
Copy link
Author

@lessp lessp Oct 14, 2025

Choose a reason for hiding this comment

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

Not sure about this, perhaps it'd be useful to get e.g. 0 or false for primitive types as well?

@lessp lessp marked this pull request as ready for review October 14, 2025 07:44
@voodoos
Copy link
Collaborator

voodoos commented Oct 15, 2025

Thank you @lessp, that's an interesting proposition. To clarify a bit, this PR does two things:

  1. Add : as a trigger character for completion
  2. Use Construct results to suggest labeled argument values in function application

Item 1 seems all-right when calling a function with a labelled parameter, this might actually be a good thing, but we should make sure that it is acceptable to trigger completion on all other usages of : in the syntax. And add appropriate tests.

About 2, I wonder how often this is going to be useful in practice, once we leave the realm of small handcrafted examples ? I think it's worth experimenting though, maybe by having this behavior configurable. Also, it feels like this should work for every function arguments, not only labelled ones... and maybe other places too ?

@xvw, @rgrinberg, do you have an opinion on this ?

@lessp
Copy link
Author

lessp commented Oct 15, 2025

Thank you @lessp, that's an interesting proposition. To clarify a bit, this PR does two things:

Thanks for leaving your thoughts @voodoos! I primarily thought I'd open up the discussion on this.

  1. Add : as a trigger character for completion

Right, I realise one would have to be wary of where we'd trigger, and I don't want to get ahead of anything, but ideally we'd also be able to trigger for = for MLX and Reason JSX to get padding=.

  1. Use Construct results to suggest labeled argument values in function application

Yes, although I did realise that these are available without having to query merlin if you add e.g. ~color:_. They're quite noisy though see e.g. image, furthermore e.g. variants are scattered across and not sorted within the list.

image

Item 1 seems all-right when calling a function with a labelled parameter, this might actually be a good thing, but we should make sure that it is acceptable to trigger completion on all other usages of : in the syntax. And add appropriate tests.

About 2, I wonder how often this is going to be useful in practice, once we leave the realm of small handcrafted examples ? I think it's worth experimenting though, maybe by having this behavior configurable. Also, it feels like this should work for every function arguments, not only labelled ones... and maybe other places too ?

Can you expand on the statement below?

we leave the realm of small handcrafted examples?

The actual need for this sprung out of it being non-obvious what an argument takes, especially when the type itself is defined a few indirections away. In my experience this becomes even more useful in a larger code-base. The behaviour is something that I miss when switching between other popular languages like TypeScript.

For e.g.

(* style_vars.ml *)
type padding = 
  | NoPaddingSmall
  | Medium
  | LargeCustom of int

type size = 
  | Small
  | Medium
  | Large

type z_layer =
  | Tooltip
  | Popover
  | Modal
  | ExpandableRow
  | Above of z_layer

(* row.ml *)
let create ~(padding: Css.padding) ... = ...

(* consumer.ml *)
let my_row = Row.create ~padding:

As for e.g. records. What is your current approach to constructing the person for something like the following?

(* some_file.ml *)
type employee = { 
  name: string;
  email: string;
}

type customer = {
  name: string;
  age: int
}

... additional types

(* some_consumer.ml *)
let x = create ~customer:

I'd argue that even if you just see the auto-completion, you'd know immediately what arguments you'd need to provide. The alternative as I see it would be to look at the hover information, see something like let create ~(customer: Some_file.customer) -> ... and you'd then do create ~customer:Some_file. and try to find a shape that matches, only to see what you'd need to construct it.

@xvw, @rgrinberg, do you have an opinion on this ?

@lessp
Copy link
Author

lessp commented Oct 15, 2025

Another experiment I did was to auto-complete valid constructors that return the type in question.

So, instead of doing this:

CleanShot.2025-10-15.at.23.05.33.mp4

You'd, by some heuristic, list definitions that return that type:

image (1)

And on selection, you'd get the correct syntax e.g. ~color:(Test_re.Color.custom ~r:_ ~g:_ ~b:_) similar to the Custom variant.

CleanShot.2025-10-15.at.23.07.04.mp4

@xvw
Copy link
Collaborator

xvw commented Oct 16, 2025

Hi !
I totally agree with one, but I think that construct is usually to much used as an hack entry door so I guess that just relaying on completion is sufficient?

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