Repository: rust-lang/rust-analyzer (rust)

Issue:
Extract into function assist fails to convert continue or break to ControlFlow return
I tried:

```rust
pub fn hello() {
    for _ in 0..1 {
        // Extract the following condition without this comment
        if true {
            continue;
        }

        if false {
            break;
        }
    }
}
```

and got

```rust
pub fn hello() {
    for _ in 0..1 {
        // Extract the following condition without this comment
        if let ControlFlow::Break(_) = fun_name() {
            continue;
        }

        if false {
            break;
        }
    }
}

fn fun_name() {
    if true {
        continue;
    }
}
```

when it should have been

```rust
use std::ops::ControlFlow;

pub fn hello() {
    for _ in 0..1 {
        // Extract the following condition without this comment
        if let ControlFlow::Break(_) = fun_name() {
            continue;
        }

        if false {
            break;
        }
    }
}

fn fun_name() -> ControlFlow<()> {
    if true {
        return ControlFlow::Break(());
    }
    ControlFlow::Continue(())
}
```

Same thing happens when using extract-function on the `if false` expression.

**rust-analyzer version**: `rust-analyzer version: 0.4.1286-standalone`

**rustc version**: `rustc 1.65.0 (897e37553 2022-11-02)`

**relevant settings**: -


Pull request:
fix: check tail expressions more precisely in `extract_function`
Fixes #13620

When extracting expressions with control flows into a function, we can avoid wrapping tail expressions in `Option` or `Result` when they are also tail expressions of the container we're extracting from (see #7840, #9773). This is controlled by `ContainerInfo::is_in_tail`, but we've been computing it by checking if the tail expression of the range to extract is contained in the container's syntactically last expression, which may be a block that contains both tail and non-tail expressions (e.g. in #13620, the range to be extracted is not a tail expression but we set the flag to true).

This PR tries to compute the flag as precise as possible by utilizing `for_each_tail_expr()` (and also moves the flag to `Function` struct as it's more of a property of the function to be extracted than of the container).

Patch:
diff --git a/crates/ide-assists/src/handlers/extract_function.rs b/crates/ide-assists/src/handlers/extract_function.rs
index c1e2f19ab1..0483cfdc64 100644
--- a/crates/ide-assists/src/handlers/extract_function.rs
+++ b/crates/ide-assists/src/handlers/extract_function.rs
@@ -11,7 +11,9 @@ use ide_db::{
     helpers::mod_path_to_ast,
     imports::insert_use::{insert_use, ImportScope},
     search::{FileReference, ReferenceCategory, SearchScope},
-    syntax_helpers::node_ext::{preorder_expr, walk_expr, walk_pat, walk_patterns_in_expr},
+    syntax_helpers::node_ext::{
+        for_each_tail_expr, preorder_expr, walk_expr, walk_pat, walk_patterns_in_expr,
+    },
     FxIndexSet, RootDatabase,
 };
 use itertools::Itertools;
@@ -78,7 +80,7 @@ pub(crate) fn extract_function(acc: &mut Assists, ctx: &AssistContext<'_>) -> Op
     };
 
     let body = extraction_target(&node, range)?;
-    let container_info = body.analyze_container(&ctx.sema)?;
+    let (container_info, contains_tail_expr) = body.analyze_container(&ctx.sema)?;
 
     let (locals_used, self_param) = body.analyze(&ctx.sema);
 
@@ -119,6 +121,7 @@ pub(crate) fn extract_function(acc: &mut Assists, ctx: &AssistContext<'_>) -> Op
                 ret_ty,
                 body,
                 outliving_locals,
+                contains_tail_expr,
                 mods: container_info,
             };
 
@@ -245,6 +248,8 @@ struct Function {
     ret_ty: RetType,
     body: FunctionBody,
     outliving_locals: Vec<OutlivedLocal>,
+    /// Whether at least one of the container's tail expr is contained in the range we're extracting.
+    contains_tail_expr: bool,
     mods: ContainerInfo,
 }
 
@@ -265,7 +270,7 @@ enum ParamKind {
     MutRef,
 }
 
-#[derive(Debug, Eq, PartialEq)]
+#[derive(Debug)]
 enum FunType {
     Unit,
     Single(hir::Type),
@@ -294,7 +299,6 @@ struct ControlFlow {
 #[derive(Clone, Debug)]
 struct ContainerInfo {
     is_const: bool,
-    is_in_tail: bool,
     parent_loop: Option<SyntaxNode>,
     /// The function's return type, const's type etc.
     ret_type: Option<hir::Type>,
@@ -743,7 +747,10 @@ impl FunctionBody {
         (res, self_param)
     }
 
-    fn analyze_container(&self, sema: &Semantics<'_, RootDatabase>) -> Option<ContainerInfo> {
+    fn analyze_container(
+        &self,
+        sema: &Semantics<'_, RootDatabase>,
+    ) -> Option<(ContainerInfo, bool)> {
         let mut ancestors = self.parent()?.ancestors();
         let infer_expr_opt = |expr| sema.type_of_expr(&expr?).map(TypeInfo::adjusted);
         let mut parent_loop = None;
@@ -815,28 +822,36 @@ impl FunctionBody {
                 }
             };
         };
-        let container_tail = match expr? {
-            ast::Expr::BlockExpr(block) => block.tail_expr(),
-            expr => Some(expr),
-        };
-        let is_in_tail =
-            container_tail.zip(self.tail_expr()).map_or(false, |(container_tail, body_tail)| {
-                container_tail.syntax().text_range().contains_range(body_tail.syntax().text_range())
+
+        let expr = expr?;
+        let contains_tail_expr = if let Some(body_tail) = self.tail_expr() {
+            let mut contains_tail_expr = false;
+            let tail_expr_range = body_tail.syntax().text_range();
+            for_each_tail_expr(&expr, &mut |e| {
+                if tail_expr_range.contains_range(e.syntax().text_range()) {
+                    contains_tail_expr = true;
+                }
             });
+            contains_tail_expr
+        } else {
+            false
+        };
 
         let parent = self.parent()?;
         let parents = generic_parents(&parent);
         let generic_param_lists = parents.iter().filter_map(|it| it.generic_param_list()).collect();
         let where_clauses = parents.iter().filter_map(|it| it.where_clause()).collect();
 
-        Some(ContainerInfo {
-            is_in_tail,
-            is_const,
-            parent_loop,
-            ret_type: ty,
-            generic_param_lists,
-            where_clauses,
-        })
+        Some((
+            ContainerInfo {
+                is_const,
+                parent_loop,
+                ret_type: ty,
+                generic_param_lists,
+                where_clauses,
+            },
+            contains_tail_expr,
+        ))
     }
 
     fn return_ty(&self, ctx: &AssistContext<'_>) -> Option<RetType> {
@@ -1368,7 +1383,7 @@ impl FlowHandler {
             None => FlowHandler::None,
             Some(flow_kind) => {
                 let action = flow_kind.clone();
-                if *ret_ty == FunType::Unit {
+                if let FunType::Unit = ret_ty {
                     match flow_kind {
                         FlowKind::Return(None)
                         | FlowKind::Break(_, None)
@@ -1633,7 +1648,7 @@ impl Function {
 
     fn make_ret_ty(&self, ctx: &AssistContext<'_>, module: hir::Module) -> Option<ast::RetType> {
         let fun_ty = self.return_type(ctx);
-        let handler = if self.mods.is_in_tail {
+        let handler = if self.contains_tail_expr {
             FlowHandler::None
         } else {
             FlowHandler::from_ret_ty(self, &fun_ty)
@@ -1707,7 +1722,7 @@ fn make_body(
     fun: &Function,
 ) -> ast::BlockExpr {
     let ret_ty = fun.return_type(ctx);
-    let handler = if fun.mods.is_in_tail {
+    let handler = if fun.contains_tail_expr {
         FlowHandler::None
     } else {
         FlowHandler::from_ret_ty(fun, &ret_ty)
@@ -1946,7 +1961,7 @@ fn update_external_control_flow(handler: &FlowHandler, syntax: &SyntaxNode) {
                 if nested_scope.is_none() {
                     if let Some(expr) = ast::Expr::cast(e.clone()) {
                         match expr {
-                            ast::Expr::ReturnExpr(return_expr) if nested_scope.is_none() => {
+                            ast::Expr::ReturnExpr(return_expr) => {
                                 let expr = return_expr.expr();
                                 if let Some(replacement) = make_rewritten_flow(handler, expr) {
                                     ted::replace(return_expr.syntax(), replacement.syntax())
@@ -5582,6 +5597,153 @@ impl <T, U> Struct<T, U> where T: Into<i32> + Copy, U: Debug {
 fn $0fun_name<T, V>(t: T, v: V) -> i32 where T: Into<i32> + Copy, V: Into<i32> {
     t.into() + v.into()
 }
+"#,
+        );
+    }
+
+    #[test]
+    fn non_tail_expr_of_tail_expr_loop() {
+        check_assist(
+            extract_function,
+            r#"
+pub fn f() {
+    loop {
+        $0if true {
+            continue;
+        }$0
+
+        if false {
+            break;
+        }
+    }
+}
+"#,
+            r#"
+pub fn f() {
+    loop {
+        if let ControlFlow::Break(_) = fun_name() {
+            continue;
+        }
+
+        if false {
+            break;
+        }
+    }
+}
+
+fn $0fun_name() -> ControlFlow<()> {
+    if true {
+        return ControlFlow::Break(());
+    }
+    ControlFlow::Continue(())
+}
+"#,
+        );
+    }
+
+    #[test]
+    fn non_tail_expr_of_tail_if_block() {
+        // FIXME: double semicolon
+        check_assist(
+            extract_function,
+            r#"
+//- minicore: option, try
+impl<T> core::ops::Try for Option<T> {
+    type Output = T;
+    type Residual = Option<!>;
+}
+impl<T> core::ops::FromResidual for Option<T> {}
+
+fn f() -> Option<()> {
+    if true {
+        let a = $0if true {
+            Some(())?
+        } else {
+            ()
+        }$0;
+        Some(a)
+    } else {
+        None
+    }
+}
+"#,
+            r#"
+impl<T> core::ops::Try for Option<T> {
+    type Output = T;
+    type Residual = Option<!>;
+}
+impl<T> core::ops::FromResidual for Option<T> {}
+
+fn f() -> Option<()> {
+    if true {
+        let a = fun_name()?;;
+        Some(a)
+    } else {
+        None
+    }
+}
+
+fn $0fun_name() -> Option<()> {
+    Some(if true {
+        Some(())?
+    } else {
+        ()
+    })
+}
+"#,
+        );
+    }
+
+    #[test]
+    fn tail_expr_of_tail_block_nested() {
+        check_assist(
+            extract_function,
+            r#"
+//- minicore: option, try
+impl<T> core::ops::Try for Option<T> {
+    type Output = T;
+    type Residual = Option<!>;
+}
+impl<T> core::ops::FromResidual for Option<T> {}
+
+fn f() -> Option<()> {
+    if true {
+        $0{
+            let a = if true {
+                Some(())?
+            } else {
+                ()
+            };
+            Some(a)
+        }$0
+    } else {
+        None
+    }
+}
+"#,
+            r#"
+impl<T> core::ops::Try for Option<T> {
+    type Output = T;
+    type Residual = Option<!>;
+}
+impl<T> core::ops::FromResidual for Option<T> {}
+
+fn f() -> Option<()> {
+    if true {
+        fun_name()?
+    } else {
+        None
+    }
+}
+
+fn $0fun_name() -> Option<()> {
+    let a = if true {
+        Some(())?
+    } else {
+        ()
+    };
+    Some(a)
+}
 "#,
         );
     }
diff --git a/crates/ide-completion/src/context.rs b/crates/ide-completion/src/context.rs
index aa77f44953..9850813a0c 100644
--- a/crates/ide-completion/src/context.rs
+++ b/crates/ide-completion/src/context.rs
@@ -19,7 +19,7 @@ use syntax::{
     ast::{self, AttrKind, NameOrNameRef},
     AstNode,
     SyntaxKind::{self, *},
-    SyntaxToken, TextRange, TextSize, T,
+    SyntaxToken, TextRange, TextSize,
 };
 use text_edit::Indel;
 
@@ -569,32 +569,6 @@ impl<'a> CompletionContext<'a> {
         // completing on
         let original_token = original_file.syntax().token_at_offset(offset).left_biased()?;
 
-        // try to skip completions on path with invalid colons
-        // this approach works in normal path and inside token tree
-        match original_token.kind() {
-            T![:] => {
-                // return if no prev token before colon
-                let prev_token = original_token.prev_token()?;
-
-                // only has a single colon
-                if prev_token.kind() != T![:] {
-                    return None;
-                }
-
-                // has 3 colon or 2 coloncolon in a row
-                // special casing this as per discussion in https://github.com/rust-lang/rust-analyzer/pull/13611#discussion_r1031845205
-                // and https://github.com/rust-lang/rust-analyzer/pull/13611#discussion_r1032812751
-                if prev_token
-                    .prev_token()
-                    .map(|t| t.kind() == T![:] || t.kind() == T![::])
-                    .unwrap_or(false)
-                {
-                    return None;
-                }
-            }
-            _ => {}
-        }
-
         let AnalysisResult {
             analysis,
             expected: (expected_type, expected_name),
diff --git a/crates/ide-completion/src/lib.rs b/crates/ide-completion/src/lib.rs
index 4b48ec6bc3..9d0044e55f 100644
--- a/crates/ide-completion/src/lib.rs
+++ b/crates/ide-completion/src/lib.rs
@@ -164,6 +164,7 @@ pub fn completions(
                 completions::vis::complete_vis_path(&mut completions, ctx, path_ctx, has_in_token);
             }
         }
+        // prevent `(` from triggering unwanted completion noise
         return Some(completions.into());
     }
 
diff --git a/crates/rust-analyzer/src/handlers.rs b/crates/rust-analyzer/src/handlers.rs
index 4f318f39de..d190a9f4e2 100644
--- a/crates/rust-analyzer/src/handlers.rs
+++ b/crates/rust-analyzer/src/handlers.rs
@@ -28,7 +28,7 @@ use lsp_types::{
 use project_model::{ManifestPath, ProjectWorkspace, TargetKind};
 use serde_json::json;
 use stdx::{format_to, never};
-use syntax::{algo, ast, AstNode, TextRange, TextSize};
+use syntax::{algo, ast, AstNode, TextRange, TextSize, T};
 use vfs::AbsPathBuf;
 
 use crate::{
@@ -812,6 +812,18 @@ pub(crate) fn handle_completion(
     let completion_trigger_character =
         params.context.and_then(|ctx| ctx.trigger_character).and_then(|s| s.chars().next());
 
+    if Some(':') == completion_trigger_character {
+        let source_file = snap.analysis.parse(position.file_id)?;
+        let left_token = source_file.syntax().token_at_offset(position.offset).left_biased();
+        let completion_triggered_after_single_colon = match left_token {
+            Some(left_token) => left_token.kind() == T![:],
+            None => true,
+        };
+        if completion_triggered_after_single_colon {
+            return Ok(None);
+        }
+    }
+
     let completion_config = &snap.config.completion();
     let items = match snap.analysis.completions(
         completion_config,


Test patch:
diff --git a/crates/ide-completion/src/tests/attribute.rs b/crates/ide-completion/src/tests/attribute.rs
index 4e60820dd6..1578ba2c37 100644
--- a/crates/ide-completion/src/tests/attribute.rs
+++ b/crates/ide-completion/src/tests/attribute.rs
@@ -607,30 +607,6 @@ fn attr_in_source_file_end() {
     );
 }
 
-#[test]
-fn invalid_path() {
-    check(
-        r#"
-//- proc_macros: identity
-#[proc_macros:::$0]
-struct Foo;
-"#,
-        expect![[r#""#]],
-    );
-
-    check(
-        r#"
-//- minicore: derive, copy
-mod foo {
-    pub use Copy as Bar;
-}
-#[derive(foo:::::$0)]
-struct Foo;
-"#,
-        expect![""],
-    );
-}
-
 mod cfg {
     use super::*;
 
diff --git a/crates/ide-completion/src/tests/special.rs b/crates/ide-completion/src/tests/special.rs
index cad4af4937..033dc99c26 100644
--- a/crates/ide-completion/src/tests/special.rs
+++ b/crates/ide-completion/src/tests/special.rs
@@ -2,22 +2,13 @@
 
 use expect_test::{expect, Expect};
 
-use crate::tests::{check_edit, completion_list_no_kw, completion_list_with_trigger_character};
+use crate::tests::{check_edit, completion_list_no_kw};
 
 fn check(ra_fixture: &str, expect: Expect) {
     let actual = completion_list_no_kw(ra_fixture);
     expect.assert_eq(&actual)
 }
 
-pub(crate) fn check_with_trigger_character(
-    ra_fixture: &str,
-    trigger_character: Option<char>,
-    expect: Expect,
-) {
-    let actual = completion_list_with_trigger_character(ra_fixture, trigger_character);
-    expect.assert_eq(&actual)
-}
-
 #[test]
 fn completes_if_prefix_is_keyword() {
     check_edit(
@@ -902,82 +893,3 @@ fn f() {
         "#]],
     );
 }
-
-#[test]
-fn completes_after_colon_with_trigger() {
-    check_with_trigger_character(
-        r#"
-//- minicore: option
-fn foo { ::$0 }
-"#,
-        Some(':'),
-        expect![[r#"
-            md core
-        "#]],
-    );
-    check_with_trigger_character(
-        r#"
-//- minicore: option
-fn foo { /* test */::$0 }
-"#,
-        Some(':'),
-        expect![[r#"
-            md core
-        "#]],
-    );
-
-    check_with_trigger_character(
-        r#"
-fn foo { crate::$0 }
-"#,
-        Some(':'),
-        expect![[r#"
-            fn foo() fn()
-        "#]],
-    );
-
-    check_with_trigger_character(
-        r#"
-fn foo { crate:$0 }
-"#,
-        Some(':'),
-        expect![""],
-    );
-}
-
-#[test]
-fn completes_after_colon_without_trigger() {
-    check_with_trigger_character(
-        r#"
-fn foo { crate::$0 }
-"#,
-        None,
-        expect![[r#"
-            fn foo() fn()
-        "#]],
-    );
-
-    check_with_trigger_character(
-        r#"
-fn foo { crate:$0 }
-"#,
-        None,
-        expect![""],
-    );
-}
-
-#[test]
-fn no_completions_in_invalid_path() {
-    check(
-        r#"
-fn foo { crate:::$0 }
-"#,
-        expect![""],
-    );
-    check(
-        r#"
-fn foo { crate::::$0 }
-"#,
-        expect![""],
-    )
-}