Unnamed repository; edit this file 'description' to name the repository.
| -rw-r--r-- | crates/hir-ty/src/diagnostics/expr.rs | 29 | ||||
| -rw-r--r-- | crates/hir/src/diagnostics.rs | 16 | ||||
| -rw-r--r-- | crates/ide-diagnostics/src/handlers/mutability_errors.rs | 8 | ||||
| -rw-r--r-- | crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs | 319 | ||||
| -rw-r--r-- | crates/ide-diagnostics/src/lib.rs | 2 | ||||
| -rw-r--r-- | crates/ide-diagnostics/src/tests.rs | 10 |
6 files changed, 380 insertions, 4 deletions
diff --git a/crates/hir-ty/src/diagnostics/expr.rs b/crates/hir-ty/src/diagnostics/expr.rs index 52e635a26e..c09351390a 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 b161265cd9..487e0c8f7a 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 773a075f8f..d9804cbd94 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 0000000000..3ba1c20ad6 --- /dev/null +++ b/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs @@ -0,0 +1,319 @@ +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 indent = IndentLevel::from_node(if_expr.syntax()); + let 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 range = 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_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 ad5e66c5cc..7423de0be7 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 792d4a371e..d8a796e01b 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 |