Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add "unnecessary else" diagnostic and fix #16502

Merged
merged 2 commits into from Feb 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 28 additions & 1 deletion crates/hir-ty/src/diagnostics/expr.rs
Expand Up @@ -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,
};

Expand All @@ -44,6 +44,9 @@ pub enum BodyValidationDiagnostic {
match_expr: ExprId,
uncovered_patterns: String,
},
RemoveUnnecessaryElse {
if_expr: ExprId,
},
}

impl BodyValidationDiagnostic {
Expand Down Expand Up @@ -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);
}
_ => {}
}
}
Expand Down Expand Up @@ -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 {
Expand Down
16 changes: 16 additions & 0 deletions crates/hir/src/diagnostics.rs
Expand Up @@ -68,6 +68,7 @@ diagnostics![
PrivateAssocItem,
PrivateField,
ReplaceFilterMapNextWithFindMap,
RemoveUnnecessaryElse,
TraitImplIncorrectSafety,
TraitImplMissingAssocItems,
TraitImplOrphan,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
}
Expand Down
8 changes: 5 additions & 3 deletions crates/ide-diagnostics/src/handlers/mutability_errors.rs
Expand Up @@ -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() {
Expand Down Expand Up @@ -428,7 +428,7 @@ fn main() {
}
"#,
);
check_diagnostics(
check_diagnostics_with_disabled(
r#"
enum X {}
fn g() -> X {
Expand All @@ -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 {
Expand All @@ -462,6 +463,7 @@ fn main(b: bool) {
&mut x;
}
"#,
std::iter::once("remove-unnecessary-else".to_string()),
);
}

Expand Down