From eec6c7244a6bca160be056322ba7ec8295933cf0 Mon Sep 17 00:00:00 2001 From: Adam Welc Date: Fri, 1 Nov 2024 17:29:44 -0700 Subject: [PATCH] [move-ide] Improvements to enum and curly brace auto-completion (#20106) ## Description This PR fixes addresses some problems with auto-completion: 1. Previously, auto-completion for enum types/variants did not work inside of match expression. We needed to add both some parsing resilience to match arm parsing and update the cursor to be able to find match patterns in the AST. Now we have: ![image](https://github.com/user-attachments/assets/76426e84-59e6-4855-9f20-2ea3135e0483) 2. A smaller update addresses a minor problem with auto-completion after `{`. Previously, if no intelligent match was found, we were still returning "default" matches which had an annoying effect of accidentally auto-completing an essentially random string after any `{` (e.g., the on starting a function body block). ## Test plan All old (updated) and new tests must pass. --- .../src/analysis/parsing_analysis.rs | 8 ++- .../move-analyzer/src/completions/mod.rs | 9 +-- .../move/crates/move-analyzer/src/symbols.rs | 20 ++++++ .../tests/colon_colon_completion.exp | 62 +++++++++++++++++++ .../tests/colon_colon_completion.ide | 4 ++ .../tests/completion/sources/colon_colon.move | 5 ++ .../move-compiler/src/naming/translate.rs | 2 +- .../crates/move-compiler/src/parser/syntax.rs | 49 +++++++++------ .../matching/invalid_match_tuple.exp | 23 +++++-- 9 files changed, 149 insertions(+), 33 deletions(-) diff --git a/external-crates/move/crates/move-analyzer/src/analysis/parsing_analysis.rs b/external-crates/move/crates/move-analyzer/src/analysis/parsing_analysis.rs index c8d68aeb97743..aa390bd839f1a 100644 --- a/external-crates/move/crates/move-analyzer/src/analysis/parsing_analysis.rs +++ b/external-crates/move/crates/move-analyzer/src/analysis/parsing_analysis.rs @@ -485,9 +485,13 @@ impl<'a> ParsingAnalysisContext<'a> { } } - fn match_pattern_symbols(&mut self, sp!(_, pattern): &P::MatchPattern) { + fn match_pattern_symbols(&mut self, pattern: &P::MatchPattern) { use P::MatchPattern_ as MP; - match pattern { + // If the cursor is in this match pattern, mark that down. + // This may be overridden by the recursion below. + update_cursor!(self.cursor, pattern, MatchPattern); + + match &pattern.value { MP::PositionalConstructor(chain, sp!(_, v)) => { self.chain_symbols(chain); v.iter().for_each(|e| { diff --git a/external-crates/move/crates/move-analyzer/src/completions/mod.rs b/external-crates/move/crates/move-analyzer/src/completions/mod.rs index 470c8e0c6923b..1e9be2622ec85 100644 --- a/external-crates/move/crates/move-analyzer/src/completions/mod.rs +++ b/external-crates/move/crates/move-analyzer/src/completions/mod.rs @@ -269,12 +269,13 @@ fn cursor_completion_items( completions.extend(custom_completions); completion_finalized |= custom_finalized; if !completion_finalized { - let (use_decl_completions, use_decl_finalized) = - use_decl_completions(symbols, cursor); + let (use_decl_completions, _) = use_decl_completions(symbols, cursor); completions.extend(use_decl_completions); - completion_finalized |= use_decl_finalized; } - (completions, completion_finalized) + // do not offer default completions after `{` as this may get annoying + // when simply starting a body of a function and hitting enter triggers + // auto-completion of an essentially random identifier + (completions, true) } // TODO: should we handle auto-completion on `:`? If we model our support after // rust-analyzer then it does not do this - it starts auto-completing types after the first diff --git a/external-crates/move/crates/move-analyzer/src/symbols.rs b/external-crates/move/crates/move-analyzer/src/symbols.rs index fd9fa72c781dd..f766342ea3577 100644 --- a/external-crates/move/crates/move-analyzer/src/symbols.rs +++ b/external-crates/move/crates/move-analyzer/src/symbols.rs @@ -531,6 +531,20 @@ impl CursorContext { } } + /// Returns access chain for a match pattern, if any + fn find_access_chain_in_match_pattern(&self, p: &P::MatchPattern_) -> Option { + use ChainCompletionKind as CT; + use P::MatchPattern_ as MP; + match p { + MP::PositionalConstructor(chain, _) => { + Some(ChainInfo::new(chain.clone(), CT::Type, false)) + } + MP::FieldConstructor(chain, _) => Some(ChainInfo::new(chain.clone(), CT::Type, false)), + MP::Name(_, chain) => Some(ChainInfo::new(chain.clone(), CT::All, false)), + MP::Literal(_) | MP::Or(..) | MP::At(..) => None, + } + } + /// Returns access chain at cursor position (if any) along with the information of what the chain's /// auto-completed target kind should be, and weather it is part of the use statement. pub fn find_access_chain(&self) -> Option { @@ -575,6 +589,7 @@ impl CursorContext { return Some(ChainInfo::new(*(ty.clone()), CT::Type, true)); } } + CP::MatchPattern(sp!(_, p)) => return self.find_access_chain_in_match_pattern(p), _ => (), }; None @@ -600,6 +615,7 @@ pub enum CursorPosition { DefName, Attribute(P::AttributeValue), Use(Spanned), + MatchPattern(P::MatchPattern), Unknown, // FIXME: These two are currently unused because these forms don't have enough location // recorded on them during parsing. @@ -964,6 +980,10 @@ impl fmt::Display for CursorContext { writeln!(f, "parameter")?; writeln!(f, "- value: {:#?}", value)?; } + CursorPosition::MatchPattern(value) => { + writeln!(f, "match pattern")?; + writeln!(f, "- value: {:#?}", value)?; + } CursorPosition::DatatypeTypeParameter(value) => { writeln!(f, "datatype type param")?; writeln!(f, "- value: {:#?}", value)?; diff --git a/external-crates/move/crates/move-analyzer/tests/colon_colon_completion.exp b/external-crates/move/crates/move-analyzer/tests/colon_colon_completion.exp index 84f7f87073f55..3ef070bbec6b8 100644 --- a/external-crates/move/crates/move-analyzer/tests/colon_colon_completion.exp +++ b/external-crates/move/crates/move-analyzer/tests/colon_colon_completion.exp @@ -48,6 +48,10 @@ Method 'complete_chains()' INSERT TEXT: 'complete_chains(${1:s})' TARGET : '(Completion::colon_colon::complete_chains)' TYPE : 'fun (SomeStruct)' +Method 'match_pattern()' + INSERT TEXT: 'match_pattern(${1:e})' + TARGET : '(Completion::colon_colon::match_pattern)' + TYPE : 'fun (SomeEnum)' Method 'multi_colon_colon()' INSERT TEXT: 'multi_colon_colon()' TARGET : '(Completion::colon_colon::multi_colon_colon)' @@ -106,6 +110,10 @@ Method 'complete_chains()' INSERT TEXT: 'complete_chains(${1:s})' TARGET : '(Completion::colon_colon::complete_chains)' TYPE : 'fun (SomeStruct)' +Method 'match_pattern()' + INSERT TEXT: 'match_pattern(${1:e})' + TARGET : '(Completion::colon_colon::match_pattern)' + TYPE : 'fun (SomeEnum)' Method 'multi_colon_colon()' INSERT TEXT: 'multi_colon_colon()' TARGET : '(Completion::colon_colon::multi_colon_colon)' @@ -240,6 +248,10 @@ Method 'complete_chains()' INSERT TEXT: 'complete_chains(${1:s})' TARGET : '(Completion::colon_colon::complete_chains)' TYPE : 'fun (SomeStruct)' +Method 'match_pattern()' + INSERT TEXT: 'match_pattern(${1:e})' + TARGET : '(Completion::colon_colon::match_pattern)' + TYPE : 'fun (SomeEnum)' Method 'multi_colon_colon()' INSERT TEXT: 'multi_colon_colon()' TARGET : '(Completion::colon_colon::multi_colon_colon)' @@ -441,6 +453,10 @@ Method 'complete_chains()' INSERT TEXT: 'complete_chains(${1:s})' TARGET : '(Completion::colon_colon::complete_chains)' TYPE : 'fun (SomeStruct)' +Method 'match_pattern()' + INSERT TEXT: 'match_pattern(${1:e})' + TARGET : '(Completion::colon_colon::match_pattern)' + TYPE : 'fun (SomeEnum)' Method 'multi_colon_colon()' INSERT TEXT: 'multi_colon_colon()' TARGET : '(Completion::colon_colon::multi_colon_colon)' @@ -498,6 +514,10 @@ Method 'complete_chains()' INSERT TEXT: 'complete_chains(${1:s})' TARGET : '(Completion::colon_colon::complete_chains)' TYPE : 'fun (SomeStruct)' +Method 'match_pattern()' + INSERT TEXT: 'match_pattern(${1:e})' + TARGET : '(Completion::colon_colon::match_pattern)' + TYPE : 'fun (SomeEnum)' Method 'multi_colon_colon()' INSERT TEXT: 'multi_colon_colon()' TARGET : '(Completion::colon_colon::multi_colon_colon)' @@ -569,6 +589,10 @@ Method 'complete_chains()' INSERT TEXT: 'complete_chains(${1:s})' TARGET : '(Completion::colon_colon::complete_chains)' TYPE : 'fun (SomeStruct)' +Method 'match_pattern()' + INSERT TEXT: 'match_pattern(${1:e})' + TARGET : '(Completion::colon_colon::match_pattern)' + TYPE : 'fun (SomeEnum)' Method 'multi_colon_colon()' INSERT TEXT: 'multi_colon_colon()' TARGET : '(Completion::colon_colon::multi_colon_colon)' @@ -705,6 +729,10 @@ Method 'complete_chains()' INSERT TEXT: 'complete_chains(${1:s})' TARGET : '(Completion::colon_colon::complete_chains)' TYPE : 'fun (SomeStruct)' +Method 'match_pattern()' + INSERT TEXT: 'match_pattern(${1:e})' + TARGET : '(Completion::colon_colon::match_pattern)' + TYPE : 'fun (SomeEnum)' Method 'multi_colon_colon()' INSERT TEXT: 'multi_colon_colon()' TARGET : '(Completion::colon_colon::multi_colon_colon)' @@ -829,6 +857,10 @@ Method 'complete_chains()' INSERT TEXT: 'complete_chains(${1:s})' TARGET : '(Completion::colon_colon::complete_chains)' TYPE : 'fun (SomeStruct)' +Method 'match_pattern()' + INSERT TEXT: 'match_pattern(${1:e})' + TARGET : '(Completion::colon_colon::match_pattern)' + TYPE : 'fun (SomeEnum)' Method 'multi_colon_colon()' INSERT TEXT: 'multi_colon_colon()' TARGET : '(Completion::colon_colon::multi_colon_colon)' @@ -876,6 +908,10 @@ Method 'complete_chains()' INSERT TEXT: 'complete_chains(${1:s})' TARGET : '(Completion::colon_colon::complete_chains)' TYPE : 'fun (SomeStruct)' +Method 'match_pattern()' + INSERT TEXT: 'match_pattern(${1:e})' + TARGET : '(Completion::colon_colon::match_pattern)' + TYPE : 'fun (SomeEnum)' Method 'multi_colon_colon()' INSERT TEXT: 'multi_colon_colon()' TARGET : '(Completion::colon_colon::multi_colon_colon)' @@ -947,6 +983,10 @@ Method 'complete_chains()' INSERT TEXT: 'complete_chains(${1:s})' TARGET : '(Completion::colon_colon::complete_chains)' TYPE : 'fun (SomeStruct)' +Method 'match_pattern()' + INSERT TEXT: 'match_pattern(${1:e})' + TARGET : '(Completion::colon_colon::match_pattern)' + TYPE : 'fun (SomeEnum)' Method 'multi_colon_colon()' INSERT TEXT: 'multi_colon_colon()' TARGET : '(Completion::colon_colon::multi_colon_colon)' @@ -1216,6 +1256,10 @@ Method 'complete_chains()' INSERT TEXT: 'complete_chains(${1:s})' TARGET : '(Completion::colon_colon::complete_chains)' TYPE : 'fun (SomeStruct)' +Method 'match_pattern()' + INSERT TEXT: 'match_pattern(${1:e})' + TARGET : '(Completion::colon_colon::match_pattern)' + TYPE : 'fun (SomeEnum)' Method 'multi_colon_colon()' INSERT TEXT: 'multi_colon_colon()' TARGET : '(Completion::colon_colon::multi_colon_colon)' @@ -1253,6 +1297,16 @@ Method 'targ_type()' TARGET : '(Completion::colon_colon::targ_type)' TYPE : 'fun (SOME_TYPE)' +-- test 37 ------------------- +use line: 87, use_col: 22 +EnumMember 'SomeNamedVariant' +EnumMember 'SomeNamedVariant{..}' + INSERT TEXT: 'SomeNamedVariant { ${1:name1}, ${2:name2} }' +EnumMember 'SomePositionalVariant' +EnumMember 'SomePositionalVariant(..)' + INSERT TEXT: 'SomePositionalVariant(${1}, ${2})' +EnumMember 'SomeVariant' + == uses.move ======================================================== -- test 0 ------------------- use line: 2, use_col: 9 @@ -1404,6 +1458,10 @@ Method 'complete_chains()' INSERT TEXT: 'complete_chains' TARGET : '(Completion::colon_colon::complete_chains)' TYPE : 'fun (SomeStruct)' +Method 'match_pattern()' + INSERT TEXT: 'match_pattern' + TARGET : '(Completion::colon_colon::match_pattern)' + TYPE : 'fun (SomeEnum)' Method 'multi_colon_colon()' INSERT TEXT: 'multi_colon_colon' TARGET : '(Completion::colon_colon::multi_colon_colon)' @@ -1457,6 +1515,10 @@ Method 'complete_chains()' INSERT TEXT: 'complete_chains' TARGET : '(Completion::colon_colon::complete_chains)' TYPE : 'fun (SomeStruct)' +Method 'match_pattern()' + INSERT TEXT: 'match_pattern' + TARGET : '(Completion::colon_colon::match_pattern)' + TYPE : 'fun (SomeEnum)' Method 'multi_colon_colon()' INSERT TEXT: 'multi_colon_colon' TARGET : '(Completion::colon_colon::multi_colon_colon)' diff --git a/external-crates/move/crates/move-analyzer/tests/colon_colon_completion.ide b/external-crates/move/crates/move-analyzer/tests/colon_colon_completion.ide index 75d9c20512a38..1ba89f12abc2f 100644 --- a/external-crates/move/crates/move-analyzer/tests/colon_colon_completion.ide +++ b/external-crates/move/crates/move-analyzer/tests/colon_colon_completion.ide @@ -153,6 +153,10 @@ { "use_line": 82, "use_col": 18 + }, + { + "use_line": 87, + "use_col": 22 } ], "uses.move": [ diff --git a/external-crates/move/crates/move-analyzer/tests/completion/sources/colon_colon.move b/external-crates/move/crates/move-analyzer/tests/completion/sources/colon_colon.move index b5eb6f85dfa08..a2a4cbe324bc5 100644 --- a/external-crates/move/crates/move-analyzer/tests/completion/sources/colon_colon.move +++ b/external-crates/move/crates/move-analyzer/tests/completion/sources/colon_colon.move @@ -82,4 +82,9 @@ module Completion::colon_colon { MultiField } + public fun match_pattern(e: SomeEnum) { + match (e) { + SomeEnum:: + } + } } diff --git a/external-crates/move/crates/move-compiler/src/naming/translate.rs b/external-crates/move/crates/move-compiler/src/naming/translate.rs index 048250cef3546..905132032466f 100644 --- a/external-crates/move/crates/move-compiler/src/naming/translate.rs +++ b/external-crates/move/crates/move-compiler/src/naming/translate.rs @@ -1172,7 +1172,7 @@ impl<'outer, 'env> Context<'outer, 'env> { } _ => match self.resolve_datatype_constructor(sp(mloc, ma_), "pattern") { Some(ctor) => ResolvedPatternTerm::Constructor(Box::new(ctor)), - None => todo!(), + None => ResolvedPatternTerm::Unbound, // TODO: some cases here may be handled }, } } diff --git a/external-crates/move/crates/move-compiler/src/parser/syntax.rs b/external-crates/move/crates/move-compiler/src/parser/syntax.rs index acc26a2e77651..ca99059d14a26 100644 --- a/external-crates/move/crates/move-compiler/src/parser/syntax.rs +++ b/external-crates/move/crates/move-compiler/src/parser/syntax.rs @@ -2068,27 +2068,36 @@ fn parse_match_arm(context: &mut Context) -> Result> { } _ => None, }; - consume_token(context.tokens, Tok::EqualGreater)?; - let rhs = match context.tokens.peek() { - Tok::LBrace => { - let block_start_loc = context.tokens.start_loc(); - context.tokens.advance()?; // consume the LBrace - let block_ = Exp_::Block(parse_sequence(context)?); - let block_end_loc = context.tokens.previous_end_loc(); - let exp = spanned( - context.tokens.file_hash(), - block_start_loc, - block_end_loc, - block_, - ); - Box::new(exp) + if let Err(diag) = consume_token(context.tokens, Tok::EqualGreater) { + // report incomplete pattern so that auto-completion can work + context.add_diag(*diag); + MatchArm_ { + pattern, + guard, + rhs: Box::new(sp(Loc::invalid(), Exp_::UnresolvedError)), + } + } else { + let rhs = match context.tokens.peek() { + Tok::LBrace => { + let block_start_loc = context.tokens.start_loc(); + context.tokens.advance()?; // consume the LBrace + let block_ = Exp_::Block(parse_sequence(context)?); + let block_end_loc = context.tokens.previous_end_loc(); + let exp = spanned( + context.tokens.file_hash(), + block_start_loc, + block_end_loc, + block_, + ); + Box::new(exp) + } + _ => Box::new(parse_exp(context)?), + }; + MatchArm_ { + pattern, + guard, + rhs, } - _ => Box::new(parse_exp(context)?), - }; - MatchArm_ { - pattern, - guard, - rhs, } }) } diff --git a/external-crates/move/crates/move-compiler/tests/move_2024/matching/invalid_match_tuple.exp b/external-crates/move/crates/move-compiler/tests/move_2024/matching/invalid_match_tuple.exp index 76b4f0f35ae57..3ae3051767d79 100644 --- a/external-crates/move/crates/move-compiler/tests/move_2024/matching/invalid_match_tuple.exp +++ b/external-crates/move/crates/move-compiler/tests/move_2024/matching/invalid_match_tuple.exp @@ -7,12 +7,6 @@ error[E04005]: expected a single type 6 │ match (x()) { │ ^^^ Invalid 'match' subject -error[E04036]: non-exhaustive pattern - ┌─ tests/move_2024/matching/invalid_match_tuple.move:6:16 - │ -6 │ match (x()) { - │ ^^^ Pattern '_' not covered - error[E01002]: unexpected token ┌─ tests/move_2024/matching/invalid_match_tuple.move:7:15 │ @@ -22,6 +16,23 @@ error[E01002]: unexpected token │ Unexpected ',' │ Expected ')' +warning[W09002]: unused variable + ┌─ tests/move_2024/matching/invalid_match_tuple.move:7:17 + │ +7 │ (x, y) => () + │ ^ Unused local variable 'y'. Consider removing or prefixing with an underscore: '_y' + │ + = This warning can be suppressed with '#[allow(unused_variable)]' applied to the 'module' or module member ('const', 'fun', or 'struct') + +error[E04005]: expected a single type + ┌─ tests/move_2024/matching/invalid_match_tuple.move:7:17 + │ +3 │ fun x(): (u64, u64) { (0, 42) } + │ ---------- Expected a single type, but found expression list type: '(u64, u64)' + · +7 │ (x, y) => () + │ ^ Invalid type for pattern + error[E01002]: unexpected token ┌─ tests/move_2024/matching/invalid_match_tuple.move:7:18 │