Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[move-ide] Added support for init function auto-completion #16140

Closed
wants to merge 23 commits into from

Conversation

awelc
Copy link
Contributor

@awelc awelc commented Feb 8, 2024

Description

This PR adds preliminary support for auto-completing init function definition. It contains the beginning of the proposed fine-grain error recovery in the parser, currently only implemented for function definitions. The gist of the idea is that we parse function definition as much as we can, and regardless of whether the parsing was fully successful or not, we still return a correct function Function AST node if we can (i.e., if we correctly parse attributes, fun keyword, and function name).

In particular, this would generate a correct Function AST node, for function ini even though its definition is incomplete:

module 0x0::some_module {
    fun ini
}

What this allows us to do, is context-sensitive auto-completion for init functions using LSP concept of snippets. In particular, when triggering auto-completion in the code fragment above right after ini, the IDE will recognize that we are trying to auto-complete a function whose name starts with ini and will offer a code snippet expanding this to a full init function definition (we will be able to omit sui::tx_context prefix once auto-imports are in place):

module 0x0::some_module {
    fun init(ctx: &mut sui::tx_context::TxContext) {   
    }
}

Furthermore, if the module contains a struct definition being a one-time-witness candidate, the IDE will be smart enough to recognize this and offer an expanded snippet:

module 0x0::some_module {
    struct SOME_MODULE has drop {}
    fun init(witness: SOME_MODULE, ctx: &mut sui::tx_context::TxContext) {   
    }
}

Finally, this snippet will not be offered for all identifiers with a given prefix, only those that represent function definitions. In particular, it will not do it for local variable declaration in the following code fragment:

module 0x0::some_module {
    fun foo() {
        let ini   
    }
}

The main current limitation is that at this point auto-completion must be preceded by a save (to rebuild symbols information in the LSP server) and manual re-triggering of the auto-completion command. In order to support a smooth user experience, we need to add support for the compiler to use a virtual file system (files stored in memory).

Test Plan

Added a compiler test and a symbolicator test for this feature.


If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section.

Type of Change (Check all that apply)

  • protocol change
  • user-visible impact
  • breaking change for a client SDKs
  • breaking change for FNs (FN binary must upgrade)
  • breaking change for validators or node operators (must upgrade binaries)
  • breaking change for on-chain data layout
  • necessitate either a data wipe or data migration

Release notes

Developers might see more compiler diagnostics as selected parsing errors no longer prevent compilation and diagnostics from the compiler reaching later compilation stages where additional diagnostics may be generated.

@awelc awelc self-assigned this Feb 8, 2024
@awelc awelc requested a review from tnowacki February 8, 2024 02:18
Copy link

vercel bot commented Feb 8, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
explorer ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 14, 2024 10:42pm
multisig-toolkit ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 14, 2024 10:42pm
mysten-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 14, 2024 10:42pm
sui-core ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 14, 2024 10:42pm
sui-kiosk ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 14, 2024 10:42pm
sui-typescript-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 14, 2024 10:42pm

start_loc,
context.tokens.previous_end_loc(),
);
result.body = sp(context.tokens.current_token_loc(), FunctionBody_::Native);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm dubious about marking this native, as it will likely produce very strange results. Inserting abort 0 or something is safer. We could even consider adding a FunctionBody_::Error form.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is exactly the kind of feedback I am looking for! I was wondering about it myself (also because we introduce an essentially fake location here), but did not want to go to crazy with the changes without getting feedback first.

In a similar vein (though perhaps more acceptable) is that an incomplete function actually has a Type_::Unit return type (rather then perhaps something like Type_::Error).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 For something like FunctionBody_::Error
Though you could probably achieve the same thing by doing FunctionBody_::Defined(...UnresolvedError...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 For something like FunctionBody_::Error Though you could probably achieve the same thing by doing FunctionBody_::Defined(...UnresolvedError...)

I went this route (actually just provided empty Sequence as the function body) as this does not require changes to subsequent passes (to handle the new type of UnresolvedError).

start_loc,
context.tokens.previous_end_loc(),
);
result.body = sp(context.tokens.current_token_loc(), FunctionBody_::Native);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 For something like FunctionBody_::Error
Though you could probably achieve the same thing by doing FunctionBody_::Defined(...UnresolvedError...)

@@ -2327,36 +2327,123 @@ fn parse_function_decl(
start_loc: usize,
modifiers: Modifiers,
context: &mut Context,
) -> Result<Function, Box<Diagnostic>> {
) -> Result<(Function, Option<Box<Diagnostic>>), Box<Diagnostic>> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels strange to me. Can we not just log the diagnostic if we have it? Why bother passing it back instead of just logging it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess what I'm suggesting is that the function parsing can locally handle trying to advance the parser to the next valid state, much like we did in the lexer, and hide the error handling complexity from the caller

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was not sure what the best way was to structure this, but you are obviously right - we don't really have to pass the diagnostics on the success case when we can log them on the spot

Copy link
Contributor

@tnowacki tnowacki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will take a deeper look tomorrow, but most of these parser errors look really suspicious, like something has gone wrong with the thing that is supposed to consume tokens until the start of the next module member

warning[W09002]: unused variable
┌─ tests/move_2024/parser/global_access_exists_invalid.move:11:11
11 │ fun t(account: &signer) acquires Self::R {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, could you add an underscore here

Comment on lines 35 to 37
13 │ let _ : bool = ::exists<Self::R>(0x0);
│ ^
│ │
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did we end up expecting a module member here?

Comment on lines 13 to 17
3 │ ::S { }
│ ^
│ │
│ Unexpected '{'
│ Expected a module member: 'spec', 'use', 'friend', 'const', 'fun', or 'struct'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar question as above

Comment on lines 14 to 15
│ --- Expected: 'u64'
3 │ 1 + ::global_value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks a bit suspicious

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because the function definition is parsed correctly as having the u64 return type but the body was not parsed correctly and (being treated as empty), typing throws an error on incompatible return type. I adjusted all tests (including this one) to only report parsing errors.

@awelc
Copy link
Contributor Author

awelc commented Apr 12, 2024

Superseded by #17137 and #16673

@awelc awelc closed this Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants