Add clippy to CI #448

tcharding · 2022-06-10T03:06:19Z

The first 8 patches clear clippy warnings. Next we add a CI job to run clippy. Finally we add a githooks directory that includes running clippy, also adds a section to the README on how to use the githooks. This is identical to the text in the open PR on rust-bitcoin that adds githooks without yet adding clippy.

Note: The new clippy CI job runs and is green :)

apoelstra

ACK 5551f2c

apoelstra · 2022-06-15T15:46:25Z

Needs rebase now, sorry.

Clippy emits: warning: this import is redundant This is a remnant of edition 2015, now we have edition 2018 we no longer need this import statement.

Clippy emits: warning: statics have by default a `'static` lifetime Static strings no longer need an explicit lifetime, remove it.

Clippy emits: warning: this expression creates a reference which is immediately dereferenced by the compiler Remove the explicit reference.

tcharding · 2022-06-15T23:58:03Z

No changes in force push, rebase only, there was a merge conflict in one of the unit tests in commit: 35d59e7 Remove explicit 'static lifetime.

elichai

ACK d81d2fe

RCasatta · 2022-06-16T08:58:28Z

githooks/pre-commit

+# exit with non-zero status after issuing an appropriate message if
+# it wants to stop the commit.
+#
+# To enable this hook, rename this file to "pre-commit".


that's already the name of the file?

Nice catch, I modified the whole comment to better describe the hook. Thanks.

Kixunil

The exec should be fixed, I believe.

Kixunil · 2022-06-16T11:22:51Z

githooks/pre-commit

+exec git diff-index --check --cached $against --
+
+# Check that code lints cleanly.
+exec cargo clippy --features=rand-std,recovery,lowmemory,global-context --all-targets -- -D warnings


This doesn't work since exec replaces the process (just like the exec syscall) thus this exec never executes

Thanks man, seems I tested whitespace but not clippy before pushing this. Out of interest I tried removing both the execs and adding set -e but that did not work, I then tried the form cmd || exit 1 for both and that works.

Kixunil · 2022-06-16T11:24:01Z

src/context.rs

@@ -191,7 +188,8 @@ mod alloc_only {
        /// ctx.seeded_randomize(&seed);
        /// # }
        /// ```
-        #[allow(unused_mut)]    // Unused when `rand-std` is not enabled.
+        #[allow(unused_mut)]             // Unused when `rand-std` is not enabled.
+        #[allow(clippy::let_and_return)] // Triggers when `rand-std` is not enabled.


Perhaps cfg_attr to limit the scope? Not a big deal though.

Nice trick! I had to think about your suggestion for a bit then I worked it out

#[cfg_attr(not(feature = "rand-std"), allow(clippy::let_and_return, unused_mut))]

For the second time today; thanks for the lesson :)

Kixunil · 2022-06-16T11:29:26Z

src/ecdsa/mod.rs

-                    let le_counter_bytes : [u8; 4] = mem::transmute(le_counter);
-                    for (i, b) in le_counter_bytes.iter().enumerate() {
+                    let le_counter = counter.to_le_bytes();
+                    for (i, b) in le_counter.iter().enumerate() {
                        extra_entropy[i] = *b;
                    }


This whole chunk could be just extra_entropy[..4].copy_from_slice(&counter.to_le_bytes);

Also below (can't comment there) cast is probably better than as. extra_entropy.as_ptr().cast::<ffi::types::c_void>();

I used both suggestions, I was unable to justify the as -> cast change in the commit log, can you explain why its better please, just for my learning?

apoelstra

ACK d81d2fe

apoelstra · 2022-06-16T12:52:22Z

Will hold off on merging since @Kixunil's nits look good (and the exec thing might be more than a nit)

Kixunil · 2022-06-16T13:55:47Z

exec is definitely not a nit. It breaks the main purpose of this PR. :)

apoelstra · 2022-06-16T19:26:49Z

Well, it breaks the main purpose of the pre-commit hook :) but the CI job is not affected. But yes, I see your point.

Since we bumped the MSRV we have `to_le_bytes` available now, use it instead of `mem::transmute`. Remove code comment suggesting to do exactly this and clear clippy warning. While we are at it, use `cast` instead of `as`.

We have a whole bunch of unsafe code that calls down to the FFI layer. It would be nice to have clippy running on CI, these safety docs warnings are prohibiting that. Until we can add the docs add a compiler attribute to allow the lint.

Clippy emits: warning: returning the result of a `let` binding from a block This is due to feature guarded code, add 'allow' attribute. Use `cfg_attr` to restrict the allow to when it is needed. Add the already present `unused_mut` inside the `cfg_attr` guard also.

Clippy emits: warning: manual implementation of `str::repeat` using iterators As suggested, use `"a".repeats()`.

We are explicitly testing various boolean statements, tell clippy to allow less than minimal statements.

In order to keep the codebase linting cleanly add a CI job to run clippy.

Add `githooks` directory and: - Copy the default pre-commit hook into new githooks directory - Add a call to clippy to the pre-commit hook - Add a section to the README instructing devs how to use the new githooks

tcharding · 2022-06-17T00:19:10Z

Solid reviewing crew, thank you. Force push includes the various changes above.

Kixunil

ACK 65186e7

apoelstra

ACK 65186e7

apoelstra

ACK 65186e7

apoelstra · 2022-10-06T19:16:33Z

Lol, sorry, I meant to test and ack 488.

tcharding mentioned this pull request Jun 10, 2022

Document unsafe code #447

Closed

apoelstra previously approved these changes Jun 10, 2022

View reviewed changes

tcharding mentioned this pull request Jun 14, 2022

Bump crate version to 0.23.0 #444

Merged

tcharding added 3 commits June 16, 2022 09:54

Remove redundant import

1a582db

Clippy emits: warning: this import is redundant This is a remnant of edition 2015, now we have edition 2018 we no longer need this import statement.

Remove explicit 'static lifetime

35d59e7

Clippy emits: warning: statics have by default a `'static` lifetime Static strings no longer need an explicit lifetime, remove it.

Remove unneeded explicit reference

c15b9d2

Clippy emits: warning: this expression creates a reference which is immediately dereferenced by the compiler Remove the explicit reference.

tcharding dismissed apoelstra’s stale review via d81d2fe June 15, 2022 23:57

tcharding force-pushed the 06-10-clippy branch from 5551f2c to d81d2fe Compare June 15, 2022 23:57

elichai previously approved these changes Jun 16, 2022

View reviewed changes

RCasatta reviewed Jun 16, 2022

View reviewed changes

Kixunil requested changes Jun 16, 2022

View reviewed changes

apoelstra previously approved these changes Jun 16, 2022

View reviewed changes

tcharding dismissed stale reviews from apoelstra and elichai via fddca7c June 16, 2022 23:45

tcharding force-pushed the 06-10-clippy branch 2 times, most recently from fddca7c to 6c6a577 Compare June 16, 2022 23:58

tcharding added 7 commits June 17, 2022 10:16

Use to_le_bytes instead of mem::transmute

2cb687f

Since we bumped the MSRV we have `to_le_bytes` available now, use it instead of `mem::transmute`. Remove code comment suggesting to do exactly this and clear clippy warning. While we are at it, use `cast` instead of `as`.

Allow missing_safety_doc

d64132c

We have a whole bunch of unsafe code that calls down to the FFI layer. It would be nice to have clippy running on CI, these safety docs warnings are prohibiting that. Until we can add the docs add a compiler attribute to allow the lint.

Use "a".repeats() instead of manual implementation

685444c

Clippy emits: warning: manual implementation of `str::repeat` using iterators As suggested, use `"a".repeats()`.

Allow nonminimal_bool in unit test

9f1ebb9

We are explicitly testing various boolean statements, tell clippy to allow less than minimal statements.

Add clippy to CI

6d76bd4

In order to keep the codebase linting cleanly add a CI job to run clippy.

Add githooks

65186e7

Add `githooks` directory and: - Copy the default pre-commit hook into new githooks directory - Add a call to clippy to the pre-commit hook - Add a section to the README instructing devs how to use the new githooks

tcharding force-pushed the 06-10-clippy branch from 6c6a577 to 65186e7 Compare June 17, 2022 00:19

Kixunil approved these changes Jun 17, 2022

View reviewed changes

apoelstra approved these changes Jun 17, 2022

View reviewed changes

apoelstra merged commit a1ac3fb into rust-bitcoin:master Jun 17, 2022

tcharding deleted the 06-10-clippy branch June 20, 2022 22:03

apoelstra approved these changes Oct 6, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add clippy to CI #448

Add clippy to CI #448

tcharding commented Jun 10, 2022 •

edited

Loading

apoelstra left a comment

apoelstra commented Jun 15, 2022

tcharding commented Jun 15, 2022

elichai left a comment

RCasatta Jun 16, 2022

tcharding Jun 16, 2022

Kixunil left a comment

Kixunil Jun 16, 2022

tcharding Jun 16, 2022

Kixunil Jun 16, 2022

tcharding Jun 17, 2022

Kixunil Jun 16, 2022

tcharding Jun 17, 2022

apoelstra left a comment

apoelstra commented Jun 16, 2022

Kixunil commented Jun 16, 2022

apoelstra commented Jun 16, 2022

tcharding commented Jun 17, 2022

Kixunil left a comment

apoelstra left a comment

apoelstra left a comment

apoelstra commented Oct 6, 2022

Add clippy to CI #448

Add clippy to CI #448

Conversation

tcharding commented Jun 10, 2022 • edited Loading

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Jun 15, 2022

tcharding commented Jun 15, 2022

elichai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil 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

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Jun 16, 2022

Kixunil commented Jun 16, 2022

apoelstra commented Jun 16, 2022

tcharding commented Jun 17, 2022

Kixunil left a comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Oct 6, 2022

tcharding commented Jun 10, 2022 •

edited

Loading