move-language / move

Apache License 2.0
2.24k stars 676 forks source link

[move-analyzer] Enhancement for 'goto definition' and add 'multi-project management' functionality #1064

Open robinlzw opened 1 year ago

robinlzw commented 1 year ago

Motivation

This PR introduces two features, 'goto definition' and 'multi-project' functionality, to enhance the capabilities of the [move-analyzer] plugin. These features are aimed at providing users with a more efficient and comprehensive code analysis experience.

The changelog is below:

  1. Added semantic analysis to the Move language and Sui Move, and enhanced some features of the plug-in, such as go-to-definition, multi-project management, etc.;
  2. Support parallel development of multiple projects under the same directory;
  3. Integrated common Sui development commands into Command Palette;

Please review this PR, considering the value it adds to the plugin and its potential impact. Any feedback or suggestions for improvement would be greatly appreciated.

Have you read the Contributing Guidelines on pull requests?

Yes

Feature Description

The 'goto definition' feature allows users to quickly navigate to the definition of a symbol in the codebase. This is particularly useful when dealing with large projects with complex code structures. Users can now easily jump to the exact location where a symbol is defined, improving their productivity and understanding of the codebase.

The 'multi-project' functionality enables the plugin to perform code analysis across multiple projects simultaneously. This feature allows users to analyze and compare code from different projects within the same workspace, providing a holistic view of the code quality and potential issues. It simplifies the code review process for projects that share dependencies or have interconnected codebases.

Overview of Implementation

To implement 'goto definition' functionality, the PR introduces a new navigation algorithm that analyzes the syntax tree of the code and identifies the location of the symbol definition. The algorithm efficiently handles complex code structures, including nested scopes and LambdaExp and so on.

More specifically, item.rs defines grammar attributes related to move language analysis, such as module/LambdaExp/use/test/enum/Fun, etc. syntax.rs is responsible for Parsing, such as parse_module,parse_exp,parse_bind_list. types.rs provides some methods for determining and converting types.

For 'multi-project' functionality, the PR optimizes the existing analyzer to accept multiple project configurations which implemented by project*.rs, and introduces a project selector interface(get_project() in context.rs). This enables users to switch between different projects and perform code analysis seamlessly within a single instance of the plugin.

And also, you can get more other information from language\move-analyzer\doc.

Impact on Existing Functionality

The introduction of these new features does not have any adverse impact on the existing functionality of the [move-analyzer] plugin. Both features are designed to seamlessly integrate with the existing code analysis capabilities, improving the overall user experience.

Test Plan

The PR includes unit tests both on front-end and back-end code to ensure the correctness and stability of the newly added features. Additionally, manual testing was conducted on various codebases with different project structures to verify the functionality in real-world scenarios.

sblackshear commented 1 year ago

Thanks for this improvement! I have asked @awelc to take a look.

robinlzw commented 12 months ago

Thanks for this improvement! I have asked @awelc to take a look.

We have already submitted the PR for the first feature, and the PR for the remaining 4 features (Find references, Hover, Auto completion, Inlay hints)are also ready. Due to the need to rely on the first PR, please review and merge first, and we will immediately submit the remaining 4 PRs in sequence.

awelc commented 11 months ago

The enhancements in this PR look very useful, thanks! It looks like you went the route some other language analyzers followed which is to re-implement the front-end portion of the compilation pipeline to support Move analyzer. While it's a totally legit strategy, adopting it by the "core" team would mean that from now on we would have both the Move compiler and the Move analyzer front-end that we would need to keep maintaining and updating. Considering the currently available resources and also the upcoming Move language changes, it's not something that we would be able to commit to at this point. I fully understand that this is not the ideal outcome but I would really like to work out something that would be satisfactory for both sides.

There are some parts of this PR that can be introduced much more incrementally, and would not require the new compiler front end, namely supporting parallel development of multiple projects and integration of Sui CLI into VSCode's command palette. We would be happy to accept and maintain those as a separate contribution.

With respect to the changes requiring the new compiler front-end, we are happy to work with you on extending our own Move compiler front-end to support whatever's necessary to support the new features, so that we can keep maintaining a single compilation pipeline (though supporting MSL is, admittedly, not a super-high priority for us at this point...). Clearly, you are also welcome to keep the new functionality requiring the new front-end as a separate project, and we could advertise/market is together as part of the Sui ecosystem.

robinlzw commented 11 months ago

uiring the new compiler front-end

Unfortunately, we added new features (multiple projects and Sui CLI integration) based on the new compiler front-end. Can we take over the maintenance of the Move Analyzer? This change is limited to the Move Analyzer and does not affect other modules. Our version has been used in many projects, and is liked by many developers. We also received many requests to improve the MA, we do want to add more features.

awelc commented 11 months ago

Can we take over the maintenance of the Move Analyzer?

In principle, we like this idea, but we need to figure out if we can work it out at a technical level. Will have an update on this shortly.

Also, in order to take over Move Analyzer, you'd have to make sure that it's configurable for different Move flavors (though not all functionality has to be implemented for each flavor). I think this is already happening, though.

awelc commented 11 months ago

@robinlzw - it would also be good to know how you see maintaining the original Move Plugin on your side. In particular, at this point there are two VSCode marketplace entries for "move-analyzer" (or for the original one and one for the Sui one created by you). On our side, we would ideally like to keep continuity with the original VSCode marketplace entry so that folks that already downloaded the plugin (and there is over 10k of them) would have a smooth upgrade experience.

robinlzw commented 11 months ago

@robinlzw - it would also be good to know how you see maintaining the original Move Plugin on your side. In particular, at this point there are two VSCode marketplace entries for "move-analyzer" (or for the original one and one for the Sui one created by you). On our side, we would ideally like to keep continuity with the original VSCode marketplace entry so that folks that already downloaded the plugin (and there is over 10k of them) would have a smooth upgrade experience.

Yes, we'd like to merge our features into the original VSCode marketplace "move-analyzer". Based on the current situation, some features of sui-move-analyzer can be merged into the main branch, and we have maintained compatibility with the original move-analyzer. In the future, we also hope to continue maintaining and iterating on the move-analyzer. If in the future, there are significant syntax changes in sui-move or aptos-move that make it impossible to merge into the main branch, we can also maintain different versions for different branches.

awelc commented 11 months ago

we need to figure out if we can work it out at a technical level

We tried to see if this would be possible but the consensus among the Move repository stakeholders is that the best solution is to keep the new version of the plugin in a separate repository and, if needed, to depend on the Move repository (or Sui repository) upstream. I really wish I had better news...

wrwg commented 11 months ago

@robinlzw - it would also be good to know how you see maintaining the original Move Plugin on your side. In particular, at this point there are two VSCode marketplace entries for "move-analyzer" (or for the original one and one for the Sui one created by you). On our side, we would ideally like to keep continuity with the original VSCode marketplace entry so that folks that already downloaded the plugin (and there is over 10k of them) would have a smooth upgrade experience.

Yes, we'd like to merge our features into the original VSCode marketplace "move-analyzer". Based on the current situation, some features of sui-move-analyzer can be merged into the main branch, and we have maintained compatibility with the original move-analyzer. In the future, we also hope to continue maintaining and iterating on the move-analyzer. If in the future, there are significant syntax changes in sui-move or aptos-move that make it impossible to merge into the main branch, we can also maintain different versions for different branches.

Notice those syntax changes have already happened. For example, aptos-move has lambdas and high-order functions which are not in the main branch.