Merge #233: types: introduce TypeInner making Type the only publi…

…c "incomplete Simplicity type" type

dada0f2 ci: fix recent breakage from `cc` (Andrew Poelstra)
d7cfc34 types: introduce TypeInner, make Type the only public type (Andrew Poelstra)
ded63fd types: replace Context::bind with Context::bind_product (Andrew Poelstra)
e68151b types: use `Type` rather than `Bound` in the error enum (Andrew Poelstra)
d63f87b types: refactor to pull some Bound constructors into Context (Andrew Poelstra)
7c47778 types: refactor to pull out some type destructuring logic from bind() (Andrew Poelstra)
f3e5230 types: refactor to reuse some code across constructors (Andrew Poelstra)
eb58756 types: refactor bind/unify error return a bit (Andrew Poelstra)

Pull request description:

  Previously we had two types, `Type` and `Bound`, which were mutually recursive. `Type` was also exposed in the public API as a type representing an incomplete Simplicity type. `Bound` was exposed in the public API for a couple of reasons which this PR eliminates.

  However, because `Type` is both public and unusable without a live type inference context, this means that it needs to contain an `Arc<Context>`. But this means that if we have a self-referential type, this leaks to a somewhat-convoluted reference cycle in which a `Type` holds an `Arc<Context>` which owns a set of `Bound`s which contain the original `Type`. If you are skeptical of this, try running the test harness in valgrind before and after the last commit of this PR -- you will see that previously we were leaking memory in the `memory_leak` unit test but now we are not.

  This completes the "type inference overhaul" needed to eliminate the memory leak in infinitely-sized types.

ACKs for top commit:
  uncomputable:
    ACK dada0f2

Tree-SHA512: 93038e20f63ba4aabb7491a7daf94fb55cd9d4742bdd3ba42bd0dd3f7ff5133b8666a066e1ce0657ec664e62aea8eaa4463177591ae1c85b14e117e9f318407e

.github/workflows/fuzz.yml

@@ -39,7 +39,7 @@ parse_human,
       - name: fuzz
         run: |
           echo "Using RUSTFLAGS $RUSTFLAGS"
-          cd fuzz && ./fuzz.sh "${{ matrix.fuzz_target }}"
+          cd fuzz && cargo update && cargo update -p cc --precise 1.0.83 && ./fuzz.sh "${{ matrix.fuzz_target }}"
       - run: echo "${{ matrix.fuzz_target }}" >executed_${{ matrix.fuzz_target }}
       - uses: actions/upload-artifact@v3
         with:

fuzz/generate-files.sh

@@ -22,7 +22,7 @@ cargo-fuzz = true
 
 [dependencies]
 honggfuzz = { version = "0.5.55", default-features = false }
-simplicity-lang = { path = ".." }
+simplicity-lang = { path = "..", features = ["test-utils"] }
 EOF
 
 for targetFile in $(listTargetFiles); do
@@ -75,7 +75,7 @@ $(for name in $(listTargetNames); do echo "$name,"; done)
       - name: fuzz
         run: |
           echo "Using RUSTFLAGS \$RUSTFLAGS"
-          cd fuzz && ./fuzz.sh "\${{ matrix.fuzz_target }}"
+          cd fuzz && cargo update && cargo update -p cc --precise 1.0.83 && ./fuzz.sh "\${{ matrix.fuzz_target }}"
       - run: echo "\${{ matrix.fuzz_target }}" >executed_\${{ matrix.fuzz_target }}
       - uses: actions/upload-artifact@v3
         with:

src/types/arrow.rs

@@ -18,7 +18,7 @@ use crate::node::{
     CoreConstructible, DisconnectConstructible, JetConstructible, NoDisconnect,
     WitnessConstructible,
 };
-use crate::types::{Bound, Context, Error, Final, Type};
+use crate::types::{Context, Error, Final, Type};
 use crate::{jet::Jet, Value};
 
 use super::variable::new_name;
