[move-analyzer] Support document symbol PSL

[yubing744]

Motivation

starcoin-ide expects that users can select a single test function on the interface, and then execute the test. In order to realize this function, I need to extract the test function entry in the move file. I found that move-analyzer has parsed the AST of Move, and found the textDocument/DocumentSymbol interface of the LSP protocol, which can get all the function information in the move file, but move-analyzer has not implemented this interface yet, so I submitted this PR.

Have you read the Contributing Guidelines on pull requests?

YES

Test Plan

I added a use case to the move-analyzer plugin to test the textDocument/DocumentSymbol protocol. Test file: https://github.com/yubing744/starcoin-move/blob/support-document-symbol-psl/language/move-analyzer/editors/code/tests/lsp.test.ts

[yubing744]

@awelc @tnowacki I implemented the telemetry/event protocol of PSL, so that I can wait for the Symbolicator to be ready before initiating the call. Now that the CI check has passed, I want to review it again?

[awelc]

I have only now fully processed your comment about telemetry/event protocol and its introduction to wait for Symbolicator to be ready, so here are some additional comments.

I am not sure why you need to wait for the Symbolicator to be ready. Building symbolication information is pretty fast and it's ready almost instantaneously after the server is up. It just seems like using telemetry calls for this is an overkill, but perhaps I do not fully understand the consequences of the request being unsuccessful in this case.

[yubing744]

Symbolicator

Since the current implementation of Symbolicator is asynchronous, the specific timing of Symbolicator depends on the size and running environment of the project, and by setting a fixed timeout, it will occasionally fail during the ci check phase.

Failed CI check: https://github.com/move-language/move/runs/7034422261?check_suite_focus=true

[yubing744]

@awelc The code has been updated and needs to be reviewed again.

[awelc]

@awelc The code has been updated and needs to be reviewed again.

I am going to re-review it shortly, but I was wondering if you are going to put up a short PR for renaming the default language server path (previously included here). I am happy to do it (we are also getting reports that this is confusing to users) but I don't want to take away the credit form you for noticing this and providing a fix. Please let me know.

[yubing744]

@awelc The code has been updated and needs to be reviewed again.

I am going to re-review it shortly, but I was wondering if you are going to put up a short PR for renaming the default language server path (previously included here). I am happy to do it (we are also getting reports that this is confusing to users) but I don't want to take away the credit form you for noticing this and providing a fix. Please let me know.

I'll make another PR, but I found that a better solution is to modify the verification code to support the move-analyzer command in the PATH, which I am implementing

[yubing744]

@awelc The code has been updated and needs to be reviewed again.

I am going to re-review it shortly, but I was wondering if you are going to put up a short PR for renaming the default language server path (previously included here). I am happy to do it (we are also getting reports that this is confusing to users) but I don't want to take away the credit form you for noticing this and providing a fix. Please let me know.

I'll make another PR, but I found that a better solution is to modify the verification code to support the move-analyzer command in the PATH, which I am implementing

The PR is ok.

[yubing744]

I closed by mistake and now reopen.

[awelc]

Symbolicator

Since the current implementation of Symbolicator is asynchronous, the specific timing of Symbolicator depends on the size and running environment of the project, and by setting a fixed timeout, it will occasionally fail during the ci check phase.

Failed CI check: https://github.com/move-language/move/runs/7034422261?check_suite_focus=true

From what I understand then, the telemetry is introduced mainly to get tests in CI to work. I would still say that this is an overkill and if we can work around it, it would make this PR much cleaner.

Please correct me if I am wrong but the test in question (that is failing without the synchronization currently provided by the telemetry) queries the language server for symbols only once. If this is the case, then the only thing we need to do to avoid getting invalid response is to make the first call to the symbolicator synchronous and have it run before the language server responds to the IDE for the first time after receiving initialization information. It should be much easier to get Symbolicator::get_symbols to be called synchronously from the main thread to compute the symbolication information for the first time than to introduce telemetry just for that purpose (probably best to be done in a separate PR as well).

