clippy #740

mightyiam · 2023-08-27T13:54:55Z

Co-authored-by: warren2k 846021+warren2k@users.noreply.github.com

mightyiam · 2023-08-27T15:35:52Z

This is based on top of #720 . I suggest reviewing that one, first.

mightyiam · 2023-09-09T14:52:10Z

Rebased. Please approve workflow and review. Reminder: this is based on top of #720.

mightyiam · 2023-09-10T14:08:25Z

Our last workflow run failed. Force-pushed an attempt to resolve the issue. Please approve another workflow run.

jswrenn · 2023-09-15T14:26:13Z

.github/workflows/ci.yml

@@ -35,7 +35,8 @@ jobs:
      - uses: dtolnay/rust-toolchain@master
        with:
          toolchain: ${{ matrix.rust }}
-      - run: cargo check ${{ matrix.features }}
+          components: ${{ matrix.rust == 'stable' && 'clippy' || '' }}


Let's only run clippy on the msrv toolchain. Clippy recommendations that violate msrv are not actionable.

Those could be allowed and reviewed on MSRV bumps? I'd imagine there's a lot of good lints between MSRV and stable.

Your valid concern is that some clippy lints would suggest changes that include syntax or items that are not available in the minimum supported Rust version (MSRV).

Option A

In CI use MSRV clippy.

Those who have a more recent toolchain in their editor/IDE ("everyone") may be shown warnings from clippy lints that cannot be acted upon due to MSRV. Degraded developer experience.

Of course, those could be #[allow()]ed in order to improve the developer experience, but allowing those would not be enforced by CI.

And we'd have to invoke MSRV clippy in CI with --allow clippy::unknown_clippy_lints, allowing the unknown clippy lints inside of #[allow()].

Option B

In CI use stable clippy.

For the lints that would not be actionable in MSRV, #[allow()] them.
This would be enforced in CI. Better developer experience.

It seems that the lint unknown_lints does not apply to lints under the namespace clippy (clippy::*).

When bumping MSRV, go through all the allowed clippy lints in the codebase and examine which ones can now be acted upon.

It seems that option B would probably result in a better developer experience.

@jswrenn again, we recommend option B. Call it and we'll do it.

jswrenn · 2023-09-15T14:27:46Z

wrappers.rs

+//! This module helps suppress two kinds of warnings: `deprecated` and `unstable_name_collisions`.
+//! New items are created that are noop-wrappers of the original items.
+//! The items' original paths are preserved.
+
+use itertools::Itertools;
+
+pub mod free {
+    // it seems the compiler is not able to discern that this is used
+    #[allow(dead_code)]
+    pub fn zip<I, J>(i: I, j: J) -> core::iter::Zip<I::IntoIter, J::IntoIter>
+        where I: IntoIterator,
+              J: IntoIterator
+    {
+        #[allow(deprecated)]
+        itertools::free::zip(i, j)
+    }
+}
+
+pub trait Ext: Itertools {
+    fn intersperse_wrap(self, element: Self::Item) -> itertools::Intersperse<Self>
+    where
+        Self: Sized,
+        Self::Item: Clone,
+    {
+        <Self as Itertools>::intersperse(self, element)
+    }
+
+    fn fold1_wrap<F>(self, f: F) -> Option<Self::Item>
+        where F: FnMut(Self::Item, Self::Item) -> Self::Item,
+              Self: Sized,
+    {
+        #[allow(deprecated)]
+        <Self as Itertools>::fold1(self, f)
+    }
+}
+
+impl<T: Itertools> Ext for T {}
+


As I wrote on #720, I'm not keen on the wrapper pattern to suppress warnings.

mightyiam · 2023-09-17T15:01:42Z

Could you please approve workflow? This isn't ready, though.

mightyiam · 2023-08-27T15:40:13Z

benches/bench1.rs

+
+    #[allow(clippy::explicit_counter_loop)]
    for (_, elt) in data1.iter_mut().enumerate() {


Benchmarks are kind of weird code, anyway.
https://rust-lang.github.io/rust-clippy/master/index.html#/explicit_counter_loop

benches/bench1.rs

benches/extra/zipslices.rs

benches/fold_specialization.rs

examples/iris.rs

tests/quick.rs

tests/test_std.rs

src/lib.rs

mightyiam · 2023-09-15T15:39:50Z

.github/workflows/ci.yml

@@ -35,7 +35,8 @@ jobs:
      - uses: dtolnay/rust-toolchain@master
        with:
          toolchain: ${{ matrix.rust }}
-      - run: cargo check ${{ matrix.features }}
+          components: ${{ matrix.rust == 'stable' && 'clippy' || '' }}


Those could be allowed and reviewed on MSRV bumps? I'd imagine there's a lot of good lints between MSRV and stable.

mightyiam · 2023-09-24T15:39:21Z

.github/workflows/ci.yml

@@ -35,7 +35,8 @@ jobs:
      - uses: dtolnay/rust-toolchain@master
        with:
          toolchain: ${{ matrix.rust }}
-      - run: cargo check ${{ matrix.features }}
+          components: ${{ matrix.rust == 'stable' && 'clippy' || '' }}


Your valid concern is that some clippy lints would suggest changes that include syntax or items that are not available in the minimum supported Rust version (MSRV).

Option A

In CI use MSRV clippy.

Those who have a more recent toolchain in their editor/IDE ("everyone") may be shown warnings from clippy lints that cannot be acted upon due to MSRV. Degraded developer experience.

Of course, those could be #[allow()]ed in order to improve the developer experience, but allowing those would not be enforced by CI.

And we'd have to invoke MSRV clippy in CI with --allow clippy::unknown_clippy_lints, allowing the unknown clippy lints inside of #[allow()].

Option B

In CI use stable clippy.

For the lints that would not be actionable in MSRV, #[allow()] them.
This would be enforced in CI. Better developer experience.

It seems that the lint unknown_lints does not apply to lints under the namespace clippy (clippy::*).

When bumping MSRV, go through all the allowed clippy lints in the codebase and examine which ones can now be acted upon.

It seems that option B would probably result in a better developer experience.

jswrenn · 2023-10-08T00:47:02Z

Alright, let's do clippy on stable. I don't like the idea that our CI might spontaneously break upon new Rust releases, but hopefully that's rare.

mightyiam · 2024-01-07T12:29:33Z

Looks like conflicts. I'll see what I can do.

Philippe-Cholet · 2024-01-07T13:43:54Z

I don't know if we will apply CI changes, I leave that decision to jswrenn but I sure would like to fix lints.

jswrenn · 2024-01-07T14:52:48Z

Whoops, I must have missed when this PR was marked ready for review. I'm still fine running clippy with the stable toolchain.

Co-authored-by: warren2k <846021+warren2k@users.noreply.github.com>

mightyiam

Rebased and resolved 1.75.0 lints.

mightyiam · 2024-01-08T03:17:04Z

benches/bench1.rs

+
+    #[allow(clippy::explicit_counter_loop, clippy::unused_enumerate_index)]


I'm not sure what this benchmark is about. I'm only guessing that calling enumerate and then ignoring the index is intentional. Same goes for the similar ones below.

@mightyiam After merging this PR, I search for PRs/issues about clippy (and closed some), saw #618 and eventually ran cargo clippy --all-targets and got clippy::unused_enumerate_index as "unknown lint" (yet I found it online). What am I missing here?
And second thing, we don't run clippy with --all-target and I'm wondering why, I don't see any discussion about it.

I think we run clippy four times because of a matrix value features. One of the is --all-targets --all-features.

What version toolchain did you get an error with and what is the error exactly, please?

Ahh! unused_enumerate_index is added in 1.75 and I'm still on 1.74, my bad sorry! I'm so used on thinking I can't use recent features of Rust I did not thought it could be such a recent lint.
And another mistake of mine on matrix features. Ok, thanks!

mightyiam · 2024-01-08T03:20:15Z

src/either_or_both.rs

@@ -500,6 +494,7 @@ impl<T> EitherOrBoth<T, T> {
    }
 }

+#[allow(clippy::from_over_into)]


Does anyone know why we're implementing Into instead of From?

