Auto merge of #18132 - ChayimFriedman2:fix-closure-semi, r=Veykril

fix: Don't complete `;` when in closure return expression Completing it will break syntax. Fixes #18130.
rust-lang · Sep 20, 2024 · 00037c1 · 00037c1
2 parents d4689f1 + c9758da
commit 00037c1
Show file tree

Hide file tree

Showing 2 changed files with 100 additions and 32 deletions.
diff --git a/crates/ide-completion/src/context.rs b/crates/ide-completion/src/context.rs
@@ -4,7 +4,7 @@ mod analysis;
 #[cfg(test)]
 mod tests;
 
-use std::iter;
+use std::{iter, ops::ControlFlow};
 
 use hir::{
     HasAttrs, Local, Name, PathResolution, ScopeDef, Semantics, SemanticsScope, Type, TypeInfo,
@@ -15,7 +15,7 @@ use ide_db::{
 };
 use syntax::{
     ast::{self, AttrKind, NameOrNameRef},
-    AstNode, Edition, SmolStr,
+    match_ast, AstNode, Edition, SmolStr,
     SyntaxKind::{self, *},
     SyntaxToken, TextRange, TextSize, T,
 };
@@ -457,6 +457,16 @@ pub(crate) struct CompletionContext<'a> {
     ///
     /// Here depth will be 2
     pub(crate) depth_from_crate_root: usize,
+
+    /// Whether and how to complete semicolon for unit-returning functions.
+    pub(crate) complete_semicolon: CompleteSemicolon,
+}
+
+#[derive(Debug)]
+pub(crate) enum CompleteSemicolon {
+    DoNotComplete,
+    CompleteSemi,
+    CompleteComma,
 }
 
 impl CompletionContext<'_> {
