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

auto-fix for redundant_else lint #13936

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lapla-cogito
Copy link
Contributor

@lapla-cogito lapla-cogito commented Jan 3, 2025

redundant_else can be fixed automatically.

changelog: [redundant_else]: auto-fix when appropriate

@rustbot
Copy link
Collaborator

rustbot commented Jan 3, 2025

r? @Jarcho

rustbot has assigned @Jarcho.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 3, 2025
@lapla-cogito lapla-cogito force-pushed the autofix_redundant_else branch from e395c27 to 8ade7a6 Compare January 3, 2025 16:32
@samueltardieu
Copy link
Contributor

What if the else block declares variables? Not only will they be dragged into the outer scope, but also their dropping will be deferred until the end of the outer scope, which may be problematic in case of a guard.

For example, the PR fails with this code:

fn with_drop(t: bool) -> u32 {
    let a = 42;
    if t {
        return 0;
    } else {
        let a = "foobar";
        println!("this a = {a}");
    }
    a + 1
}

because let a = "foobar"; has been dragged into the outer scope and hides the outer a. Here the example is constructed specially to exhibit the failure, but with a mutex guard we might have real issues.

Maybe the suggestion should apply only when no bindings are introduced inside the else block, even though the lint could warn about the redundant else.

@samueltardieu
Copy link
Contributor

Another (very minor) issue is that if the fix belongs to a macro, the indentation shown will be the one of the macro call, not the proper one, e.g.,

macro_rules! mac {
    ($t:expr) => {{
        if $t {
            return 0;
        } else {
            return 42;
        }
    }}
}

fn call_macro() -> u32 {
                       mac!(true)
}

will suggest using

macro_rules! mac {
    ($t:expr) => {{
        if $t {
            return 0;
        }
                       return 42;
    }}
}

@lapla-cogito lapla-cogito marked this pull request as draft January 5, 2025 05:35
@lapla-cogito lapla-cogito force-pushed the autofix_redundant_else branch from f397659 to edf3828 Compare January 8, 2025 21:56
@lapla-cogito lapla-cogito marked this pull request as ready for review January 8, 2025 22:25
indent_relative_to: Option<Span>,
) -> Cow<'a, str> {
let extracted = extract_else_block(&snippet(cx, els_span, default));
let indent = indent_relative_to.and_then(|s| indent_of(cx, s));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@samueltardieu As you point out, the indentation of the suggestion would be the same as the indentation of the macro invocation in this implementation. However, I wonder if there is a solution for this since Clippy can only know the structure after the macro has been expanded...

Copy link
Contributor

Choose a reason for hiding this comment

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

This is why I said this was a very minor nit: I don't think it is important to fix, and I'm not sure it is easy to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants