Merge #74: Remove type_check from Property trait

500b4a5 Remove type_check from Property trait (Riccardo Casatta)

Pull request description:

  I was looking to port rust-bitcoin/rust-miniscript#584 here, but I realized we can probably do better (and maybe port this in rust-miniscript to simplify things)

  The blanket implementation of `type_check` is only used on `CompilerExtData` while `ExtData` and `Type` override the impl and don't need the `child` parameter. Moreover, the fn isn't called as Property generic.

  So the blanket implementation of `type_check` is moved as a impl in `CompilerExtData` and the overrides in `ExtData` and `Type` are moved as simple impl on the type, making it possible to remove  the unused `child` parameter.

ACKs for top commit:
  apoelstra:
    ACK 500b4a5

Tree-SHA512: 1d3ba9c7a53a3813d83efc0fcf89888c18e8477b5da5372b6beccd1930c509d8a420d4f07b949c73f6f75193f01a901104c708cdf6aead78a262b048ebfc9394

src/miniscript/astelem.rs

@@ -18,7 +18,7 @@ use elements::{opcodes, script, Sequence};
 use super::limits::{MAX_SCRIPT_ELEMENT_SIZE, MAX_STANDARD_P2WSH_STACK_ITEM_SIZE};
 use crate::extensions::ParseableExt;
 use crate::miniscript::context::SigType;
-use crate::miniscript::types::{self, Property};
+use crate::miniscript::types;
 use crate::miniscript::ScriptContext;
 use crate::util::MsKeyBuilder;
 use crate::{
@@ -303,7 +303,7 @@ where
 {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         f.write_str("[")?;
-        if let Ok(type_map) = types::Type::type_check(self, |_| None) {
+        if let Ok(type_map) = types::Type::type_check(self) {
             f.write_str(match type_map.corr.base {
                 types::Base::B => "B",
                 types::Base::K => "K",

src/miniscript/decode.rs

@@ -17,18 +17,14 @@ use crate::extensions::ParseableExt;
 use crate::miniscript::lex::{Token as Tk, TokenIter};
 use crate::miniscript::limits::{MAX_BLOCK_WEIGHT, MAX_PUBKEYS_PER_MULTISIG};
 use crate::miniscript::types::extra_props::ExtData;
-use crate::miniscript::types::{Property, Type};
+use crate::miniscript::types::Type;
 use crate::miniscript::ScriptContext;
 #[cfg(doc)]
 use crate::Descriptor;
 use crate::{
     bitcoin, hash256, AbsLockTime, Error, Extension, Miniscript, MiniscriptKey, NoExt, ToPublicKey,
 };
 
-fn return_none<T>(_: usize) -> Option<T> {
-    None
-}
-
 /// Trait for parsing keys from byte slices
 pub trait ParseableKey: Sized + ToPublicKey + private::Sealed {
     /// Parse a key from slice
@@ -211,8 +207,8 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext, Ext: Extension> TerminalStack<Pk, Ct
 
     ///reduce, type check and push a 0-arg node
     fn reduce0(&mut self, ms: Terminal<Pk, Ctx, Ext>) -> Result<(), Error> {
-        let ty = Type::type_check(&ms, return_none)?;
-        let ext = ExtData::type_check(&ms, return_none)?;
+        let ty = Type::type_check(&ms)?;
+        let ext = ExtData::type_check(&ms)?;
         let ms = Miniscript {
             node: ms,
             ty,
@@ -232,8 +228,8 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext, Ext: Extension> TerminalStack<Pk, Ct
         let top = self.pop().unwrap();
         let wrapped_ms = wrap(Arc::new(top));
 
-        let ty = Type::type_check(&wrapped_ms, return_none)?;
-        let ext = ExtData::type_check(&wrapped_ms, return_none)?;
+        let ty = Type::type_check(&wrapped_ms)?;
+        let ext = ExtData::type_check(&wrapped_ms)?;
         let ms = Miniscript {
             node: wrapped_ms,
             ty,
@@ -258,8 +254,8 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext, Ext: Extension> TerminalStack<Pk, Ct
 
         let wrapped_ms = wrap(Arc::new(left), Arc::new(right));
 
-        let ty = Type::type_check(&wrapped_ms, return_none)?;
-        let ext = ExtData::type_check(&wrapped_ms, return_none)?;
+        let ty = Type::type_check(&wrapped_ms)?;
+        let ext = ExtData::type_check(&wrapped_ms)?;
         let ms = Miniscript {
             node: wrapped_ms,
             ty,
@@ -562,8 +558,8 @@ pub fn parse<Ctx: ScriptContext, Ext: ParseableExt>(
                 let c = term.pop().unwrap();
                 let wrapped_ms = Terminal::AndOr(Arc::new(a), Arc::new(c), Arc::new(b));
 
-                let ty = Type::type_check(&wrapped_ms, return_none)?;
-                let ext = ExtData::type_check(&wrapped_ms, return_none)?;
+                let ty = Type::type_check(&wrapped_ms)?;
+                let ext = ExtData::type_check(&wrapped_ms)?;
 
                 term.0.push(Miniscript {
                     node: wrapped_ms,

src/miniscript/mod.rs

@@ -36,7 +36,6 @@ use std::cmp;
 use std::sync::Arc;
 
 use self::lex::{lex, TokenIter};
-use self::types::Property;
 use crate::extensions::ParseableExt;
 pub use crate::miniscript::context::ScriptContext;
 use crate::miniscript::decode::Terminal;
@@ -117,8 +116,8 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext, Ext: Extension> Miniscript<Pk, Ctx,
     /// Display code of type_check.
     pub fn from_ast(t: Terminal<Pk, Ctx, Ext>) -> Result<Miniscript<Pk, Ctx, Ext>, Error> {
         Ok(Miniscript {
-            ty: Type::type_check(&t, |_| None)?,
-            ext: ExtData::type_check(&t, |_| None)?,
+            ty: Type::type_check(&t)?,
+            ext: ExtData::type_check(&t)?,
             node: t,
             phantom: PhantomData,
         })
@@ -177,7 +176,7 @@ where
 
         let top = decode::parse(&mut iter)?;
         Ctx::check_global_validity(&top)?;
-        let type_check = types::Type::type_check(&top.node, |_| None)?;
+        let type_check = types::Type::type_check(&top.node)?;
         if type_check.corr.base != types::Base::B {
             return Err(Error::NonTopLevel(format!("{:?}", top)));
         };
@@ -491,8 +490,8 @@ impl_from_tree!(
     fn from_tree(top: &expression::Tree<'_>) -> Result<Miniscript<Pk, Ctx, Ext>, Error> {
         let inner: Terminal<Pk, Ctx, Ext> = expression::FromTree::from_tree(top)?;
         Ok(Miniscript {
-            ty: Type::type_check(&inner, |_| None)?,
-            ext: ExtData::type_check(&inner, |_| None)?,
+            ty: Type::type_check(&inner)?,
+            ext: ExtData::type_check(&inner)?,
             node: inner,
             phantom: PhantomData,
         })

src/miniscript/types/extra_props.rs

@@ -890,15 +890,14 @@ impl Property for ExtData {
     fn from_ext<E: Extension>(e: &E) -> Self {
         e.extra_prop()
     }
-
+}
+impl ExtData {
     /// Compute the type of a fragment assuming all the children of
     /// Miniscript have been computed already.
-    fn type_check<Pk, Ctx, C, Ext>(
+    pub fn type_check<Pk, Ctx, Ext>(
         fragment: &Terminal<Pk, Ctx, Ext>,
-        _child: C,
     ) -> Result<Self, Error<Pk, Ctx, Ext>>
     where
-        C: FnMut(usize) -> Option<Self>,
         Ctx: ScriptContext,
         Pk: MiniscriptKey,
         Ext: Extension,

src/miniscript/types/mod.rs

@@ -19,12 +19,6 @@ pub use self::malleability::{Dissat, Malleability};
 use super::ScriptContext;
 use crate::{Extension, MiniscriptKey, NoExt, Terminal};
 
-/// None-returning function to help type inference when we need a
-/// closure that simply returns `None`
-fn return_none<T>(_: usize) -> Option<T> {
-    None
-}
-
 /// Detailed type of a typechecker error
 #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)]
 pub enum ErrorKind {
@@ -378,161 +372,6 @@ pub trait Property: Sized {
     /// Extensions are always leaf, they should have a fixed type property and hence
     /// should not fail
     fn from_ext<E: Extension>(e: &E) -> Self;
-
-    /// Compute the type of a fragment, given a function to look up
-    /// the types of its children, if available and relevant for the
-    /// given fragment
-    fn type_check<Pk, Ctx, C, Ext>(
-        fragment: &Terminal<Pk, Ctx, Ext>,
-        mut child: C,
-    ) -> Result<Self, Error<Pk, Ctx, Ext>>
-    where
-        C: FnMut(usize) -> Option<Self>,
-        Pk: MiniscriptKey,
-        Ctx: ScriptContext,
-        Ext: Extension,
-    {
-        let mut get_child = |sub, n| {
-            child(n)
-                .map(Ok)
-                .unwrap_or_else(|| Self::type_check(sub, return_none))
-        };
-        let wrap_err = |result: Result<Self, ErrorKind>| {
-            result.map_err(|kind| Error {
-                fragment: fragment.clone(),
-                error: kind,
-            })
-        };
-
-        let ret = match *fragment {
-            Terminal::True => Ok(Self::from_true()),
-            Terminal::False => Ok(Self::from_false()),
-            Terminal::PkK(..) => Ok(Self::from_pk_k::<Ctx>()),
-            Terminal::PkH(..) | Terminal::RawPkH(..) => Ok(Self::from_pk_h::<Ctx>()),
-            Terminal::Multi(k, ref pks) | Terminal::MultiA(k, ref pks) => {
-                if k == 0 {
-                    return Err(Error {
-                        fragment: fragment.clone(),
-                        error: ErrorKind::ZeroThreshold,
-                    });
-                }
-                if k > pks.len() {
-                    return Err(Error {
-                        fragment: fragment.clone(),
-                        error: ErrorKind::OverThreshold(k, pks.len()),
-                    });
-                }
-                match *fragment {
-                    Terminal::Multi(..) => Ok(Self::from_multi(k, pks.len())),
-                    Terminal::MultiA(..) => Ok(Self::from_multi_a(k, pks.len())),
-                    _ => unreachable!(),
-                }
-            }
-            Terminal::After(t) => {
-                // Note that for CLTV this is a limitation not of Bitcoin but Miniscript. The
-                // number on the stack would be a 5 bytes signed integer but Miniscript's B type
-                // only consumes 4 bytes from the stack.
-                if t == LockTime::ZERO.into() {
-                    return Err(Error {
-                        fragment: fragment.clone(),
-                        error: ErrorKind::InvalidTime,
-                    });
-                }
-                Ok(Self::from_after(t.into()))
-            }
-            Terminal::Older(t) => {
-                if t == Sequence::ZERO || !t.is_relative_lock_time() {
-                    return Err(Error {
-                        fragment: fragment.clone(),
-                        error: ErrorKind::InvalidTime,
-                    });
-                }
-                Ok(Self::from_older(t))
-            }
-            Terminal::Sha256(..) => Ok(Self::from_sha256()),
-            Terminal::Hash256(..) => Ok(Self::from_hash256()),
-            Terminal::Ripemd160(..) => Ok(Self::from_ripemd160()),
-            Terminal::Hash160(..) => Ok(Self::from_hash160()),
-            Terminal::Alt(ref sub) => wrap_err(Self::cast_alt(get_child(&sub.node, 0)?)),
-            Terminal::Swap(ref sub) => wrap_err(Self::cast_swap(get_child(&sub.node, 0)?)),
-            Terminal::Check(ref sub) => wrap_err(Self::cast_check(get_child(&sub.node, 0)?)),
-            Terminal::DupIf(ref sub) => wrap_err(Self::cast_dupif(get_child(&sub.node, 0)?)),
-            Terminal::Verify(ref sub) => wrap_err(Self::cast_verify(get_child(&sub.node, 0)?)),
-            Terminal::NonZero(ref sub) => wrap_err(Self::cast_nonzero(get_child(&sub.node, 0)?)),
-            Terminal::ZeroNotEqual(ref sub) => {
-                wrap_err(Self::cast_zeronotequal(get_child(&sub.node, 0)?))
-            }
-            Terminal::AndB(ref l, ref r) => {
-                let ltype = get_child(&l.node, 0)?;
-                let rtype = get_child(&r.node, 1)?;
-                wrap_err(Self::and_b(ltype, rtype))
-            }
-            Terminal::AndV(ref l, ref r) => {
-                let ltype = get_child(&l.node, 0)?;
-                let rtype = get_child(&r.node, 1)?;
-                wrap_err(Self::and_v(ltype, rtype))
-            }
-            Terminal::OrB(ref l, ref r) => {
-                let ltype = get_child(&l.node, 0)?;
-                let rtype = get_child(&r.node, 1)?;
-                wrap_err(Self::or_b(ltype, rtype))
-            }
-            Terminal::OrD(ref l, ref r) => {
-                let ltype = get_child(&l.node, 0)?;
-                let rtype = get_child(&r.node, 1)?;
-                wrap_err(Self::or_d(ltype, rtype))
-            }
-            Terminal::OrC(ref l, ref r) => {
-                let ltype = get_child(&l.node, 0)?;
-                let rtype = get_child(&r.node, 1)?;
-                wrap_err(Self::or_c(ltype, rtype))
-            }
-            Terminal::OrI(ref l, ref r) => {
-                let ltype = get_child(&l.node, 0)?;
-                let rtype = get_child(&r.node, 1)?;
-                wrap_err(Self::or_i(ltype, rtype))
-            }
-            Terminal::AndOr(ref a, ref b, ref c) => {
-                let atype = get_child(&a.node, 0)?;
-                let btype = get_child(&b.node, 1)?;
-                let ctype = get_child(&c.node, 2)?;
-                wrap_err(Self::and_or(atype, btype, ctype))
-            }
-            Terminal::Thresh(k, ref subs) => {
-                if k == 0 {
-                    return Err(Error {
-                        fragment: fragment.clone(),
-                        error: ErrorKind::ZeroThreshold,
-                    });
-                }
-                if k > subs.len() {
-                    return Err(Error {
-                        fragment: fragment.clone(),
-                        error: ErrorKind::OverThreshold(k, subs.len()),
-                    });
-                }
-
-                let mut last_err_frag = None;
-                let res = Self::threshold(k, subs.len(), |n| match get_child(&subs[n].node, n) {
-                    Ok(x) => Ok(x),
-                    Err(e) => {
-                        last_err_frag = Some(e.fragment);
-                        Err(e.error)
-                    }
-                });
-
-                res.map_err(|kind| Error {
-                    fragment: last_err_frag.unwrap_or_else(|| fragment.clone()),
-                    error: kind,
-                })
-            }
-            Terminal::Ext(ref ext) => Ok(Self::from_ext(ext)),
-        };
-        if let Ok(ref ret) = ret {
-            ret.sanity_checks()
-        }
-        ret
-    }
 }
 
 impl Property for Type {
@@ -783,15 +622,14 @@ impl Property for Type {
             mall: Property::from_ext(e),
         }
     }
-
+}
+impl Type {
     /// Compute the type of a fragment assuming all the children of
     /// Miniscript have been computed already.
-    fn type_check<Pk, Ctx, C, Ext>(
+    pub fn type_check<Pk, Ctx, Ext>(
         fragment: &Terminal<Pk, Ctx, Ext>,
-        _child: C,
     ) -> Result<Self, Error<Pk, Ctx, Ext>>
     where
-        C: FnMut(usize) -> Option<Self>,
         Pk: MiniscriptKey,
         Ctx: ScriptContext,
         Ext: Extension,