@@ -735,6 +745,53 @@ impl<'a> CompletionContext<'a> {
 
         let depth_from_crate_root = iter::successors(module.parent(db), |m| m.parent(db)).count();
 
+        let complete_semicolon = if config.add_semicolon_to_unit {
+            let inside_closure_ret = token.parent_ancestors().try_for_each(|ancestor| {
+                match_ast! {
+                    match ancestor {
+                        ast::BlockExpr(_) => ControlFlow::Break(false),
+                        ast::ClosureExpr(_) => ControlFlow::Break(true),
+                        _ => ControlFlow::Continue(())
+                    }
+                }
+            });
+
+            if inside_closure_ret == ControlFlow::Break(true) {
+                CompleteSemicolon::DoNotComplete
+            } else {
+                let next_non_trivia_token =
+                    std::iter::successors(token.next_token(), |it| it.next_token())
+                        .find(|it| !it.kind().is_trivia());
+                let in_match_arm = token.parent_ancestors().try_for_each(|ancestor| {
+                    if ast::MatchArm::can_cast(ancestor.kind()) {
+                        ControlFlow::Break(true)
+                    } else if matches!(
+                        ancestor.kind(),
+                        SyntaxKind::EXPR_STMT | SyntaxKind::BLOCK_EXPR
+                    ) {
+                        ControlFlow::Break(false)
+                    } else {
+                        ControlFlow::Continue(())
+                    }
+                });
+                // FIXME: This will assume expr macros are not inside match, we need to somehow go to the "parent" of the root node.
+                let in_match_arm = match in_match_arm {
+                    ControlFlow::Continue(()) => false,
+                    ControlFlow::Break(it) => it,
+                };
+                let complete_token = if in_match_arm { T![,] } else { T![;] };
+                if next_non_trivia_token.map(|it| it.kind()) == Some(complete_token) {
+                    CompleteSemicolon::DoNotComplete
+                } else if in_match_arm {
+                    CompleteSemicolon::CompleteComma
+                } else {
+                    CompleteSemicolon::CompleteSemi
+                }
+            }
+        } else {
+            CompleteSemicolon::DoNotComplete
+        };
+
         let ctx = CompletionContext {
             sema,
             scope,
@@ -752,6 +809,7 @@ impl<'a> CompletionContext<'a> {
             qualifier_ctx,
             locals,
             depth_from_crate_root,
+            complete_semicolon,
         };
         Some((ctx, analysis))
     }

diff --git a/crates/ide-completion/src/render/function.rs b/crates/ide-completion/src/render/function.rs
@@ -1,15 +1,15 @@
 //! Renderer for function calls.
 
-use std::ops::ControlFlow;
-
 use hir::{db::HirDatabase, AsAssocItem, HirDisplay};
 use ide_db::{SnippetCap, SymbolKind};
 use itertools::Itertools;
 use stdx::{format_to, to_lower_snake_case};
-use syntax::{ast, format_smolstr, AstNode, Edition, SmolStr, SyntaxKind, ToSmolStr, T};
+use syntax::{format_smolstr, AstNode, Edition, SmolStr, ToSmolStr};
 
 use crate::{
-    context::{CompletionContext, DotAccess, DotAccessKind, PathCompletionCtx, PathKind},
+    context::{
+        CompleteSemicolon, CompletionContext, DotAccess, DotAccessKind, PathCompletionCtx, PathKind,
+    },
     item::{
         Builder, CompletionItem, CompletionItemKind, CompletionRelevance, CompletionRelevanceFn,
         CompletionRelevanceReturnType, CompletionRelevanceTraitInfo,
@@ -277,32 +277,21 @@ pub(super) fn add_call_parens<'b>(
 
         (snippet, "(…)")
     };
-    if ret_type.is_unit() && ctx.config.add_semicolon_to_unit {
-        let next_non_trivia_token =
-            std::iter::successors(ctx.token.next_token(), |it| it.next_token())
-                .find(|it| !it.kind().is_trivia());
-        let in_match_arm = ctx.token.parent_ancestors().try_for_each(|ancestor| {
-            if ast::MatchArm::can_cast(ancestor.kind()) {
-                ControlFlow::Break(true)
-            } else if matches!(ancestor.kind(), SyntaxKind::EXPR_STMT | SyntaxKind::BLOCK_EXPR) {
-                ControlFlow::Break(false)
-            } else {
-                ControlFlow::Continue(())
-            }
-        });
-        // FIXME: This will assume expr macros are not inside match, we need to somehow go to the "parent" of the root node.
-        let in_match_arm = match in_match_arm {
-            ControlFlow::Continue(()) => false,
-            ControlFlow::Break(it) => it,
-        };
-        let complete_token = if in_match_arm { T![,] } else { T![;] };
-        if next_non_trivia_token.map(|it| it.kind()) != Some(complete_token) {
-            cov_mark::hit!(complete_semicolon);
-            let ch = if in_match_arm { ',' } else { ';' };
-            if snippet.ends_with("$0") {
-                snippet.insert(snippet.len() - "$0".len(), ch);
-            } else {
-                snippet.push(ch);
+    if ret_type.is_unit() {
+        match ctx.complete_semicolon {
+            CompleteSemicolon::DoNotComplete => {}
+            CompleteSemicolon::CompleteSemi | CompleteSemicolon::CompleteComma => {
+                cov_mark::hit!(complete_semicolon);
+                let ch = if matches!(ctx.complete_semicolon, CompleteSemicolon::CompleteComma) {
+                    ','
+                } else {
+                    ';'
+                };
+                if snippet.ends_with("$0") {
+                    snippet.insert(snippet.len() - "$0".len(), ch);
+                } else {
+                    snippet.push(ch);
+                }
             }
         }
     }
@@ -886,6 +875,27 @@ fn bar() {
         v => foo()$0,
     }
 }
+"#,
+        );
+    }
+
+    #[test]
+    fn no_semicolon_in_closure_ret() {
+        check_edit(
+            r#"foo"#,
+            r#"
+fn foo() {}
+fn baz(_: impl FnOnce()) {}
+fn bar() {
+    baz(|| fo$0);
+}
+"#,
+            r#"
+fn foo() {}
+fn baz(_: impl FnOnce()) {}
+fn bar() {
+    baz(|| foo()$0);
+}
 "#,
         );
     }