I don't see a point of not doing it so I suggest you convert it to a From implementation in a separate commit.

EDIT: After searching, it originated in #327 and nobody said it should not be From instead.

Here's an issue for that.

Now that shouldn't block this PR?

I sure want this PR to be merged as soon as possible as you made quite a work here!

src/multipeek_impl.rs

src/tuple_impl.rs

mightyiam · 2024-01-08T03:27:19Z

src/either_or_both.rs

-        match *self {
-            Left(_) => true,
-            _ => false,
-        }
+        matches!(*self, Left(_))


https://rust-lang.github.io/rust-clippy/master/index.html#/match_like_matches_macro

tests/quick.rs

tests/test_std.rs

Philippe-Cholet

Thanks for updating your work.

Philippe-Cholet · 2024-01-08T08:04:29Z

src/either_or_both.rs

@@ -500,6 +494,7 @@ impl<T> EitherOrBoth<T, T> {
    }
 }

+#[allow(clippy::from_over_into)]


I don't see a point of not doing it so I suggest you convert it to a From implementation in a separate commit.

EDIT: After searching, it originated in #327 and nobody said it should not be From instead.

Philippe-Cholet · 2024-01-08T13:06:02Z

So we chose "Option B" from your #740 (comment) in which you mention something interesting.

When bumping MSRV, go through all the allowed clippy lints in the codebase and examine which ones can now be acted upon.

I think we might easily forgot that so I think that we should have a comment on the "rust-version" line in "Cargo.toml".
Once done, I'll merge this PR as jswrenn's last comment is enough confirmation to me.

bishopcheckmate

nit, actually dereferencing is not needed

bishopcheckmate · 2024-01-08T18:58:11Z

src/either_or_both.rs

-            Left(_) => true,
-            _ => false,
-        }
+        matches!(*self, Left(_))


Suggested change

matches!(*self, Left(_))

matches!(self, Left(_))

Indeed, it compiles to the exact same thing (I just checked a simple example on godbolt).
I'm not sure we should care when I did not find any clippy lint for it.
I'll let the author decide here.

yeah, that's definitely a nitpick. on the other hand in a pr which satisfies clippy, it might be kind of future proofing 😄

Sorry thank you done.

bishopcheckmate · 2024-01-08T18:58:25Z

src/either_or_both.rs

-            Right(_) => true,
-            _ => false,
-        }
+        matches!(*self, Right(_))


Suggested change

matches!(*self, Right(_))

matches!(self, Right(_))

Co-authored-by: warren2k <846021+warren2k@users.noreply.github.com>

mightyiam · 2024-01-09T00:48:00Z

Cargo.toml

+# When bumping, please resolve all `#[allow(clippy::*)]` that are newly resolvable.
 rust-version = "1.43.1"


Added this here. Good enough?

Philippe-Cholet

This is it, thanks for your dedicated work. 🎉

mightyiam force-pushed the clippy branch from 86ac53b to 77cf260 Compare August 27, 2023 15:34

mightyiam marked this pull request as ready for review August 27, 2023 15:36

mightyiam force-pushed the clippy branch 4 times, most recently from f88da92 to 8769af0 Compare August 27, 2023 15:58

warren2k force-pushed the clippy branch from 8769af0 to 56bc6b9 Compare September 2, 2023 13:54

mightyiam marked this pull request as draft September 9, 2023 14:26

warren2k force-pushed the clippy branch from 56bc6b9 to d137da7 Compare September 9, 2023 14:27

mightyiam force-pushed the clippy branch from d137da7 to b5b2fec Compare September 9, 2023 14:39

mightyiam marked this pull request as ready for review September 9, 2023 14:39

mightyiam force-pushed the clippy branch from b5b2fec to 6cc5e79 Compare September 10, 2023 14:06

jswrenn requested changes Sep 15, 2023

View reviewed changes

mightyiam force-pushed the clippy branch from 6cc5e79 to db7f7be Compare September 17, 2023 14:28

warren2k force-pushed the clippy branch from db7f7be to 4b428f1 Compare September 17, 2023 14:43

mightyiam marked this pull request as draft September 17, 2023 15:01

jswrenn approved these changes Sep 19, 2023

View reviewed changes

Philippe-Cholet mentioned this pull request Sep 21, 2023

Address clippy lints #759

Closed

mightyiam force-pushed the clippy branch 2 times, most recently from df848e0 to 5b0235a Compare September 24, 2023 14:19

mightyiam commented Sep 30, 2023

View reviewed changes

mightyiam force-pushed the clippy branch 2 times, most recently from c9f2887 to 1211903 Compare September 30, 2023 15:00

mightyiam mentioned this pull request Sep 30, 2023

Disallow compile warnings in CI #720

Merged

mightyiam force-pushed the clippy branch from 1211903 to e9fdb70 Compare October 8, 2023 13:08

warren2k force-pushed the clippy branch from 265ae29 to 2f1badf Compare October 8, 2023 13:27

mightyiam marked this pull request as ready for review October 8, 2023 13:36

mightyiam requested a review from jswrenn October 8, 2023 13:37

Philippe-Cholet approved these changes Jan 7, 2024

View reviewed changes

build: enforce clippy

7473afb

Co-authored-by: warren2k <846021+warren2k@users.noreply.github.com>

mightyiam force-pushed the clippy branch from 2f1badf to 17d9cd5 Compare January 8, 2024 01:23

mightyiam marked this pull request as draft January 8, 2024 01:28

mightyiam force-pushed the clippy branch 2 times, most recently from f93e8f4 to abb3f50 Compare January 8, 2024 03:12

mightyiam commented Jan 8, 2024

View reviewed changes

mightyiam marked this pull request as ready for review January 8, 2024 03:31

Philippe-Cholet approved these changes Jan 8, 2024

View reviewed changes

jswrenn approved these changes Jan 8, 2024

View reviewed changes

bishopcheckmate reviewed Jan 8, 2024

View reviewed changes

refactor: resolve clippy lints

5e2377b

Co-authored-by: warren2k <846021+warren2k@users.noreply.github.com>

mightyiam force-pushed the clippy branch from abb3f50 to 5e2377b Compare January 9, 2024 00:36

mightyiam commented Jan 9, 2024

View reviewed changes

Philippe-Cholet approved these changes Jan 9, 2024

View reviewed changes

Philippe-Cholet added this pull request to the merge queue Jan 9, 2024

Merged via the queue into rust-itertools:master with commit c5a1b16 Jan 9, 2024
8 checks passed

mightyiam deleted the clippy branch January 9, 2024 08:25

Philippe-Cholet mentioned this pull request Jan 9, 2024

impl a From instead of an Into #836

Closed

Philippe-Cholet added this to the next milestone Jan 10, 2024

This was referenced Jan 11, 2024

add rustdoc and clippy to github workflow #618

Closed

Either or both: impl From implies impl Into #843

Merged


		#[allow(clippy::explicit_counter_loop)]
		for (_, elt) in data1.iter_mut().enumerate() {


		#[allow(clippy::explicit_counter_loop, clippy::unused_enumerate_index)]

		# When bumping, please resolve all `#[allow(clippy::*)]` that are newly resolvable.
		rust-version = "1.43.1"

clippy #740

clippy #740

Conversation

mightyiam commented Aug 27, 2023

mightyiam commented Aug 27, 2023

mightyiam commented Sep 9, 2023

mightyiam commented Sep 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Option A

Option B

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mightyiam commented Sep 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Option A

Option B

jswrenn commented Oct 8, 2023

mightyiam commented Jan 7, 2024

Philippe-Cholet commented Jan 7, 2024

jswrenn commented Jan 7, 2024

mightyiam left a 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

Philippe-Cholet Jan 8, 2024 • edited

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

Philippe-Cholet left a comment

Choose a reason for hiding this comment

Philippe-Cholet Jan 8, 2024 • edited

Choose a reason for hiding this comment

Philippe-Cholet commented Jan 8, 2024

bishopcheckmate left a 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

Choose a reason for hiding this comment

Philippe-Cholet left a comment

Choose a reason for hiding this comment

Philippe-Cholet Jan 8, 2024 •

edited

Philippe-Cholet Jan 8, 2024 •

edited