Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explore clippy rules and apply quality of life recommendations #122

Closed
wants to merge 57 commits into from
Closed
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
b7e300f
Set clippy rules in root
xla Jan 3, 2020
f4bc257
Remove Error prefix in kind enum
xla Jan 3, 2020
04fec06
Merge branch 'master' into xla/clippy
xla Jan 3, 2020
4d82f36
Remove superfluours Info suffix
xla Jan 3, 2020
ae598b7
Remove superfluours Params suffix
xla Jan 3, 2020
f1f4f9f
Remove superfluours Config suffix
xla Jan 3, 2020
6f423a7
Remove superfluours Id suffix
xla Jan 3, 2020
e2220a1
Remove superfluours Version suffix
xla Jan 3, 2020
2810c89
Remove superfluous BlockId suffix
xla Jan 3, 2020
faf0fe6
Remove superfluous Amino prefix
xla Jan 3, 2020
827285e
Remove superfluous Ping prefix
xla Jan 3, 2020
17c7996
Remove superfluous RemoteError prefix
xla Jan 3, 2020
2d7607d
Remove superfluous RemoteError prefix
xla Jan 3, 2020
5ddb642
Lock down amino module surface
xla Jan 3, 2020
7e3b87f
Remove superfluous Time prefix
xla Jan 3, 2020
d3dc4b9
Remove superfluous Vote suffix
xla Jan 3, 2020
61dce99
Remove superfluous Vote suffix
xla Jan 3, 2020
a0c842f
Use markdown emphasize & link for types
xla Jan 3, 2020
bad770b
Use _ for literal suffix
xla Jan 3, 2020
63c673c
Make clippy rule exception for db names
xla Jan 3, 2020
56d1f48
Remove print statements
xla Jan 3, 2020
7bf1093
Replace similiar names
xla Jan 3, 2020
a01d234
Allow multiple crate versions
xla Jan 3, 2020
41cbd02
Consolidate impl blocks
xla Jan 4, 2020
67f7358
Use Self consistently
xla Jan 4, 2020
0ee13e2
Mark functions as const where possible
xla Jan 5, 2020
37078c4
Unify matches with equal arm bodies
xla Jan 5, 2020
834263c
Remove unecessary closures
xla Jan 5, 2020
9b9e875
Remove possible truncation
xla Jan 5, 2020
85100a3
Remove possible truncation
xla Jan 5, 2020
71f0cd3
Allow fallible from impls for now and leave todos
xla Jan 5, 2020
d3651c4
Replace enum glob use with proper qualified variants
xla Jan 5, 2020
da3a7c9
Allow integer arithmetic
xla Jan 5, 2020
749c679
Use proper division methoods on ints
xla Jan 5, 2020
c4d4c56
Allow index slicing
xla Jan 5, 2020
21b2a8f
Allow index slicing
xla Jan 5, 2020
15e37ca
Alllow default_trait_access mostly for defive
xla Jan 5, 2020
1144a4f
Allow sign loss for cast
xla Jan 7, 2020
d7ef18a
Address wildcard enum matches
xla Jan 7, 2020
85e57aa
Avoid warning for inline attribute
xla Jan 7, 2020
d53540f
Ignore must use lint as it is only a recommendation
xla Jan 7, 2020
ab338d7
Address missing docs
xla Jan 7, 2020
8534547
Remove superfluous unwrap_or_else
xla Jan 7, 2020
bd9b608
Address unwrap used on Options
xla Jan 7, 2020
5cfcb37
Replace result unwrap with expect
xla Jan 18, 2020
255ba11
Rework use of unreachable
xla Jan 18, 2020
3dbd0d8
Remove use of panic
xla Jan 18, 2020
dd405fa
Remove bare panics
xla Jan 20, 2020
e90bc89
Remove needles pass by value
xla Jan 20, 2020
2b9b699
Replace explicit iter calls
xla Jan 20, 2020
99e323c
Remove needless borrows
xla Jan 20, 2020
7750986
Address cast possible wrap
xla Jan 20, 2020
ab1e77a
Merge branch 'master' into xla/clippy
xla Jan 20, 2020
dfb094a
Add documentation for unit enum
xla Jan 20, 2020
3ebb516
Add cargo fields to lite crate
xla Jan 20, 2020
357a791
Address lite merge follow-ups
xla Jan 20, 2020
b2b5db7
Address missing docs
xla Jan 20, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .gitignore
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Generated by Cargo
# will have compiled files and executables
/target/
target

# Remove Cargo.lock from gitignore if creating an executable, leave it for libraries
# More information here https://doc.rust-lang.org/cargo/guide/cargo-toml-vs-cargo-lock.html
Expand Down
2 changes: 1 addition & 1 deletion tendermint/src/abci.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//! Application BlockChain Interface (ABCI)
//! Application Block Chain Interface (ABCI)
//!
//! NOTE: This module contains types for ABCI responses as consumed from RPC
//! endpoints. It does not contain an ABCI protocol implementation.
Expand Down
16 changes: 8 additions & 8 deletions tendermint/src/abci/code.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,17 @@ pub enum Code {
}

impl Default for Code {
fn default() -> Code {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in general I think this is probably good, but I wonder if in the case of large impl blocks if it would be better to use the explicit type name.

Code::Ok
fn default() -> Self {
Self::Ok
}
}

impl Code {
/// Was the response OK?
pub fn is_ok(self) -> bool {
match self {
Code::Ok => true,
Code::Err(_) => false,
Self::Ok => true,
Self::Err(_) => false,
}
}

Expand All @@ -45,16 +45,16 @@ impl Code {
}

impl From<u32> for Code {
fn from(value: u32) -> Code {
fn from(value: u32) -> Self {
match value {
0 => Code::Ok,
err => Code::Err(err),
0 => Self::Ok,
err => Self::Err(err),
}
}
}

impl From<Code> for u32 {
fn from(code: Code) -> u32 {
fn from(code: Code) -> Self {
match code {
Code::Ok => 0,
Code::Err(err) => err,
Expand Down
2 changes: 1 addition & 1 deletion tendermint/src/abci/data.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ impl FromStr for Data {
.or_else(|_| hex::decode(s))
.map_err(|_| ErrorKind::Parse)?;

Ok(Data(bytes))
Ok(Self(bytes))
}
}

Expand Down
8 changes: 4 additions & 4 deletions tendermint/src/abci/gas.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,19 +18,19 @@ pub struct Gas(u64);

impl Gas {
/// Get the inner integer value
pub fn value(self) -> u64 {
pub const fn value(self) -> u64 {
self.0
}
}

impl From<u64> for Gas {
fn from(amount: u64) -> Gas {
Gas(amount)
fn from(amount: u64) -> Self {
Self(amount)
}
}

impl From<Gas> for u64 {
fn from(gas: Gas) -> u64 {
fn from(gas: Gas) -> Self {
gas.0
}
}
Expand Down
2 changes: 1 addition & 1 deletion tendermint/src/abci/log.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ impl Log {

impl From<&str> for Log {
fn from(s: &str) -> Self {
Log(s.to_owned())
Self(s.to_owned())
}
}

Expand Down
2 changes: 1 addition & 1 deletion tendermint/src/abci/path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,6 @@ impl FromStr for Path {
type Err = Error;

fn from_str(s: &str) -> Result<Self, Error> {
Ok(Path(s.to_owned()))
Ok(Self(s.to_owned()))
}
}
2 changes: 1 addition & 1 deletion tendermint/src/abci/proof.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ impl FromStr for Proof {

fn from_str(s: &str) -> Result<Self, Error> {
let bytes = Hex::upper_case().decode(s)?;
Ok(Proof(bytes))
Ok(Self(bytes))
}
}

Expand Down
4 changes: 2 additions & 2 deletions tendermint/src/abci/tag.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ impl FromStr for Key {
type Err = Error;

fn from_str(s: &str) -> Result<Self, Error> {
Ok(Key(s.into()))
Ok(Self(s.into()))
}
}

Expand All @@ -52,7 +52,7 @@ impl FromStr for Value {
type Err = Error;

fn from_str(s: &str) -> Result<Self, Error> {
Ok(Value(s.into()))
Ok(Self(s.into()))
}
}

Expand Down
9 changes: 5 additions & 4 deletions tendermint/src/abci/transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,14 @@ pub struct Transaction(Vec<u8>);

impl Transaction {
/// Create a new raw transaction from a byte vector
pub fn new<V>(into_vec: V) -> Transaction
pub fn new<V>(into_vec: V) -> Self
where
V: Into<Vec<u8>>,
{
Transaction(into_vec.into())
Self(into_vec.into())
}

#[allow(clippy::missing_const_for_fn)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are there exceptions?

/// Convert this transaction into a byte vector
pub fn into_vec(self) -> Vec<u8> {
self.0
Expand Down Expand Up @@ -68,11 +69,11 @@ pub struct Data {

impl Data {
/// Create a new transaction data collection
pub fn new<I>(into_transactions: I) -> Data
pub fn new<I>(into_transactions: I) -> Self
where
I: Into<Vec<Transaction>>,
{
Data {
Self {
txs: Some(into_transactions.into()),
}
}
Expand Down
12 changes: 6 additions & 6 deletions tendermint/src/abci/transaction/hash.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//! Transaction hashes

use crate::error::{Error, ErrorKind};
use crate::{Error, ErrorKind};
use serde::{de, Deserialize, Deserializer, Serialize, Serializer};
use std::{
fmt::{self, Debug, Display},
Expand All @@ -18,8 +18,8 @@ pub struct Hash([u8; LENGTH]);

impl Hash {
/// Create a new transaction hash from raw bytes
pub fn new(bytes: [u8; LENGTH]) -> Hash {
Hash(bytes)
pub const fn new(bytes: [u8; LENGTH]) -> Self {
Self(bytes)
}

/// Borrow the transaction hash as a byte slice
Expand All @@ -36,7 +36,7 @@ impl AsRef<[u8]> for Hash {

impl ConstantTimeEq for Hash {
#[inline]
fn ct_eq(&self, other: &Hash) -> subtle::Choice {
fn ct_eq(&self, other: &Self) -> subtle::Choice {
self.as_bytes().ct_eq(other.as_bytes())
}
}
Expand Down Expand Up @@ -70,9 +70,9 @@ impl FromStr for Hash {
return Err(ErrorKind::Parse.into());
}

let mut result_bytes = [0u8; LENGTH];
let mut result_bytes = [0_u8; LENGTH];
result_bytes.copy_from_slice(&bytes);
Ok(Hash(result_bytes))
Ok(Self(result_bytes))
}
}

Expand Down
24 changes: 12 additions & 12 deletions tendermint/src/account.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//! Tendermint accounts

use crate::error::{Error, ErrorKind};
use crate::{Error, ErrorKind};
use serde::{de, Deserialize, Deserializer, Serialize, Serializer};
use sha2::{Digest, Sha256};
use signatory::{ecdsa::curve::secp256k1, ed25519};
Expand All @@ -20,8 +20,8 @@ pub struct Id([u8; LENGTH]);

impl Id {
/// Create a new account ID from raw bytes
pub fn new(bytes: [u8; LENGTH]) -> Id {
Id(bytes)
pub const fn new(bytes: [u8; LENGTH]) -> Self {
Self(bytes)
}

/// Borrow the account ID as a byte slice
Expand All @@ -38,7 +38,7 @@ impl AsRef<[u8]> for Id {

impl ConstantTimeEq for Id {
#[inline]
fn ct_eq(&self, other: &Id) -> subtle::Choice {
fn ct_eq(&self, other: &Self) -> subtle::Choice {
self.as_bytes().ct_eq(other.as_bytes())
}
}
Expand All @@ -60,21 +60,21 @@ impl Debug for Id {

// TODO: should be RIPEMD160(SHA256(pk))
impl From<secp256k1::PublicKey> for Id {
fn from(pk: secp256k1::PublicKey) -> Id {
fn from(pk: secp256k1::PublicKey) -> Self {
let digest = Sha256::digest(pk.as_bytes());
let mut bytes = [0u8; LENGTH];
let mut bytes = [0_u8; LENGTH];
bytes.copy_from_slice(&digest[..LENGTH]);
Id(bytes)
Self(bytes)
}
}

// SHA256(pk)[:20]
impl From<ed25519::PublicKey> for Id {
fn from(pk: ed25519::PublicKey) -> Id {
fn from(pk: ed25519::PublicKey) -> Self {
let digest = Sha256::digest(pk.as_bytes());
let mut bytes = [0u8; LENGTH];
let mut bytes = [0_u8; LENGTH];
bytes.copy_from_slice(&digest[..LENGTH]);
Id(bytes)
Self(bytes)
}
}

Expand All @@ -92,9 +92,9 @@ impl FromStr for Id {
return Err(ErrorKind::Parse.into());
}

let mut result_bytes = [0u8; LENGTH];
let mut result_bytes = [0_u8; LENGTH];
result_bytes.copy_from_slice(&bytes);
Ok(Id(result_bytes))
Ok(Self(result_bytes))
}
}

Expand Down
36 changes: 20 additions & 16 deletions tendermint/src/amino_types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,27 +3,31 @@

#![allow(missing_docs)]

pub mod block_id;
pub mod ed25519;
pub mod message;
pub mod ping;
pub mod proposal;
pub mod remote_error;
pub mod signature;
pub mod time;
pub mod validate;
pub mod version;
pub mod vote;
mod block_id;
mod ed25519;
mod message;
mod ping;
mod proposal;
mod remote_error;
mod signature;
mod time;
mod validate;
mod version;
mod vote;
Copy link
Contributor

@tarcieri tarcieri Jan 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The disadvantage of relying on re-exports of types defined in non-pub modules is rustc will refer to those types by their fully qualified path regardless of the visibility of the module it's contained in, which leaves someone trying to debug in the bizarre position that the compiler referring to a path which cannot be accessed and does not exist in rustdoc.

That said, I do sometimes do this myself, but it's usually when I'm trying to create a crate where the code is factored into modules internally but all types are exported at the toplevel (and when I do that, it's generally because the external API surface of the crate is small to begin with).

The other disadvantage is this approach eliminates the ability to have module-level documentation visible in the rustdoc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would you propose generally? I think amino_types module organisation is ripe for a review anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-export them, but leave the containing modules pub if they're part of the public API


pub use self::{
block_id::{BlockId, CanonicalBlockId, CanonicalPartSetHeader, PartsSetHeader},
block_id::{BlockId, Canonical as CanonicalBlockId, CanonicalPartSetHeader, PartsSetHeader},
ed25519::{PubKeyRequest, PubKeyResponse, AMINO_NAME as PUBKEY_AMINO_NAME},
ping::{PingRequest, PingResponse, AMINO_NAME as PING_AMINO_NAME},
message::Message as AminoMessage,
ping::{Request as PingRequest, Response as PingResponse, AMINO_NAME as PING_AMINO_NAME},
proposal::{SignProposalRequest, SignedProposalResponse, AMINO_NAME as PROPOSAL_AMINO_NAME},
remote_error::RemoteError,
signature::{SignableMsg, SignedMsgType},
time::TimeMsg,
time::Msg as TimeMsg,
validate::ConsensusMessage,
version::ConsensusVersion,
vote::{SignVoteRequest, SignedVoteResponse, AMINO_NAME as VOTE_AMINO_NAME},
version::Consensus as ConsensusVersion,
vote::{
Canonical as CanonicalVote, SignVoteRequest, SignedVoteResponse, Vote,
AMINO_NAME as VOTE_AMINO_NAME,
},
};
24 changes: 11 additions & 13 deletions tendermint/src/amino_types/block_id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ pub struct BlockId {

impl BlockId {
pub fn new(hash: Vec<u8>, parts_header: Option<PartsSetHeader>) -> Self {
BlockId { hash, parts_header }
Self { hash, parts_header }
}
}

Expand All @@ -35,7 +35,7 @@ impl block::ParseId for BlockId {
impl From<&block::Id> for BlockId {
fn from(bid: &block::Id) -> Self {
let bid_hash = bid.hash.as_bytes();
BlockId::new(
Self::new(
bid_hash.to_vec(),
bid.parts.as_ref().map(PartsSetHeader::from),
)
Expand All @@ -55,14 +55,14 @@ impl ConsensusMessage for BlockId {
}

#[derive(Clone, PartialEq, Message)]
pub struct CanonicalBlockId {
pub struct Canonical {
#[prost(bytes, tag = "1")]
pub hash: Vec<u8>,
#[prost(message, tag = "2")]
pub parts_header: Option<CanonicalPartSetHeader>,
}

impl block::ParseId for CanonicalBlockId {
impl block::ParseId for Canonical {
fn parse_block_id(&self) -> Result<block::Id, Error> {
let hash = Hash::new(hash::Algorithm::Sha256, &self.hash)?;
let parts_header = self
Expand All @@ -83,24 +83,22 @@ pub struct PartsSetHeader {

impl PartsSetHeader {
pub fn new(total: i64, hash: Vec<u8>) -> Self {
PartsSetHeader { total, hash }
Self { total, hash }
}
}

impl From<&parts::Header> for PartsSetHeader {
fn from(parts: &parts::Header) -> Self {
PartsSetHeader::new(parts.total as i64, parts.hash.as_bytes().to_vec())
}
}

impl PartsSetHeader {
fn parse_parts_header(&self) -> Option<block::parts::Header> {
Hash::new(hash::Algorithm::Sha256, &self.hash)
.map(|hash| block::parts::Header::new(self.total as u64, hash))
.ok()
}
}

impl From<&parts::Header> for PartsSetHeader {
fn from(parts: &parts::Header) -> Self {
Self::new(parts.total as i64, parts.hash.as_bytes().to_vec())
}
}

impl ConsensusMessage for PartsSetHeader {
fn validate_basic(&self) -> Result<(), ValidationError> {
if self.total < 0 {
Expand Down
Loading