Clippy Pedantic by dcampbell24 · Pull Request #6720 · uutils/coreutils

dcampbell24 · 2024-09-20T22:11:31Z

I ran cargo clippy --workspace --all-targets --all-features -- --warn clippy::LINT for all the lints listed in the below thread. I can't figure out what is different about running clippy through the file pre-commit-config.yaml, but it gives different results. I fixed all the lints in the below listed thread except for clippy::match_same_arm, clippy::redundant_else, clippy::single_match_else, clippy::unnecessary_wraps, and clippy::unnested_or_patterns. They seemed either unlike they should be fixed or hard to fix.

This works on #4949.

dcampbell24 · 2024-09-20T22:29:33Z

I don't have a windows machine and don't understand why the windows checks are failing.

github-actions · 2024-09-20T22:38:37Z

GNU testsuite comparison:

GNU test failed: tests/timeout/timeout. tests/timeout/timeout is passing on 'main'. Maybe you have to rebase?

sylvestre · 2024-09-21T07:01:45Z

src/uu/chroot/src/chroot.rs


    let commands = match matches.get_many::<String>(options::COMMAND) {
-        Some(v) => v.map(|s| s.as_str()).collect(),
+        Some(v) => v.map(std::string::String::as_str).collect(),


i would prefer just
String::as_str
and have an import

repeating std::string isn't providing much value

sylvestre · 2024-09-21T07:02:06Z

src/uu/cp/src/copydir.rs

        let root_path = current_dir.join(root);
        let root_parent = if target.exists() && !root.to_str().unwrap().ends_with("/.") {
-            root_path.parent().map(|p| p.to_path_buf())
+            root_path.parent().map(std::path::Path::to_path_buf)


same comment about Path

Suggested change

root_path.parent().map(std::path::Path::to_path_buf)

root_path.parent().map(Path::to_path_buf)

sylvestre · 2024-09-21T07:03:43Z

For the windows error, you can test using MS images:
https://developer.microsoft.com/windows/downloads/virtual-machines/

Here, for this error, the type is probably different on windows vs linux

 error[E0631]: type mismatch in function arguments
   --> src\uu\env\src\native_int_str.rs:268:48
    |
268 |             |o| o.strip_prefix(&*n_prefix).map(<[u8]>::to_vec).ok_or(()),
    |                                            --- ^^^^^^^^^^^^^^
    |                                            |   |
    |                                            |   expected due to this
    |                                            |   found signature defined here
    |                                            required by a bound introduced by this call
    |
    = note: expected function signature `fn(&[u16]) -> _`
               found function signature `fn(&[u8]) -> _`
note: required by a bound in `std::option::Option::<T>::map`

sylvestre · 2024-09-21T07:05:46Z

src/uu/fmt/src/fmt.rs

            (Some(w), None) => {
                // Only allow a goal of zero if the width is set to be zero
-                let g = (w * DEFAULT_GOAL_TO_WIDTH_RATIO / 100).max(if w == 0 { 0 } else { 1 });
+                let g = (w * DEFAULT_GOAL_TO_WIDTH_RATIO / 100).max(usize::from(w != 0));


not sure to see the improvement here

Maybe this should be rephrased anyway? Something like

let computed_goal = w * DEFAULT_GOAL_TO_WIDTH_RATIO / 100; let g = if computed_goal > 0 { computed_goal } else if width == 0 { 0 } else { 1 }

This is probably not yet ideal, but it might make the intent clearer.

tertsdiepraam

Interesting! Thanks! I like most of this, especially the format string improvements and the underscores in literals. I also think that it's important to consider that these are pedantic lints for a reason: they are more subjective than others. For instance, I dislike the map_or lint in complex expressions, because I think it's easier to read the the map and unwrap parts as separate calls. I also agree with Sylvestre that the paths should be shortened for the map calls.

Here are the commits I'd definitely accept:

manual_assert
uninlined_format_args
unnecessary_join
unreadable_literal
inconsistent_struct_literal
cloned_instead_of_copied
flat_map_option

The more difficult cases that we should maybe split off into separate PRs for more discussion are:

format_collect
bool_to_int_with_if
inefficient_to_string
map_unwrap_or
redundant_closure_for_method_calls
used_underscore_binding (I agree with the lint, but maybe it can be fixed without allows)
map_or_else

I think splitting this up makes it much easier to discuss. Hope that makes sense!

tertsdiepraam · 2024-09-21T09:47:41Z

src/uu/df/src/filesystem.rs

        };
        #[cfg(unix)]
+        #[allow(clippy::used_underscore_binding)]
        let usage = FsUsage::new(statfs(_stat_path).ok()?);


Is there are reason to keep this as an underscore?

If I change it to not an underscore, then I would have to add #[allow(unused_variables)] to the windows side of things.

Oh. in this case I was worried about the possibility of other #[cfg(system)]s existing, like one for browsers, or some other unknown system.

If that's not the case, then why isn't it #[cfg(unix)] and #[cfg(not(unix))].

Probably should be yeah!

tertsdiepraam · 2024-09-21T09:55:41Z

src/uu/fmt/src/fmt.rs

            (Some(w), None) => {
                // Only allow a goal of zero if the width is set to be zero
-                let g = (w * DEFAULT_GOAL_TO_WIDTH_RATIO / 100).max(if w == 0 { 0 } else { 1 });
+                let g = (w * DEFAULT_GOAL_TO_WIDTH_RATIO / 100).max(usize::from(w != 0));


Maybe this should be rephrased anyway? Something like

let computed_goal = w * DEFAULT_GOAL_TO_WIDTH_RATIO / 100; let g = if computed_goal > 0 { computed_goal } else if width == 0 { 0 } else { 1 }

This is probably not yet ideal, but it might make the intent clearer.

tertsdiepraam · 2024-09-21T10:00:26Z

src/uu/fmt/src/parasplit.rs

+                    .opts
+                    .prefix
+                    .as_ref()
+                    .map_or(0, std::string::String::len)..]


Maybe we can extract this to a variable:

let prefix_len = self.opts.prefix.as_ref().map_or(0, String::len);

I think that makes the intent and the formatting clearer and allows us to reuse the value.

tertsdiepraam · 2024-09-21T10:01:44Z

src/uu/head/src/parse.rs


    fn obsolete_result(src: &[&str]) -> Option<Result<Vec<String>, ParseError>> {
-        Some(Ok(src.iter().map(|s| s.to_string()).collect()))
+        Some(Ok(src.iter().map(|s| (*s).to_string()).collect()))


What clippy lint triggered this? It seems to make it more complicted.

inefficient_to_string the compiler specializes the to_string in the changed case: https://rust-lang.github.io/rust-clippy/master/index.html#/to_string.

dcampbell24 · 2024-09-21T20:10:13Z

Do you suggest an easy way to split the commits up? I looked online and there were a few different suggestions including copy and pasting. Sorry for being unexperienced in git.

sylvestre · 2024-09-21T20:15:03Z

make a backup and:
git rebase -i origin/main
and dropped the various patches

dcampbell24

I want to get my comments out of "Pending" status.

dcampbell24 · 2024-09-21T19:43:08Z

src/uu/df/src/filesystem.rs

        };
        #[cfg(unix)]
+        #[allow(clippy::used_underscore_binding)]
        let usage = FsUsage::new(statfs(_stat_path).ok()?);


If that's not the case, then why isn't it #[cfg(unix)] and #[cfg(not(unix))].

dcampbell24 · 2024-09-21T19:53:16Z

src/uu/head/src/parse.rs


    fn obsolete_result(src: &[&str]) -> Option<Result<Vec<String>, ParseError>> {
-        Some(Ok(src.iter().map(|s| s.to_string()).collect()))
+        Some(Ok(src.iter().map(|s| (*s).to_string()).collect()))


inefficient_to_string the compiler specializes the to_string in the changed case: https://rust-lang.github.io/rust-clippy/master/index.html#/to_string.

BenWiederhake · 2024-09-23T21:39:00Z

I can't review this, as this touches way too many files at once. Consider splitting this PR into several smaller PRs. That's my personal opinion, and not coordinated with sylvestre or tertsdiepraam, who seem to take no issue with this.

dcampbell24 · 2024-09-23T21:47:18Z

tertsdiepraam said to split it up too and that he only found some of the changes agreeable without further consideration. I am going to split up the PR based on his feedback, I just haven't gotten to it yet.

BenWiederhake · 2024-09-23T22:22:23Z

Ah, sorry for my duplication then. Awesome!

dcampbell24 · 2024-10-13T22:38:20Z

This was automatically closed when I tried to force push rebasing and merging with main.

sylvestre reviewed Sep 21, 2024

View reviewed changes

tertsdiepraam reviewed Sep 21, 2024

View reviewed changes

dcampbell24 commented Sep 21, 2024

View reviewed changes

BenWiederhake mentioned this pull request Sep 24, 2024

CI: GNU test "timeout" is flaky #6732

Open

dcampbell24 mentioned this pull request Sep 25, 2024

Clippy Pedantic Acceptable Commits #6738

Closed

cakebaker changed the title ~~Clippy Pendantic~~ Clippy Pedantic Oct 4, 2024

dcampbell24 closed this Oct 13, 2024

dcampbell24 force-pushed the fix-clippy branch from 418cd10 to 3c26dd8 Compare October 13, 2024 21:39

Qelxiros mentioned this pull request May 17, 2025

clippy: fix clippy::unnecessary_debug_formatting and clippy::io_other_error, adjusting cp tests to align with GNU #7941

Closed

gabelluardo mentioned this pull request Jun 16, 2025

Prepare for clippy pedantic rules #8201

Merged

	root_path.parent().map(std::path::Path::to_path_buf)
	root_path.parent().map(Path::to_path_buf)

Uh oh!

Conversation

dcampbell24 commented Sep 20, 2024

Uh oh!

dcampbell24 commented Sep 20, 2024

Uh oh!

github-actions bot commented Sep 20, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sylvestre commented Sep 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tertsdiepraam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcampbell24 Sep 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcampbell24 commented Sep 21, 2024

Uh oh!

sylvestre commented Sep 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcampbell24 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenWiederhake commented Sep 23, 2024

Uh oh!

dcampbell24 commented Sep 23, 2024

Uh oh!

BenWiederhake commented Sep 23, 2024

Uh oh!

dcampbell24 commented Oct 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dcampbell24 Sep 21, 2024 •

edited

Loading

sylvestre commented Sep 21, 2024 •

edited

Loading

dcampbell24 commented Oct 13, 2024 •

edited

Loading