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

Code Lens #513

Merged
merged 10 commits into from
Jul 25, 2022
Merged

Code Lens #513

merged 10 commits into from
Jul 25, 2022

Conversation

zth
Copy link
Collaborator

@zth zth commented Jul 24, 2022

This implements code lens for function definitions. It's only implemented for function definitions because we also have inlay hints, which fill much of the same purpose as code lenses do, but IMO in a better way. Function definitions are however much better in the code lens format, since the type of the function definition tends to be quite verbose, and that fits better in a code lens rather than an inlay hint.

It's hidden behind a setting + marked as experimental.

image

I also intend this PR to be an educational example of how to implement new commands in the analysis bin. Will do fairly elaborate comments on what I've done.

zth added 7 commits July 24, 2022 18:43
…ng from the language client. This just ensures that the server tells the client that sending codeLens requests is fine. Next we'll implement the actual functionality that resolves the code leneses.
…rinter towards printing function types for code lenses on one line
@zth zth requested a review from cristianoc July 24, 2022 20:16
Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

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

This looks great. Left some minor comments.

I was wondering whether the existence of a feature could slow down the extension: does vscode even know that the feature is turned off?

CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
analysis/src/Hint.ml Show resolved Hide resolved
analysis/src/Protocol.ml Show resolved Hide resolved
analysis/src/Shared.ml Outdated Show resolved Hide resolved
analysis/tests/src/expected/Dce.res.txt Show resolved Hide resolved
analysis/tests/src/expected/Debug.res.txt Show resolved Hide resolved
client/src/extension.ts Show resolved Hide resolved
@@ -30,6 +31,7 @@ interface extensionConfiguration {
enable: boolean;
maxLength: number | null;
};
codeLens: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wondering whether extension config should go to its own file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You mean the type definition?

@zth
Copy link
Collaborator Author

zth commented Jul 25, 2022

@cristianoc I think I addressed all of your comments. Ready for another round.

@zth
Copy link
Collaborator Author

zth commented Jul 25, 2022

This looks great. Left some minor comments.

I was wondering whether the existence of a feature could slow down the extension: does vscode even know that the feature is turned off?

Yes it knows. Whether VSCode does anything from a LS feature or not is controlled by whether the server tells the client that it's capable of receiving requests for that feature or not. If the server doesn't say it supports a feature, the client won't ask for it nor do anything for it. So we should always keep potentially resource intensive features behind settings that are off by default, to ensure the default experience isn't bloated or potentially slow.

@cristianoc
Copy link
Collaborator

Good to merge!

@zth zth merged commit e9756ee into master Jul 25, 2022
@zth zth deleted the functions-codelens branch July 25, 2022 11:03
Comment on lines +398 to +400
| "cle" ->
print_endline ("Code Lens " ^ path);
codeLens ~path ~debug:false
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A "gotcha" that I needed some time to figure out is that these commands that you add to the code can only be 3 chars. I had more than 3 chars and couldn't figure out why it didn't pick them up in the tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Feel free to generalise that.

(* Code lenses are only emitted for functions right now. So look for value bindings that are functions,
and use the loc of the value binding itself so we can look up the full function type for our code lens. *)
let value_binding (iterator : Ast_iterator.iterator)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This creates an AST iterator targeting value bindings, let whatever = .... Since we're only looking for function definitions, we can target value bindings only, since that's the only place they'll appear.

Comment on lines +135 to +138
pvb_pat = {ppat_desc = Ppat_var _; ppat_loc};
pvb_expr = {pexp_desc = Pexp_fun _};
} ->
Copy link
Collaborator Author

@zth zth Jul 25, 2022

Choose a reason for hiding this comment

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

This pattern matches the value binding to target 2 things:

  1. Ppat_var means it's a variable, like let whatever. Assignments can also be destructures let {something} = ...., but this ensures we only target variables.
  2. Pexp_fun means it's a function definition.

pvb_pat is the pattern, which is essentially the left hand side of let whatever = something. pvb_expr is the expression being assigned, so the right hand side after =. We ensure that it is indeed a variable being assigned Ppat_var, and that it's being assigned a function definition Pexp_fun.

Notice that we're also extracting ppat_loc from pvb_pat. That's the location of the variable name itself (whatever in let whatever = ....). That location is important, because we'll extract what type that location has, and that will lead us to the type definition for the function itself. Same logic as if you were to hover the variable holding the function - that's what'll produce the correct function type, not hovering the actual function itself.

Ast_iterator.default_iterator.value_binding iterator vb
in
let iterator = {Ast_iterator.default_iterator with value_binding} in
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This creates an iterator, and ensures that value_binding is the only thing we're overriding with our own logic. There's a ton of other things you can override as well if you'd want to.

Comment on lines +154 to +156
| None -> None
| Some full -> (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This here looks up the compiler artifact for path, which will be the file we're extracting code lenses from. The compiler artifacts are what holds the type information as the compiler has compiled successfully.

Comment on lines +157 to +162
References.getLocItem ~full
~pos:(range.start.line, range.start.character + 1)
~debug
with
| Some {locType = Typed (_, typeExpr, _)} ->
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We then use a helper to get the full type via the compiler artifact for the target file, using the positions where we found variables being assigned functions. This will lead us to the full, correct type definitions for all functions assigned to variables.

A type at a location can be a bunch of different things - we target Typed specifically here, because we know that's the only thing our function definition can be. In Typed, typeExpr is what we're interested in. typeExpr comes from the type checker, and we have helpers that can print those type definitions as strings.

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