Skip to content

fix: False positive unnecessary else block diagnostic #16567

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

fix: False positive unnecessary else block diagnostic #16567

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b01b42f
fix: False positive diagnostic for necessary `else`
ShoyuVanilla Feb 14, 2024
9a178c7
Handle cases for `else if`
ShoyuVanilla Feb 14, 2024
b22987c
Check for let expr ancestors instead of tail expr
ShoyuVanilla Feb 16, 2024
07f462e
Fix the remove unnecessary else action to preserve block tail expr
ShoyuVanilla Feb 16, 2024
87f930c
Apply indent fix in #16575
ShoyuVanilla Feb 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion crates/hir-ty/src/diagnostics/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,28 @@ 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
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;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't be looking for a tail expression in the else branch, you should instead be checking whether the if expression has a let statement as a "direct ancestor" (i.e. either a parent or an ancestor with only if expressions btn it and the current if expression - the latter is mostly to prevent the diagnostic from being to opinionated) 🙂

Suggested change
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 {
return;
}
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;
}

You'll also need to swap out the body: &Body parameter for db: &dyn HirDatabase

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also possible to replace the loop in the suggestion with

let has_parent_let_stmt = if_expr.syntax().parent().and_then(ast::LetStmt::cast).is_some();
if has_parent_let_stmt {
    return;
}

i.e. only look for a parent let stmt.

But that would mean this new test would be linted

let _x = if a {
    return;
} else if b {
    return;
} else if c {
//^^^^ 💡 weak: remove unnecessary else block
    1
} else {
    return;
};

and fixed with

let _x = if a {
    return;
} else {
    if b {
        return;
    }
    if c {
        1
    } else {
        return;
    }
};

I'm not quite sure if that's desirable or too opinionated.
So I think its best to wait for one of the maintainers to chime in for that part.

Copy link
Member Author

@ShoyuVanilla ShoyuVanilla Feb 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should instead be checking whether the if expression has a let statement as a "direct ancestor"

Yes, this would be the right way for handling let a = if ... cases and I had considered that method - though quite ugly way iterating exprs once more for let bindings 😅, not as sophisticated as yours(I've learned a lot from your suggestion codes) - but I think that we should also filter out the cases that if-else expression is a tail expression of the other block, like the following example.

let foo = match bar {
    ...
    _ => {
        if baz {
            return;
        } else {
            a
        }
    }
};

I've tried to express this in the test code bellow you commented, but yes, as you pointed, that case is not well explaining this and should be linted as unnecessary.

Thinking about this again, it's tempting to take the method you suggested + extra filtering for the case that whole if-else chain is a tail expression in a block. I'll gonna look into this more once I get home

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it's worth, I still think the example above should still be linted and fixed with below 🙂

let foo = match bar {
    ...
    _ => {
        if baz {
            return;
        }
        a
    }
};

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, I was dumb. You're right 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, well I'm dumber for writing this diagnostic and not thinking about let statements at all in the first place 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not at all. I think that writing something not exists is the hardest thing 😄
BTW, after applying your suggestions, I'm gonna think about nicer way to fix the triple if-else case you've mentioned. (I'm still at work 😢 )
Thank you for your kind and smart advices 👍

if let Expr::Block { statements, tail, .. } = &body.exprs[*then_branch] {
Expand Down
35 changes: 35 additions & 0 deletions crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,41 @@ 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
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should still be linted

}

fn test3(a: bool, b: bool, c: bool) {
let _x = if a {
return;
} else if b {
return;
} else if c {
1
} else {
return;
};
}
"#,
);
}
Expand Down