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 if_not_else #13809

Merged
merged 2 commits into from
Dec 24, 2024
Merged

auto-fix if_not_else #13809

merged 2 commits into from
Dec 24, 2024

Conversation

lapla-cogito
Copy link
Contributor

@lapla-cogito lapla-cogito commented Dec 11, 2024

fix #13411

The if_not_else lint can be fixed automatically, but the issue above reports that there is no implementation to do so. Therefore, this PR implements it.


changelog: [if_not_else]: make suggestions for modified code

@rustbot
Copy link
Collaborator

rustbot commented Dec 11, 2024

r? @Alexendoo

rustbot has assigned @Alexendoo.
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 Dec 11, 2024
@@ -54,7 +58,7 @@ fn is_zero_const(expr: &Expr<'_>, cx: &LateContext<'_>) -> bool {

impl LateLintPass<'_> for IfNotElse {
fn check_expr(&mut self, cx: &LateContext<'_>, e: &Expr<'_>) {
if let ExprKind::If(cond, _, Some(els)) = e.kind
if let ExprKind::If(cond, inter, Some(els)) = e.kind
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The processing performed by the if statement is necessary to present the proposed modification in the make_sugg function.

@lapla-cogito lapla-cogito changed the title suggest if_not_else auto-fix if_not_else Dec 11, 2024
@lapla-cogito lapla-cogito force-pushed the suggest_if_else branch 4 times, most recently from 0322f5f to 654c288 Compare December 11, 2024 09:00
@lapla-cogito
Copy link
Contributor Author

r? clippy

@rustbot rustbot assigned llogiq and unassigned Alexendoo Dec 22, 2024
@llogiq
Copy link
Contributor

llogiq commented Dec 24, 2024

I'd like to see more test cases involving comments and annotations, just to be sure the suggestion reproduces them faithfully.

@lapla-cogito
Copy link
Contributor Author

@llogiq I added several test cases in 887aa26.

Comment on lines +32 to +46
if !(foo() && bla()) {
#[cfg(not(debug_assertions))]
println!("not debug");
#[cfg(debug_assertions)]
println!("debug");
if foo() {
println!("foo");
} else if bla() {
println!("bla");
} else {
println!("both false");
}
} else {
println!("both true");
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Considering the following code, which is similar in structure to this one, except that the parts to be linted are nested:

if !(foo() && bla()) {
    if !foo() {
        println!("not foo");
    } else {
        println!("some output");
    }
} else {
    println!("both true");
}

Then, when I run cargo bless, I get a cannot replace slice of data that was already replaced error. But I don't think this is a problem since rustfix will apply the first fix and ignore the subsequent when put to practical use.

@llogiq
Copy link
Contributor

llogiq commented Dec 24, 2024

Thank you!

@llogiq llogiq added this pull request to the merge queue Dec 24, 2024
Merged via the queue into rust-lang:master with commit 85b6094 Dec 24, 2024
9 checks passed
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.

auto-fix if_not_else
4 participants