From e9c80a9c256677d85398d93880eec1e54e226d2a Mon Sep 17 00:00:00 2001 From: Shoyu Vanilla Date: Thu, 15 Feb 2024 00:59:13 +0900 Subject: [PATCH 1/8] fix: False positive diagnostic for necessary `else` --- crates/hir-ty/src/diagnostics/expr.rs | 8 ++++++- .../src/handlers/remove_unnecessary_else.rs | 23 +++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/crates/hir-ty/src/diagnostics/expr.rs b/crates/hir-ty/src/diagnostics/expr.rs index 0c5d6399619a..571f01dde2ef 100644 --- a/crates/hir-ty/src/diagnostics/expr.rs +++ b/crates/hir-ty/src/diagnostics/expr.rs @@ -338,7 +338,13 @@ impl ExprValidator { 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() { + if let Some(else_branch) = else_branch { + // If else branch has a tail, it is an "expression" that produces a value, + // e.g. `let a = if { ... } else { ... };` and this `else` is not unnecessary + if let Expr::Block { tail: Some(_), .. } = body.exprs[*else_branch] { + return; + } + } else { return; } if let Expr::Block { statements, tail, .. } = &body.exprs[*then_branch] { diff --git a/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs b/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs index ae8241ec2c69..813c07a505db 100644 --- a/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs +++ b/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs @@ -384,6 +384,29 @@ 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) -> i32 { + if a { + return 1; + } else { + 0 + } +} "#, ); } From d14b22863bd58e78a8e9193db987ae22ba1e57e1 Mon Sep 17 00:00:00 2001 From: Shoyu Vanilla Date: Thu, 15 Feb 2024 01:29:48 +0900 Subject: [PATCH 2/8] Handle cases for `else if` --- crates/hir-ty/src/diagnostics/expr.rs | 19 +++++++++++++++++-- .../src/handlers/remove_unnecessary_else.rs | 12 ++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/crates/hir-ty/src/diagnostics/expr.rs b/crates/hir-ty/src/diagnostics/expr.rs index 571f01dde2ef..ff70618ca129 100644 --- a/crates/hir-ty/src/diagnostics/expr.rs +++ b/crates/hir-ty/src/diagnostics/expr.rs @@ -341,8 +341,23 @@ impl ExprValidator { if let Some(else_branch) = else_branch { // If else branch has a tail, it is an "expression" that produces a value, // e.g. `let a = if { ... } else { ... };` and this `else` is not unnecessary - if let Expr::Block { tail: Some(_), .. } = body.exprs[*else_branch] { - return; + let mut branch = *else_branch; + loop { + match body.exprs[branch] { + Expr::Block { tail: Some(_), .. } => return, + Expr::If { then_branch, else_branch, .. } => { + if let Expr::Block { tail: Some(_), .. } = body.exprs[then_branch] { + return; + } + if let Some(else_branch) = else_branch { + // Continue checking for branches like `if { ... } else if { ... } else...` + branch = else_branch; + continue; + } + } + _ => break, + } + break; } } else { return; diff --git a/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs b/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs index 813c07a505db..bbc10e96cef8 100644 --- a/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs +++ b/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs @@ -407,6 +407,18 @@ fn test2(a: bool) -> i32 { 0 } } + +fn test3(a: bool, b: bool, c: bool) { + let _x = if a { + return; + } else if b { + return; + } else if c { + 1 + } else { + return; + }; +} "#, ); } From 21f4ff03516eebb3cbdd8947ad3a7a00c980a692 Mon Sep 17 00:00:00 2001 From: Shoyu Vanilla Date: Fri, 16 Feb 2024 23:53:00 +0900 Subject: [PATCH 3/8] Check for let expr ancestors instead of tail expr --- crates/hir-ty/src/diagnostics/expr.rs | 52 ++++++++++--------- .../src/handlers/remove_unnecessary_else.rs | 10 +--- 2 files changed, 29 insertions(+), 33 deletions(-) diff --git a/crates/hir-ty/src/diagnostics/expr.rs b/crates/hir-ty/src/diagnostics/expr.rs index ff70618ca129..718409e15997 100644 --- a/crates/hir-ty/src/diagnostics/expr.rs +++ b/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 tracing::debug; use triomphe::Arc; use typed_arena::Arena; @@ -108,7 +109,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); } Expr::Block { .. } => { self.validate_block(db, expr); @@ -336,31 +337,34 @@ 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 let Some(else_branch) = else_branch { - // If else branch has a tail, it is an "expression" that produces a value, - // e.g. `let a = if { ... } else { ... };` and this `else` is not unnecessary - let mut branch = *else_branch; - loop { - match body.exprs[branch] { - Expr::Block { tail: Some(_), .. } => return, - Expr::If { then_branch, else_branch, .. } => { - if let Expr::Block { tail: Some(_), .. } = body.exprs[then_branch] { - return; - } - if let Some(else_branch) = else_branch { - // Continue checking for branches like `if { ... } else if { ... } else...` - branch = else_branch; - continue; - } - } - _ => break, - } - break; - } - } else { + 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()? { diff --git a/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs b/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs index bbc10e96cef8..351f728747ef 100644 --- a/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs +++ b/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs @@ -400,15 +400,7 @@ fn test1(a: bool) { }; } -fn test2(a: bool) -> i32 { - if a { - return 1; - } else { - 0 - } -} - -fn test3(a: bool, b: bool, c: bool) { +fn test2(a: bool, b: bool, c: bool) { let _x = if a { return; } else if b { From 8f6e2127c3a1537298cbda11a8f261875edd6659 Mon Sep 17 00:00:00 2001 From: Shoyu Vanilla Date: Fri, 16 Feb 2024 23:54:01 +0900 Subject: [PATCH 4/8] Fix the remove unnecessary else action to preserve block tail expr --- .../src/handlers/remove_unnecessary_else.rs | 43 +++++++++++++++++-- 1 file changed, 40 insertions(+), 3 deletions(-) diff --git a/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs b/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs index 351f728747ef..289ce6403543 100644 --- a/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs +++ b/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs @@ -41,9 +41,11 @@ fn fixes(ctx: &DiagnosticsContext<'_>, d: &RemoveUnnecessaryElse) -> Option { - block.statements().map(|stmt| format!("\n{indent}{stmt}")).join("") - } + ast::ElseBranch::Block(ref block) => block + .statements() + .map(|stmt| format!("\n{indent}{stmt}")) + .chain(block.tail_expr().map(|tail| format!("\n{indent}{tail}"))) + .join(""), ast::ElseBranch::IfExpr(ref nested_if_expr) => { format!("\n{indent}{nested_if_expr}") } @@ -171,6 +173,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( From 1205853c3689a69e81578dfd066b17e3ebe376cf Mon Sep 17 00:00:00 2001 From: Shoyu Vanilla Date: Sat, 17 Feb 2024 00:55:45 +0900 Subject: [PATCH 5/8] Apply indent fix in #16575 --- .../src/handlers/remove_unnecessary_else.rs | 47 +++++++++++++++++-- 1 file changed, 44 insertions(+), 3 deletions(-) diff --git a/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs b/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs index 289ce6403543..9564807a334e 100644 --- a/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs +++ b/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,12 +44,15 @@ fn fixes(ctx: &DiagnosticsContext<'_>, d: &RemoveUnnecessaryElse) -> Option block + 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(ref nested_if_expr) => { + 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}") } }; @@ -251,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( From ff7031008651021c330b93d4bd502810022b045d Mon Sep 17 00:00:00 2001 From: davidsemakula Date: Fri, 16 Feb 2024 20:39:52 +0300 Subject: [PATCH 6/8] fix: only emit "unnecessary else" diagnostic for expr stmts --- crates/hir-ty/src/diagnostics/expr.rs | 64 +++++++++++-------- .../src/handlers/remove_unnecessary_else.rs | 14 +++- 2 files changed, 49 insertions(+), 29 deletions(-) diff --git a/crates/hir-ty/src/diagnostics/expr.rs b/crates/hir-ty/src/diagnostics/expr.rs index 718409e15997..4fe75f24b80c 100644 --- a/crates/hir-ty/src/diagnostics/expr.rs +++ b/crates/hir-ty/src/diagnostics/expr.rs @@ -109,7 +109,7 @@ impl ExprValidator { self.check_for_trailing_return(*body_expr, &body); } Expr::If { .. } => { - self.check_for_unnecessary_else(id, expr, db); + self.check_for_unnecessary_else(id, expr, &body, db); } Expr::Block { .. } => { self.validate_block(db, expr); @@ -337,35 +337,17 @@ impl ExprValidator { } } - fn check_for_unnecessary_else(&mut self, id: ExprId, expr: &Expr, db: &dyn HirDatabase) { + fn check_for_unnecessary_else( + &mut self, + id: ExprId, + expr: &Expr, + body: &Body, + 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), @@ -374,6 +356,36 @@ impl ExprValidator { 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() { + // Only look at sources if the then branch diverges and we have an else branch. + let (_, 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_expr_stmt_or_stmt_list = + parent.as_ref().map_or(false, |node| { + ast::ExprStmt::can_cast(node.kind()) + | ast::StmtList::can_cast(node.kind()) + }); + if has_parent_expr_stmt_or_stmt_list { + // Only emit diagnostic if parent or direct ancestor is either + // an expr stmt or a stmt list. + break; + } + let Some(parent_if_expr) = parent.and_then(ast::IfExpr::cast) else { + // Bail if parent is neither an if expr, an expr stmt nor a stmt list. + return; + }; + // Check parent if expr. + top_if_expr = parent_if_expr; + } + self.diagnostics .push(BodyValidationDiagnostic::RemoveUnnecessaryElse { if_expr: id }) } diff --git a/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs b/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs index 9564807a334e..7bfd64596ed2 100644 --- a/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs +++ b/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs @@ -467,10 +467,10 @@ fn test() { } #[test] - fn no_diagnostic_if_tail_exists_in_else_branch() { + fn no_diagnostic_if_not_expr_stmt() { check_diagnostics_with_needless_return_disabled( r#" -fn test1(a: bool) { +fn test1() { let _x = if a { return; } else { @@ -478,7 +478,7 @@ fn test1(a: bool) { }; } -fn test2(a: bool, b: bool, c: bool) { +fn test2() { let _x = if a { return; } else if b { @@ -491,5 +491,13 @@ fn test2(a: bool, b: bool, c: bool) { } "#, ); + check_diagnostics_with_disabled( + r#" +fn test3() { + foo(if a { return 1 } else { 0 }) +} +"#, + std::iter::once("E0308".to_owned()), + ); } } From 7dfeb2cdcc2a899f929bb3da1c2db7fe6725fb47 Mon Sep 17 00:00:00 2001 From: davidsemakula Date: Mon, 19 Feb 2024 14:43:43 +0300 Subject: [PATCH 7/8] refactor "unnecessary else" diagnostic test --- .../ide-diagnostics/src/handlers/remove_unnecessary_else.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs b/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs index 7bfd64596ed2..9c63d79d9103 100644 --- a/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs +++ b/crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs @@ -491,13 +491,12 @@ fn test2() { } "#, ); - check_diagnostics_with_disabled( + check_diagnostics( r#" -fn test3() { +fn test3() -> u8 { foo(if a { return 1 } else { 0 }) } "#, - std::iter::once("E0308".to_owned()), ); } } From f2218e727840ec0286d89ecc758322674e1efb6d Mon Sep 17 00:00:00 2001 From: davidsemakula Date: Mon, 19 Feb 2024 15:35:47 +0300 Subject: [PATCH 8/8] refactor: remove body parameter for "unnecessary else" diagnostic --- crates/hir-ty/src/diagnostics/expr.rs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/crates/hir-ty/src/diagnostics/expr.rs b/crates/hir-ty/src/diagnostics/expr.rs index 4fe75f24b80c..6c8a18751657 100644 --- a/crates/hir-ty/src/diagnostics/expr.rs +++ b/crates/hir-ty/src/diagnostics/expr.rs @@ -109,7 +109,7 @@ impl ExprValidator { self.check_for_trailing_return(*body_expr, &body); } Expr::If { .. } => { - self.check_for_unnecessary_else(id, expr, &body, db); + self.check_for_unnecessary_else(id, expr, db); } Expr::Block { .. } => { self.validate_block(db, expr); @@ -337,18 +337,12 @@ impl ExprValidator { } } - fn check_for_unnecessary_else( - &mut self, - id: ExprId, - expr: &Expr, - body: &Body, - db: &dyn HirDatabase, - ) { + 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; } - if let Expr::Block { statements, tail, .. } = &body.exprs[*then_branch] { + if let Expr::Block { statements, tail, .. } = &self.body.exprs[*then_branch] { let last_then_expr = tail.or_else(|| match statements.last()? { Statement::Expr { expr, .. } => Some(*expr), _ => None,