Fix clippy lints and run clippy in CI #29

smklein · 2021-01-27T15:55:16Z

Admittedly, I ran this with cargo +nightly clippy to avoid rust-lang/rust-clippy#6545, but with this patch, all clippy lints are resolved.

Miscellaneous cleanups include:

https://rust-lang.github.io/rust-clippy/master/index.html#needless_lifetimes
https://rust-lang.github.io/rust-clippy/master/index.html#redundant_clone
https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_wraps

davepacheco

Thanks for taking a look at this.

davepacheco · 2021-01-27T16:59:21Z

src/oxide_controller/oxide_controller.rs

@@ -739,19 +739,19 @@ impl OxideController {
        fn disk_attachment_for(
            instance: &Arc<ApiInstance>,
            disk: &Arc<ApiDisk>,
-        ) -> CreateResult<ApiDiskAttachment> {
+        ) -> Arc<ApiDiskAttachment> {


I don't understand this change. Is clippy saying that this thing only ever returns success, so we should drop the Result from the type? That's true, but the caller always wraps the return value in a Result, so it makes sense to me to just do that here, in one place.

If I'm understanding right, I think this is one of those examples of clippy's advice being pretty subjective, which is why we don't run it automatically all the time. I don't really mind this change if there's value in being clippy-clean, but my past experience has been that it's not really worth it (see oxidecomputer/dropshot#4). What about you?

Ah, thanks for the context with that link.

Clippy itself is pretty configurable - it has different "lint levels" (allow / warn / deny) and "lint groups" (correctness, style, pedantic, complexity, perf, etc) that can be turned on or off depending on invocation.

The default is:

correctness group: deny

style: warn

complexity: warn

perf: warn

pedantic: allow

But this can be (and probably should be, depending on a project's requirements) adjusted, at a broad category but also a per-lint basis.

As an example, take a look at the "deny" lint level on https://rust-lang.github.io/rust-clippy/master/index.html - a lot of these things are in the bucket of "technically not a compiler error, but are almost always inadvisable".

Even if there is opposition to some of the on-the-fence lints, I'd still advocate for some threshold of analysis here - the cost is pretty low.

That being said, if there's a preference for (as @ahl suggested) "run[ning] clippy periodically", that seems fine too.

With regards to this specific lint, yeah, clippy is identifying that the function type signature is misleading.

For what it's worth, I do not think this lint would be raised on a public function (where API stability matters), but in the context of a private function, my gut instinct agrees with the lint: the prototype is imprecise regarding the behavior of the code - the use of the function implies that an error could be returned, and should be handled, but this is never the case. All the current uses propagate the Result directly, but a future invocation might have more complexity, and error-handling around this function would become impossible-to-reach code.

(I also totally admit, this is a weakly-held opinion, if you feel strongly, I'd be fine reverting the suggestions based on this lint)

I think that's a good rule of thumb and it's useful that clippy can identify that. Here's another one: if you have a private function, and it has N callers, and all N callers apply the same transformation to the returned value, then the function ought to apply that transformation instead for the usual DRY reasons. I could imagine a clippy lint for this, too. 🤷

How about this: if you think it's worthwhile for us to pick a set of lints and stay clippy-clean, want to update this PR to do that as part of CI and document it in the README? It's up to you if you think this is worth it. I haven't run it on this code base in months, and I'm mostly indifferent to what it found here (though matches! one is a nice readability improvement!).

Mmkay - I added clippy to the readme, and have the invocation within .github/workflows/rust.yml, so it'll run on new PRs too.

Disabling a lint is really easy, just add a -D clippy::lint_name, and it'll be ignored. I think it's super super reasonable to look at a lint, decide it's irritating, and throw it in that bucket - like your stance on unnecessary_wraps (which I think is reasonable - removed those changes).

For now the linter is catching defaults + filtering out some undesired lints - WDYT? I'm flexible here.

For the historical record, this specific lint came up again under #38. (It was also pedantic in the latest version of clippy.)

api_identity/src/lib.rs

davepacheco

Thanks for this! I think this will be a good improvement.

For some reason I don't understand, GitHub won't let me reply on the thread above (maybe because it's "outdated"?), so I'm putting it here:

Disabling a lint is really easy, just add a -D clippy::lint_name, and it'll be ignored. I think it's super super reasonable to look at a lint, decide it's irritating, and throw it in that bucket - like your stance on unnecessary_wraps (which I think is reasonable - removed those changes).

I think it's -A clippy::lint_name that will cause a lint check to be ignored, right? I haven't used clippy much before but that's my understanding from the help output and it seems to match the example in the clippy README.

For now the linter is catching defaults + filtering out some undesired lints - WDYT? I'm flexible here.

That sounds like a good plan.

Just checking my understanding here: I don't see an exclusion for unnecessary_wraps, yet clippy doesn't complain about that code we talked about. I gather that's because unnecessary_wraps is not on stable clippy. Is that right?

davepacheco · 2021-01-29T19:29:53Z

.github/workflows/rust.yml

+      # TODO: Migrate these options to a config file in the workspace,
+      # once https://github.com/rust-lang/rust-clippy/issues/6625 is resolved.
+      #
+      # - field_reassign_with_default:


Is this comment just here to remind ourselves in the future that we should drop this exception? If so can you file an issue instead?

Sure, I'll do that now: #32

davepacheco · 2021-01-29T19:33:13Z

.github/workflows/rust.yml

+    # actions/checkout@v2
+    - uses: actions/checkout@28c7f3d2b5162b5ddd3dfd9a45aa55eaf396478b
+    - name: Run Clippy Lints
+      # TODO: Migrate these options to a config file in the workspace,


From my understanding, if people run cargo clippy, that won't use the same configuration that the CI check will, right? I see that a config file isn't supported yet, but is there something we can do so that a developer running cargo clippy will get the right thing? There seem to be a few options here:
rust-lang/rust-clippy#1313

Maybe we can put attributes into particular files or globally in lib.rs?

Sure, I'll go with the lib.rs route - that seems like a good approach!

smklein · 2021-01-29T20:43:21Z

Thanks for this! I think this will be a good improvement.

For some reason I don't understand, GitHub won't let me reply on the thread above (maybe because it's "outdated"?), so I'm putting it here:

Disabling a lint is really easy, just add a -D clippy::lint_name, and it'll be ignored. I think it's super super reasonable to look at a lint, decide it's irritating, and throw it in that bucket - like your stance on unnecessary_wraps (which I think is reasonable - removed those changes).

I think it's -A clippy::lint_name that will cause a lint check to be ignored, right? I haven't used clippy much before but that's my understanding from the help output and it seems to match the example in the clippy README.

Errrr, yes, the code is correct, my text here was wrong.

-A lint is "allow this lint", -D lint is "deny this lint". For purposes of the GitHub action, I denied warnings, since they are otherwise seen as "passing".

For now the linter is catching defaults + filtering out some undesired lints - WDYT? I'm flexible here.

That sounds like a good plan.

Just checking my understanding here: I don't see an exclusion for unnecessary_wraps, yet clippy doesn't complain about that code we talked about. I gather that's because unnecessary_wraps is not on stable clippy. Is that right?

Yeah, that's correct. I had been hacking around with clippy +nightly previously, but this patch is currently tuned to clippy +stable, as that matches the GitHub action.

(I did actually try to explicitly enable unnecessary_wraps ahead of time, but it doesn't exist on stable. I'll add the exception when we roll the toolchain?)

davepacheco

Thanks for sticking with this. I just had one nit here. (Sorry!)

davepacheco · 2021-01-29T21:39:59Z

src/lib.rs

@@ -57,6 +57,10 @@
 * it's expected that we'll have links to private items in the docs.
 */
 #![allow(private_intra_doc_links)]
+/*
+ * TODO(#32): Remove this exception once resolved.


No need for this TODO here. The issue should cover it. If somebody wants to know why it was added, they can see the notes in this PR.

Fix clippy lints

d23492c

smklein requested a review from davepacheco January 27, 2021 15:55

davepacheco reviewed Jan 27, 2021

View reviewed changes

smklein added 4 commits January 29, 2021 11:40

Merge branch 'master' into clippy

03b905b

Add Clippy to README, github workflow (expect failure)

e3640c3

Use non-conflicting workflow names

d16e9f2

Welp no unstable options on stable, go figure

f2e6269

smklein changed the title ~~Fix clippy lints~~ [WIP] Fix clippy lints Jan 29, 2021

smklein added 5 commits January 29, 2021 12:27

Update clippy flags

87c3617

Comments

3de03b5

Patch the workflow name

ee40f54

Dangit yaml

2f562f2

Welp that lint doesn't exist on stable

08679d0

smklein changed the title ~~[WIP] Fix clippy lints~~ Fix clippy lints Jan 29, 2021

davepacheco reviewed Jan 29, 2021

View reviewed changes

smklein mentioned this pull request Jan 29, 2021

Clippy: False positive for field_reassign_with_default exception #32

Open

re: code review feedback

a32caee

davepacheco reviewed Jan 29, 2021

View reviewed changes

davepacheco changed the title ~~Fix clippy lints~~ Fix clippy lints and run clippy in CI Jan 30, 2021

davepacheco merged commit d254e72 into master Jan 30, 2021

davepacheco mentioned this pull request Feb 25, 2021

Clippy "unnecessary_wraps" lint should be off by default, isn't #38

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix clippy lints and run clippy in CI #29

Fix clippy lints and run clippy in CI #29

smklein commented Jan 27, 2021

davepacheco left a comment

davepacheco Jan 27, 2021

smklein Jan 27, 2021

smklein Jan 27, 2021

davepacheco Jan 29, 2021

smklein Jan 29, 2021

davepacheco Feb 25, 2021

davepacheco left a comment

davepacheco Jan 29, 2021

smklein Jan 29, 2021

davepacheco Jan 29, 2021

smklein Jan 29, 2021

smklein commented Jan 29, 2021

davepacheco left a comment

davepacheco Jan 29, 2021

Fix clippy lints and run clippy in CI #29

Fix clippy lints and run clippy in CI #29

Conversation

smklein commented Jan 27, 2021

davepacheco 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

davepacheco 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

smklein commented Jan 29, 2021

davepacheco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment