Display signature when hovering over function definitions or module constants #2302

nino · 2023-08-19T18:37:37Z

Hi!

I had a go at #2241.

Hovering over constants now displays their type:

And hovering over function definitions displays their signature:

Happy to make any changes you need to this PR :)

Thanks!

nino · 2023-08-19T20:34:24Z

On closer inspection I noticed hovering over function arguments still just shows the whole function signature. (See picture.)

I'll try to handle hovering on function arguments too.

EDIT: I made things maybe a bit uglier by passing byte_index to hover_for_module_statement, but now it should work correctly when hovering over a function argument (showing its type) and hovering over the function body (showing nothing).

Now hovering over the function body is ignored, and hovering over an argument will display just the type of that argument. Hovering anywhere else in the function head will display the signature.

lpil

Fab! Thank you. I've left a note inline, and we'll need tests for the new functionality.

lpil · 2023-08-20T11:05:52Z

compiler-core/src/language_server/engine.rs

+                        range: Some(src_span_to_lsp_range(arg.location, &line_numbers)),
+                    });
+                }
+            }


This finding logic should live with the other finding logic in node_at_position. This function is only about showing the information rather than hunting through the AST.

I think that would involve extending the Located enum, right? I can have a go at that in the next few days (and also try to write some tests 🤞)

Now the hover_for_... functions don't do a bunch of unpacking of the AST nodes anymore, but it's still happening in the `hover` function. To move the functionality into `node_at_position`, I think we'd have to change the `Located` enum.

lpil · 2023-08-21T08:17:39Z

compiler-core/src/language_server/engine.rs

+                        } else {
+                            Some(hover_for_function_head(fun, lines))
+                        }
+                    }


This finding logic should go in the node_at_position functions with the other finding logic that deals with byte indexes and AST. This function here doesn't perform any finding logic, it just maps from a found node to a hover response function.

Sorry, I was unclear previously!

That sounds like the best way to do this, but I wanted to check if my understanding/approach sounds reasonable before I try to do stuff (also this is my first open-source contribution of non-trivial size, so this is a bit scary).

Does this sound right:

node_at_position should be capable of finding the function head and each function arg. The main type node_at_position returns is Located, and I don't think there's a way to represent a function argument in a Located. So I think I'd add an entry to Located called Arg(&'a TypedArg). Then in TypedDefinition::find_node, I'd try to return a Located::Arg if possible, and a Located::ModuleStatement if not?

