Auto merge of #16502 - davidsemakula:unnecessary-else-diagnostic, r=Veykril

feat: Add "unnecessary else" diagnostic and fix

Fixes #9457

6 files changed, 447 insertions, 4 deletions

diff --git a/crates/hir-ty/src/diagnostics/expr.rs b/crates/hir-ty/src/diagnostics/expr.rs
index 52e635a26ea..c09351390af 100644
--- a/crates/hir-ty/src/diagnostics/expr.rs
+++ b/crates/hir-ty/src/diagnostics/expr.rs
@@ -27,7 +27,7 @@ use crate::{
 
 pub(crate) use hir_def::{
     body::Body,
-    hir::{Expr, ExprId, MatchArm, Pat, PatId},
+    hir::{Expr, ExprId, MatchArm, Pat, PatId, Statement},
     LocalFieldId, VariantId,
 };
 
@@ -44,6 +44,9 @@ pub enum BodyValidationDiagnostic {
         match_expr: ExprId,
         uncovered_patterns: String,
     },
+    RemoveUnnecessaryElse {
+        if_expr: ExprId,
+    },
 }
 
 impl BodyValidationDiagnostic {
@@ -90,6 +93,9 @@ impl ExprValidator {
                 Expr::Call { .. } | Expr::MethodCall { .. } => {
                     self.validate_call(db, id, expr, &mut filter_map_next_checker);
                 }
+                Expr::If { .. } => {
+                    self.check_for_unnecessary_else(id, expr, &body);
+                }
                 _ => {}
             }
         }
@@ -237,6 +243,27 @@ impl ExprValidator {
         }
         pattern
     }
+
+    fn check_for_unnecessary_else(&mut self, id: ExprId, expr: &Expr, body: &Body) {
+        if let Expr::If { condition: _, then_branch, else_branch } = expr {
+            if else_branch.is_none() {
+                return;
+            }
+            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),
+                    _ => None,
+                });
+                if let Some(last_then_expr) = last_then_expr {
+                    let last_then_expr_ty = &self.infer[last_then_expr];
+                    if last_then_expr_ty.is_never() {
+                        self.diagnostics
+                            .push(BodyValidationDiagnostic::RemoveUnnecessaryElse { if_expr: id })
+                    }
+                }
+            }
+        }
+    }
 }
 
 struct FilterMapNextChecker {
diff --git a/crates/hir/src/diagnostics.rs b/crates/hir/src/diagnostics.rs
index b161265cd95..487e0c8f7a5 100644
--- a/crates/hir/src/diagnostics.rs
+++ b/crates/hir/src/diagnostics.rs
@@ -68,6 +68,7 @@ diagnostics![
     PrivateAssocItem,
     PrivateField,
     ReplaceFilterMapNextWithFindMap,
+    RemoveUnnecessaryElse,
     TraitImplIncorrectSafety,
     TraitImplMissingAssocItems,
     TraitImplOrphan,
@@ -342,6 +343,11 @@ pub struct TraitImplRedundantAssocItems {
     pub assoc_item: (Name, AssocItem),
 }
 
+#[derive(Debug)]
+pub struct RemoveUnnecessaryElse {
+    pub if_expr: InFile<AstPtr<ast::IfExpr>>,
+}
+
 impl AnyDiagnostic {
     pub(crate) fn body_validation_diagnostic(
         db: &dyn HirDatabase,
@@ -444,6 +450,16 @@ impl AnyDiagnostic {
                     Err(SyntheticSyntax) => (),
                 }
             }
+            BodyValidationDiagnostic::RemoveUnnecessaryElse { if_expr } => {
+                if let Ok(source_ptr) = source_map.expr_syntax(if_expr) {
+                    if let Some(ptr) = source_ptr.value.cast::<ast::IfExpr>() {
+                        return Some(
+                            RemoveUnnecessaryElse { if_expr: InFile::new(source_ptr.file_id, ptr) }
+                                .into(),
+                        );
+                    }
+                }
+            }
         }
         None
     }
diff --git a/crates/ide-diagnostics/src/handlers/mutability_errors.rs b/crates/ide-diagnostics/src/handlers/mutability_errors.rs
index 773a075f8f5..d9804cbd94a 100644
--- a/crates/ide-diagnostics/src/handlers/mutability_errors.rs
+++ b/crates/ide-diagnostics/src/handlers/mutability_errors.rs
@@ -86,7 +86,7 @@ pub(super) fn token(parent: &SyntaxNode, kind: SyntaxKind) -> Option<SyntaxToken
 
 #[cfg(test)]
 mod tests {
-    use crate::tests::{check_diagnostics, check_fix};
+    use crate::tests::{check_diagnostics, check_diagnostics_with_disabled, check_fix};
 
     #[test]
     fn unused_mut_simple() {
@@ -428,7 +428,7 @@ fn main() {
 }
 "#,
         );
-        check_diagnostics(
+        check_diagnostics_with_disabled(
             r#"
 enum X {}
 fn g() -> X {
@@ -448,8 +448,9 @@ fn main(b: bool) {
     &mut x;
 }
 "#,
+            std::iter::once("remove-unnecessary-else".to_string()),
         );
-        check_diagnostics(
+        check_diagnostics_with_disabled(
             r#"
 fn main(b: bool) {
     if b {
@@ -462,6 +463,7 @@ fn main(b: bool) {
     &mut x;
 }
 "#,
+            std::iter::once("remove-unnecessary-else".to_string()),
         );
     }
 
diff --git a/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs b/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs
new file mode 100644
index 00000000000..c6c85256f93
--- /dev/null
+++ b/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs
@@ -0,0 +1,386 @@
+use hir::{db::ExpandDatabase, diagnostics::RemoveUnnecessaryElse, HirFileIdExt};
+use ide_db::{assists::Assist, source_change::SourceChange};
+use itertools::Itertools;
+use syntax::{
+    ast::{self, edit::IndentLevel},
+    AstNode, SyntaxToken, TextRange,
+};
+use text_edit::TextEdit;
+
+use crate::{
+    adjusted_display_range, fix, Diagnostic, DiagnosticCode, DiagnosticsContext, Severity,
+};
+
+// Diagnostic: remove-unnecessary-else
+//
+// This diagnostic is triggered when there is an `else` block for an `if` expression whose
+// then branch diverges (e.g. ends with a `return`, `continue`, `break` e.t.c).
+pub(crate) fn remove_unnecessary_else(
+    ctx: &DiagnosticsContext<'_>,
+    d: &RemoveUnnecessaryElse,
+) -> Diagnostic {
+    let display_range = adjusted_display_range(ctx, d.if_expr, &|if_expr| {
+        if_expr.else_token().as_ref().map(SyntaxToken::text_range)
+    });
+    Diagnostic::new(
+        DiagnosticCode::Ra("remove-unnecessary-else", Severity::WeakWarning),
+        "remove unnecessary else block",
+        display_range,
+    )
+    .with_fixes(fixes(ctx, d))
+}
+
+fn fixes(ctx: &DiagnosticsContext<'_>, d: &RemoveUnnecessaryElse) -> Option<Vec<Assist>> {
+    let root = ctx.sema.db.parse_or_expand(d.if_expr.file_id);
+    let if_expr = d.if_expr.value.to_node(&root);
+    let if_expr = ctx.sema.original_ast_node(if_expr.clone())?;
+
+    let mut indent = IndentLevel::from_node(if_expr.syntax());
+    let has_parent_if_expr = if_expr.syntax().parent().and_then(ast::IfExpr::cast).is_some();
+    if has_parent_if_expr {
+        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) => {
+            format!("\n{indent}{nested_if_expr}")
+        }
+    };
+    let (replacement, range) = if has_parent_if_expr {
+        let base_indent = IndentLevel::from_node(if_expr.syntax());
+        let then_indent = base_indent + 1;
+        let then_child_indent = then_indent + 1;
+
+        let condition = if_expr.condition()?;
+        let then_stmts = if_expr
+            .then_branch()?
+            .statements()
+            .map(|stmt| format!("\n{then_child_indent}{stmt}"))
+            .join("");
+        let then_replacement =
+            format!("\n{then_indent}if {condition} {{{then_stmts}\n{then_indent}}}",);
+        let replacement = format!("{{{then_replacement}{else_replacement}\n{base_indent}}}");
+        (replacement, if_expr.syntax().text_range())
+    } else {
+        (
+            else_replacement,
+            TextRange::new(
+                if_expr.then_branch()?.syntax().text_range().end(),
+                if_expr.syntax().text_range().end(),
+            ),
+        )
+    };
+
+    let edit = TextEdit::replace(range, replacement);
+    let source_change =
+        SourceChange::from_text_edit(d.if_expr.file_id.original_file(ctx.sema.db), edit);
+
+    Some(vec![fix(
+        "remove_unnecessary_else",
+        "Remove unnecessary else block",
+        source_change,
+        range,
+    )])
+}
+
+#[cfg(test)]
+mod tests {
+    use crate::tests::{check_diagnostics, check_fix};
+
+    #[test]
+    fn remove_unnecessary_else_for_return() {
+        check_diagnostics(
+            r#"
+fn test() {
+    if foo {
+        return bar;
+    } else {
+    //^^^^ 💡 weak: remove unnecessary else block
+        do_something_else();
+    }
+}
+"#,
+        );
+        check_fix(
+            r#"
+fn test() {
+    if foo {
+        return bar;
+    } else$0 {
+        do_something_else();
+    }
+}
+"#,
+            r#"
+fn test() {
+    if foo {
+        return bar;
+    }
+    do_something_else();
+}
+"#,
+        );
+    }
+
+    #[test]
+    fn remove_unnecessary_else_for_return2() {
+        check_diagnostics(
+            r#"
+fn test() {
+    if foo {
+        return bar;
+    } else if qux {
+    //^^^^ 💡 weak: remove unnecessary else block
+        do_something_else();
+    } else {
+        do_something_else2();
+    }
+}
+"#,
+        );
+        check_fix(
+            r#"
+fn test() {
+    if foo {
+        return bar;
+    } else$0 if qux {
+        do_something_else();
+    } else {
+        do_something_else2();
+    }
+}
+"#,
+            r#"
+fn test() {
+    if foo {
+        return bar;
+    }
+    if qux {
+        do_something_else();
+    } else {
+        do_something_else2();
+    }
+}
+"#,
+        );
+    }
+
+    #[test]
+    fn remove_unnecessary_else_for_return_in_child_if_expr() {
+        check_diagnostics(
+            r#"
+fn test() {
+    if foo {
+        do_something();
+    } else if qux {
+        return bar;
+    } else {
+    //^^^^ 💡 weak: remove unnecessary else block
+        do_something_else();
+    }
+}
+"#,
+        );
+        check_fix(
+            r#"
+fn test() {
+    if foo {
+        do_something();
+    } else if qux {
+        return bar;
+    } else$0 {
+        do_something_else();
+    }
+}
+"#,
+            r#"
+fn test() {
+    if foo {
+        do_something();
+    } else {
+        if qux {
+            return bar;
+        }
+        do_something_else();
+    }
+}
+"#,
+        );
+    }
+
+    #[test]
+    fn remove_unnecessary_else_for_break() {
+        check_diagnostics(
+            r#"
+fn test() {
+    loop {
+        if foo {
+            break;
+        } else {
+        //^^^^ 💡 weak: remove unnecessary else block
+            do_something_else();
+        }
+    }
+}
+"#,
+        );
+        check_fix(
+            r#"
+fn test() {
+    loop {
+        if foo {
+            break;
+        } else$0 {
+            do_something_else();
+        }
+    }
+}
+"#,
+            r#"
+fn test() {
+    loop {
+        if foo {
+            break;
+        }
+        do_something_else();
+    }
+}
+"#,
+        );
+    }
+
+    #[test]
+    fn remove_unnecessary_else_for_continue() {
+        check_diagnostics(
+            r#"
+fn test() {
+    loop {
+        if foo {
+            continue;
+        } else {
+        //^^^^ 💡 weak: remove unnecessary else block
+            do_something_else();
+        }
+    }
+}
+"#,
+        );
+        check_fix(
+            r#"
+fn test() {
+    loop {
+        if foo {
+            continue;
+        } else$0 {
+            do_something_else();
+        }
+    }
+}
+"#,
+            r#"
+fn test() {
+    loop {
+        if foo {
+            continue;
+        }
+        do_something_else();
+    }
+}
+"#,
+        );
+    }
+
+    #[test]
+    fn remove_unnecessary_else_for_never() {
+        check_diagnostics(
+            r#"
+fn test() {
+    if foo {
+        never();
+    } else {
+    //^^^^ 💡 weak: remove unnecessary else block
+        do_something_else();
+    }
+}
+
+fn never() -> ! {
+    loop {}
+}
+"#,
+        );
+        check_fix(
+            r#"
+fn test() {
+    if foo {
+        never();
+    } else$0 {
+        do_something_else();
+    }
+}
+
+fn never() -> ! {
+    loop {}
+}
+"#,
+            r#"
+fn test() {
+    if foo {
+        never();
+    }
+    do_something_else();
+}
+
+fn never() -> ! {
+    loop {}
+}
+"#,
+        );
+    }
+
+    #[test]
+    fn no_diagnostic_if_no_else_branch() {
+        check_diagnostics(
+            r#"
+fn test() {
+    if foo {
+        return bar;
+    }
+
+    do_something_else();
+}
+"#,
+        );
+    }
+
+    #[test]
+    fn no_diagnostic_if_no_divergence() {
+        check_diagnostics(
+            r#"
+fn test() {
+    if foo {
+        do_something();
+    } else {
+        do_something_else();
+    }
+}
+"#,
+        );
+    }
+
+    #[test]
+    fn no_diagnostic_if_no_divergence_in_else_branch() {
+        check_diagnostics(
+            r#"
+fn test() {
+    if foo {
+        do_something();
+    } else {
+        return bar;
+    }
+}
+"#,
+        );
+    }
+}
diff --git a/crates/ide-diagnostics/src/lib.rs b/crates/ide-diagnostics/src/lib.rs
index ad5e66c5ccd..7423de0be74 100644
--- a/crates/ide-diagnostics/src/lib.rs
+++ b/crates/ide-diagnostics/src/lib.rs
@@ -43,6 +43,7 @@ mod handlers {
     pub(crate) mod no_such_field;
     pub(crate) mod private_assoc_item;
     pub(crate) mod private_field;
+    pub(crate) mod remove_unnecessary_else;
     pub(crate) mod replace_filter_map_next_with_find_map;
     pub(crate) mod trait_impl_incorrect_safety;
     pub(crate) mod trait_impl_missing_assoc_item;
@@ -382,6 +383,7 @@ pub fn diagnostics(
             AnyDiagnostic::UnusedVariable(d) => handlers::unused_variables::unused_variables(&ctx, &d),
             AnyDiagnostic::BreakOutsideOfLoop(d) => handlers::break_outside_of_loop::break_outside_of_loop(&ctx, &d),
             AnyDiagnostic::MismatchedTupleStructPatArgCount(d) => handlers::mismatched_arg_count::mismatched_tuple_struct_pat_arg_count(&ctx, &d),
+            AnyDiagnostic::RemoveUnnecessaryElse(d) => handlers::remove_unnecessary_else::remove_unnecessary_else(&ctx, &d),
         };
         res.push(d)
     }
diff --git a/crates/ide-diagnostics/src/tests.rs b/crates/ide-diagnostics/src/tests.rs
index 792d4a371ee..d8a796e01b1 100644
--- a/crates/ide-diagnostics/src/tests.rs
+++ b/crates/ide-diagnostics/src/tests.rs
@@ -91,6 +91,16 @@ pub(crate) fn check_diagnostics(ra_fixture: &str) {
 }
 
 #[track_caller]
+pub(crate) fn check_diagnostics_with_disabled(
+    ra_fixture: &str,
+    disabled: impl Iterator<Item = String>,
+) {
+    let mut config = DiagnosticsConfig::test_sample();
+    config.disabled.extend(disabled);
+    check_diagnostics_with_config(config, ra_fixture)
+}
+
+#[track_caller]
 pub(crate) fn check_diagnostics_with_config(config: DiagnosticsConfig, ra_fixture: &str) {
     let (db, files) = RootDatabase::with_many_files(ra_fixture);
     let mut annotations = files