Add new lints: `manual_and` and `manual_or` #12832

mira-eanda · 2024-05-22T23:34:49Z

Added two new lints: manual_and that suggests replacing if-else expressions where one branch is a boolean literal with && and
manual_or which does the same but for the || operator.

changelog: new lint: [manual_and]
changelog: new lint: [manual_or]

rustbot · 2024-05-22T23:34:54Z

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

J-ZhengLi · 2024-05-23T07:16:58Z

clippy_lints/src/manual_and.rs

+    /// ```no_run
+    /// if a {
+    ///     b


doc test is failling, maybe you meant to use /// ```ignore ?. Or assign values to a and b using:

/// ```no_run /// # let a = true; /// # let b = false; /// if a { /// b

something like that~

J-ZhengLi

Since manual_and.rs and manual_or.rs shares a lot same code, could you merge them into a single file?

Similar to the structures inside of src/booleans.rs.

J-ZhengLi · 2024-05-23T08:02:50Z

clippy_lints/src/manual_or.rs

+        if let ExprKind::If(cond, then, Some(else_expr)) = expr.kind {
+            if contains_let(cond) {
+                return;
+            }


I think clippy_utils::higher::If should achieve the same result here.

you could try replacing it with:

use clippy_utils::higher; if let Some(higher::If { cond, then, r#else: Some(else_expr) }) = higher::If::hir(expr) { // ...

using visitor might seems a bit of overkill

mira-eanda · 2024-05-24T10:14:05Z

We made the requested changes

J-ZhengLi · 2024-05-31T15:48:59Z

clippy_lints/src/manual_and_or.rs

+            if contains_let(cond) {
+                return;
+            }


I think this is not needed anymore? yes?

That's true, it's fixed now!

J-ZhengLi · 2024-05-31T15:52:17Z

(Jarcho has too many PR assigned, I hope this make sense...)

r? clippy

bors · 2024-06-11T23:22:10Z

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

rustbot · 2024-06-17T15:45:15Z

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 pull --rebase https://github.com/rust-lang/rust-clippy.git master
$ git push --force-with-lease

The following commits are merge commits:

7b68c50

Suggests replacing if-else expressions where one branch is a boolean literal with && / || operators. Co-authored-by: Francisco Salgueiro <[email protected]>

Alexendoo · 2024-06-20T12:11:28Z

clippy_lints/src/manual_and_or.rs

+fn extract_final_expression_snippet<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> Option<String> {
+    if let ExprKind::Block(block, _) = expr.kind {
+        if let Some(final_expr) = block.expr {
+            return cx.sess().source_map().span_to_snippet(final_expr.span).ok();


Can use snippet_opt here & elsewhere

Alexendoo · 2024-06-20T12:20:13Z

clippy_lints/src/manual_and_or.rs

+    if contains_or(cond) || contains_or(then) || fetch_bool_expr(then).is_some() {
+        return;
+    }


The precedence of the context the if appears in is also significant:

let _ = c == if a { b } else { false }; // becomes let _ = c == a && b; // AKA let _ = (c == a) && b;

Alexendoo · 2024-06-20T12:45:43Z

clippy_lints/src/manual_and_or.rs

+        return;
+    }
+    if match then.kind {
+        ExprKind::Block(block, _) => !block.stmts.is_empty(),


We should check that the type of block.expr isn't ! (never), to exclude things like

if a { return } else { false }

The lint isn't semantically incorrect here but a && return is unusual

Alexendoo · 2024-06-20T12:52:19Z

clippy_lints/src/manual_and_or.rs

+        .unwrap_or_else(|_| "..".to_string());
+
+    let then_snippet = extract_final_expression_snippet(cx, then).unwrap_or_else(|| "..".to_string());
+    let suggestion = format!("{cond_snippet} && {then_snippet}");


We'll want to handle when the if contains comments -

if a { // Comment b } else { false }

Preserving them is an option but it might be better to not lint here, if the comment is large it would be weird to have in the middle of an &&/||

There's span_contains_comment to help with this

bors · 2024-07-04T05:51:05Z

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

xFrednet · 2024-08-03T08:28:08Z

Hey @mira-eanda , this is a ping from triage, since there hasn't been any activity in some time. Are you still planning to continue this implementation?

If you have any questions, you're always welcome to ask them in this PR or on Zulip.

@rustbot author

rustbot assigned Jarcho May 22, 2024

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

mira-eanda force-pushed the manual_and_or_lints branch from 9d0b00d to 4165cd5 Compare May 22, 2024 23:58

J-ZhengLi reviewed May 23, 2024

View reviewed changes

mira-eanda force-pushed the manual_and_or_lints branch from 4165cd5 to 66758bc Compare May 24, 2024 10:08

J-ZhengLi reviewed May 31, 2024

View reviewed changes

rustbot assigned Alexendoo and unassigned Jarcho May 31, 2024

mira-eanda force-pushed the manual_and_or_lints branch from 66758bc to 0b884ca Compare June 3, 2024 10:26

mira-eanda force-pushed the manual_and_or_lints branch from 0b884ca to 32b35e7 Compare June 16, 2024 09:32

rustbot added 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 Jun 17, 2024

Implement manual_and and manual_or lints

3658ef6

Suggests replacing if-else expressions where one branch is a boolean literal with && / || operators. Co-authored-by: Francisco Salgueiro <[email protected]>

mira-eanda force-pushed the manual_and_or_lints branch from 7b68c50 to 3658ef6 Compare June 17, 2024 16:26

rustbot removed 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 Jun 17, 2024

Alexendoo reviewed Jun 20, 2024

View reviewed changes

Alexendoo added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jul 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new lints: `manual_and` and `manual_or` #12832

Add new lints: `manual_and` and `manual_or` #12832

mira-eanda commented May 22, 2024

rustbot commented May 22, 2024

J-ZhengLi May 23, 2024 •

edited

Loading

J-ZhengLi left a comment •

edited

Loading

J-ZhengLi May 23, 2024

mira-eanda commented May 24, 2024

J-ZhengLi May 31, 2024

mira-eanda Jun 3, 2024

J-ZhengLi commented May 31, 2024 •

edited

Loading

bors commented Jun 11, 2024

rustbot commented Jun 17, 2024

Alexendoo Jun 20, 2024

Alexendoo Jun 20, 2024

Alexendoo Jun 20, 2024

Alexendoo Jun 20, 2024

bors commented Jul 4, 2024

xFrednet commented Aug 3, 2024

Add new lints: manual_and and manual_or #12832

Are you sure you want to change the base?

Add new lints: manual_and and manual_or #12832

Conversation

mira-eanda commented May 22, 2024

rustbot commented May 22, 2024

J-ZhengLi May 23, 2024 • edited Loading

Choose a reason for hiding this comment

J-ZhengLi left a comment • edited Loading

Choose a reason for hiding this comment

J-ZhengLi May 23, 2024

Choose a reason for hiding this comment

mira-eanda commented May 24, 2024

J-ZhengLi May 31, 2024

Choose a reason for hiding this comment

mira-eanda Jun 3, 2024

Choose a reason for hiding this comment

J-ZhengLi commented May 31, 2024 • edited Loading

bors commented Jun 11, 2024

rustbot commented Jun 17, 2024

Alexendoo Jun 20, 2024

Choose a reason for hiding this comment

Alexendoo Jun 20, 2024

Choose a reason for hiding this comment

Alexendoo Jun 20, 2024

Choose a reason for hiding this comment

Alexendoo Jun 20, 2024

Choose a reason for hiding this comment

bors commented Jul 4, 2024

xFrednet commented Aug 3, 2024

Add new lints: `manual_and` and `manual_or` #12832

Add new lints: `manual_and` and `manual_or` #12832

J-ZhengLi May 23, 2024 •

edited

Loading

J-ZhengLi left a comment •

edited

Loading

J-ZhengLi commented May 31, 2024 •

edited

Loading