Add ifs_as_logical_ops lint

[barun511]

View all comments

Fixes #14865 by adding the ifs_as_logical_ops lint.

Only adds the conjunctive version (transform if x else y => x && y) for now.

Please write a short comment explaining your change (or "none" for internal only changes)

changelog: [ifs_as_logical_ops] : "Warn about cases where x && y could be used in place of if conditions"

[rustbot]

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

[barun511]

Hm, it doesn't look like my rebase really worked, let me have another go...

[barun511]

I'll take a look at merge conflicts once I've got a review, I think

[Jarcho]

Did a first pass over this. You'll need a late pass to avoid an annoying false positive.

[barun511]

Thanks for the review! I had a go at writing it with a late lint pass

[barun511]

@rustbot ready

[Jarcho]

First just a couple of small changes. These are a little nitpicky.

[barun511]

Thanks! Responded

[barun511]

@rustbot ready

[Jarcho]

Next you'll need to handle macros and fill in the docs.

[barun511]

@rustbot ready

[Jarcho]

Ping @barun511 are you willing to continue working on this? There's #17449 which continues this if you'd prefer to let this continue there.

Sorry for originally letting this sit for a while.

[barun511]

No worries!

Sorry for not responding to the message above, it wasn't clear to me how far away from approval the PR was - and given that it didn't seem to get any traction after a point I didn't want to keep maintaining the PR if the work wasn't a priority.

I'm happy to clean this up tomorrow

[barun511]

Oh I still had my unanswered question about proc_macro above, but I can try and figure that out myself

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[ada4a]

The first commit still adds the .DS_Store file (which the second commit then removes), please remove 👉👈

View changes since this review

[ada4a]

it wasn't clear to me how far away from approval the PR was

I'd say it looks pretty much ready to merge, after we figure out the proc-macro stuff (if you don't manage to figure it out yourself, consider asking @.Jarcho). Thanks again for all your work so far:)

[barun511]

Regarding the question: "Why does removing the is_proc_macro check not cause the test to fail?"

I investigated by doing the following:

• I first tried to remove is_proc_macro check from other similiar tests, and see if they failed. In fact, they did - so I concluded that is_proc_macro was not at fault.
• I then theorized that potentially is_external_macro, or some other checks before this were triggering and causing is_proc_macro to be irrelevant, but even on removing all these checks the test continued to pass.
• After doing all of this, I realised I was quite dumb and just put in some dbg statements between the ifs to see what exact condition was causing it to fail.

It seems like this piece of code returns false for proc_macros, because the span of the macro usually does not have the character '{' in it.

I'm not quite sure what to make of this - of course after we add the check we won't be hitting this case, but we then don't have a test case that would fail if the in_proc_macro or in_external_macro checks are removed.

We could always consider taking this as a follow up/finding a more robust way of checking that there are no comments in the span

[barun511]

(or also potentially I'm not walking the span to the context correctly)?

[Jarcho]

You can practically ignore the proc macro check since you're basically already doing it when you check for comments. If you wrote the test as if b1 $({ num > 30 }) else $({ false }) then you would need it ($() escapes the span assignment), but I don't think there are any real macros that would pose a problem here.

[Jarcho]

You might want to add .DS_store to your global .gitignore just to avoid problems with it in the future. We should add it and some other OS files to ours as well.

[barun511]

Yep, I did update my global gitignore, and i've updated to no longer use proc macro check!

Lmk if anything else is blocking

[ada4a]

LGTM, thank you again:)

Now let's start the FCP

@rustbot label lint-nominated

View changes since this review

[rustbot]

This lint has been nominated for inclusion.

A FCP topic has been created on Zulip.

[barun511]

@Jarcho just pinging for re-review here!

[rustbot]

☔ The latest upstream changes (possibly #13598) made this pull request unmergeable. Please resolve the merge conflicts.

[ada4a]

Sorry, forgot to finish the FCP... There has been one approval by @llogiq, so I'd say let's merge this after the rebase and one fix

View changes since this review

[ada4a]

Suggested change

	#[clippy::version = "1.100.0"]
	#[clippy::version = "1.101.0"]