feat(lsp): Add Inlay types #2005

spotandjake · 2024-02-02T17:23:55Z

This pr adds inlay types which closes #1359.

This pr is marked as a draft as there is still some work that needs to be done:

We probably want to simplify some of the type signatures (i.e function types can get very complex, might be better to just say Function, and show the return type and param types on the actual lambda)
We probably want to add a config to the vscode extension to toggle inlay types, as you might want inlay hints like the module hints but not the types.
We should probably decide if we want to enable inlays on Tuple patterns, record patterns, constructor patterns

Current Demo Below:

spotandjake · 2024-07-29T01:10:21Z

This pr should be ready for review now.

ospencer · 2024-07-31T19:00:20Z

Here I'd want us to hide hints for function definitions—they're long and get in the way of the function arguments, which already have hints on them/show much of the same information.

It'd also be cool to have hints on destructures, but that could maybe come in a later PR.

marcusroberts

I'm going to approve because the code looks good, but subject to the request to be able to hide/show them as a setting

spotandjake · 2024-08-02T02:00:32Z

Here I'd want us to hide hints for function definitions—they're long and get in the way of the function arguments, which already have hints on them/show much of the same information.

It'd also be cool to have hints on destructures, but that could maybe come in a later PR.

I made that change, I also implemented a cli flag --disable-inlay-types which can be used to dissable the type hints when we release we can add a setting to the vscode extension that injects the flag.

I think we should leave the destructures for a future pr.

I also plan on coming back and adding type hints on function calls that show the label in a future pr.

alex-snezhko · 2024-08-02T02:42:30Z

@spotandjake can't you just disable inlay hints in the client, and then users can decide if they want them or not? What is the purpose of the flag?

spotandjake · 2024-08-02T02:44:49Z

@spotandjake can't you just disable inlay hints in the client, and then users can decide if they want them or not? What is the purpose of the flag?

As we add more inlays, such as parameter hints. and other inlays I can see cases where the user might want to dissable the type hints but keep the others, because of how verbose the type hints are.

alex-snezhko · 2024-08-02T04:56:10Z

compiler/src/language_server/inlayhint.re

+        };
+
+      let enter_binding = ({vb_pat, vb_expr}: value_binding, toplevel: bool) =>
+        if (!toplevel) {


Nit: cut down on the condition nesting, perhaps if (!toplevel && toggle_type_hints) and then

switch (vb_pat) { | {pat_extra: [], pat_desc: TPatVar(_, {loc}) => ... | _ => () }

alex-snezhko · 2024-08-02T05:00:48Z

compiler/src/language_server/inlayhint.re

+                    };
+                  | _ => ()
+                  }
+                | _ => ()


Would this ever be possible? If not should we do failwith("Impossible: ...")?

It seems as though there is a case where it can be a constructor.

alex-snezhko · 2024-08-02T05:01:57Z

compiler/src/language_server/inlayhint.re

+                        List.combine(args, argument_typs),
+                      );
+                    };
+                  | _ => ()


I think this would never be possible. Should we do failwith("Impossible: ...")?

That case can be hit if we are omitting the annotation because typ is None

alex-snezhko · 2024-08-02T05:08:58Z

compiler/src/language_server/inlayhint.re

