[move-analyzer] Add Docstring rendering on-hover #328

Deagler · 2022-07-31T21:26:08Z

Motivation

The existing move-analyzer does not render docstrings when you hover over functions, structs, or fields.

Due to the lack of docstring rendering, new Move developers need to keep a separate window open where they can reference the standard library and the Sui/Aptos codebase.

These changes speed up DX by adding docstring rendering when hovering over functions, structs, constants, etc...

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Build the move-analyzer locally and install the move analyzer extension. Now setup .vscode/settings.json like this:

{
  "move-analyzer.server.path": "<some path>/move/target/debug/move-analyzer"
}

You will see that the docstrings are now attached on-hover (test via the files provided inmove-analyzer/tests/symbols/sources)

awelc · 2022-08-02T19:12:42Z

I was wondering if you'd consider adding an automated test to the newly extended testing framework to test this feature.

awelc · 2022-08-02T20:10:27Z

Also a bit of heads up, modifying source code of the Move test files will break the move-analyzer tests (cargo test) as they hard-code line/column numbers for testing if symbolication returns correct results.

The only Move source file you can modify freely without worrying about it is M2.move

This in addition to the current problem which seems unrelated (unwrap is failing at some point).

…-rendering

awelc · 2022-08-02T22:06:26Z

The only Move source file you can modify freely without worrying about it is M2.move

Crap... I guess I was wrong about that... My apologies... You have to adjust line numbers in the test portion of symbols.rs or create a new module for the new tests.

Deagler · 2022-08-02T22:11:34Z

I was wondering if you'd consider adding an automated test to the newly extended testing framework to test this feature.

By newly extended testing framework do you mean the tests in editors/code/tests ? Or would it be sufficient to add tests into symbols.rs

The only Move source file you can modify freely without worrying about it is M2.move

Crap... I guess I was wrong about that... My apologies... You have to adjust line numbers in the test portion of symbols.rs or create a new module for the new tests.

No problem - will create new modules and fix up M2.move as well

awelc · 2022-08-02T22:19:46Z

By newly extended testing framework do you mean the tests in editors/code/tests ? Or would it be sufficient to add tests into symbols.rs

I see the unit tests (currently in symbols.rs but we may decide to move the to a separate file if the number gets totally unwieldy) as more comprehensive ones (e.g., they cover all typed AST nodes to make sure that symbolication works correctly for them) whereas the integration tests are more to guarantee that the info is correctly received by the editor and are in my opinion also worthwhile as they can, for example, indicate changes in the language server protocol upon the editor version upgrade.

Admittedly there are not "integration" tests (editors/code/tests) for existing "newer" features, but I will try adding the when I have a spare moment.

So, in short, ideally we'd have both - more functionally testing (if necessary) in unit tests and perhaps just one integration test just to make sure that things work properly between the move analyzer and the editor.

Deagler · 2022-08-03T15:55:01Z

So, in short, ideally we'd have both - more functionally testing (if necessary) in unit tests and perhaps just one integration test just to make sure that things work properly between the move analyzer and the editor.

Changes

Added several unit tests that test docstring construction in symbols.rs
Two integration tests (one that tests docstring recognition within a module, and one for external modules).
Changed the ESLint max line length from 100 --> 120 (makes the docstring integration tests much cleaner)

LMK how that looks!

jolestar · 2022-08-03T16:25:34Z

@yubing744: bors r+

Help review

yubing744 · 2022-08-04T00:24:31Z

@Deagler You don't seem to be dealing with another form of documentation comments /**, which is mentioned in the lexical analysis related code:

move/language/move-compiler/src/parser/lexer.rs

Lines 212 to 216 in f618b1b

    
               /// Documentation comments are comments which start with 
        
               /// `///` or `/**`, but not `////` or `/***`. The actually comment delimiters 
        
               /// (`/// .. <newline>` and `/** .. */`) will be not included in extracted comment string. The 
        
               /// span in the returned map, however, covers the whole region of the comment, including the 
        
               /// delimiters.

The rest of the code looks fine.

…ocstrings

Deagler · 2022-08-04T17:06:47Z

You don't seem to be dealing with another form of documentation comments /**, which is mentioned in the lexical analysis related code

Thanks for the heads up. For some reason I thought Move only supported /// as docstrings. I've added the relevant changes + unit tests + integration test to support that format.

multi-line asterix docstrings

single-line asterix docstrings

jolestar

LGTM

awelc

Overall looks great! I requested a few small(ish) changes but we are almost good to go!

awelc · 2022-08-05T20:00:30Z

language/move-analyzer/editors/code/tests/lsp-demo/sources/M6.move

@@ -0,0 +1,42 @@
+module Symbols::M6 {


It seems like these test files are duplicates of those in language/move-analyzer/tests/symbols/sources. Could we keep just one copy?

I suggest we we adopt the following convention. If a move source file is used only for integration test, it would be in language/move-analyzer/editors/code/tests/lsp-demo/sources, but if it's used for both integration and unit tests, let's keep it in language/move-analyzer/tests/symbols/sources.

I've been unable to reliably access files from language/move-analyzer/tests/symbols/sources via the test-runner in editors/code.

Instead, I've stripped the irrelevant portions of code from the tests so they can be used purely as integration tests. Similar to what @yubing744 did for porting M1.move over as an integration test.

It just rubs me the wrong way to have Move source files used for testing (and similar ones at that) in two different locations.

I've been unable to reliably access files from language/move-analyzer/tests/symbols/sources via the test-runner in editors/code.

If the opposite is true (you can access editors/code tests from symbolicator tests, and if changing your tests to use the same files is not a huge problem, please use tests in editors/code for everything and I will move my tests there (will also try to re-do @yubing744's integration test to use the original, possibly augmented, M1 module code).

I can't access editors/code tests from symbolicator tests with the current setup because cargo test runs the tests at a different level compared to editors/code

This causes the reference to MoveStdLib to break (see: Move.toml in editors/code). If I update the path to MoveStdLib then the symbolicator tests start working but the editors/code tests break.

If you know how to fix that reference to the StdLib in both cases then I think what you're proposing will work!

If you know how to fix that reference to the StdLib in both cases then I think what you're proposing will work!

Technically we could pull the dependency remotely but perhaps it's indeed not worth it at the moment. Thank you for your patience!

awelc · 2022-08-05T20:01:27Z

language/move-analyzer/editors/code/src/commands/lsp_command.ts

+/**
+ * An LSP command textDocument/hover
+ */
+export async function textDocumentHover(


Just for my own information - this is only used in tests, right (and would not be needed otherwise)?

Yes - this is only used in the integration tests. I've followed the same format as the existing LSP command in that file (textDocumentDocumentSymbol).

language/move-analyzer/tests/symbols/sources/M1.move

language/move-analyzer/src/symbols.rs

awelc · 2022-08-05T22:02:15Z

language/move-analyzer/src/symbols.rs

@@ -147,6 +147,8 @@ pub struct UseDef {
    def_loc: DefLoc,
    /// Location of the type definition
    type_def_loc: Option<DefLoc>,
+    /// Doc string for the relevant identifier/function


I also realized that it might be more intuitive to make the doc_string part of DefLoc (possibly renaming it to DefInfo), particularly keeping in mind that we may want to optimize this code at some point and use a reference to DefLoc in UseDef rather than "inlining" it.

It will cause a lot of diffs across symbols.rs and it will require a few changes to the code itself. I don't think I can include a String inside DefLoc as it has the Copy trait and DefLoc is copied in multiple places across this file.

Unless there's a simple way to do it, I won't have time to make these changes in this PR.

It will cause a lot of diffs across symbols.rs and it will require a few changes to the code itself. I don't think I can include a String inside DefLoc as it has the Copy trait and DefLoc is copied in multiple places across this file.

Unless there's a simple way to do it, I won't have time to make these changes in this PR.

Not necessarily. It's just that after looking at this again it seemed like comment is associated with a def and we have a structure that describes the def already, so it would be a more natural place for the doc string. I think it may be worth doing in the future, though.

awelc · 2022-08-06T00:36:40Z

@Deagler - linting is failing and it's a required check....

Deagler · 2022-08-06T00:40:37Z

@Deagler - linting is failing and it's a required check....

~~Fixed~~

edit: my bad - failed again... fixed one more time and force-pushed.

* [move-analyzer] Add docstring rendering for consts, structs, functions * [move-analyzer] Optimize docstring construction * [move-analyzer] Add rendering for docstrings across other modules * [move-analyzer] Fix M1.move tests * [move-analyzer] Add unit tests for docstring construction * [move-analyzer] Add integration tests for docstring construction * [move-analyzer] Add support for /** .. */ docstring comments * [move-analyzer] Update unit/integration tests to test asterix-based docstrings * [move-analyzer] Fix general PR comments * [move-analyzer] Improve extract_doc_string readability * [move-analyzer] Fix linter issues

Deagler added 3 commits July 31, 2022 15:20

[move-analyzer] Add docstring rendering for consts, structs, functions

21f1d13

[move-analyzer] Optimize docstring construction

f5b6535

[move-analyzer] Add rendering for docstrings across other modules

0e9c919

sblackshear requested review from awelc and jolestar August 2, 2022 19:02

Deagler added 2 commits August 2, 2022 14:37

Merge remote-tracking branch 'origin/main' into Deagler/add-docstring…

ef03145

…-rendering

[move-analyzer] Fix M1.move tests

a0e2a91

[move-analyzer] Add unit tests for docstring construction

a7f08b9

Deagler force-pushed the Deagler/add-docstring-rendering branch from bc759f2 to a7f08b9 Compare August 3, 2022 15:32

[move-analyzer] Add integration tests for docstring construction

8b40c50

Deagler added 2 commits August 4, 2022 09:55

[move-analyzer] Add support for /** .. */ docstring comments

9dc1159

[move-analyzer] Update unit/integration tests to test asterix-based d…

2ed77ac

…ocstrings

jolestar approved these changes Aug 5, 2022

View reviewed changes

awelc suggested changes Aug 5, 2022

View reviewed changes

[move-analyzer] Fix general PR comments

a4a9784

0xYao mentioned this pull request Aug 5, 2022

[move-analyzer] basic function name completion #338

Merged

awelc reviewed Aug 5, 2022

View reviewed changes

[move-analyzer] Improve extract_doc_string readability

8923e41

Deagler force-pushed the Deagler/add-docstring-rendering branch from 4c485d6 to 8923e41 Compare August 5, 2022 22:05

[move-analyzer] Fix linter issues

f2fdfb6

Deagler force-pushed the Deagler/add-docstring-rendering branch from 390b73d to f2fdfb6 Compare August 6, 2022 01:09

awelc approved these changes Aug 6, 2022

View reviewed changes

awelc merged commit 3c47a95 into move-language:main Aug 6, 2022

brson pushed a commit to brson/move that referenced this pull request Sep 7, 2023

Small fix: correct exit on non-existing outfile (move-language#328)

c1ca260

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[move-analyzer] Add Docstring rendering on-hover #328

[move-analyzer] Add Docstring rendering on-hover #328

Deagler commented Jul 31, 2022 •

edited

Loading

awelc commented Aug 2, 2022

awelc commented Aug 2, 2022

awelc commented Aug 2, 2022

Deagler commented Aug 2, 2022 •

edited

Loading

awelc commented Aug 2, 2022

Deagler commented Aug 3, 2022 •

edited

Loading

jolestar commented Aug 3, 2022 •

edited

Loading

yubing744 commented Aug 4, 2022 •

edited

Loading

Deagler commented Aug 4, 2022

jolestar left a comment

awelc left a comment

awelc Aug 5, 2022

Deagler Aug 5, 2022

awelc Aug 5, 2022

Deagler Aug 5, 2022 •

edited

Loading

awelc Aug 6, 2022

awelc Aug 5, 2022

Deagler Aug 5, 2022

awelc Aug 5, 2022

Deagler Aug 5, 2022 •

edited

Loading

awelc Aug 5, 2022

awelc commented Aug 6, 2022

Deagler commented Aug 6, 2022 •

edited

Loading

[move-analyzer] Add Docstring rendering on-hover #328

[move-analyzer] Add Docstring rendering on-hover #328

Conversation

Deagler commented Jul 31, 2022 • edited Loading

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

awelc commented Aug 2, 2022

awelc commented Aug 2, 2022

awelc commented Aug 2, 2022

Deagler commented Aug 2, 2022 • edited Loading

awelc commented Aug 2, 2022

Deagler commented Aug 3, 2022 • edited Loading

jolestar commented Aug 3, 2022 • edited Loading

yubing744 commented Aug 4, 2022 • edited Loading

Deagler commented Aug 4, 2022

jolestar left a comment

Choose a reason for hiding this comment

awelc 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

Deagler Aug 5, 2022 • 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

Deagler Aug 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awelc commented Aug 6, 2022

Deagler commented Aug 6, 2022 • edited Loading

Deagler commented Jul 31, 2022 •

edited

Loading

Deagler commented Aug 2, 2022 •

edited

Loading

Deagler commented Aug 3, 2022 •

edited

Loading

jolestar commented Aug 3, 2022 •

edited

Loading

yubing744 commented Aug 4, 2022 •

edited

Loading

Deagler Aug 5, 2022 •

edited

Loading

Deagler Aug 5, 2022 •

edited

Loading

Deagler commented Aug 6, 2022 •

edited

Loading