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.

Sign up for GitHub

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

Jump to bottom

Introduce rustfmt in a non-invasive manner #1040

Merged
merged 8 commits into from Jul 19, 2022
+234 −160
Merged

Introduce rustfmt in a non-invasive manner #1040

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fd1c658
Add a rustfmt configuration file with explicit ignores
tcharding Jun 6, 2022
6461e2d
Run formatter on examples/
tcharding Jun 6, 2022
65d19d9
Refactor compile_error message
tcharding Jun 6, 2022
01471f7
Shield hash_newtypes from rustfmt
tcharding Jun 6, 2022
7b50a96
Do not format prelude module
tcharding Jun 6, 2022
fd217e7
Use f instead of formatter
tcharding Jun 20, 2022
743b197
Add use for Unexpected
tcharding Jun 21, 2022
ee9a3ec
Enable formatter for "src"
tcharding Jun 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 6 additions & 12 deletions examples/bip32.rs
View file
Open in desktop
@@ -1,17 +1,14 @@
extern crate bitcoin;

use std::{env, process};
use std::str::FromStr;
use std::{env, process};

use bitcoin::hashes::hex::FromHex;
use bitcoin::secp256k1::ffi::types::AlignedType;
use bitcoin::secp256k1::Secp256k1;
use bitcoin::PublicKey;
use bitcoin::util::bip32::ExtendedPrivKey;
use bitcoin::util::bip32::ExtendedPubKey;
use bitcoin::util::bip32::DerivationPath;
use bitcoin::util::bip32::ChildNumber;
use bitcoin::util::address::Address;
use bitcoin::secp256k1::ffi::types::AlignedType;
use bitcoin::hashes::hex::FromHex;
use bitcoin::util::bip32::{ChildNumber, DerivationPath, ExtendedPrivKey, ExtendedPubKey};
use bitcoin::PublicKey;

