move icon indicating copy to clipboard operation
move copied to clipboard

[move-analyzer] function signature help

Open 0xYao opened this issue 2 years ago • 11 comments

Motivation

This is a follow-up PR from https://github.com/move-language/move/pull/338. Providing signature help should help developers when calling a function inside a file.

Have you read the Contributing Guidelines on pull requests?

Yes.

Test Plan

(Share your test plan here. If you changed code, please provide us with clear instructions for verifying that your changes work.)

  • Main test cases have been discussed and covered in signatureHelp.test.ts
  • Open the extension host in debug mode. You should see a signature help popup when calling a function.
    • There are two key parts of a signature help. The first is the signature label and the second is the position of the active parameter, and you should see the expected signature label and the expected active parameter position when you move the cursor around.

Demo

https://www.loom.com/share/16e5c241c78a4433bae71046089b73b6

0xYao avatar Aug 13 '22 18:08 0xYao

I have also discovered a problem with the previous PR (https://github.com/move-language/move/pull/338) underlying this one. I added a comment there even though the PR is already merged: https://github.com/move-language/move/pull/338/files#r948387534

Please provide a fix first along with appropriate test before the current PR is merged.

While it might have not mattered in https://github.com/move-language/move/pull/338 where the only thing we cared about was to see if a given identifier is a function, I am pretty sure it will this the current PR as we may mis-assign signatures (only one of two functions in the file will be in the map).

EDIT: It seems like the simplest way to support this would be to have a list of TypeIdents in the BTreeMap instead of only one as signature help response supports passing multiple signatures. If you decide to go for this solution (or another one for that matter), please create a separate PR rather than bundling this with the current one.

If we want to be fancy, we may consider figuring out where the cursor is (in terms of enclosing module) and mark a signature for a function defined in this module (if any) as the active one.

ANOTHER EDIT: Actually, let me provide the fix. I need it done in a specific way that will provide some additional data I need for another enhancement.

ONE MORE EDIIT: I have addressed this in https://github.com/move-language/move/pull/389 (please note that I removed the IdentType for functions is now available via Symbols::file_mods.

awelc avatar Aug 17 '22 20:08 awelc

I have addressed this in #389 (please note that I removed the IdentType for functions is now available via Symbols::file_mods.

Sounds good, let me push the fixes using the latest changes.

0xYao avatar Aug 19 '22 01:08 0xYao

I do not get the parameter name auto-completion

Sorry, what do you mean by parameter name auto-completion? Is this addressed in your #389 PR already?

0xYao avatar Aug 19 '22 01:08 0xYao

Given the cursor position of the current line, I need to find out the function name and the corresponding module that it belongs. It seems like I can reuse some of the logic from Symbolicator::mod_call_symbols, but maybe it's better to do custom string parsing and then extract the correct information from file_mods?

Edit: I think I will check out on_hover_request and see if I can reuse any of the logic there.

0xYao avatar Aug 19 '22 03:08 0xYao

Given the cursor position of the current line, I need to find out the function name and the corresponding module that it belongs.

I thought that you could simply use Symbols::file_mods. In most cases there will be one module per file, and even if there isn't, iterating over the list of modules in the set shouldn't be a huge deal (in terms of speed and complexity). Unless I misunderstand what information you need to obtain and how.

awelc avatar Aug 19 '22 20:08 awelc

I thought that you could simply use Symbols::file_mods. In most cases there will be one module per file, and even if there isn't, iterating over the list of modules in the set shouldn't be a huge deal (in terms of speed and complexity). Unless I misunderstand what information you need to obtain and how.

Additionally, I also need to find the current enclosing scope given a cursor position, and determine which module the developer is calling the function from.

0xYao avatar Aug 19 '22 23:08 0xYao

Additionally, I also need to find the current enclosing scope given a cursor position, and determine which module the developer is calling the function from.

Good point. Here are some thoughts.

First off, it may make sense to special-case implementation for one-module-per-file - we don't have to worry about multiple modules in this case (which should be the common case).

In terms of figuring out module scope (in terms of file lines), when constructing ModuleDefs, we iterate over typed AST unique map that gives us location of modules (I think) in pos: https://github.com/move-language/move/blob/70b34a66473c34ad30d101290b249f2db3c847a2/language/move-analyzer/src/symbols.rs#L708

We currently only store the starting position of the module but this should give us the ending position as well, which we could use to figure out module's scope within a file. To make it fully robust we'd probably have to make an adjustment for a new line (or lines) being inserted that would change start/end of modules in the file, but I am not sure if this is feasible at the moment.

If this does not work for some reason, then I believe a function call can only be inserted inside another function. If we then remembered both starting line of function definition (we already have this in FunctionDef) and the ending line (we could conceivably obtain it from the outermost code block in function's body) we could use Symbol::file_mods to iterate over all functions in a given file and figure out if the cursor is located inside one of them. This is potentially more fragile than the previous suggestion, but I haven't checked if module start/end locations are indeed faithfully represented in the AST.

awelc avatar Aug 19 '22 23:08 awelc

First off, it may make sense to special-case implementation for one-module-per-file - we don't have to worry about multiple modules in this case (which should be the common case).

I am going with the case where we have one-module-per-file for now. I have added a note in the on_signature_help_request function in case we want to address this in the future.

0xYao avatar Aug 20 '22 01:08 0xYao

I am going with the case where we have one-module-per-file for now. I have added a note in the on_signature_help_request function in case we want to address this in the future.

What are you going to to in the short term then if we have two modules in a file and each module defines a function with the same name? Are you going to list them both in the function signature help?

awelc avatar Aug 20 '22 02:08 awelc

What are you going to to in the short term then if we have two modules in a file and each module defines a function with the same name? Are you going to list them both in the function signature help?

The current implementation just returns the signature help of the function in the first module with that function name that we encounter in file_mods. However, if this behaviour is not acceptable, I can update the implementation to return the signature help of the function inside the enclosing module only in the short-term.

But feel free to propose an alternative solution.

0xYao avatar Aug 20 '22 02:08 0xYao

@awelc can you please review?

0xYao avatar Aug 23 '22 12:08 0xYao