Skip to content

Conversation

nyonson
Copy link
Contributor

@nyonson nyonson commented Sep 18, 2025

Inspired by #5003 (which I believe is the elidable_lifetime_names clippy rule) I experimented with some lifetime elision in the encoder_newtype macro. This drops the implicit requirement that the encoder struct's lifetime be called 'e to match the internal macro's usage.

This might be a subtle change in the requirements themselves since they are no longer explicitly tied together, however, I haven't been able to come up with a unit test to break that in any way.

@apoelstra
Copy link
Member

Wait, so now we're never tying the lifetime in the Encoder trait to the lifetime in the type? Do we even need the lifetime in the Encoder trait?

It seems like it's not actually used, and we can remove it entirely and stuff still works. Let's just do that.

@apoelstra
Copy link
Member

--- a/consensus_encoding/src/encode/mod.rs
+++ b/consensus_encoding/src/encode/mod.rs
@@ -12,7 +12,7 @@
 pub trait Encodable {
     /// The encoder associated with this type. Conceptually, the encoder is like
     /// an iterator which yields byte slices.
-    type Encoder<'s>: Encoder<'s>
+    type Encoder<'s>: Encoder
     where
         Self: 's;

@@ -21,7 +21,7 @@ pub trait Encodable {
 }

 /// An encoder for a consensus-encodable object.
-pub trait Encoder<'e> {
+pub trait Encoder {
     /// Yields the current encoded byteslice.
     ///
     /// Will always return the same value until [`Self::advance`] is called.
@@ -52,7 +52,7 @@ macro_rules! encoder_newtype{
         $(#[$($struct_attr)*])*
         pub struct $name$(<$lt>)?($encoder);

-        impl<'e $(, $lt)?> $crate::Encoder<'e> for $name$(<$lt>)? {
+        impl<$($lt)?> $crate::Encoder for $name$(<$lt>)? {
             #[inline]
             fn current_chunk(&self) -> Option<&[u8]> { self.0.current_chunk() }

(and then update a bunch of impl sites)

@nyonson
Copy link
Contributor Author

nyonson commented Sep 18, 2025

You mentioned there was a rust requirement with GATs that the trait needs an explicit lifetime even though its not used, so we can't drop it entirely right? 6e979a2

@apoelstra
Copy link
Member

Yeah. I must have been wrong about that. I can definitely delete it and things still compile

But maybe we should first get #4982 in -- since that actually uses the GAT -- and then try this.

@nyonson nyonson force-pushed the elided-lifetime-syntax branch from 9e3193d to b56183d Compare September 25, 2025 20:27
Drop the encoder_newtype macro's implicit requirement that the struct
lifetime be called 'e to match the impl block's. This patch elides
the lifetime on the Encoder impl.
@nyonson nyonson force-pushed the elided-lifetime-syntax branch from b56183d to cf99839 Compare September 25, 2025 21:02
@nyonson
Copy link
Contributor Author

nyonson commented Sep 25, 2025

@apoelstra looks like you are right! Updated the patch with your change.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK cf99839; successfully ran local tests; neat!

@apoelstra apoelstra merged commit 8c72d46 into rust-bitcoin:master Sep 25, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants