Run clippy on CI by kchibisov · Pull Request #2315 · rust-windowing/winit

kchibisov · 2022-06-07T19:13:03Z

.github/workflows/ci.yml

filnet · 2022-06-07T22:00:53Z

src/platform_impl/ios/window.rs

            // event on window creation if the DPI factor != 1.0
            let scale_factor: CGFloat = msg_send![view, contentScaleFactor];
-            let scale_factor: f64 = scale_factor.into();
+            let scale_factor: f64 = scale_factor as _;


The variable type was removed in a similar expression higher up.

I don't understand what do you want from me here. The problem is that CGFLoat is either f32 or f64, but winit API is f64, so we need to cast it to f64 here. Using .into() won't work, since clippy finds it as redundant when CGFloat is f64, however we can cast it like as _ and explicitly state that we assign to f64 value.

I think because this change was made in view.rs:

- let scale_factor: f64 = scale_factor.into(); + let scale_factor = scale_factor as f64;

Isn't that affected in the same way?

src/platform_impl/linux/wayland/window/shim.rs

filnet · 2022-06-07T22:05:53Z

src/platform_impl/linux/x11/util/geometry.rs

            // This is the case for xmonad and dwm, AKA the only WMs tested that supplied a
            // border value. This is convenient, since we can use it to get an accurate frame.
-            let frame_extents = FrameExtents::from_border(border.into());
+            let frame_extents = FrameExtents::from_border(border as c_ulong);


Which clippy gets triggered here ?

PS: the best is to do one commit per clippy type...

we call into() to convert from u64 to u64. The problem is that the code was correct, it just bitness dependent, so it's a clippy warning on 64-bit, but works fine on 32-bit.

PS: the best is to do one commit per clippy type...

That's very hard and wasteful if you're tackling them all at once using clippy --fix, unless collecting all the individual warnings and running clippy --fix one by one, allowing all the lints except one.

src/platform_impl/macos/util/async.rs

kchibisov · 2022-06-07T22:27:21Z

@MarijnS95 Do you happen to know why android target randomly fails from time to time? I think I've seen it a bunch of times here, rerunning do fix the issues.

madsmtm

iOS and macOS changes looks fine (I never did like the let () = msg_send![...], so cool that Clippy doesn't either).

src/event_loop.rs

madsmtm · 2022-06-08T02:11:38Z

src/lib.rs

 #![deny(rust_2018_idioms)]
 #![deny(rustdoc::broken_intra_doc_links)]
+#![deny(clippy::all)]
+#![cfg_attr(feature = "cargo-clippy", deny(warnings))]


How does this work?

clippy enables the cargo-clippy feature on all crates when running through it, automatically denying all rustc warnings in this instance. However, this implies warnings from clippy::all (that lint group should only include warn/deny lints) and the #![deny(clippy::all)] is thus superfluous.

deny clippy all means that it'll error on clippy warnings instead of treating them as warnings.

But that's also what #![cfg_attr(feature = "cargo-clippy", deny(warnings))] does :)

I think those about rustc warnings, and not clippy onces? I think it was like that in the past, at least.

I tested it and it includes all warnings, including clippy ones.

examples/fullscreen.rs

src/platform_impl/macos/view.rs

madsmtm · 2022-06-08T02:24:43Z

src/window.rs


 /// Fullscreen modes.
-#[derive(Clone, Debug, PartialEq)]
+#[derive(Clone, Debug, PartialEq, Eq)]


Nit: Add changelog entry

I don't think internal impl requires it? Also I'm not sure that we should add changelog notes for Eq impls, since they are not breaking the code and transparent.

Internal impls don't matter.

It's not a breaking change, but in general I think all API changes should be documented in the changelog, including small ones like this. But I don't have a strong opinion on this, you're free to disagree.

A changelog entry would indeed be nice.

MarijnS95 · 2022-06-08T07:32:20Z

.github/workflows/ci.yml

+    - name: Lint with clippy
+      shell: bash
+      if: (matrix.rust_version != 'nightly') && !contains(matrix.platform.options, '--no-default-features')
+      run: cargo clippy --all-targets --target ${{ matrix.platform.target }} $OPTIONS --features $FEATURES


As said in the glutin PR, I'd like to keep -- -Dwarnings in here so that new crates and bin (example/test/bench) targets are included too.

I think we don't want to run clippy for examples actually.

I think we should be consistent and enforce it everywhere, so that the examples are also clean-enough.

In addition there's --all-targets here and you fixed+allowed clippy lints in the examples/, so this is only sensible?

I'd add -D warnings on CI then, I guess.

Having deny in lib.rs is still easier to understand that you must fix those issues.

Yeah, leave the deny in lib.rs, that should hopefully decrease the amount of noise in PRs that would otherwise require lots of cleanup pushes to "make the CI happy".

I'm fairly certain that the general advice here is to not deny warnings in *.rs files, and instead only do it in CI, as it might break compilation with future versions of rustc and/or clippy.

That's the point, we don't deny warnings for rustc or any related command, just when running clippy. So there's no way the build will break, only clippy could, but it would only mean that CI is broken.

Ah, I see. That's clever.

src/platform_impl/linux/wayland/window/shim.rs

src/platform_impl/windows/ime.rs

src/platform_impl/web/web_sys/mod.rs

src/platform_impl/web/web_sys/canvas.rs

src/platform_impl/ios/app_state.rs

src/platform_impl/android/mod.rs

src/platform_impl/ios/mod.rs

MarijnS95 · 2022-06-08T08:14:48Z

src/platform_impl/ios/window.rs

            // event on window creation if the DPI factor != 1.0
            let scale_factor: CGFloat = msg_send![view, contentScaleFactor];
-            let scale_factor: f64 = scale_factor.into();
+            let scale_factor: f64 = scale_factor as _;


I think because this change was made in view.rs:

- let scale_factor: f64 = scale_factor.into(); + let scale_factor = scale_factor as f64;

Isn't that affected in the same way?

MarijnS95 · 2022-06-08T08:17:00Z

src/platform_impl/linux/x11/util/geometry.rs

            // This is the case for xmonad and dwm, AKA the only WMs tested that supplied a
            // border value. This is convenient, since we can use it to get an accurate frame.
-            let frame_extents = FrameExtents::from_border(border.into());
+            let frame_extents = FrameExtents::from_border(border as c_ulong);


PS: the best is to do one commit per clippy type...

That's very hard and wasteful if you're tackling them all at once using clippy --fix, unless collecting all the individual warnings and running clippy --fix one by one, allowing all the lints except one.

MarijnS95

It looks like all my comments were addressed minus some minor nits which we can continue to discuss, nice job!

src/event_loop.rs

src/platform_impl/ios/mod.rs

src/platform_impl/web/web_sys/scaling.rs

maroider

I don't have any further blocking concerns, although it would be nice to have changelog entries for all the added trait impls on public types.

Fixes rust-windowing#1402.

kchibisov · 2022-06-10T10:43:52Z

Android failures were unrelated and quite random recently.

madsmtm · 2022-06-14T05:15:36Z

src/platform_impl/macos/mod.rs

@@ -1,4 +1,5 @@
 #![cfg(target_os = "macos")]
+#![allow(clippy::let_unit_value)]


That we have to specify this is actually a regression, I've filed an issue about it here: rust-lang/rust-clippy#8998.

kchibisov force-pushed the add-clippy-ci branch 2 times, most recently from ae6aee5 to 938ef8d Compare June 7, 2022 19:38

kchibisov requested review from ArturKovacs, MarijnS95, madsmtm and maroider June 7, 2022 19:38

kchibisov added this to the Version 0.27 milestone Jun 7, 2022

MarijnS95 reviewed Jun 7, 2022

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

kchibisov force-pushed the add-clippy-ci branch 6 times, most recently from c2718c4 to d0bce5a Compare June 7, 2022 20:31

filnet reviewed Jun 7, 2022

View reviewed changes

src/platform_impl/linux/wayland/window/shim.rs Show resolved Hide resolved

filnet reviewed Jun 7, 2022

View reviewed changes

src/platform_impl/macos/util/async.rs Show resolved Hide resolved

kchibisov force-pushed the add-clippy-ci branch from d006287 to 3d9ab91 Compare June 7, 2022 22:15

madsmtm requested changes Jun 8, 2022

View reviewed changes

MarijnS95 reviewed Jun 8, 2022

View reviewed changes

MarijnS95 approved these changes Jun 8, 2022

View reviewed changes

kchibisov force-pushed the add-clippy-ci branch from da43815 to aed5f1a Compare June 8, 2022 18:44

maroider reviewed Jun 8, 2022

View reviewed changes

src/event_loop.rs Outdated Show resolved Hide resolved

src/platform_impl/ios/mod.rs Outdated Show resolved Hide resolved

src/platform_impl/web/web_sys/scaling.rs Outdated Show resolved Hide resolved

src/platform_impl/web/web_sys/scaling.rs Outdated Show resolved Hide resolved

kchibisov force-pushed the add-clippy-ci branch from aed5f1a to 2178a0f Compare June 8, 2022 18:51

kchibisov requested review from madsmtm and maroider June 8, 2022 18:56

madsmtm approved these changes Jun 8, 2022

View reviewed changes

maroider approved these changes Jun 9, 2022

View reviewed changes

msiglreith approved these changes Jun 9, 2022

View reviewed changes

Run clippy on CI

87670b2

Fixes rust-windowing#1402.

kchibisov force-pushed the add-clippy-ci branch from 436fba6 to 87670b2 Compare June 10, 2022 10:17

kchibisov merged commit 10419ff into rust-windowing:master Jun 10, 2022

kchibisov deleted the add-clippy-ci branch June 10, 2022 10:43

madsmtm reviewed Jun 14, 2022

View reviewed changes

		@@ -1,4 +1,5 @@
		#![cfg(target_os = "macos")]
		#![allow(clippy::let_unit_value)]

Conversation

kchibisov commented Jun 7, 2022

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kchibisov Jun 7, 2022 • 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!

Uh oh!

kchibisov commented Jun 7, 2022

Uh oh!

madsmtm left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarijnS95 left a comment

Choose a reason for hiding this comment

Uh oh!

kchibisov Jun 7, 2022 •

edited

Loading