Add lints to catch missing traits #364
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This PR came about from discussion here. |
Clippy emits a few warnings: warning: unneeded `return` statement As suggested, remove the unneeded return statements.
Clippy emits: warning: useless use of `format!` As suggested, remove the useless call to `format!`.
tcharding
force-pushed
the
lint-derives
branch
from
3e9fadf
to
dfe46b0
Compare
Rustc can warn us when we forget to add `Copy` and `Deubg` trait implementations to types. Add lint directives to enable warnings for missing `Copy` and `Debug` implementations. Use the newly emitted warnings to find types that do not implement our 'standard' traits. These 'standard' traits are defined as the set of attributes that it has been found beneficial to opportunistically add to all types, these are - Copy - Clone - Debug - PartialEq and Eq - PartialOrd and Ord - Hash
Currently we have an implementation of `Debug` (also used by `Display`) for `Signature` that first converts the sig to a `SerializedSignature` then prints it as hex. We would like to have an implementation of `Debug` for `SerializedSignature`, this cannot be derived because of the `data: [u8; field]`. We can manually implement `Debug` for `SerializedSignature` exactly as it is currently done for `Signature` and call this new implementation from `Signature::fmt()`. This code path is already tested in `lib.rs` in the test function `signature_display`.
tcharding
force-pushed
the
lint-derives
branch
from
dfe46b0
to
69f44d9
Compare
thomaseizinger
approved these changes
Jan 7, 2022
There was a problem hiding this comment.
ACK 69f44d9
I don't think we currently fail the build on warnings so this is mere a reminder for devs while working locally but that is already an improvement.
|
The lints for |
|
I am up for having a new point release: rust-bitcoin/rust-bitcoin@73f7e96#diff-5d6afe76bb8d0b9a17c480f8f357e3818d9e82dd9c53557428d274106bb6e3e7R61 |
|
Another point release sounds good to me. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We can use the linters to help us catch type definitions that are missing 'standard' derives. 'standard' is project defined to be
(I've assumed this to be true based on the code and an open PR in rust-bitcoin.)
While neither Rustc nor Clippy can find all of these, Rustc can warn for missing
CopyandDebugimplementations and these warnings can assist us find types that may need additional derives.First two patches are trivial Clippy fixes in preparation for using the linter to improve type definitions crate wide.
Patch 3 adds
and fixes newly emitted warnings.