[Merged by Bors] - Clippy improvements #4665

ManevilleF · 2022-05-05T08:28:05Z

Objective

Follow up to my previous MR #3718 to add new clippy warnings to bevy:

There is one commit per clippy warning, and the matching flags are added to the CI execution.

To test the CI execution you may run cargo run -p ci -- clippy at the root.

I choose the add the flags in the ci tool crate to avoid having them in every lib.rs but I guess it could become an issue with suprise warnings coming up after a commit/push

alice-i-cecile · 2022-05-05T17:14:16Z

tools/ci/src/main.rs

@@ -46,7 +46,7 @@ fn main() {
    if what_to_run.contains(Check::CLIPPY) {
        // See if clippy has any complaints.
        // - Type complexity must be ignored because we use huge templates for queries
-        cmd!("cargo clippy --workspace --all-targets --all-features -- -A clippy::type_complexity -W clippy::doc_markdown -D warnings")
+        cmd!("cargo clippy --workspace --all-targets --all-features -- -A clippy::type_complexity -W clippy::doc_markdown -W clippy::option_if_let_else -W clippy::redundant_else -W clippy::match_same_arms -W clippy::semicolon_if_nothing_returned -W clippy::explicit_iter_loop -W clippy::map_flatten -D warnings")


Can we use some string parsing to list out these lints in a cleaner fashion?

I tried many different ways but xshell (even updated to 0.2) keeps printing the flags between double quotes, so I gave up

I'm pretty sure that xshell doesn't do argument splitting outside cmd! macro, so we need to keep the flags in a vec and expand like cmd!("cargo clippy {args...}")

I tried that, and it still printed every flag between double quotes even though I'm sure I did exactly as the xshell doc does it

@infmagic2047 wether flags are stored in a Vec<&str>, a Vec<String>, or a [&str], const or in a let:

let flags = vec![ "-A clippy::type_complexity", "-W clippy::doc_markdown", "-W clippy::option_if_let_else", "-W clippy::redundant_else", "-W clippy::match_same_arms", "-W clippy::semicolon_if_nothing_returned", "-W clippy::explicit_iter_loop", "-W clippy::map_flatten", "-D warnings", ]; cmd!("cargo clippy --workspace --all-targets --all-features -- {flags...}")

This is the output:

cargo clippy --workspace --all-targets --all-features -- "-A clippy::type_complexity" "-W clippy::doc_markdown" "-W clippy::option_if_let_else" "-W clippy::redundant_else" "-W clippy::match_same_arms" "-W clippy::semicolon_if_nothing_returned" "-W clippy::explicit_iter_loop" "-W clippy::map_flatten" "-D warnings"

-A clippy::type_complexity is actually two arguments -A and clippy::type_complexity, so writing them as one won't work. You can either write them as two elements, or use the concatenated form -Aclippy::type_complexity.

Thanks that seems to work, committing now

alice-i-cecile · 2022-05-05T17:15:00Z

I like these lints; I think they add clarity and consistency without being tedious or frustrating.

The corresponding code changes are good: I think these are clear improvements in every case.

Added as a candidate to https://github.com/orgs/bevyengine/projects/4/views/1 to try to avoid bitrot.

alice-i-cecile · 2022-05-05T17:15:51Z

Note: this gets the Controversial tag because it's a project-wide change to style.

james7132

While I can agree with most of these changes, the use of map_or_else is a regression in terms of readability IMO. The use of two closures makes it visually hard to tell which branch is which.

mockersf · 2022-05-06T08:27:10Z

map_or and map_or_else are bad for readability as they have the map closure as second parameter

ManevilleF · 2022-05-06T11:49:26Z

I agree that in the case of large closures it's not great, I rwill ollback the changes and the restriction but should every change be rollbacked?

For example the changes in crates/bevy_ecs/src/schedule/run_criteria.rs

we go from:

if let Some(ref label) = self.label {
       std::slice::from_ref(label)
 } else {
       &[]
 }

to

self.label.as_ref().map_or(&[], std::slice::from_ref)

which to me is a clear improvement

ManevilleF · 2022-05-10T11:07:20Z

I reverted the changes from the first commit (clippy::option_if_let_else) and removed the clippy warning in the CI.

Every added map_or and map_or_else was reverted, I just left a few instances of map where I thought it was better. I can revert those changes if necessary.

@mockersf @alice-i-cecile

ManevilleF · 2022-05-10T11:22:03Z

Rebased on staging, fixed additional warnings

IceSentry · 2022-05-10T15:19:30Z

I'm not a huge fan of having redundant_else as a rule. I agree that it can sometimes be redundant, but in some situations I think it's clearer to show that each block are separate branches. I don't like that it's enforced in all situations. I haven't seen that many situations in this PR where the end result was necessarily worse, but I don't like enforcing that rule.

james7132

Less code is better code. This doubles as a great cleanup as well.

james7132 · 2022-05-24T00:48:18Z

I'm not a huge fan of having redundant_else as a rule. I agree that it can sometimes be redundant, but in some situations I think it's clearer to show that each block are separate branches. I don't like that it's enforced in all situations. I haven't seen that many situations in this PR where the end result was necessarily worse, but I don't like enforcing that rule.

I personally like it because it just avoids the pressure to push code to the right, particularly if the code block is large.

ManevilleF · 2022-05-24T08:01:29Z

Rebased on staging and fixed additional warnings. Maybe I should change the MR target to staging instead of main ? I'm not sure what the role of staging is

@IceSentry I don't think having a redundant else (meaning you return in the if) adds clarity, but maybe enforcing that rule is a bit much.

@james7132 Thanks for the review

IceSentry · 2022-05-24T13:53:06Z

So apparently I was misunderstanding the redundant_else lint. I thought it would complain about code like this

fn foo(input: i32) -> String {
    if input == 42 {
        String::from("The answer!")
    } else {
        String::from("Wrong answerr")
    }
}

to turn it into this

fn foo(input: i32) -> String {
    if input == 42 {
        return String::from("The answer!");
    }
    String::from("Wrong")
}

just to avoid the last else, but that's not actually what it warns about. It will only warn if the else can be removed without adding or removing a return.

I still think it shouldn't be forced through a lint, but I'm not as opposed to it as I originally was.

cart · 2022-05-31T01:37:44Z

Just resolved conflicts, rebased, and removed our manual allow(type_complexity) cases, now that they are allowed by default.

cart · 2022-05-31T01:37:51Z

bors r+

# Objective Follow up to my previous MR #3718 to add new clippy warnings to bevy: - [x] [~~option_if_let_else~~](https://rust-lang.github.io/rust-clippy/master/#option_if_let_else) (reverted) - [x] [redundant_else](https://rust-lang.github.io/rust-clippy/master/#redundant_else) - [x] [match_same_arms](https://rust-lang.github.io/rust-clippy/master/#match_same_arms) - [x] [semicolon_if_nothing_returned](https://rust-lang.github.io/rust-clippy/master/#semicolon_if_nothing_returned) - [x] [explicit_iter_loop](https://rust-lang.github.io/rust-clippy/master/#explicit_iter_loop) - [x] [map_flatten](https://rust-lang.github.io/rust-clippy/master/#map_flatten) There is one commit per clippy warning, and the matching flags are added to the CI execution. To test the CI execution you may run `cargo run -p ci -- clippy` at the root. I choose the add the flags in the `ci` tool crate to avoid having them in every `lib.rs` but I guess it could become an issue with suprise warnings coming up after a commit/push Co-authored-by: Carter Anderson <mcanders1@gmail.com>

bors · 2022-05-31T01:55:46Z

Pull request successfully merged into main.

Build succeeded:

# Objective Follow up to my previous MR bevyengine#3718 to add new clippy warnings to bevy: - [x] [~~option_if_let_else~~](https://rust-lang.github.io/rust-clippy/master/#option_if_let_else) (reverted) - [x] [redundant_else](https://rust-lang.github.io/rust-clippy/master/#redundant_else) - [x] [match_same_arms](https://rust-lang.github.io/rust-clippy/master/#match_same_arms) - [x] [semicolon_if_nothing_returned](https://rust-lang.github.io/rust-clippy/master/#semicolon_if_nothing_returned) - [x] [explicit_iter_loop](https://rust-lang.github.io/rust-clippy/master/#explicit_iter_loop) - [x] [map_flatten](https://rust-lang.github.io/rust-clippy/master/#map_flatten) There is one commit per clippy warning, and the matching flags are added to the CI execution. To test the CI execution you may run `cargo run -p ci -- clippy` at the root. I choose the add the flags in the `ci` tool crate to avoid having them in every `lib.rs` but I guess it could become an issue with suprise warnings coming up after a commit/push Co-authored-by: Carter Anderson <mcanders1@gmail.com>

github-actions bot added the S-Needs-Triage This issue needs to be labelled label May 5, 2022

ManevilleF marked this pull request as ready for review May 5, 2022 12:50

alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change X-Controversial There is active debate or serious implications around merging this PR and removed S-Needs-Triage This issue needs to be labelled labels May 5, 2022

alice-i-cecile approved these changes May 5, 2022

View reviewed changes

james7132 requested changes May 6, 2022

View reviewed changes

ManevilleF force-pushed the feat/clippy_restrictions branch from bc9bce2 to 1ad1b3d Compare May 10, 2022 11:21

james7132 approved these changes May 24, 2022

View reviewed changes

james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 24, 2022

ManevilleF force-pushed the feat/clippy_restrictions branch from 1ad1b3d to 7ab1bb2 Compare May 24, 2022 07:57

ManevilleF force-pushed the feat/clippy_restrictions branch 2 times, most recently from bae6e2c to 9cea556 Compare May 25, 2022 09:40

ManevilleF added 7 commits May 30, 2022 18:28

clippy::option_if_let_else

bb66cd3

clippy::redundant_else

8733dcb

clippy::match_same_arms

f4e0b96

clippy::semicolon_if_nothing_returned

f743d19

clippy::explicit_iter_loop

9dec066

clippy::map_flatten

52ab6e2

Revert clippy::option_if_let_selse

7111b9e

ManevilleF and others added 5 commits May 30, 2022 18:33

const clippy flags in CI

5fb9d9a

Rebased on staging, fixed additionnal warnings

6930dad

Rebased on staging

da5bb36

remove manual type_complexity allows

79b1a5c

fmt

4227dbf

cart force-pushed the feat/clippy_restrictions branch from 9cea556 to 4227dbf Compare May 31, 2022 01:37

bors bot changed the title ~~Clippy improvements~~ [Merged by Bors] - Clippy improvements May 31, 2022

bors bot closed this May 31, 2022

ManevilleF deleted the feat/clippy_restrictions branch May 31, 2022 06:35

tim-blackbird mentioned this pull request Jun 2, 2022

[Merged by Bors] - Add methods for querying lists of entities. #4879

Closed

rparrett mentioned this pull request Sep 9, 2022

Docs tell contributors to run clippy with incomplete set of lints. bevyengine/bevy-website#458

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Clippy improvements #4665

[Merged by Bors] - Clippy improvements #4665

ManevilleF commented May 5, 2022 •

edited

Loading

alice-i-cecile May 5, 2022

ManevilleF May 5, 2022

infmagic2047 May 6, 2022

ManevilleF May 6, 2022

ManevilleF May 6, 2022

infmagic2047 May 6, 2022

ManevilleF May 10, 2022

alice-i-cecile commented May 5, 2022

alice-i-cecile commented May 5, 2022

james7132 left a comment

mockersf commented May 6, 2022 •

edited

Loading

ManevilleF commented May 6, 2022 •

edited

Loading

ManevilleF commented May 10, 2022

ManevilleF commented May 10, 2022

IceSentry commented May 10, 2022 •

edited

Loading

james7132 left a comment

james7132 commented May 24, 2022

ManevilleF commented May 24, 2022

IceSentry commented May 24, 2022

cart commented May 31, 2022

cart commented May 31, 2022

bors bot commented May 31, 2022

[Merged by Bors] - Clippy improvements #4665

[Merged by Bors] - Clippy improvements #4665

Conversation

ManevilleF commented May 5, 2022 • edited Loading

Objective

alice-i-cecile May 5, 2022

Choose a reason for hiding this comment

ManevilleF May 5, 2022

Choose a reason for hiding this comment

infmagic2047 May 6, 2022

Choose a reason for hiding this comment

ManevilleF May 6, 2022

Choose a reason for hiding this comment

ManevilleF May 6, 2022

Choose a reason for hiding this comment

infmagic2047 May 6, 2022

Choose a reason for hiding this comment

ManevilleF May 10, 2022

Choose a reason for hiding this comment

alice-i-cecile commented May 5, 2022

alice-i-cecile commented May 5, 2022

james7132 left a comment

Choose a reason for hiding this comment

mockersf commented May 6, 2022 • edited Loading

ManevilleF commented May 6, 2022 • edited Loading

ManevilleF commented May 10, 2022

ManevilleF commented May 10, 2022

IceSentry commented May 10, 2022 • edited Loading

james7132 left a comment

Choose a reason for hiding this comment

james7132 commented May 24, 2022

ManevilleF commented May 24, 2022

IceSentry commented May 24, 2022

cart commented May 31, 2022

cart commented May 31, 2022

bors bot commented May 31, 2022

ManevilleF commented May 5, 2022 •

edited

Loading

mockersf commented May 6, 2022 •

edited

Loading

ManevilleF commented May 6, 2022 •

edited

Loading

IceSentry commented May 10, 2022 •

edited

Loading