@@ -123,17 +123,19 @@ impl Arrow {
 
         let target = Type::free(&ctx, String::new());
         if let Some(lchild_arrow) = lchild_arrow {
-            ctx.bind(
+            ctx.bind_product(
                 &lchild_arrow.source,
-                Bound::Product(a, c.shallow_clone()),
+                &a,
+                &c,
                 "case combinator: left source = A × C",
             )?;
             ctx.unify(&target, &lchild_arrow.target, "").unwrap();
         }
         if let Some(rchild_arrow) = rchild_arrow {
-            ctx.bind(
+            ctx.bind_product(
                 &rchild_arrow.source,
-                Bound::Product(b, c),
+                &b,
+                &c,
                 "case combinator: left source = B × C",
             )?;
             ctx.unify(
@@ -162,21 +164,21 @@ impl Arrow {
         let c = rchild_arrow.source.shallow_clone();
         let d = rchild_arrow.target.shallow_clone();
 
-        let prod_256_a = Bound::Product(Type::two_two_n(ctx, 8), a.shallow_clone());
-        let prod_b_c = Bound::Product(b.shallow_clone(), c);
-        let prod_b_d = Type::product(ctx, b, d);
-
-        ctx.bind(
+        ctx.bind_product(
             &lchild_arrow.source,
-            prod_256_a,
+            &Type::two_two_n(ctx, 8),
+            &a,
             "disconnect combinator: left source = 2^256 × A",
         )?;
-        ctx.bind(
+        ctx.bind_product(
             &lchild_arrow.target,
-            prod_b_c,
+            &b,
+            &c,
             "disconnect combinator: left target = B × C",
         )?;
 
+        let prod_b_d = Type::product(ctx, b, d);
+
         Ok(Arrow {
             source: a,
             target: prod_b_d,

src/types/context.rs

@@ -19,7 +19,7 @@ use std::sync::{Arc, Mutex, MutexGuard};
 
 use crate::dag::{Dag, DagLike};
 
-use super::{Bound, CompleteBound, Error, Final, Type};
+use super::{Bound, CompleteBound, Error, Final, Type, TypeInner};
 
 /// Type inference context, or handle to a context.
 ///
@@ -60,13 +60,7 @@ impl Context {
     /// Helper function to allocate a bound and return a reference to it.
     fn alloc_bound(&self, bound: Bound) -> BoundRef {
         let mut lock = self.lock();
-        lock.slab.push(bound);
-        let index = lock.slab.len() - 1;
-
-        BoundRef {
-            context: Arc::as_ptr(&self.slab),
-            index,
-        }
+        lock.alloc_bound(bound)
     }
 
     /// Allocate a new free type bound, and return a reference to it.
@@ -90,10 +84,21 @@ impl Context {
     ///
     /// Panics if either of the child types are from a different inference context.
     pub fn alloc_sum(&self, left: Type, right: Type) -> BoundRef {
-        left.bound.root().assert_matches_context(self);
-        right.bound.root().assert_matches_context(self);
+        assert_eq!(
+            left.ctx, *self,
+            "left type did not match inference context of sum"
+        );
+        assert_eq!(
+            right.ctx, *self,
+            "right type did not match inference context of sum"
+        );
 
-        self.alloc_bound(Bound::sum(left, right))
+        let mut lock = self.lock();
+        if let Some((data1, data2)) = lock.complete_pair_data(&left.inner, &right.inner) {
+            lock.alloc_bound(Bound::Complete(Final::sum(data1, data2)))
+        } else {
+            lock.alloc_bound(Bound::Sum(left.inner, right.inner))
+        }
     }
 
     /// Allocate a new product-type bound, and return a reference to it.
@@ -102,10 +107,21 @@ impl Context {
     ///
     /// Panics if either of the child types are from a different inference context.
     pub fn alloc_product(&self, left: Type, right: Type) -> BoundRef {
-        left.bound.root().assert_matches_context(self);
-        right.bound.root().assert_matches_context(self);
+        assert_eq!(
+            left.ctx, *self,
+            "left type did not match inference context of product"
+        );
+        assert_eq!(
+            right.ctx, *self,
+            "right type did not match inference context of product"
+        );
 
-        self.alloc_bound(Bound::product(left, right))
+        let mut lock = self.lock();
+        if let Some((data1, data2)) = lock.complete_pair_data(&left.inner, &right.inner) {
+            lock.alloc_bound(Bound::Complete(Final::product(data1, data2)))
+        } else {
+            lock.alloc_bound(Bound::Product(left.inner, right.inner))
+        }
     }
 
     /// Creates a new handle to the context.
@@ -133,7 +149,7 @@ impl Context {
     /// # Panics
     ///
     /// Panics if passed a `BoundRef` that was not allocated by this context.
-    pub fn get(&self, bound: &BoundRef) -> Bound {
+    pub(super) fn get(&self, bound: &BoundRef) -> Bound {
         bound.assert_matches_context(self);
         let lock = self.lock();
         lock.slab[bound.index].shallow_clone()
@@ -149,32 +165,75 @@ impl Context {
     /// this is probably a bug.
     ///
     /// Also panics if passed a `BoundRef` that was not allocated by this context.
-    pub fn reassign_non_complete(&self, bound: BoundRef, new: Bound) {
+    pub(super) fn reassign_non_complete(&self, bound: BoundRef, new: Bound) {
         let mut lock = self.lock();
         lock.reassign_non_complete(bound, new);
     }
 
-    /// Binds the type to a given bound. If this fails, attach the provided
-    /// hint to the error.
+    /// Binds the type to a product bound formed by the two inner types. If this
+    /// fails, attach the provided hint to the error.
     ///
     /// Fails if the type has an existing incompatible bound.
-    pub fn bind(&self, existing: &Type, new: Bound, hint: &'static str) -> Result<(), Error> {
-        let existing_root = existing.bound.root();
+    ///
+    /// # Panics
+    ///
+    /// Panics if any of the three types passed in were allocated from a different
+    /// context than this one.
+    pub fn bind_product(
+        &self,
+        existing: &Type,
+        prod_l: &Type,
+        prod_r: &Type,
+        hint: &'static str,
+    ) -> Result<(), Error> {
+        assert_eq!(
+            existing.ctx, *self,
+            "attempted to bind existing type with wrong context",
+        );
+        assert_eq!(
+            prod_l.ctx, *self,
+            "attempted to bind product whose left type had wrong context",
+        );
+        assert_eq!(
+            prod_r.ctx, *self,
+            "attempted to bind product whose right type had wrong context",
+        );
+
+        let existing_root = existing.inner.bound.root();
+        let new_bound = Bound::Product(prod_l.inner.shallow_clone(), prod_r.inner.shallow_clone());
+
         let mut lock = self.lock();
-        lock.bind(existing_root, new, hint)
+        lock.bind(existing_root, new_bound).map_err(|e| {
+            let new_bound = lock.alloc_bound(e.new);
+            Error::Bind {
+                existing_bound: Type::wrap_bound(self, e.existing),
+                new_bound: Type::wrap_bound(self, new_bound),
+                hint,
+            }
+        })
     }
 
     /// Unify the type with another one.
     ///
     /// Fails if the bounds on the two types are incompatible
     pub fn unify(&self, ty1: &Type, ty2: &Type, hint: &'static str) -> Result<(), Error> {
+        assert_eq!(ty1.ctx, *self);
+        assert_eq!(ty2.ctx, *self);
         let mut lock = self.lock();
-        lock.unify(ty1, ty2, hint)
+        lock.unify(&ty1.inner, &ty2.inner).map_err(|e| {
+            let new_bound = lock.alloc_bound(e.new);
+            Error::Bind {
+                existing_bound: Type::wrap_bound(self, e.existing),
+                new_bound: Type::wrap_bound(self, new_bound),
+                hint,
+            }
+        })
     }
 
     /// Locks the underlying slab mutex.
     fn lock(&self) -> LockedContext {
         LockedContext {
+            context: Arc::as_ptr(&self.slab),
             slab: self.slab.lock().unwrap(),
         }
     }
@@ -245,15 +304,31 @@ pub struct OccursCheckId {
     index: usize,
 }
 
+struct BindError {
+    existing: BoundRef,
+    new: Bound,
+}
+
 /// Structure representing an inference context with its slab allocator mutex locked.
 ///
 /// This type is never exposed outside of this module and should only exist
 /// ephemerally within function calls into this module.
 struct LockedContext<'ctx> {
+    context: *const Mutex<Vec<Bound>>,
     slab: MutexGuard<'ctx, Vec<Bound>>,
 }
 
 impl<'ctx> LockedContext<'ctx> {
+    fn alloc_bound(&mut self, bound: Bound) -> BoundRef {
+        self.slab.push(bound);
+        let index = self.slab.len() - 1;
+
+        BoundRef {
+            context: self.context,
+            index,
+        }
+    }
+
     fn reassign_non_complete(&mut self, bound: BoundRef, new: Bound) {
         assert!(
             !matches!(self.slab[bound.index], Bound::Complete(..)),
@@ -262,21 +337,39 @@ impl<'ctx> LockedContext<'ctx> {
         self.slab[bound.index] = new;
     }
 
+    /// It is a common situation that we are pairing two types, and in the
+    /// case that they are both complete, we want to pair the complete types.
+    ///
+    /// This method deals with all the annoying/complicated member variable
+    /// paths to get the actual complete data out.
+    fn complete_pair_data(
+        &self,
+        inn1: &TypeInner,
+        inn2: &TypeInner,
+    ) -> Option<(Arc<Final>, Arc<Final>)> {
+        let bound1 = &self.slab[inn1.bound.root().index];
+        let bound2 = &self.slab[inn2.bound.root().index];
+        if let (Bound::Complete(ref data1), Bound::Complete(ref data2)) = (bound1, bound2) {
+            Some((Arc::clone(data1), Arc::clone(data2)))
+        } else {
+            None
+        }
+    }
+
     /// Unify the type with another one.
     ///
     /// Fails if the bounds on the two types are incompatible
-    fn unify(&mut self, existing: &Type, other: &Type, hint: &'static str) -> Result<(), Error> {
+    fn unify(&mut self, existing: &TypeInner, other: &TypeInner) -> Result<(), BindError> {
         existing.bound.unify(&other.bound, |x_bound, y_bound| {
-            self.bind(x_bound, self.slab[y_bound.index].shallow_clone(), hint)
+            self.bind(x_bound, self.slab[y_bound.index].shallow_clone())
         })
     }
 
-    fn bind(&mut self, existing: BoundRef, new: Bound, hint: &'static str) -> Result<(), Error> {
+    fn bind(&mut self, existing: BoundRef, new: Bound) -> Result<(), BindError> {
         let existing_bound = self.slab[existing.index].shallow_clone();
-        let bind_error = || Error::Bind {
-            existing_bound: existing_bound.shallow_clone(),
-            new_bound: new.shallow_clone(),
-            hint,
+        let bind_error = || BindError {
+            existing: existing.clone(),
+            new: new.shallow_clone(),
         };
 
         match (&existing_bound, &new) {
@@ -310,42 +403,36 @@ impl<'ctx> LockedContext<'ctx> {
                     | (CompleteBound::Sum(ref comp1, ref comp2), Bound::Sum(ref ty1, ref ty2)) => {
                         let bound1 = ty1.bound.root();
                         let bound2 = ty2.bound.root();
-                        self.bind(bound1, Bound::Complete(Arc::clone(comp1)), hint)?;
-                        self.bind(bound2, Bound::Complete(Arc::clone(comp2)), hint)
+                        self.bind(bound1, Bound::Complete(Arc::clone(comp1)))?;
+                        self.bind(bound2, Bound::Complete(Arc::clone(comp2)))
                     }
                     _ => Err(bind_error()),
                 }
             }
             (Bound::Sum(ref x1, ref x2), Bound::Sum(ref y1, ref y2))
             | (Bound::Product(ref x1, ref x2), Bound::Product(ref y1, ref y2)) => {
-                self.unify(x1, y1, hint)?;
-                self.unify(x2, y2, hint)?;
+                self.unify(x1, y1)?;
+                self.unify(x2, y2)?;
                 // This type was not complete, but it may be after unification, giving us
                 // an opportunity to finaliize it. We do this eagerly to make sure that
                 // "complete" (no free children) is always equivalent to "finalized" (the
                 // bound field having variant Bound::Complete(..)), even during inference.
                 //
                 // It also gives the user access to more information about the type,
                 // prior to finalization.
-                let y1_bound = &self.slab[y1.bound.root().index];
-                let y2_bound = &self.slab[y2.bound.root().index];
-                if let (Bound::Complete(data1), Bound::Complete(data2)) = (y1_bound, y2_bound) {
+                if let Some((data1, data2)) = self.complete_pair_data(y1, y2) {
                     self.reassign_non_complete(
                         existing,
                         Bound::Complete(if let Bound::Sum(..) = existing_bound {
-                            Final::sum(Arc::clone(data1), Arc::clone(data2))
+                            Final::sum(data1, data2)
                         } else {
-                            Final::product(Arc::clone(data1), Arc::clone(data2))
+                            Final::product(data1, data2)
                         }),
                     );
                 }
                 Ok(())
             }
-            (x, y) => Err(Error::Bind {
-                existing_bound: x.shallow_clone(),
-                new_bound: y.shallow_clone(),
-                hint,
-            }),
+            (_, _) => Err(bind_error()),
         }
     }
 }