Clippy Pendantic #6720

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

@@ -63,7 +63,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
    }

    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

@@ -99,7 +99,7 @@ impl<'a> Context<'a> {
        let current_dir = env::current_dir()?;
        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

@@ -112,7 +112,7 @@ impl FmtOptions {
            }
            (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

@@ -107,13 +107,15 @@ impl Filesystem {
            mount_info.mount_dir.clone()
        };
        #[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

@@ -112,7 +112,7 @@ impl FmtOptions {
            }
            (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

@@ -149,7 +149,7 @@ mod tests {
    }

    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

@@ -107,13 +107,15 @@ impl Filesystem {
            mount_info.mount_dir.clone()
        };
        #[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

@@ -149,7 +149,7 @@ mod tests {
    }

    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.

dcampbell24 added 14 commits September 19, 2024 16:31

Fix clippy::manual_assert.

5d986bf

Fix clippy::uninlined_format_args .

353eb53

Fix clippy::unnecessary_join.

feae527

Fix clippy::unreadable_literal.

cff8cc0

Fix clippy::format_collect.

c8b4045

Fix clippy::inconsistent_struct_constructor.

d7602b2

Fix clippy::bool_to_int_with_if.

7a0d7c0

Fix clippy::cloned_instead_of_copied.

961d414

Fix clippy::flat_map_option.

6e35c73

Fix clippy::inefficient_to_string.

7b98853

Fix clippy::map_unwrap_or.

ff38e8f

Fix clippy::redundant_closure_for_method_calls.

6dbc715

Fix clippy::used_underscore_binding.

dfa2322

Fix clippy::map_unwrap_or.

418cd10

sylvestre reviewed Sep 21, 2024

View reviewed changes

tertsdiepraam reviewed Sep 21, 2024

View reviewed changes

dcampbell24 commented Sep 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clippy Pendantic #6720

Clippy Pendantic #6720

dcampbell24 commented Sep 20, 2024

dcampbell24 commented Sep 20, 2024

github-actions bot commented Sep 20, 2024

sylvestre Sep 21, 2024

sylvestre Sep 21, 2024

sylvestre commented Sep 21, 2024

sylvestre Sep 21, 2024

tertsdiepraam Sep 21, 2024

tertsdiepraam left a comment

tertsdiepraam Sep 21, 2024

dcampbell24 Sep 21, 2024

dcampbell24 Sep 21, 2024 •

edited

Loading

dcampbell24 Sep 21, 2024

tertsdiepraam Sep 22, 2024

tertsdiepraam Sep 21, 2024

tertsdiepraam Sep 21, 2024

tertsdiepraam Sep 21, 2024

dcampbell24 Sep 21, 2024

dcampbell24 commented Sep 21, 2024

sylvestre commented Sep 21, 2024 •

edited

Loading

dcampbell24 left a comment

dcampbell24 Sep 21, 2024

dcampbell24 Sep 21, 2024

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

Clippy Pendantic #6720

Are you sure you want to change the base?

Clippy Pendantic #6720

Conversation

dcampbell24 commented Sep 20, 2024

dcampbell24 commented Sep 20, 2024

github-actions bot commented Sep 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sylvestre commented Sep 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tertsdiepraam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcampbell24 Sep 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcampbell24 commented Sep 21, 2024

sylvestre commented Sep 21, 2024 • edited Loading

dcampbell24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcampbell24 Sep 21, 2024 •

edited

Loading

sylvestre commented Sep 21, 2024 •

edited

Loading