fn main() {
// This example derives root xprv from a 32-byte seed,
Expand Down Expand Up @@ -55,10 +52,7 @@ fn main() {
// generate first receiving address at m/0/0
// manually creating indexes this time
let zero = ChildNumber::from_normal_idx(0).unwrap();
let public_key = xpub.derive_pub(&secp, &vec![zero, zero])
.unwrap()
.public_key;
let public_key = xpub.derive_pub(&secp, &vec![zero, zero]).unwrap().public_key;
let address = Address::p2wpkh(&PublicKey::new(public_key), network).unwrap();
println!("First receiving address: {}", address);

}
7 changes: 2 additions & 5 deletions examples/handshake.rs
View file
Open in desktop
@@ -1,9 +1,9 @@
extern crate bitcoin;

use std::io::{BufReader, Write};
use std::net::{IpAddr, Ipv4Addr, Shutdown, SocketAddr, TcpStream};
use std::time::{SystemTime, UNIX_EPOCH};
use std::{env, process};
use std::io::{Write, BufReader};

use bitcoin::consensus::{encode, Decodable};
use bitcoin::network::{address, constants, message, message_network};
Expand Down Expand Up @@ -80,10 +80,7 @@ fn build_version_message(address: SocketAddr) -> message::NetworkMessage {
let services = constants::ServiceFlags::NONE;

// "standard UNIX timestamp in seconds"
let timestamp = SystemTime::now()
.duration_since(UNIX_EPOCH)
.expect("Time error")
.as_secs();
let timestamp = SystemTime::now().duration_since(UNIX_EPOCH).expect("Time error").as_secs();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer these to be always on a new line. The practical consequence is that you can quickly comment out things if types match and diffs are easier to understand. I had real-life experience with long iterator chain where types were preserved (lot of maps) and could easily experiment with the code by commenting/adding lines.

Copy link
Member

Choose a reason for hiding this comment

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

Bleh, there's another practical consequence of such things - people miss context when reviewing/reading code
Cause
Things
End
Up
Too
Vertical.

Copy link
Collaborator

Choose a reason for hiding this comment

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

True :(

Copy link
Contributor

@dpc dpc Jun 20, 2022 •
edited

Choose a reason for hiding this comment

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

IMO: If a chain of things is so long that it hinders your ability to see stuff, you have some other problems to address.

Like in this case the whole thing could be some fn get_current_unix_ts_secs(), because why would anyone wanted to parse this lower-level 4 element implementation detail, when looking at the higher level code.

It's an example code, so 🤷 , but this whole function could be more idiomatically:

fn build_version_message(address: SocketAddr) -> message::NetworkMessage {
    message::NetworkMessage::Version(
        VersionMessage::new()
          .service_flags(ServiceFlags::NONE),
          .timestamp(get_current_unix_ts_secs() as i64),
          .recv_addr(Address::new(&address, ServiceFlags::NONE)),
          .from_addr(Address::new(&my_address, ServiceFlags::NONE)),
           /* ... */
          .build()
   )
}

with comments about what each thing means on the builder methoder, instead of each variable, and 1/4 of the LoC size, while being much easier to understand.

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW. I wish Rust just made a typed builder a language thing, because in the absence of optional and named arguments, builders are a common necessity.

Copy link
Member

Choose a reason for hiding this comment

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

IMO chain_width should really be 100, I thought it was a concrete line length not % lol.

Copy link
Member

Choose a reason for hiding this comment

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

I really, really, really dont understand the rust fascination with deliberately ensuring code is as
vertical
as
is
at
all
possible
to
make
sure
you
lose
all
context.
If we say we want lines up to 100 chars long, that should include chains, I'm not sure why chains are in any way special?

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, so that is a vote for

max_width = 100
use_small_heuristics = "Max"

And remove all the granular width configuration options, I don't believe we had previously considered that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Meh, neither is great and if there even exists optimal line, I don't think it can be expressed in code. So I give up on this discussion, do as you wish.

Copy link
Member Author

Choose a reason for hiding this comment

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

EDIT: Deleted my comment after seeing more discussion on this below.


// "The network address of the node receiving this message"
let addr_recv = address::Address::new(&address, constants::ServiceFlags::NONE);
Expand Down
86 changes: 85 additions & 1 deletion rustfmt.toml
View file
Open in desktop
@@ -1 +1,85 @@
disable_all_formatting = true
# Eventually this shoud be: ignore = []
ignore = [
"src/blockdata",
"src/consensus",
"src/network",
"src/util",
]

hard_tabs = false
tab_spaces = 4
newline_style = "Auto"
indent_style = "Block"

max_width = 100 # This is number of characters.
# `use_small_heuristics` is ignored if the granular width config values are explicitly set.
use_small_heuristics = "Max" # "Max" == All granular width settings same as `max_width`.
# # Granular width configuration settings. These are percentages of `max_width`.
# fn_call_width = 60
# attr_fn_like_width = 70
# struct_lit_width = 18
# struct_variant_width = 35
# array_width = 60
# chain_width = 60
# single_line_if_else_max_width = 50

wrap_comments = false
format_code_in_doc_comments = false
comment_width = 100 # Default 80
normalize_comments = false
normalize_doc_attributes = false
format_strings = false
format_macro_matchers = false
format_macro_bodies = true
hex_literal_case = "Preserve"
empty_item_single_line = true
struct_lit_single_line = true
fn_single_line = true # Default false
where_single_line = false
imports_indent = "Block"
imports_layout = "Mixed"
imports_granularity = "Module" # Default "Preserve"
group_imports = "StdExternalCrate" # Default "Preserve"
reorder_imports = true
reorder_modules = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

I really don't like these much. Sometimes it may be nicer to logically group the imports, e.g. pub use at top. Sadly, there's no way to implement such logic in rustfmt.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO "reliable benefit of predictable ordering" > "barely ever used benefit of expressing some logical groups, that also has to be maintained over time (which it won't)".

Copy link
Member Author

Choose a reason for hiding this comment

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

+1 for what @dpc said. I do understand the desire to have 'pub use' statements separate but its just another implicit policy for devs to have to remember and like dpc said such groupings never get maintained.

TBH I'm kind of disheartened by this discussion, taking care of the import statements is, IMO, a no-brainer. If we cannot come to some basic agreement that at some stage its better to use tools than developer time it is really hard to make progress on this PR.

Lets not become a project where no changes can be made because there is always some negative in any change - implying that we get stuck with negatives we are unable to address. rustfmt is not perfect, nothing is, rustfmt is, an can always be, constantly improved but refusing to use the tools we have just stagnates us.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Honestly, I don't care about order being lexicographically sorted much and if there's no setting to put pub items first (what I do in my projects) and enforcing is deemed to be too annoying, I don't care about these flags much. If there was a setting I'd want it on and don't care about the rest.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I remembered one more annoying thing about sorting: if you rename an item then you also have to move it so instead of clear -+ diffl lines being one after another you get them separated. Not saying that it's worth turning off sorting but if there was a nicer solution, that'd be great.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree on the diff lines separated thing. So just to make sure I've got it, we can leave the two 'reorder' flags on and acknowledge that it is a desired improvement to rustfmt to be able to separate 'pub use' statements from other imports?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, I was thinking about opening issues at their repo once we agree what we need.

reorder_impl_items = false
type_punctuation_density = "Wide"
space_before_colon = false
space_after_colon = true
spaces_around_ranges = false
binop_separator = "Front"
remove_nested_parens = true
combine_control_expr = true
overflow_delimited_expr = false
struct_field_align_threshold = 0
enum_discrim_align_threshold = 0
match_arm_blocks = false # Default true
match_arm_leading_pipes = "Never"
force_multiline_blocks = false
fn_args_layout = "Tall"
brace_style = "SameLineWhere"
control_brace_style = "AlwaysSameLine"
trailing_semicolon = true
trailing_comma = "Vertical"
match_block_trailing_comma = false
blank_lines_upper_bound = 1
blank_lines_lower_bound = 0
edition = "2018"
version = "One"
inline_attribute_width = 0
format_generated_files = true
merge_derives = true
use_try_shorthand = false
use_field_init_shorthand = false
force_explicit_abi = true
condense_wildcard_suffixes = false
color = "Auto"
required_version = "1.5.1"
unstable_features = false
disable_all_formatting = false
skip_children = false
hide_parse_errors = false
error_on_line_overflow = false
error_on_unformatted = false
emit_mode = "Files"
make_backup = false
60 changes: 33 additions & 27 deletions src/hash_types.rs
View file
Open in desktop
Expand Up @@ -9,8 +9,7 @@
//! hash).
//!

use bitcoin_hashes::{sha256, sha256d, hash160, hash_newtype};

#[rustfmt::skip]
macro_rules! impl_hashencode {
($hashtype:ident) => {
impl $crate::consensus::Encodable for $hashtype {
Expand All @@ -25,43 +24,50 @@ macro_rules! impl_hashencode {
Ok(Self::from_inner(<<$hashtype as $crate::hashes::Hash>::Inner>::consensus_decode(r)?))
}
}
}
};
}

hash_newtype!(
Txid, sha256d::Hash, 32, doc="A bitcoin transaction hash/transaction ID.
// newtypes module is solely here so we can rustfmt::skip.
pub use newtypes::*;

#[rustfmt::skip]
mod newtypes {
use crate::hashes::{sha256, sha256d, hash160, hash_newtype};

hash_newtype!(
Txid, sha256d::Hash, 32, doc="A bitcoin transaction hash/transaction ID.

For compatibility with the existing Bitcoin infrastructure and historical
and current versions of the Bitcoin Core software itself, this and
other [`sha256d::Hash`] types, are serialized in reverse
byte order when converted to a hex string via [`std::fmt::Display`] trait operations.
See [`hashes::Hash::DISPLAY_BACKWARD`] for more details.
");
hash_newtype!(Wtxid, sha256d::Hash, 32, doc="A bitcoin witness transaction ID.");
hash_newtype!(BlockHash, sha256d::Hash, 32, doc="A bitcoin block hash.");
hash_newtype!(Sighash, sha256d::Hash, 32, doc="Hash of the transaction according to the signature algorithm");
hash_newtype!(Wtxid, sha256d::Hash, 32, doc="A bitcoin witness transaction ID.");
hash_newtype!(BlockHash, sha256d::Hash, 32, doc="A bitcoin block hash.");
hash_newtype!(Sighash, sha256d::Hash, 32, doc="Hash of the transaction according to the signature algorithm");

hash_newtype!(PubkeyHash, hash160::Hash, 20, doc="A hash of a public key.");
hash_newtype!(ScriptHash, hash160::Hash, 20, doc="A hash of Bitcoin Script bytecode.");
hash_newtype!(WPubkeyHash, hash160::Hash, 20, doc="SegWit version of a public key hash.");
hash_newtype!(WScriptHash, sha256::Hash, 32, doc="SegWit version of a Bitcoin Script bytecode hash.");
hash_newtype!(PubkeyHash, hash160::Hash, 20, doc="A hash of a public key.");
hash_newtype!(ScriptHash, hash160::Hash, 20, doc="A hash of Bitcoin Script bytecode.");
hash_newtype!(WPubkeyHash, hash160::Hash, 20, doc="SegWit version of a public key hash.");
hash_newtype!(WScriptHash, sha256::Hash, 32, doc="SegWit version of a Bitcoin Script bytecode hash.");

hash_newtype!(TxMerkleNode, sha256d::Hash, 32, doc="A hash of the Merkle tree branch or root for transactions");
hash_newtype!(WitnessMerkleNode, sha256d::Hash, 32, doc="A hash corresponding to the Merkle tree root for witness data");
hash_newtype!(WitnessCommitment, sha256d::Hash, 32, doc="A hash corresponding to the witness structure commitment in the coinbase transaction");
hash_newtype!(XpubIdentifier, hash160::Hash, 20, doc="XpubIdentifier as defined in BIP-32.");
hash_newtype!(TxMerkleNode, sha256d::Hash, 32, doc="A hash of the Merkle tree branch or root for transactions");
hash_newtype!(WitnessMerkleNode, sha256d::Hash, 32, doc="A hash corresponding to the Merkle tree root for witness data");
hash_newtype!(WitnessCommitment, sha256d::Hash, 32, doc="A hash corresponding to the witness structure commitment in the coinbase transaction");
hash_newtype!(XpubIdentifier, hash160::Hash, 20, doc="XpubIdentifier as defined in BIP-32.");

hash_newtype!(FilterHash, sha256d::Hash, 32, doc="Filter hash, as defined in BIP-157");
hash_newtype!(FilterHeader, sha256d::Hash, 32, doc="Filter header, as defined in BIP-157");
hash_newtype!(FilterHash, sha256d::Hash, 32, doc="Filter hash, as defined in BIP-157");
hash_newtype!(FilterHeader, sha256d::Hash, 32, doc="Filter header, as defined in BIP-157");

impl_hashencode!(Txid);
impl_hashencode!(Wtxid);
impl_hashencode!(BlockHash);
impl_hashencode!(Sighash);

impl_hashencode!(Txid);
impl_hashencode!(Wtxid);
impl_hashencode!(BlockHash);
impl_hashencode!(Sighash);
impl_hashencode!(TxMerkleNode);
impl_hashencode!(WitnessMerkleNode);

impl_hashencode!(TxMerkleNode);
impl_hashencode!(WitnessMerkleNode);

impl_hashencode!(FilterHash);
impl_hashencode!(FilterHeader);
impl_hashencode!(FilterHash);
impl_hashencode!(FilterHeader);
}
59 changes: 28 additions & 31 deletions src/internal_macros.rs
View file
Open in desktop
Expand Up @@ -100,11 +100,9 @@ macro_rules! impl_array_newtype {
type Output = <[$ty] as core::ops::Index<I>>::Output;

#[inline]
fn index(&self, index: I) -> &Self::Output {
&self.0[index]
}
fn index(&self, index: I) -> &Self::Output { &self.0[index] }
}
}
};
}

macro_rules! display_from_debug {
Expand All @@ -114,7 +112,7 @@ macro_rules! display_from_debug {
core::fmt::Debug::fmt(self, f)
}
}
}
};
}

