Skip to content

Conversation

SeseMueller
Copy link

We all started somewhere as programmers. Nowhere is that more appearent than looking back at what code you wrote when you knew less and cringing because it was just that bad.

A very popular example, as many know, is the problem of determining whether a number is even or odd.
Knowing nothing about the modulus operator or matching in rust, a beginner might write:

if test == 1 {
        println!("odd");
    }
    if test == 2 {
        println!("even");
    }
    if test == 3 {
        println!("odd");
    }

This, however, is inefficient and takes a while to run.

However, with the power of clippy and 🚀blazingly fast lints ✨, we can help.

Introducing:

The compressable_if lint!

(Not to be confused with the collapsable_if lint)

It automatically detects if two if-blocks can be merged if their inner blocks are identical and suggesst merging them via OR-ing the conditions.

This allows even beginners to write much more concise code!
For example, the above example becomes

if test == 1 || test == 3 {
        println!("odd");
    }
    if test == 2 {
        println!("even");
    }

and with the power of clippy they can even improve their code with this lint with a single click!

All in all, this lint will help beginners understand how to write better code and will totally not lead to any code breakage whatsoever.

For further info, read here:

Details

First of all, I want to make clear that this is an april fools joke.
The lint is sloppy and has many false positives and even breaks code from time to time.

It was hard to even get it so pass because it hit so many times on clippy internals, because many tests test against empty if blocks. (Which is also the reason why 156 files were changed)

I first had the idea when the even-odd craze came over many programming social media channels.
Originally, I wanted the lint to output code with the modulus operator, but I found it a bit too hard given I have little experience and not much time.

I hardly doubt that an actual collapsable_if is possible/reasonable, since the nessecary logic is extremely complex and could lead to incomprehensible conditions. I certainly won't try.

Happy april fools!
Stay safe and don't use lint from people you don't trust.

(Also I really hope that the check passes; I worked quite hard on it.)

Sese Mueller added 2 commits April 1, 2024 11:20
I somehow messed up my references to origin/master, so I have to merge
locally.
@rustbot
Copy link
Collaborator

rustbot commented Apr 1, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Apr 1, 2024
@rustbot
Copy link
Collaborator

rustbot commented Apr 1, 2024

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

@matthiaskrgr
Copy link
Member

thanks!
@​bors r+

@xFrednet
Copy link
Contributor

xFrednet commented Apr 1, 2024

The examples is looks pretty verbose. Do we maybe want to suggest to use shorter identifies?

    if t == 1 || t == 3 {
        println!("odd");
    }
    if t == 2 {
        println!("even");
    }

Imagine how much disk space we could safe!

@@ -1,3 +1,4 @@
#![allow(clippy::compressable_if)]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit wary to add a complexity lint that needs multiple allow annotations in our own code.

I'd either see the lint applied everywhere we can (which would show if it actually is an improvement), or keep it in nursery or pedantic for now.

The last thing we need is a lint that will trigger a lot on average code with a warning that has no urgency at all.

@SvizelPritula SvizelPritula mentioned this pull request Apr 1, 2024
@J-ZhengLi
Copy link
Member

J-ZhengLi commented Apr 4, 2024

The examples is looks pretty verbose. Do we maybe want to suggest to use shorter identifies?

    if t == 1 || t == 3 {
        println!("odd");
    }
    if t == 2 {
        println!("even");
    }

Imagine how much disk space we could safe!

if this PR got merged, we can save even more unnecessary disk usage! 😆

But anyway, in case of letting more people confused by this April fools PR (especially there's an actual stacked_ifs PR exists), I'm gonna close this hope you don't mind~

Thank you for the effort tho, lol

@J-ZhengLi J-ZhengLi closed this Apr 4, 2024
@SeseMueller
Copy link
Author

Closing it is a good idea imo, I made sure that it was kinda obvious that this is an april fools joke, the other issue was more subtle. Hopefully people look into the issue a bit deeper when they wonder why it's closed vs if it just looks like it's abandoned.
(Also I haven't abandoned the other lint I was working on; it was not just warming up for this one, but the semester has begun and I'll get back to that one later)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) 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.

6 participants