fix: False positive unnecessary else block diagnostic

crates/hir-ty/src/diagnostics/expr.rs

@@ -12,6 +12,7 @@ use hir_expand::name;
 use itertools::Itertools;
 use rustc_hash::FxHashSet;
 use rustc_pattern_analysis::usefulness::{compute_match_usefulness, ValidityConstraint};
+use syntax::{ast, AstNode};
 use triomphe::Arc;
 use typed_arena::Arena;
 
@@ -104,7 +105,7 @@ impl ExprValidator {
                     self.check_for_trailing_return(*body_expr, &body);
                 }
                 Expr::If { .. } => {
-                    self.check_for_unnecessary_else(id, expr, &body);
+                    self.check_for_unnecessary_else(id, expr, db);
                 }
                 _ => {}
             }
@@ -288,11 +289,35 @@ impl ExprValidator {
         }
     }
 
-    fn check_for_unnecessary_else(&mut self, id: ExprId, expr: &Expr, body: &Body) {
+    fn check_for_unnecessary_else(&mut self, id: ExprId, expr: &Expr, db: &dyn HirDatabase) {
         if let Expr::If { condition: _, then_branch, else_branch } = expr {
             if else_branch.is_none() {
                 return;
             }
+            let (body, source_map) = db.body_with_source_map(self.owner);
+            let Ok(source_ptr) = source_map.expr_syntax(id) else {
+                return;
+            };
+            let root = source_ptr.file_syntax(db.upcast());
+            let ast::Expr::IfExpr(if_expr) = source_ptr.value.to_node(&root) else {
+                return;
+            };
+            let mut top_if_expr = if_expr;
+            loop {
+                let parent = top_if_expr.syntax().parent();
+                let has_parent_let_stmt =
+                    parent.as_ref().map_or(false, |node| ast::LetStmt::can_cast(node.kind()));
+                if has_parent_let_stmt {
+                    // Bail if parent or direct ancestor is a let stmt.
+                    return;
+                }
+                let Some(parent_if_expr) = parent.and_then(ast::IfExpr::cast) else {
+                    // Parent is neither an if expr nor a let stmt.
+                    break;
+                };
+                // Check parent if expr.
+                top_if_expr = parent_if_expr;
+            }
             if let Expr::Block { statements, tail, .. } = &body.exprs[*then_branch] {
                 let last_then_expr = tail.or_else(|| match statements.last()? {
                     Statement::Expr { expr, .. } => Some(*expr),

crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs

@@ -2,7 +2,10 @@ use hir::{db::ExpandDatabase, diagnostics::RemoveUnnecessaryElse, HirFileIdExt};
 use ide_db::{assists::Assist, source_change::SourceChange};
 use itertools::Itertools;
 use syntax::{
-    ast::{self, edit::IndentLevel},
+    ast::{
+        self,
+        edit::{AstNodeEdit, IndentLevel},
+    },
     AstNode, SyntaxToken, TextRange,
 };
 use text_edit::TextEdit;
@@ -41,10 +44,15 @@ fn fixes(ctx: &DiagnosticsContext<'_>, d: &RemoveUnnecessaryElse) -> Option<Vec<
         indent = indent + 1;
     }
     let else_replacement = match if_expr.else_branch()? {
-        ast::ElseBranch::Block(ref block) => {
-            block.statements().map(|stmt| format!("\n{indent}{stmt}")).join("")
-        }
-        ast::ElseBranch::IfExpr(ref nested_if_expr) => {
+        ast::ElseBranch::Block(block) => block
+            .statements()
+            .map(|stmt| format!("\n{indent}{stmt}"))
+            .chain(block.tail_expr().map(|tail| format!("\n{indent}{tail}")))
+            .join(""),
+        ast::ElseBranch::IfExpr(mut nested_if_expr) => {
+            if has_parent_if_expr {
+                nested_if_expr = nested_if_expr.indent(IndentLevel(1))
+            }
             format!("\n{indent}{nested_if_expr}")
         }
     };
@@ -171,6 +179,41 @@ fn test() {
         );
     }
 
+    #[test]
+    fn remove_unnecessary_else_for_return3() {
+        check_diagnostics_with_needless_return_disabled(
+            r#"
+fn test(a: bool) -> i32 {
+    if a {
+        return 1;
+    } else {
+    //^^^^ 💡 weak: remove unnecessary else block
+        0
+    }
+}
+"#,
+        );
+        check_fix(
+            r#"
+fn test(a: bool) -> i32 {
+    if a {
+        return 1;
+    } else$0 {
+        0
+    }
+}
+"#,
+            r#"
+fn test(a: bool) -> i32 {
+    if a {
+        return 1;
+    }
+    0
+}
+"#,
+        );
+    }
+
     #[test]
     fn remove_unnecessary_else_for_return_in_child_if_expr() {
         check_diagnostics_with_needless_return_disabled(
@@ -214,6 +257,41 @@ fn test() {
         );
     }
 
+    #[test]
+    fn remove_unnecessary_else_for_return_in_child_if_expr2() {
+        check_fix(
+            r#"
+fn test() {
+    if foo {
+        do_something();
+    } else if qux {
+        return bar;
+    } else$0 if quux {
+        do_something_else();
+    } else {
+        do_something_else2();
+    }
+}
+"#,
+            r#"
+fn test() {
+    if foo {
+        do_something();
+    } else {
+        if qux {
+            return bar;
+        }
+        if quux {
+            do_something_else();
+        } else {
+            do_something_else2();
+        }
+    }
+}
+"#,
+        );
+    }
+
     #[test]
     fn remove_unnecessary_else_for_break() {
         check_diagnostics(
@@ -384,6 +462,33 @@ fn test() {
         return bar;
     }
 }
+"#,
+        );
+    }
+
+    #[test]
+    fn no_diagnostic_if_tail_exists_in_else_branch() {
+        check_diagnostics_with_needless_return_disabled(
+            r#"
+fn test1(a: bool) {
+    let _x = if a {
+        return;
+    } else {
+        1
+    };
+}
+
+fn test2(a: bool, b: bool, c: bool) {
+    let _x = if a {
+        return;
+    } else if b {
+        return;
+    } else if c {
+        1
+    } else {
+        return;
+    };
+}
 "#,
         );
     }