[yubing744]

Symbolicator

Since the current implementation of Symbolicator is asynchronous, the specific timing of Symbolicator depends on the size and running environment of the project, and by setting a fixed timeout, it will occasionally fail during the ci check phase. Failed CI check: https://github.com/move-language/move/runs/7034422261?check_suite_focus=true

From what I understand then, the telemetry is introduced mainly to get tests in CI to work. I would still say that this is an overkill and if we can work around it, it would make this PR much cleaner.

Please correct me if I am wrong but the test in question (that is failing without the synchronization currently provided by the telemetry) queries the language server for symbols only once. If this is the case, then the only thing we need to do to avoid getting invalid response is to make the first call to the symbolicator synchronous and have it run before the language server responds to the IDE for the first time after receiving initialization information. It should be much easier to get Symbolicator::get_symbols to be called synchronously from the main thread to compute the symbolication information for the first time than to introduce telemetry just for that purpose (probably best to be done in a separate PR as well).

In fact, when I tested manually, I also found that if I opened the Move file without waiting for the symbol parsing to complete, the Outline view of VSCode is empty, and the Outline view depends on the document symbol PSL

But it is indeed overkill, it should be simpler to synchronize calls on the Move side, I will make another version.

[yubing744]

@awelc I've done the refactoring, take the time to review it again.

[awelc]

@awelc I've done the refactoring, take the time to review it again.

Thank you! And my apologies that it's taking time, but I want to make sure the code both fulfills your requirements and is also as clear and concise as possible.

The current version makes a lot more sense to me as it does not require another mechanism (telemetry) to support synchronization. I am still trying to understand why you need synchronization on EVERY request in the first place.

From what I understand the "textDocument/documentSymbol" request is similar to the other types of requests that are already implemented. The difference is in the type of data it returns. In the case of "textDocument/documentSymbol" it is either SymbolInformation[] or DocumentSymbol[] and, for example, in case of "Hover" request it's either Hover or null (as per LSP spec). It does not say in either case that synchronization of any kind is required, and the lack of synchronization is not an issue for any other type of requests that are already out there.

The main question then is - am I missing something and it's crucial for the user experience that the "textDocument/documentSymbol" request is synchronized?

Please note that once information needed to satisfy "textDocument/documentSymbol" request is computed initially when the language server starts, it will be available for all future queries (much like information for other requests is being available). Please also note that all the symbolication information is re-computed on saving an already opened file. What this means is that there is period of time when the already computed information may be out-of-date - between a file save and a request being issued. The re-computation is fast (a couple of seconds or less) so it's rather unlikely that a developer will be unlikely enough to issue a request after save in a shorter time frame that it takes to re-compute this information.

The worst that can happen here then is that they will get date representing the computation state from before the save, and re-issuing the request will almost certainly already provide the correct info. This is not considered a problem for "Hover", "Goto Definition" and the likes, and I would really like to understand if this is a problem for "textDocument/documentSymbol" and why.

I can certainly see that it makes sense to perform the first computation synchronously (so that the date is available to the IDE from the very beginning), but not necessarily on EVERY request.

As a side-note, blocking the main thread of the move-analyzer to wait for completion of the symbolication computation is not ideal as it blocks the entire language server, making it non-responsive from the IDE's perspective. This could be solved with the language server correctly handling cancel requests, but I'd rather avoid it altogether whenever possible.

[yubing744]

@awelc I've done the refactoring, take the time to review it again.

Thank you! And my apologies that it's taking time, but I want to make sure the code both fulfills your requirements and is also as clear and concise as possible.

The current version makes a lot more sense to me as it does not require another mechanism (telemetry) to support synchronization. I am still trying to understand why you need synchronization on EVERY request in the first place.

