Remove Library trait (#1422)

* feat(assembler): implement serialization for `CompiledLibrary` and `KernelLibrary`
* feat(assembly): Add `CompiledLibrary::write_to_dir()` and `KernelLibrary::write_to_dir()`
* feat(assembler): Remove `MaslLibrary`
* remove docs building
* fix(assembly): ensure aliases are resolved to roots

This commit fixes the issue where re-exported procedures (aliases) were
not being handled properly during assembly. It now explicitly represents
them in the call graph, so that topological ordering of the call graph
will ensure that we only visit the aliases once the aliased procedure
has been compiled/visited.

As part of the solution implemented here, some refinements to
`AliasTarget` were made, in order to explicitly represent whether the
target is absolute or not (just like `InvocationTarget`). Additionally,
to avoid confusion, `FullyQualifiedProcedureName` was renamed to
`QualifiedProcedureName`, so as to make it clear that just because the
path is qualified, does not mean it is absolute (and thus "fully"
qualified).

Some conveniences were added to `LibraryNamespace`, `LibraryPath`, and
`AliasTarget` to make certain operations/conversions more ergonomic and
expressive.

* feat: add pretty-print helper for lists of values

* feat: support assembling with compiled libraries

This commit refactors `CompiledLibrary` a bit, to remove some
unnecessary restrictions leftover from the old MASL libraries:

* A `CompiledLibrary` no longer has a name, but it has a content digest
  obtained by lexicographically ordering the exported MAST roots of the
  library, and merging the hashes in order.
* As a consequence of being unnamed/having no namespace, a
  `CompiledLibrary` can now consist of procedures from many modules
  _and_ many namespaces. Any limitation we impose on top of that can be
  done via wrapper types, like how `KernelLibrary` is implemented.
* Support for re-exported procedures in a `CompiledLibrary` is
  implemented. It is assumed that all required libraries will be
  provided to the `Host` when executing a program.
* Some ergonomic improvements to APIs which accept libraries or sets of
  modules, to allow a greater variety of ways you can pass them.

* fix(assembly): address conflicting procedure definitions bug

Previously, we assumed that two procedures with the same MAST root, but
differing numbers of locals, was a bug in the assembler. However, this
is not the case, as I will elaborate on below.

If you compile a program like so:

```masm
use.std::u64

begin
  exec.u64::checked_and
end
```

The resulting MAST would look something like:

```mast
begin
  external.0x....
end
```

This MAST will have the exact same MAST root as `std::u64::checked_and`,
because `external` nodes have the same digest as the node they refer to.
Now, if the exec'd procedure has the same number of locals as the
caller, this is presumed to be a "compatible" procedure, meaning it is
fine to let both procedures refer to the same MAST. However, when the
number of procedure locals _differs_, we were raising a compatible
definition error, because it was assumed that due to the instructions
added when procedure locals are present, two procedures with differing
numbers of locals _could not_ have the same root by definition. This is
incorrect, let me illustrate:

```masm

export.foo.2
  ...
end

use.foo

proc.bar.3
   exec.foo::foo
end

begin
   exec.foo::foo
end
```

Assume that `foo.masm` is first compiled to a `CompiledLibrary`, which
is then added to the assembler when assembling an executable program
from `bar.masm`.

Also, recall that `begin .. end` blocks are effectively an exported
procedure definition, with zero locals, that has a special name - but in
all other ways it is just a normal procedure.

The above program is perfectly legal, but we would raise an error during
assembly of the program due to the `begin .. end` block compiling to an
`external` node referencing the MAST root of `foo::foo`, which has a
non-zero number of procedure locals.

The `bar` procedure is there to illustrate that even though it too
simply "wraps" the `foo::foo` procedure, it has a non-zero number of
procedure locals, and thus cannot ever have the same MAST root as a
wrapped procedure with a non-zero number of locals, due to the presence
of locals changing the root of the wrapping procedure. A check has been
kept around that ensures we catch if ever there are two procedures with
non-zero counts of procedure locals, with the same MAST root.

* chore: update changelog

---------

Co-authored-by: Bobbin Threadbare <[email protected]>
Co-authored-by: Paul Schoenfelder <[email protected]>

CHANGELOG.md

@@ -26,6 +26,7 @@
 - Added functions to `MastForest` and `MastForestBuilder` to add and ensure nodes with fewer LOC (#1404, #1412)
 - Added `Assembler::assemble_library()` (#1413).
 - Added `Assembler::assemble_kernel()` (#1418).
+- Added `miden_core::prettier::pretty_print_csv` helper, for formatting of iterators over `PrettyPrint` values as comma-separated items
 
 #### Changed
 

assembly/src/assembler/instruction/procedures.rs

@@ -74,19 +74,12 @@ impl Assembler {
                 proc_ctx.register_external_call(&proc, false)?;
             }
             Some(proc) => proc_ctx.register_external_call(&proc, false)?,
-            None if matches!(kind, InvokeKind::SysCall) => {
-                return Err(AssemblyError::UnknownSysCallTarget {
-                    span,
-                    source_file: current_source_file.clone(),
-                    callee: mast_root,
-                });
-            }
             None => (),
         }
 
         let mast_root_node_id = {
             match kind {
-                InvokeKind::Exec => {
+                InvokeKind::Exec | InvokeKind::ProcRef => {
                     // Note that here we rely on the fact that we topologically sorted the
                     // procedures, such that when we assemble a procedure, all
                     // procedures that it calls will have been assembled, and
@@ -161,7 +154,7 @@ impl Assembler {
         mast_forest_builder: &MastForestBuilder,
     ) -> Result<(), AssemblyError> {
         let digest =
-            self.resolve_target(InvokeKind::Exec, callee, proc_ctx, mast_forest_builder)?;
+            self.resolve_target(InvokeKind::ProcRef, callee, proc_ctx, mast_forest_builder)?;
         self.procref_mast_root(digest, proc_ctx, span_builder, mast_forest_builder)
     }
 

assembly/src/assembler/mast_forest_builder.rs

@@ -20,6 +20,7 @@ pub struct MastForestBuilder {
     mast_forest: MastForest,
     node_id_by_hash: BTreeMap<RpoDigest, MastNodeId>,
     procedures: BTreeMap<GlobalProcedureIndex, Arc<Procedure>>,
+    procedure_hashes: BTreeMap<GlobalProcedureIndex, RpoDigest>,
     proc_gid_by_hash: BTreeMap<RpoDigest, GlobalProcedureIndex>,
 }
 
@@ -39,6 +40,13 @@ impl MastForestBuilder {
         self.procedures.get(&gid).cloned()
     }
 
+    /// Returns the hash of the procedure with the specified [`GlobalProcedureIndex`], or None if
+    /// such a procedure is not present in this MAST forest builder.
+    #[inline(always)]
+    pub fn get_procedure_hash(&self, gid: GlobalProcedureIndex) -> Option<RpoDigest> {
+        self.procedure_hashes.get(&gid).cloned()
+    }
+
     /// Returns a reference to the procedure with the specified MAST root, or None
     /// if such a procedure is not present in this MAST forest builder.
     #[inline(always)]
@@ -61,6 +69,17 @@ impl MastForestBuilder {
 }
 
 impl MastForestBuilder {
+    pub fn insert_procedure_hash(
+        &mut self,
+        gid: GlobalProcedureIndex,
+        proc_hash: RpoDigest,
+    ) -> Result<(), AssemblyError> {
+        // TODO(plafer): Check if exists
+        self.procedure_hashes.insert(gid, proc_hash);
+
+        Ok(())
+    }
+
     /// Inserts a procedure into this MAST forest builder.
     ///
     /// If the procedure with the same ID already exists in this forest builder, this will have
@@ -76,17 +95,9 @@ impl MastForestBuilder {
         //
         // If there is already a cache entry, but it conflicts with what we're trying to cache,
         // then raise an error.
-        if let Some(cached) = self.procedures.get(&gid) {
-            let cached_root = self.mast_forest[cached.body_node_id()].digest();
-            if cached_root != proc_root || cached.num_locals() != procedure.num_locals() {
-                return Err(AssemblyError::ConflictingDefinitions {
-                    first: cached.fully_qualified_name().clone(),
-                    second: procedure.fully_qualified_name().clone(),
-                });
-            }
-
+        if self.procedures.contains_key(&gid) {
             // The global procedure index and the MAST root resolve to an already cached version of
-            // this procedure, nothing to do.
+            // this procedure, or an alias of it, nothing to do.
             //
             // TODO: We should emit a warning for this, because while it is not an error per se, it
             // does reflect that we're doing work we don't need to be doing. However, emitting a
@@ -98,23 +109,29 @@ impl MastForestBuilder {
         // We don't have a cache entry yet, but we do want to make sure we don't have a conflicting
         // cache entry with the same MAST root:
         if let Some(cached) = self.find_procedure(&proc_root) {
-            if cached.num_locals() != procedure.num_locals() {
+            // Handle the case where a procedure with no locals is lowered to a MastForest
+            // consisting only of an `External` node to another procedure which has one or more
+            // locals. This will result in the calling procedure having the same digest as the
+            // callee, but the two procedures having mismatched local counts. When this occurs,
+            // we want to use the procedure with non-zero local count as the definition, and treat
+            // the other procedure as an alias, which can be referenced like any other procedure,
+            // but the MAST returned for it will be that of the "real" definition.
+            let cached_locals = cached.num_locals();
+            let procedure_locals = procedure.num_locals();
+            let mismatched_locals = cached_locals != procedure_locals;
+            let is_valid =
+                !mismatched_locals || core::cmp::min(cached_locals, procedure_locals) == 0;
+            if !is_valid {
                 return Err(AssemblyError::ConflictingDefinitions {
                     first: cached.fully_qualified_name().clone(),
                     second: procedure.fully_qualified_name().clone(),
                 });
             }
-
-            // We have a previously cached version of an equivalent procedure, just under a
-            // different [GlobalProcedureIndex], so insert the cached procedure into the slot for
-            // `id`, but skip inserting a record in the MAST root lookup table
-            self.make_root(procedure.body_node_id());
-            self.procedures.insert(gid, Arc::new(procedure));
-            return Ok(());
         }
 
         self.make_root(procedure.body_node_id());
         self.proc_gid_by_hash.insert(proc_root, gid);
+        self.insert_procedure_hash(gid, procedure.mast_root())?;
         self.procedures.insert(gid, Arc::new(procedure));
 
         Ok(())