(And also I'll look at how to write tests for the new behavior.)

Yup! That's perfect

Why is there still some location logic in this function? What is the case in which it is needed?

The remaining logic is to prevent the function signature coming up when you hover leading/trailing whitespace or comments in the function body. It's essentially undoing the .full_location() logic in ast.rs.

@lpil I added another commit removing all the finding logic from the hover function by adding a separate FunctionBody entry to Located. So now looking at a function head will return Located::ModuleStatement(Definition::Function(fun)), and looking at the function body will return Located::FunctionBody(fun).

I wonder if maybe there should be separate AST nodes for function body and function head.

nino · 2023-08-26T10:02:55Z

I think this might be ready for another look. I've tidied things up a bit and added some tests.

The main issues now are:

The hover function still does a bit of checking whether we're inside the function head or body:

Located::ModuleStatement(Definition::Function(fun)) => {
    if fun.location.contains(byte_index) {
        Some(hover_for_function_head(fun, lines))
    } else {
        // Don't show hover info for function body
        None
    }
}

Which could be fixed, but I'm not sure if that would break any assumptions we're making about find_node finding a Function when looking at a position inside the function body.

There are two places where I'm defining a temporary variable let empty_smolstr = SmolStr::from("");, because I couldn't figure out how to do unwrap_or_default with SmolStrs. Maybe there's a better solution for that.

lpil

This is looking really nice! I've left a couple tiny questions inline.

Would you mind also updating the CHANGELOG file? Thank you

lpil · 2023-08-26T10:30:53Z

compiler-core/src/language_server/engine.rs

+    let empty_smolstr = SmolStr::from("");
+    let documentation = fun.documentation.as_ref().unwrap_or(&empty_smolstr);


Does .unwrap_or_default() not work here? Or .unwrap_or("".into())

.unwrap_or_default() was the first thing I'd tried since you were using it for all the results of .get_documentation(). That didn't work because .documentation itself is a SmolStr, and &SmolStr doesn't have Default implemented.

Trying some more stuff, the only other way I can see is

let documentation = fun .documentation .as_ref() .map(|s| s.as_str()) .unwrap_or_default();

which feels "cleaner" in some way, but also less readable?

…-over-function-definition

lpil

Fantastic!! Thank you so much

lpil · 2023-08-30T11:36:38Z

Would you like to rebase the branch to remove the merge commits and conflicts, or should I squash merge?

nino · 2023-08-30T11:38:31Z

Probably easiest if you just squash merge? But I can also do a rebase if that's the preferred workflow

lpil · 2023-08-30T11:39:41Z

Works for me! Thanks

nino added 2 commits August 19, 2023 18:40

Display signature when hovering over function definition

c798f3e

Also show module constant's type on hover

b331405

Handle hovering over function arguments and body

976a419

Now hovering over the function body is ignored, and hovering over an argument will display just the type of that argument. Hovering anywhere else in the function head will display the signature.

lpil reviewed Aug 20, 2023

View reviewed changes

Make the hover functions smaller

1149636

Now the hover_for_... functions don't do a bunch of unpacking of the AST nodes anymore, but it's still happening in the `hover` function. To move the functionality into `node_at_position`, I think we'd have to change the `Located` enum.

lpil reviewed Aug 21, 2023

View reviewed changes

Add Arg to Located enum

0add94d

I wonder if maybe there should be separate AST nodes for function body and function head.

nino marked this pull request as draft August 23, 2023 17:18

nino added 2 commits August 26, 2023 10:44

Add tests for hovering over function definition

8c1f1c6

Add tests for hovering over module constants

69db76c

nino marked this pull request as ready for review August 26, 2023 09:57

lpil reviewed Aug 26, 2023

View reviewed changes

nino added 4 commits August 26, 2023 13:43

Add changelog entry

051c6b0

Merge branch 'main' of github.com:gleam-lang/gleam into 2241-hovering…

8a5bee8

…-over-function-definition

Refactor a bit more to remove *all* finding logic from hover

945e2fb

Merge branch 'main' of github.com:gleam-lang/gleam into 2241-hovering…

50702bd

…-over-function-definition

nino mentioned this pull request Aug 30, 2023

Display type when hovering over function arguments #2310

Closed

nino requested a review from lpil August 30, 2023 07:55

lpil approved these changes Aug 30, 2023

View reviewed changes

lpil enabled auto-merge (rebase) August 30, 2023 11:25

auto-merge was automatically disabled August 30, 2023 11:35
Rebase failed

lpil merged commit 7865442 into gleam-lang:main Aug 30, 2023
10 checks passed

nino deleted the 2241-hovering-over-function-definition branch August 30, 2023 11:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display signature when hovering over function definitions or module constants #2302

Display signature when hovering over function definitions or module constants #2302

nino commented Aug 19, 2023

nino commented Aug 19, 2023 •

edited

lpil left a comment

lpil Aug 20, 2023

nino Aug 20, 2023

lpil Aug 21, 2023

nino Aug 22, 2023

lpil Aug 26, 2023

lpil Aug 26, 2023

nino Aug 26, 2023

nino Aug 28, 2023

nino commented Aug 26, 2023

lpil left a comment

lpil Aug 26, 2023

nino Aug 26, 2023

lpil left a comment

lpil commented Aug 30, 2023

nino commented Aug 30, 2023

lpil commented Aug 30, 2023

		let empty_smolstr = SmolStr::from("");
		let documentation = fun.documentation.as_ref().unwrap_or(&empty_smolstr);

Display signature when hovering over function definitions or module constants #2302

Display signature when hovering over function definitions or module constants #2302

Conversation

nino commented Aug 19, 2023

nino commented Aug 19, 2023 • edited

lpil 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

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

nino commented Aug 26, 2023

lpil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpil left a comment

Choose a reason for hiding this comment

lpil commented Aug 30, 2023

nino commented Aug 30, 2023

lpil commented Aug 30, 2023

nino commented Aug 19, 2023 •

edited