From what I understand the "textDocument/documentSymbol" request is similar to the other types of requests that are already implemented. The difference is in the type of data it returns. In the case of "textDocument/documentSymbol" it is either SymbolInformation[] or DocumentSymbol[] and, for example, in case of "Hover" request it's either Hover or null (as per LSP spec). It does not say in either case that synchronization of any kind is required, and the lack of synchronization is not an issue for any other type of requests that are already out there.

The main question then is - am I missing something and it's crucial for the user experience that the "textDocument/documentSymbol" request is synchronized?

Please note that once information needed to satisfy "textDocument/documentSymbol" request is computed initially when the language server starts, it will be available for all future queries (much like information for other requests is being available). Please also note that all the symbolication information is re-computed on saving an already opened file. What this means is that there is period of time when the already computed information may be out-of-date - between a file save and a request being issued. The re-computation is fast (a couple of seconds or less) so it's rather unlikely that a developer will be unlikely enough to issue a request after save in a shorter time frame that it takes to re-compute this information.

The worst that can happen here then is that they will get date representing the computation state from before the save, and re-issuing the request will almost certainly already provide the correct info. This is not considered a problem for "Hover", "Goto Definition" and the likes, and I would really like to understand if this is a problem for "textDocument/documentSymbol" and why.

I can certainly see that it makes sense to perform the first computation synchronously (so that the date is available to the IDE from the very beginning), but not necessarily on EVERY request.

As a side-note, blocking the main thread of the move-analyzer to wait for completion of the symbolication computation is not ideal as it blocks the entire language server, making it non-responsive from the IDE's perspective. This could be solved with the language server correctly handling cancel requests, but I'd rather avoid it altogether whenever possible.

Your analysis is correct, my problem now is that ci-test will fail if not synchronously waiting for symbol table resolution to complete. Another solution is to set a large wait time, say 10s, after opening the document.

https://github.com/yubing744/starcoin-move/blob/d623b2172f5a20541ae5635f4e75e4cc23c7c9ae/language/move-analyzer/editors/code/tests/lsp.test.ts#L19

[awelc]

Your analysis is correct, my problem now is that ci-test will fail if not synchronously waiting for symbol table resolution to complete. Another solution is to set a large wait time, say 10s, after opening the document.

https://github.com/yubing744/starcoin-move/blob/d623b2172f5a20541ae5635f4e75e4cc23c7c9ae/language/move-analyzer/editors/code/tests/lsp.test.ts#L19

Let me make a change to the move-analyzer (will try to do it over the weekend) which may solve your problem, and let's take it from there.

[yubing744]

Your analysis is correct, my problem now is that ci-test will fail if not synchronously waiting for symbol table resolution to complete. Another solution is to set a large wait time, say 10s, after opening the document. https://github.com/yubing744/starcoin-move/blob/d623b2172f5a20541ae5635f4e75e4cc23c7c9ae/language/move-analyzer/editors/code/tests/lsp.test.ts#L19

Let me make a change to the move-analyzer (will try to do it over the weekend) which may solve your problem, and let's take it from there.

ok, waiting for your good news

[awelc]

ok, waiting for your good news

Let's see if the news are indeed good :-) I am sorry but the weekend was a bit busy and I only just managed to put it together. In any case, what I did was to make the FIRST symbolication request synchronous - once the IDE client receives confirmation of the language server initialization, the symbolication information will already be available.

