feat(lsp): Add Inlay types #2005
Conversation
spotandjake
force-pushed
the
spotandjake-inlay-types
branch
from
7925321
to
fb31ce3
Compare
spotandjake
requested review from
marcusroberts,
ospencer,
phated and
alex-snezhko
as code owners
|
|
spotandjake
force-pushed
the
spotandjake-inlay-types
branch
from
c8c64f5
to
bd9892e
Compare
|
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 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. |
|
@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. |
| }; | ||
|
|
||
| let enter_binding = ({vb_pat, vb_expr}: value_binding, toplevel: bool) => | ||
| if (!toplevel) { |
There was a problem hiding this comment.
Nit: cut down on the condition nesting, perhaps if (!toplevel && toggle_type_hints) and then
switch (vb_pat) {
| {pat_extra: [], pat_desc: TPatVar(_, {loc}) => ...
| _ => ()
}
| }; | ||
| | _ => () | ||
| } | ||
| | _ => () |
There was a problem hiding this comment.
Would this ever be possible? If not should we do failwith("Impossible: ...")?
There was a problem hiding this comment.
It seems as though there is a case where it can be a constructor.
| List.combine(args, argument_typs), | ||
| ); | ||
| }; | ||
| | _ => () |
There was a problem hiding this comment.
I think this would never be possible. Should we do failwith("Impossible: ...")?
There was a problem hiding this comment.
That case can be hit if we are omitting the annotation because typ is None
| typ_args, | ||
| ); | ||
| // Make Hints | ||
| if (List.length(argument_typs) == List.length(args)) { |
There was a problem hiding this comment.
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.
| List.map( | ||
| ((arg, typ: Types.type_expr)) => | ||
| switch (arg) { | ||
| | Default(_) => None |
There was a problem hiding this comment.
Could you expand on this? I'm seeing in your screenshot that the default argument has an inline hint on it
There was a problem hiding this comment.
The default hint comes from the binding pass, if this is enabled it shows : Option<_> after the value.
There was a problem hiding this comment.
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.
spotandjake
force-pushed
the
spotandjake-inlay-types
branch
from
b53d0ad
to
e86c6f2
Compare
|
After discussion in discord instead of the command line flag it would make more sense to use: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#initialize but I think we can defer it to a separate pr when we are going to add more inlay hint types. The final thing this pr needs is test but I don't quite understand how the new lsp tests work yet. |
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:
Function, and show the return type and param types on the actual lambda)Tuple patterns,record patterns,constructor patternsCurrent Demo Below:
