Merge rust-bitcoin/rust-miniscript#722: Eliminate a bunch more recursion; expand the TreeLike iterator trait a bit

d0c0c14ae26e353c950319182fd853ee90b8eb54 miniscript: nonrecursive implementation of PartialEq/Eq/Hash (Andrew Poelstra)
117003f3d1c89129734288153a870abb7e940e45 miniscript: implement PartialOrd/Ord nonrecursively (Andrew Poelstra)
cccaaec8bc189f5dcf8212c40d0f752ce90d9063 miniscript: non-recursive Display implementation (Andrew Poelstra)
47bed0c281aaf67cba19eb4b7feda86e464b29cb iter: get rid of Arc::clones and allocations for n-ary nodes (Andrew Poelstra)
67d6ff78ad044ab0048e796e376529b6d2be9191 iter: introduce Ternary variant (Andrew Poelstra)

Pull request description:

  This PR replaces the recursive derived implementations of `PartialEq`, `PartialOrd`, `Hash`, `fmt::Debug` and `fmt::Display`. Along the way it expands the `iter::TreeLike` trait to make it a bit more useful (adding a `Ternary` variant and making the existing `Nary` variant generic as long as you can get a length out of it and index into it).

  The new `fmt::Debug` method works using a new `DisplayNode` wrapper around `Terminal` which allows iterating over a script in the same way as it's displayed, treating things like `or_i(0, X)` as `l:X` and implementing the `c:pk_k` aliases and so on. This seems generally useful but this PR does not expose it in the public API because I'd like to make some breaking changes to `Terminal` down the line and don't want to expand the API.

  The new `Ord` impl orders things alphabetically (assuming it is bug-free at least) by using the same iterator as the display logic. This is both nonrecursive and a more useful ordering for anybody who cares about the exact ordering. On the other hand, if anybody is depending on the *existing* ordering this will break their code. I would assume not, since the existing ordering is a weird ad-hoc thing based on the order that `derive(PartialOrd)` happens to use.

  Similarly, it adds a `Terminal::fragment_name` accessor which returns a `&'static str` representing the fragment name as displayed. This is also not put into the public API.

  The block of type parameters used in the debug output of Miniscripts is moved into a `fmt::Display` impl on `Type` itself. This is part of the public API.

  This PR does **not** replace the `Clone` impl, which is currently derived. This impl actually isn't recursive, since it just clones the `Arc`s in the first layer of the script. Maybe we want to implement this manually and do a "deep copy"? This would be useful for users who have keys with interior mutability or something. I have a followup which does this, but didn't include it because I felt it might be controversial and need its own discussion.

  This PR also does **not** replace the implicit `Drop` impl, because that's hard to do and I haven't done it. But we're making progress.

ACKs for top commit:
  sanket1729:
    ACK d0c0c14ae26e353c950319182fd853ee90b8eb54.

Tree-SHA512: edc42b5ed7d2b562e93d3c061328c2020f710534a5cad3878b849d900d697be4919232e1f1c178fdc5338bd2e6ef71ebcfed90b2b8b8e577d8f781d887771bc1

Author: heap-coder

src/iter/mod.rs

@@ -18,7 +18,12 @@ use crate::sync::Arc;
 use crate::{Miniscript, MiniscriptKey, ScriptContext, Terminal};
 
 impl<'a, Pk: MiniscriptKey, Ctx: ScriptContext> TreeLike for &'a Miniscript<Pk, Ctx> {
-    fn as_node(&self) -> Tree<Self> {
+    type NaryChildren = &'a [Arc<Miniscript<Pk, Ctx>>];
+
+    fn nary_len(tc: &Self::NaryChildren) -> usize { tc.len() }
+    fn nary_index(tc: Self::NaryChildren, idx: usize) -> Self { Arc::as_ref(&tc[idx]) }
+
+    fn as_node(&self) -> Tree<Self, Self::NaryChildren> {
         use Terminal::*;
         match self.node {
             PkK(..) | PkH(..) | RawPkH(..) | After(..) | Older(..) | Sha256(..) | Hash256(..)
@@ -36,14 +41,19 @@ impl<'a, Pk: MiniscriptKey, Ctx: ScriptContext> TreeLike for &'a Miniscript<Pk,
             | OrD(ref left, ref right)
             | OrC(ref left, ref right)
             | OrI(ref left, ref right) => Tree::Binary(left, right),
-            AndOr(ref a, ref b, ref c) => Tree::Nary(Arc::from([a.as_ref(), b, c])),
-            Thresh(ref thresh) => Tree::Nary(thresh.iter().map(Arc::as_ref).collect()),
+            AndOr(ref a, ref b, ref c) => Tree::Ternary(a, b, c),
+            Thresh(ref thresh) => Tree::Nary(thresh.data()),
         }
     }
 }
 
-impl<Pk: MiniscriptKey, Ctx: ScriptContext> TreeLike for Arc<Miniscript<Pk, Ctx>> {
-    fn as_node(&self) -> Tree<Self> {
+impl<'a, Pk: MiniscriptKey, Ctx: ScriptContext> TreeLike for &'a Arc<Miniscript<Pk, Ctx>> {
+    type NaryChildren = &'a [Arc<Miniscript<Pk, Ctx>>];
+
+    fn nary_len(tc: &Self::NaryChildren) -> usize { tc.len() }
+    fn nary_index(tc: Self::NaryChildren, idx: usize) -> Self { &tc[idx] }
+
+    fn as_node(&self) -> Tree<Self, Self::NaryChildren> {
         use Terminal::*;
         match self.node {
             PkK(..) | PkH(..) | RawPkH(..) | After(..) | Older(..) | Sha256(..) | Hash256(..)
@@ -54,17 +64,45 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> TreeLike for Arc<Miniscript<Pk, Ctx>
             | DupIf(ref sub)
             | Verify(ref sub)
             | NonZero(ref sub)
-            | ZeroNotEqual(ref sub) => Tree::Unary(Arc::clone(sub)),
+            | ZeroNotEqual(ref sub) => Tree::Unary(sub),
+            AndV(ref left, ref right)
+            | AndB(ref left, ref right)
+            | OrB(ref left, ref right)
+            | OrD(ref left, ref right)
+            | OrC(ref left, ref right)
+            | OrI(ref left, ref right) => Tree::Binary(left, right),
+            AndOr(ref a, ref b, ref c) => Tree::Ternary(a, b, c),
+            Thresh(ref thresh) => Tree::Nary(thresh.data()),
+        }
+    }
+}
+
+impl<'a, Pk: MiniscriptKey, Ctx: ScriptContext> TreeLike for &'a Terminal<Pk, Ctx> {
+    type NaryChildren = &'a [Arc<Miniscript<Pk, Ctx>>];
+
+    fn nary_len(tc: &Self::NaryChildren) -> usize { tc.len() }
+    fn nary_index(tc: Self::NaryChildren, idx: usize) -> Self { tc[idx].as_inner() }
+
+    fn as_node(&self) -> Tree<Self, Self::NaryChildren> {
+        use Terminal::*;
+        match self {
+            PkK(..) | PkH(..) | RawPkH(..) | After(..) | Older(..) | Sha256(..) | Hash256(..)
+            | Ripemd160(..) | Hash160(..) | True | False | Multi(..) | MultiA(..) => Tree::Nullary,
+            Alt(ref sub)
+            | Swap(ref sub)
+            | Check(ref sub)
+            | DupIf(ref sub)
+            | Verify(ref sub)
+            | NonZero(ref sub)
+            | ZeroNotEqual(ref sub) => Tree::Unary(sub.as_inner()),
             AndV(ref left, ref right)
             | AndB(ref left, ref right)
             | OrB(ref left, ref right)
             | OrD(ref left, ref right)
             | OrC(ref left, ref right)
-            | OrI(ref left, ref right) => Tree::Binary(Arc::clone(left), Arc::clone(right)),
-            AndOr(ref a, ref b, ref c) => {
-                Tree::Nary(Arc::from([Arc::clone(a), Arc::clone(b), Arc::clone(c)]))
-            }
-            Thresh(ref thresh) => Tree::Nary(thresh.iter().map(Arc::clone).collect()),
+            | OrI(ref left, ref right) => Tree::Binary(left.as_inner(), right.as_inner()),
+            AndOr(ref a, ref b, ref c) => Tree::Ternary(a.as_inner(), b.as_inner(), c.as_inner()),
+            Thresh(ref thresh) => Tree::Nary(thresh.data()),
         }
     }
 }

src/iter/tree.rs

@@ -7,21 +7,22 @@
 //!
 
 use crate::prelude::*;
-use crate::sync::Arc;
 
 /// Abstract node of a tree.
 ///
 /// Tracks the arity (out-degree) of a node, which is the only thing that
 /// is needed for iteration purposes.
-pub enum Tree<T> {
+pub enum Tree<T, NT> {
     /// Combinator with no children.
     Nullary,
     /// Combinator with one child.
     Unary(T),
     /// Combinator with two children.
     Binary(T, T),
+    /// Combinator with two children.
+    Ternary(T, T, T),
     /// Combinator with more than two children.
-    Nary(Arc<[T]>),
+    Nary(NT),
 }
 
 /// A trait for any structure which has the shape of a Miniscript tree.
@@ -30,22 +31,36 @@ pub enum Tree<T> {
 /// rather than nodes themselves, because it provides algorithms that
 /// assume copying is cheap.
 ///
-/// To implement this trait, you only need to implement the [`TreeLike::as_node`]
-/// method, which will usually be very mechanical. Everything else is provided.
-/// However, to avoid allocations, it may make sense to also implement
-/// [`TreeLike::n_children`] and [`TreeLike::nth_child`] because the default
-/// implementations will allocate vectors for n-ary nodes.
+/// To implement this trait, you only need to implement the [`TreeLike::as_node`],
+/// [`TreeLike::nary_len`] and `[TreeLike::nary_index'] methods, which should
+/// be very mechanical. Everything else is provided.
 pub trait TreeLike: Clone + Sized {
+    /// An abstraction over the children of n-ary nodes. Typically when
+    /// implementing the trait for `&a T` this will be `&'a [T]`.
+    type NaryChildren: Clone;
+
+    /// Accessor for the length of a [`Self::NaryChildren`].
+    fn nary_len(tc: &Self::NaryChildren) -> usize;
+
+    /// Accessor for a specific child of a [`Self::NaryChildren`].
+    ///
+    /// # Panics
+    ///
+    /// May panic if asked for an element outside of the range
+    /// `0..Self::nary_len(&tc)`.
+    fn nary_index(tc: Self::NaryChildren, idx: usize) -> Self;
+
     /// Interpret the node as an abstract node.
-    fn as_node(&self) -> Tree<Self>;
+    fn as_node(&self) -> Tree<Self, Self::NaryChildren>;
 
     /// Accessor for the number of children this node has.
     fn n_children(&self) -> usize {
         match self.as_node() {
             Tree::Nullary => 0,
             Tree::Unary(..) => 1,
             Tree::Binary(..) => 2,
-            Tree::Nary(children) => children.len(),
+            Tree::Ternary(..) => 3,
+            Tree::Nary(ref children) => Self::nary_len(children),
         }
     }
 
@@ -58,7 +73,14 @@ pub trait TreeLike: Clone + Sized {
             (0, Tree::Binary(sub, _)) => Some(sub),
             (1, Tree::Binary(_, sub)) => Some(sub),
             (_, Tree::Binary(..)) => None,
-            (n, Tree::Nary(children)) => children.get(n).cloned(),
+            (0, Tree::Ternary(sub, _, _)) => Some(sub),
+            (1, Tree::Ternary(_, sub, _)) => Some(sub),
+            (2, Tree::Ternary(_, _, sub)) => Some(sub),
+            (_, Tree::Ternary(..)) => None,
+            (n, Tree::Nary(children)) if n < Self::nary_len(&children) => {
+                Some(Self::nary_index(children, n))
+            }
+            (_, Tree::Nary(..)) => None,
         }
     }
 
@@ -210,8 +232,15 @@ impl<T: TreeLike> Iterator for PreOrderIter<T> {
                 self.stack.push(right);
                 self.stack.push(left);
             }
+            Tree::Ternary(a, b, c) => {
+                self.stack.push(c);
+                self.stack.push(b);
+                self.stack.push(a);
+            }
             Tree::Nary(children) => {
-                self.stack.extend(children.iter().rev().cloned());
+                for i in (0..T::nary_len(&children)).rev() {
+                    self.stack.push(T::nary_index(children.clone(), i));
+                }
             }
         }
         Some(top)