For now I created a draft PR (#259) for this so that you can see if this works for you. The change you'd have to make (though I have not tested it myself) would be to open a directory/workspace for the test rather than the test file itself (you can certainly open the file as well, but you need to open directory/workspace first to trigger the initial symbolication computation). If it does work, I should be able to get the PR reviewed and hopefully land it soon.

[yubing744]

ok, waiting for your good news

Let's see if the news are indeed good :-) I am sorry but the weekend was a bit busy and I only just managed to put it together. In any case, what I did was to make the FIRST symbolication request synchronous - once the IDE client receives confirmation of the language server initialization, the symbolication information will already be available.

For now I created a draft PR (#259) for this so that you can see if this works for you. The change you'd have to make (though I have not tested it myself) would be to open a directory/workspace for the test rather than the test file itself (you can certainly open the file as well, but you need to open directory/workspace first to trigger the initial symbolication computation). If it does work, I should be able to get the PR reviewed and hopefully land it soon.

OK, I'll merge your PR and verify it.

[yubing744]

ok, waiting for your good news

Let's see if the news are indeed good :-) I am sorry but the weekend was a bit busy and I only just managed to put it together. In any case, what I did was to make the FIRST symbolication request synchronous - once the IDE client receives confirmation of the language server initialization, the symbolication information will already be available.

For now I created a draft PR (#259) for this so that you can see if this works for you. The change you'd have to make (though I have not tested it myself) would be to open a directory/workspace for the test rather than the test file itself (you can certainly open the file as well, but you need to open directory/workspace first to trigger the initial symbolication computation). If it does work, I should be able to get the PR reviewed and hopefully land it soon.

@awelc I verified that your new approach works, I have merged your code and refactored the current PR, you can review it again.

[yubing744]

@awelc Do you have time to help with this PR?

[awelc]

@awelc Do you have time to help with this PR?

Been really busy... In any case, I have some comments and helped some other folks to take a look, but overall the PR looks good. Since my PR to take care of synchronization problem worked, let me merge it first, though, so it does not have to be bundled with your PR.

The main problem now is that I tried functionally testing your implementation on my laptop and it does not work... I checked out your fork and and checked out the branch:

git clone https://github.com/yubing744/starcoin-move.git
git checkout support-document-symbol-psl

Then I installed the move-analyzer binary:

cd starcoin-move/language/move-analyzer
cargo install --path .

And then build the extension:

cd editors/code
npm run package

When I installed the extension from the generated move-analyzer.vsix file (my VSCode version is 1.69.1), it does not seem to be connecting to the language server at all (e.g., you cannot see Move server parts in the IDE's output). All tests pass if I run npm run test).

[yubing744]

@awelc Do you have time to help with this PR?

Been really busy... In any case, I have some comments and helped some other folks to take a look, but overall the PR looks good. Since my PR to take care of synchronization problem worked, let me merge it first, though, so it does not have to be bundled with your PR.

The main problem now is that I tried functionally testing your implementation on my laptop and it does not work... I checked out your fork and and checked out the branch:

git clone https://github.com/yubing744/starcoin-move.git
git checkout support-document-symbol-psl

Then I installed the move-analyzer binary:

cd starcoin-move/language/move-analyzer
cargo install --path .

And then build the extension:

cd editors/code
npm run package

When I installed the extension from the generated move-analyzer.vsix file (my VSCode version is 1.69.1), it does not seem to be connecting to the language server at all (e.g., you cannot see Move server parts in the IDE's output). All tests pass if I run npm run test).

I will try and follow your steps

[yubing744]

I found some problems on The Windows platform, so I'll add some more tests

[awelc]

FYI, https://github.com/move-language/move/pull/259 has landed.

[yubing744]

FYI, #259 has landed.

OK，I'll merge it

[yubing744]

@awelc I found that move-analyzer has several problems on windows platform:

1. The path to get the document URI should use to_file_path https://github.com/move-language/move/blob/dafa66adb1c35588ba5c7258b17025f03b04cbc9/language/move-analyzer/src/bin/move-analyzer.rs#L127

2. There will be stack overflow in symbol table parsing

The stack overflow problem can be solved by referring to this https://github.com/starcoinorg/starcoin/pull/3445, but there are many other problems, can you help to fix it?

[awelc]

awelc I found that move-analyzer has several problems on windows platform:

I will look into these tomorrow. We have only recently (and partially) enabled Windows support so the Move analyzer wasn't tested on this platform.

[awelc]

awelc I found that move-analyzer has several problems on windows platform:

I will look into these tomorrow. We have only recently (and partially) enabled Windows support so the Move analyzer wasn't tested on this platform.

And ... sadly I had another bug that I was tracking and it took all day. I should find some time tomorrow, though.

[yubing744]

awelc I found that move-analyzer has several problems on windows platform:

I will look into these tomorrow. We have only recently (and partially) enabled Windows support so the Move analyzer wasn't tested on this platform.

And ... sadly I had another bug that I was tracking and it took all day. I should find some time tomorrow, though.

OK

[awelc]

And ... sadly I had another bug that I was tracking and it took all day. I should find some time tomorrow, though.

OK

Got it done yesterday but needed it reviewed and merged. Here it is, though: https://github.com/move-language/move/pull/292

Please see if this solves the problems on your side.

[yubing744]

And ... sadly I had another bug that I was tracking and it took all day. I should find some time tomorrow, though.

OK

Got it done yesterday but needed it reviewed and merged. Here it is, though: #292

Please see if this solves the problems on your side.

OK, I will test again based on your PR

[yubing744]

@awelc I tested it, your PR did not fix the stack overflow problem, but I can solve it by referring to this PR

[yubing744]

@awelc I have added the tests for three platforms now, all passed, can you check again if it can be merged?

[awelc]

@awelc I tested it, your PR did not fix the stack overflow problem, but I can solve it by referring to this PR

Could you please tell me how you trigger the problem with the previous version?

[awelc]

@awelc I tested it, your PR did not fix the stack overflow problem, but I can solve it by referring to this PR

Could you please tell me how you trigger the problem with the previous version?

Will try to do it early next week.

[yubing744]

@awelc I tested it, your PR did not fix the stack overflow problem, but I can solve it by referring to this PR

Could you please tell me how you trigger the problem with the previous version?

Using my branch can trigger, find a windows computer

    git clone https://github.com/yubing744/starcoin-move
    Remove target.x86_64-pc-windows-msvc section configuration in file .cargo/config
    git checkout --track origin/support-document-symbol-psl
    cd language/move-analyzer/editors/code
    npm run dev

[awelc]

I realized I did not actually submit the re-review comments that I put in last week, so here they are.

With respect to the stack overflow fix, I don't think it works if the binary is installed using cargo install --git https://...

I believe that the reason my "old" fix did not work was that I omitted the first symbolicator run (my bad...), which should be now fixed in https://github.com/move-language/move/pull/306

[yubing744]

I realized I did not actually submit the re-review comments that I put in last week, so here they are.

With respect to the stack overflow fix, I don't think it works if the binary is installed using cargo install --git https://...

I believe that the reason my "old" fix did not work was that I omitted the first symbolicator run (my bad...), which should be now fixed in #306

I tested your new PR and stack overflow problem solved

[yubing744]

@awelc My PR has been updated, please take a look again

[yubing744]

Now the version used in automated testing is 1.64.0, I upgraded to 1.69.2 to try.

[awelc]

Now the version used in automated testing is 1.64.0, I upgraded to 1.69.2 to try.

Just to make sure - when you install a locally packaged plugin (npm install) on MacOS, is everything working for you when you interact with the editor?

[yubing744]

Now the version used in automated testing is 1.64.0, I upgraded to 1.69.2 to try.

Just to make sure - when you install a locally packaged plugin (npm install) on MacOS, is everything working for you when you interact with the editor?

ok, i also test some locally at the same time

[yubing744]

Now the version used in automated testing is 1.64.0, I upgraded to 1.69.2 to try.

Just to make sure - when you install a locally packaged plugin (npm install) on MacOS, is everything working for you when you interact with the editor?

Now the version used in automated testing is 1.64.0, I upgraded to 1.69.2 to try.

Just to make sure - when you install a locally packaged plugin (npm install) on MacOS, is everything working for you when you interact with the editor?

I reproduced your problem using 1.69.2 vscode in ci-test, I try to fix some things: https://github.com/yubing744/starcoin-move/runs/7531614817?check_suite_focus=true

[yubing744]

@awelc I fixed the problem of vscode v1.69.2, and manually installed and tested move-analyzer.vsix on the mac, can you check if it can be merged?