fix: False positive unnecessary else block diagnostic #16567

ShoyuVanilla · 2024-02-14T17:07:21Z

RA detects the else blocks of if-else statements as "unnecessary" if the if-then block's last expression is a never type - e.g. return keyword -.
But this shouldn't be done to something like

let foo = if bar.is_err {
    return;
} else {
    bar.unwrap().0
};

ShoyuVanilla · 2024-02-14T17:26:08Z

More details are in my comment of the original issue

davidsemakula · 2024-02-15T01:34:05Z

Thanks for the fix @ShoyuVanilla
I've left suggested changes and a few other considerations as inline comments

ShoyuVanilla · 2024-02-15T02:08:38Z

Thanks for the fix @ShoyuVanilla I've left suggested changes and a few other considerations as inline comments

Thanks a lot! But where can I see your reviews? BTW I'm in my office now, so it would be few hours later when I reflect them 😄

Oh, I can see your reviews now

davidsemakula · 2024-02-15T01:20:52Z

crates/hir-ty/src/diagnostics/expr.rs

+            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;
            }


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

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.

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

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 } };

Haha, I was dumb. You're right 👍

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

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 👍

davidsemakula · 2024-02-15T01:31:42Z

crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs

+    } else {
+        0
+    }


This should still be linted

davidsemakula · 2024-02-15T02:15:02Z

Thanks for the fix @ShoyuVanilla I've left suggested changes and a few other considerations as inline comments

Thanks a lot! But where can I see your reviews? BTW I'm in my office now, so it would be few hours later when I reflect them 😄

I guess I'd actually forgotten to hit submit 😅 ... you should be able to see them now 🙂

davidsemakula

LGTM! @ShoyuVanilla I left a couple of nits you can fix that were originally added by me 🙂

davidsemakula · 2024-02-16T15:34:23Z

crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs

-        ast::ElseBranch::Block(ref block) => {
-            block.statements().map(|stmt| format!("\n{indent}{stmt}")).join("")
-        }
+        ast::ElseBranch::Block(ref block) => block


nit: this was added by me 😅 ... but you can remove this unnecessary ref 🙂.

davidsemakula · 2024-02-16T15:35:36Z

crates/ide-diagnostics/src/handlers/remove_unnecessary_else.rs

+            .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) => {


nit: same here, you can remove this unnecessary ref, again my fault not yours 😅.

davidsemakula · 2024-02-16T15:42:27Z

@ShoyuVanilla also would you consider adding the minor changes in #16575 to this PR? I would then close it in favor of this to reduce the review burden on the maintainers 🙂

ShoyuVanilla · 2024-02-16T15:48:49Z

@ShoyuVanilla also would you consider adding the minor changes in #16575 to this PR? I would then close it in favor of this to reduce the review burden on the maintainers 🙂

Sure!

Veykril · 2024-02-16T16:01:30Z

crates/hir-ty/src/diagnostics/expr.rs

+            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;
+            }


This doesn't actually catch all the cases where this is wrong. The transformation is only legal if this if expression is an expression statement.

As foo(if a { return 1 } else { 0 }) shouldn't trigger this either for example, no let statements involved

Additionally parsing and such is rather expensive (ideally we could solve this without the AST, but for that we need a tree traversal here) so let's do this check directly before the self.diagnostics.push(BodyValidationDiagnostic::RemoveUnnecessaryElse { if_expr: id }) line

Okay, I'll do this tomorrow (It's midnight here 😄)

@ShoyuVanilla I can also highjack this if that's fine with you ... still relatively early for me 🙂

Sure you can

davidsemakula · 2024-02-16T18:38:28Z

Changes moved to #16590

… r=Veykril fix: Fix false positives for "unnecessary else" diagnostic Completes #16567 by `@ShoyuVanilla` (see #16567 (comment)) Fixes #16556

ShoyuVanilla added 2 commits February 15, 2024 00:59

fix: False positive diagnostic for necessary else

b01b42f

Handle cases for else if

9a178c7

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 14, 2024

davidsemakula suggested changes Feb 15, 2024

View reviewed changes

Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 15, 2024

ShoyuVanilla added 2 commits February 16, 2024 23:53

Check for let expr ancestors instead of tail expr

b22987c

Fix the remove unnecessary else action to preserve block tail expr

07f462e

ShoyuVanilla requested a review from davidsemakula February 16, 2024 15:04

davidsemakula approved these changes Feb 16, 2024

View reviewed changes

davidsemakula mentioned this pull request Feb 16, 2024

fix: Fix indenting for child if expr with an else branch for "unnecessary else" diagnostic #16575

Closed

Apply indent fix in rust-lang#16575

87f930c

Veykril reviewed Feb 16, 2024

View reviewed changes

davidsemakula mentioned this pull request Feb 16, 2024

fix: Fix false positives for "unnecessary else" diagnostic #16590

Merged

ShoyuVanilla closed this Feb 17, 2024

bors added a commit that referenced this pull request Feb 19, 2024

Auto merge of #16590 - davidsemakula:unnecessary-else-diagnostic-fix,…

e6b96db

… r=Veykril fix: Fix false positives for "unnecessary else" diagnostic Completes #16567 by `@ShoyuVanilla` (see #16567 (comment)) Fixes #16556

ShoyuVanilla deleted the fix-necessary-else branch July 12, 2024 23:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: False positive unnecessary else block diagnostic #16567

fix: False positive unnecessary else block diagnostic #16567

ShoyuVanilla commented Feb 14, 2024

ShoyuVanilla commented Feb 14, 2024

davidsemakula commented Feb 15, 2024

ShoyuVanilla commented Feb 15, 2024 •

edited

Loading

davidsemakula Feb 15, 2024

davidsemakula Feb 15, 2024

ShoyuVanilla Feb 15, 2024 •

edited

Loading

davidsemakula Feb 15, 2024

ShoyuVanilla Feb 15, 2024

davidsemakula Feb 15, 2024

ShoyuVanilla Feb 15, 2024

davidsemakula Feb 15, 2024

davidsemakula commented Feb 15, 2024

davidsemakula left a comment

davidsemakula Feb 16, 2024

davidsemakula Feb 16, 2024

davidsemakula commented Feb 16, 2024

ShoyuVanilla commented Feb 16, 2024

Veykril Feb 16, 2024

Veykril Feb 16, 2024

ShoyuVanilla Feb 16, 2024

davidsemakula Feb 16, 2024

ShoyuVanilla Feb 16, 2024

davidsemakula commented Feb 16, 2024

fix: False positive unnecessary else block diagnostic #16567

fix: False positive unnecessary else block diagnostic #16567

Conversation

ShoyuVanilla commented Feb 14, 2024

ShoyuVanilla commented Feb 14, 2024

davidsemakula commented Feb 15, 2024

ShoyuVanilla commented Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShoyuVanilla Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsemakula commented Feb 15, 2024

davidsemakula left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsemakula commented Feb 16, 2024

ShoyuVanilla commented Feb 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsemakula commented Feb 16, 2024

ShoyuVanilla commented Feb 15, 2024 •

edited

Loading

ShoyuVanilla Feb 15, 2024 •

edited

Loading