#[cfg(test)]
Expand Down Expand Up @@ -142,8 +140,8 @@ macro_rules! serde_string_impl {
impl<'de> $crate::serde::de::Visitor<'de> for Visitor {
type Value = $name;

fn expecting(&self, formatter: &mut Formatter) -> fmt::Result {
formatter.write_str($expecting)
fn expecting(&self, f: &mut Formatter) -> fmt::Result {
f.write_str($expecting)
}

fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
Expand Down Expand Up @@ -190,8 +188,8 @@ macro_rules! serde_struct_human_string_impl {
impl<'de> $crate::serde::de::Visitor<'de> for Visitor {
type Value = $name;

fn expecting(&self, formatter: &mut Formatter) -> fmt::Result {
formatter.write_str($expecting)
fn expecting(&self, f: &mut Formatter) -> fmt::Result {
f.write_str($expecting)
}

fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
Expand All @@ -215,8 +213,8 @@ macro_rules! serde_struct_human_string_impl {
impl<'de> $crate::serde::de::Visitor<'de> for EnumVisitor {
type Value = Enum;

fn expecting(&self, formatter: &mut Formatter) -> fmt::Result {
formatter.write_str("a field name")
fn expecting(&self, f: &mut Formatter) -> fmt::Result {
f.write_str("a field name")
}

fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
Expand Down Expand Up @@ -246,8 +244,8 @@ macro_rules! serde_struct_human_string_impl {
impl<'de> $crate::serde::de::Visitor<'de> for Visitor {
type Value = $name;

fn expecting(&self, formatter: &mut Formatter) -> fmt::Result {
formatter.write_str("a struct")
fn expecting(&self, f: &mut Formatter) -> fmt::Result {
f.write_str("a struct")
}

fn visit_seq<V>(self, mut seq: V) -> Result<Self::Value, V::Error>
Expand Down Expand Up @@ -352,8 +350,7 @@ macro_rules! serde_struct_human_string_impl {
/// - core::str::FromStr
/// - hashes::hex::FromHex
macro_rules! impl_bytes_newtype {
($t:ident, $len:literal) => (

($t:ident, $len:literal) => {
impl core::fmt::LowerHex for $t {
fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result {
for &ch in self.0.iter() {
Expand All @@ -378,9 +375,9 @@ macro_rules! impl_bytes_newtype {
impl $crate::hashes::hex::FromHex for $t {
fn from_byte_iter<I>(iter: I) -> Result<Self, $crate::hashes::hex::Error>
where
I: core::iter::Iterator<Item=Result<u8, $crate::hashes::hex::Error>>
+ core::iter::ExactSizeIterator
+ core::iter::DoubleEndedIterator,
I: core::iter::Iterator<Item = Result<u8, $crate::hashes::hex::Error>>
+ core::iter::ExactSizeIterator
+ core::iter::DoubleEndedIterator,
{
if iter.len() == $len {
let mut ret = [0; $len];
Expand Down Expand Up @@ -423,18 +420,20 @@ macro_rules! impl_bytes_newtype {
impl<'de> $crate::serde::de::Visitor<'de> for HexVisitor {
type Value = $t;

fn expecting(&self, formatter: &mut core::fmt::Formatter) -> core::fmt::Result {
formatter.write_str("an ASCII hex string")
fn expecting(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result {
f.write_str("an ASCII hex string")
}

fn visit_bytes<E>(self, v: &[u8]) -> Result<Self::Value, E>
where
E: $crate::serde::de::Error,
{
use $crate::serde::de::Unexpected;

if let Ok(hex) = core::str::from_utf8(v) {
$crate::hashes::hex::FromHex::from_hex(hex).map_err(E::custom)
} else {
return Err(E::invalid_value($crate::serde::de::Unexpected::Bytes(v), &self));
return Err(E::invalid_value(Unexpected::Bytes(v), &self));
}
}

Expand All @@ -453,8 +452,8 @@ macro_rules! impl_bytes_newtype {
impl<'de> $crate::serde::de::Visitor<'de> for BytesVisitor {
type Value = $t;

fn expecting(&self, formatter: &mut core::fmt::Formatter) -> core::fmt::Result {
formatter.write_str("a bytestring")
fn expecting(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result {
f.write_str("a bytestring")
}

fn visit_bytes<E>(self, v: &[u8]) -> Result<Self::Value, E>
Expand All @@ -475,7 +474,7 @@ macro_rules! impl_bytes_newtype {
}
}
}
)
};
}

macro_rules! user_enum {
Expand Down Expand Up @@ -531,8 +530,8 @@ macro_rules! user_enum {
impl<'de> $crate::serde::de::Visitor<'de> for Visitor {
type Value = $name;

fn expecting(&self, formatter: &mut Formatter) -> fmt::Result {
formatter.write_str("an enum value")
fn expecting(&self, f: &mut Formatter) -> fmt::Result {
f.write_str("an enum value")
}

fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
Expand Down Expand Up @@ -586,9 +585,7 @@ macro_rules! write_err {

/// Asserts a boolean expression at compile time.
macro_rules! const_assert {
($x:expr) => {
{
const _: [(); 0 - !$x as usize] = [];
}
};
($x:expr) => {{
const _: [(); 0 - !$x as usize] = [];
}};
}