From 58876e2be973f7995aa79d1f263ed75520676636 Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Fri, 24 Feb 2023 21:38:36 +0100 Subject: [PATCH 1/2] Remove unused macro This macro was a part of previous failed design and is no longer used. This change removes it. --- hashes/src/util.rs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/hashes/src/util.rs b/hashes/src/util.rs index 743e94c7..d9d6f5e1 100644 --- a/hashes/src/util.rs +++ b/hashes/src/util.rs @@ -302,13 +302,6 @@ macro_rules! hash_newtype_struct { }; } -#[doc(hidden)] -#[macro_export] -macro_rules! hash_newtype_our_attrs { - (hash_newtype($($attr:tt)*)) => { $($attr)* }; - ($($ignore:tt)*) => {}; -} - #[doc(hidden)] #[macro_export] macro_rules! hash_newtype_get_direction { From 8ccfb412c18abaabc4365ab6a2e61adb6a8951f2 Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Fri, 24 Feb 2023 21:39:29 +0100 Subject: [PATCH 2/2] Improve documentation of `hash_newtype!` The macro is non-trivial, so documenting it well is very useful. This change improves both user-facing and developer-facing code with appropriate warnings about the limitations of the code and Rust macro system. --- hashes/src/util.rs | 68 +++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 64 insertions(+), 4 deletions(-) diff --git a/hashes/src/util.rs b/hashes/src/util.rs index d9d6f5e1..4448ef88 100644 --- a/hashes/src/util.rs +++ b/hashes/src/util.rs @@ -121,11 +121,11 @@ macro_rules! engine_input_impl( /// } /// ``` /// -/// You can use any valid visibility specifier in place of `pub` or you can leave it out if you -/// don't want the type or its field to be private. +/// You can use any valid visibility specifier in place of `pub` or you can omit either or both, if +/// you want the type or its field to be private. /// -/// Whether the hash is reversed or not depends on the inner type. However you can override it like -/// this: +/// Whether the hash is reversed or not when displaying depends on the inner type. However you can +/// override it like this: /// /// ``` /// # use bitcoin_hashes::{hash_newtype, sha256}; @@ -156,6 +156,40 @@ macro_rules! engine_input_impl( /// struct Newtype2(hash160::Hash); /// } /// ``` +/// +/// Note: the macro is internally recursive. If you use too many attributes (> 256 tokens) you may +/// hit recursion limit. If you have so many attributes for a good reason, just raising the limit +/// should be OK. Note however that attribute-processing part has to use [TT muncher] which has +/// quadratic complexity, so having many attributes may blow up compile time. This should be rare. +/// +/// [TT muncher]: https://danielkeep.github.io/tlborm/book/pat-incremental-tt-munchers.html +/// +// Ever heard of legendary comments warning developers to not touch the code? Yep, here's another +// one. The following code is written the way it is for some specific reasons. If you think you can +// simplify it, I suggest spending your time elsewhere. +// +// If you looks at the code carefully you might ask these questions: +// +// * Why are attributes using `tt` and not `meta`?! +// * Why are the macros split like that?! +// * Why use recursion instead of `$()*`? +// +// None of these are here by accident. For some reason unknown to me, if you accept an argument to +// macro with any fragment specifier other than `tt` it will **not** match any of the rules +// requiring a specific token. Yep, I tried it, I literally got error that `hash_newtype` doesn't +// match `hash_newtype`. So all input attributes must be `tt`. +// +// Originally I wanted to define a bunch of macros that would filter-out hash_type attributes. Then +// I remembered (by seeing compiler error) that calling macros is not allowed inside attributes. +// And no, you can't bypass it by calling a helper macro and passing "output of another macro" into +// it. The whole macro gets passed, not the resulting value. So we have to generate the entire +// attributes. And you can't just place an attribute-producing macro above struct - they are +// considered separate items. This is not C. +// +// Thus struct is generated in a separate macro together with attributes. And since the macro needs +// attributes as the input and I didn't want to create confusion by using `#[]` syntax *after* +// struct, I opted to use `{}` as a separator. Yes, a separator is required because an attribute +// may be composed of multiple token trees - that's the point of "double repetition". #[macro_export] macro_rules! hash_newtype { ($($(#[$($type_attrs:tt)*])* $type_vis:vis struct $newtype:ident($(#[$field_attrs:tt])* $field_vis:vis $hash:path);)+) => { @@ -275,6 +309,16 @@ macro_rules! hash_newtype { }; } +// Generates the struct only (no impls) +// +// This is a separate macro to make it more readable and have a separate interface that allows for +// two groups of type attributes: processed and not-yet-processed ones (think about it like +// computation via recursion). The macro recursively matches unprocessed attributes, popping them +// one at a time and either ignoring them (`hash_newtype`) or appending them to the list of +// processed attributes to be added to the struct. +// +// Once the list of not-yet-processed attributes is empty the struct is generated with processed +// attributes added. #[doc(hidden)] #[macro_export] macro_rules! hash_newtype_struct { @@ -302,6 +346,14 @@ macro_rules! hash_newtype_struct { }; } +// Extracts `hash_newtype(forward)` and `hash_newtype(backward)` attributes if any and turns them +// into bool, defaulting to `DISPLAY_BACKWARD` of the wrapped type if the attribute is omitted. +// +// Once an appropriate attribute is found we pass the remaining ones into another macro to detect +// duplicates/conflicts and report an error. +// +// FYI, no, we can't use a helper macro to first filter all `hash_newtype` attributes. We would be +// attempting to match on macros instead. So we must write `hashe_newtype` in each branch. #[doc(hidden)] #[macro_export] macro_rules! hash_newtype_get_direction { @@ -311,6 +363,9 @@ macro_rules! hash_newtype_get_direction { ($hash:ty, #[$($ignore:tt)*] $($others:tt)*) => { $crate::hash_newtype_get_direction!($hash, $($others)*) }; } +// Reports an error if any of the attributes is `hash_newtype($direction)`. +// +// This is used for detection of duplicates/conflicts, see the macro above. #[doc(hidden)] #[macro_export] macro_rules! hash_newtype_forbid_direction { @@ -326,6 +381,11 @@ macro_rules! hash_newtype_forbid_direction { }; } +// Checks (at compile time) that all `hash_newtype` attributes are known. +// +// An unknown attribute could be a typo that could cause problems - e.g. wrong display direction if +// it's missing. To prevent this, we call this macro above. The macro produces nothing unless an +// unknown attribute is found in which case it produces `compile_error!`. #[doc(hidden)] #[macro_export] macro_rules! hash_newtype_known_attrs {