[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.
MystenLabs · Nov 2, 2024 · eec6c72 · eec6c72
1 parent a61b157
commit eec6c72
Show file tree

Hide file tree

Showing 9 changed files with 149 additions and 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
@@ -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
@@ -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
@@ -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<ChainInfo> {
+        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<ChainInfo> {
@@ -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<P::Use>),
+    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
@@ -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>(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
@@ -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
@@ -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
@@ -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
@@ -2068,27 +2068,36 @@ fn parse_match_arm(context: &mut Context) -> Result<MatchArm, Box<Diagnostic>> {
             }
             _ => 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
@@ -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
   │