Clippy fixes by ManevilleF · Pull Request #3718 · bevyengine/bevy

ManevilleF · 2022-01-18T14:43:35Z

Objective

Fixes some nursery and pedantic clippy warnings like

clippy::missing_const_fn
clippy::option_if_let_else
clippy::use_self
clippy::redundant_else
clippy::match_same_arms
clippy::semicolon_if_returned
clippy::explicit_iter_loop
clippy::cloned_instead_of_copied
clippy::map_flatten

Solution

I applied many small changes accross all crates.

MinerSebas

There were several places where an fn was made const, that can't be actually called in a const context as the Type they are implemented for can only be constructed at runtime.
Please revert those functions or try to get a const-constructor.

Also please when change as many Files/Places as here, make 1. Commit for every clippy Lint. this makes it much easier to review each individual Lint.

crates/bevy_asset/src/loader.rs

crates/bevy_diagnostic/src/diagnostic.rs

crates/bevy_ecs/src/archetype.rs

crates/bevy_core/src/time/timer.rs

ManevilleF · 2022-01-18T16:02:41Z

@MinerSebas const functions don't require a const context, there is no need for const constructors as it just allows the evaluation of the function at compile time.
Is there maybe a convention regarding const functions that I'm not aware of?

mockersf · 2022-01-18T17:03:32Z

A const function can be evaluated at compile time, and will if all its parameters are const. In the cases raised by MinerSebas, there will never be a self that is const, so even if the functions are marked, they will never be evaluated at compile time.

Adding const here won't change anything for now, but it's an extra constraint on how Bevy may evolve as they may need to not be const later on and the person may be hesitant to remove it.

alice-i-cecile · 2022-01-18T18:01:41Z

Some of this work was duplicated in #2934. I think I prefer the clarity and freshness of this PR, if we're picking one to merge.

ManevilleF · 2022-01-19T09:18:10Z

Thanks @mockersf (Merci François !) I'll apply the suggestions of @MinerSebas and remove the extra const I added

@alice-i-cecile I added more clippy fixes like clippy::semicolon_if_nothing_is_returned and clippy::explicit_iter_loop which are in the similar PR.

Should I add to the CI config or the lib.rs files the now fixed clippy warnings to avoid them in the future?

ManevilleF · 2022-01-19T09:40:29Z

crates/bevy_ecs/src/world/mod.rs


    /// Returns an iterator of entities that had components of type `T` removed
    /// since the last call to [`World::clear_trackers`].
+    // TODO: return `std::iter::Copied` instead


I think we should return a std::iter::Copied here but I don't know if it would be a breaking change or not

ManevilleF · 2022-01-19T09:40:34Z

crates/bevy_ecs/src/world/mod.rs


    /// Returns an iterator of entities that had components with the given `component_id` removed
    /// since the last call to [`World::clear_trackers`].
+    // TODO: return `std::iter::Copied` instead


I think we should return a std::iter::Copied here but I don't know if it would be a breaking change or not

alice-i-cecile · 2022-01-19T17:23:41Z

Yeah, if we're going to fix pedantic / nursery lints we should toggle them on in CI so they stay fixed.

mockersf · 2022-01-19T19:02:50Z

Yeah, if we're going to fix pedantic / nursery lints we should toggle them on in CI so they stay fixed.

To be transparent, this is exactly why I think most of those lints should not be fixed. Adding more things to check will slow down development. Some add enough values to be worth it, but that's not the case of most of the pedantic lints.

Also, the pedantic lints are often quite... pedantic and a matter of taste. Some of them are fine, I disagree with a few, and some should be done only sometimes - doing them by a lint remove sense from the code.

ManevilleF · 2022-01-20T08:40:46Z

@mockersf I agree, many pedantic lints are not worth it.
I only fixed a few that I think are good practice or useful and ignored the others.

alice-i-cecile · 2022-04-22T06:24:26Z

@ManevilleF do you want to rebase this? If so we can merge it in ASAP after rebasing; otherwise I think we should just close it out to reduce merge conflicts.

ManevilleF · 2022-04-22T07:35:05Z

As discussed I will close this and open it again from the main branch with the warnings enabled on the CI.

@mockersf @cart I think the current list of warnings is a good start:

    clippy::option_if_let_else
    clippy::use_self
    clippy::redundant_else
    clippy::match_same_arms
    clippy::semicolon_if_returned
    clippy::explicit_iter_loop
    clippy::cloned_instead_of_copied
    clippy::map_flatten

Do you have more warnings you wish to add or some to remove?

alice-i-cecile · 2022-04-22T08:45:48Z

List of lints, for easy reference.

alice-i-cecile · 2022-04-22T08:54:10Z

Personal opinions:

option_if_let_else: genuinely clearer.
use_self: actively dislike this. See use Self keyword #4152.
redundant_else: seems nice
match_same_arms: nice clarity win
semicolon_if_nothing_returned: presuming this is what you meant? Seems like a minor improvement.
explicit_iter_loop: no strong feelings
cloned_instead_of_copied: eh sure why not
map_flatten: I like it! Much clearer.

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

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

Clippy fixes

3861fbd

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

Fixed CI

75b04be

ManevilleF changed the title ~~Clippy fixes~~ Draft: Clippy fixes Jan 18, 2022

ManevilleF added 3 commits January 18, 2022 16:14

Fixed some redundant match arms

fa3de67

Fixed some redundant else blocks

cb85153

Additional clippy fixes

f7f9cf2

ManevilleF changed the title ~~Draft: Clippy fixes~~ Clippy fixes Jan 18, 2022

Fixed color factoring mix up

43274ee

MinerSebas suggested changes Jan 18, 2022

View reviewed changes

mockersf added C-Code-Quality A section of code that is hard to understand or change and removed S-Needs-Triage This issue needs to be labelled labels Jan 18, 2022

ManevilleF added 2 commits January 19, 2022 09:57

clippy::explicit_iter_loop

7e2085d

clippy::semicolon_if_nothing_is_returned

7d6d12b

ManevilleF added 2 commits January 19, 2022 10:20

Fixed archetypes issue

36555fd

clippy::cloned_instead_of_copied and clippy::map_flatten

ea9b9ec

ManevilleF commented Jan 19, 2022

View reviewed changes

PR requested changes: Rollback most of const methods

db8190c

Merge branch 'main' into main

7de6cd4

ManevilleF closed this Apr 22, 2022

ManevilleF mentioned this pull request May 5, 2022

[Merged by Bors] - Clippy improvements #4665

Closed

6 tasks

Uh oh!

Conversation

ManevilleF commented Jan 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Uh oh!

MinerSebas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ManevilleF commented Jan 18, 2022

Uh oh!

mockersf commented Jan 18, 2022

Uh oh!

alice-i-cecile commented Jan 18, 2022

Uh oh!

ManevilleF commented Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ManevilleF Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

ManevilleF Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile commented Jan 19, 2022

Uh oh!

mockersf commented Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ManevilleF commented Jan 20, 2022

Uh oh!

alice-i-cecile commented Apr 22, 2022

Uh oh!

ManevilleF commented Apr 22, 2022

Uh oh!

alice-i-cecile commented Apr 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alice-i-cecile commented Apr 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ManevilleF commented Jan 18, 2022 •

edited

Loading

ManevilleF commented Jan 19, 2022 •

edited

Loading

mockersf commented Jan 19, 2022 •

edited

Loading

alice-i-cecile commented Apr 22, 2022 •

edited

Loading