+                        typ_args,
+                      );
+                    // Make Hints
+                    if (List.length(argument_typs) == List.length(args)) {


Would this ever be false?

I don't think so but I also think it makes sense to ensure we don't make a change somewhere else in the compiler and break this. I am going to add a failwith("Impossible") though if it is the not equal.

alex-snezhko · 2024-08-02T05:22:03Z

compiler/src/language_server/inlayhint.re

+                      List.map(
+                        ((arg, typ: Types.type_expr)) =>
+                          switch (arg) {
+                          | Default(_) => None


Could you expand on this? I'm seeing in your screenshot that the default argument has an inline hint on it

The default hint comes from the binding pass, if this is enabled it shows : Option<_> after the value.

ospencer

Played with this locally and I love how this looks and feels! I understand your point about having the command line flag, but I feel like we should wait to add that when we actually are adding more hints and want them to be controlled separately and not commit to having the flag right now.

ospencer · 2024-08-05T00:11:45Z

compiler/src/language_server/inlayhint.re

-      // Inlay hints for various expressions can be included here.
+
+      let enter_expression = ({exp_desc, exp_type}: expression) =>
+        // Function parameter type hints


Suggested change

// Function parameter type hints

ospencer · 2024-08-05T00:11:53Z

compiler/src/language_server/inlayhint.re

+          | TExpLambda(bindings, _) =>
+            List.iter(
+              ({mb_pat, mb_loc}: match_branch) => {
+                // Get Parameters


Suggested change

// Get Parameters

ospencer · 2024-08-05T00:12:01Z

compiler/src/language_server/inlayhint.re

+                | TPatTuple(args) =>
+                  switch (resolve_typ(exp_type).desc) {
+                  | TTyArrow(typ_args, _, _) =>
+                    // Iterate For Types


Suggested change

// Iterate For Types

ospencer · 2024-08-05T00:12:10Z

compiler/src/language_server/inlayhint.re

+                          },
+                        typ_args,
+                      );
+                    // Make Hints


Suggested change

// Make Hints

spotandjake added the lsp Issues related to the language server. label Feb 2, 2024

spotandjake self-assigned this Feb 2, 2024

spotandjake force-pushed the spotandjake-inlay-types branch from 7925321 to fb31ce3 Compare July 29, 2024 01:08

spotandjake marked this pull request as ready for review July 29, 2024 01:08

spotandjake requested review from marcusroberts, ospencer, phated and alex-snezhko as code owners July 29, 2024 01:08

spotandjake requested a review from peblair as a code owner July 29, 2024 01:57

spotandjake added 4 commits July 28, 2024 21:58

feat(lsp): Add Inlay types

939038a

chore: Simplify Function inlays

9f19ef3

feat(lsp): Implement Inlays For parameters

7b119b7

Do not show hints for top level statements

bd9892e

spotandjake force-pushed the spotandjake-inlay-types branch from c8c64f5 to bd9892e Compare July 29, 2024 01:58

marcusroberts approved these changes Aug 1, 2024

View reviewed changes

spotandjake added 2 commits August 1, 2024 21:38

chore: Disable type hints for functions.

991d79d

chore: Add flag to toggle the type hints

35e2623

alex-snezhko reviewed Aug 2, 2024

View reviewed changes

chore: Apply suggestions from code review

70630cb

alex-snezhko approved these changes Aug 3, 2024

View reviewed changes

ospencer requested changes Aug 5, 2024

View reviewed changes

chore: Apply suggestions from oscar code review

b53d0ad

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(lsp): Add Inlay types #2005

feat(lsp): Add Inlay types #2005

spotandjake commented Feb 2, 2024

spotandjake commented Jul 29, 2024

ospencer commented Jul 31, 2024

marcusroberts left a comment

spotandjake commented Aug 2, 2024 •

edited

Loading

alex-snezhko commented Aug 2, 2024

spotandjake commented Aug 2, 2024 •

edited

Loading

alex-snezhko Aug 2, 2024

alex-snezhko Aug 2, 2024

spotandjake Aug 2, 2024

alex-snezhko Aug 2, 2024

spotandjake Aug 2, 2024

alex-snezhko Aug 2, 2024

spotandjake Aug 2, 2024

alex-snezhko Aug 2, 2024

spotandjake Aug 2, 2024

ospencer left a comment

ospencer Aug 5, 2024

ospencer Aug 5, 2024

ospencer Aug 5, 2024

ospencer Aug 5, 2024

feat(lsp): Add Inlay types #2005

Are you sure you want to change the base?

feat(lsp): Add Inlay types #2005

Conversation

spotandjake commented Feb 2, 2024

spotandjake commented Jul 29, 2024

ospencer commented Jul 31, 2024

marcusroberts left a comment

Choose a reason for hiding this comment

spotandjake commented Aug 2, 2024 • edited Loading

alex-snezhko commented Aug 2, 2024

spotandjake commented Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ospencer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spotandjake commented Aug 2, 2024 •

edited

Loading

spotandjake commented Aug 2, 2024 •

edited

Loading