feat: noir-wasm takes dependency graph (#3213)

Co-authored-by: Tom French <[email protected]>

.github/workflows/test-noir_wasm.yml

@@ -101,12 +101,16 @@ jobs:
           name: nargo
           path: ./nargo
 
-      - name: Compile test program with Nargo CLI
-        working-directory: ./compiler/wasm/noir-script
+      - name: Compile fixtures with Nargo CLI
+        working-directory: ./compiler/wasm/fixtures
         run: |
           nargo_binary=${{ github.workspace }}/nargo/nargo
           chmod +x $nargo_binary
-          $nargo_binary compile
+          for dir in $(ls -d */); do
+            pushd $dir/noir-script
+            $nargo_binary compile
+            popd
+          done
 
       - name: Install Yarn dependencies
         uses: ./.github/actions/setup

Cargo.toml

@@ -77,7 +77,7 @@ dirs = "4"
 lsp-types = "0.94"
 serde = { version = "1.0.136", features = ["derive"] }
 serde_json = "1.0"
-smol_str = "0.1.17"
+smol_str = { version = "0.1.17", features = ["serde"] }
 thiserror = "1.0.21"
 toml = "0.7.2"
 tower = "0.4"

compiler/noirc_frontend/Cargo.toml

@@ -18,6 +18,7 @@ chumsky.workspace = true
 thiserror.workspace = true
 smol_str.workspace = true
 serde_json.workspace = true
+serde.workspace = true
 rustc-hash = "1.1.0"
 small-ord-set = "0.1.3"
 regex = "1.9.1"

compiler/noirc_frontend/src/graph/mod.rs

@@ -8,6 +8,7 @@ use std::{fmt::Display, str::FromStr};
 
 use fm::FileId;
 use rustc_hash::{FxHashMap, FxHashSet};
+use serde::{Deserialize, Serialize};
 use smol_str::SmolStr;
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)]
@@ -32,7 +33,7 @@ impl CrateId {
     }
 }
 
-#[derive(Debug, Clone, PartialEq, Eq, Hash, Ord, PartialOrd)]
+#[derive(Debug, Clone, PartialEq, Eq, Hash, Ord, PartialOrd, Serialize, Deserialize)]
 pub struct CrateName(SmolStr);
 
 impl CrateName {
@@ -90,6 +91,11 @@ mod crate_name {
             assert!(!CrateName::is_valid_name(&bad_char_string));
         }
     }
+
+    #[test]
+    fn it_rejects_bad_crate_names_when_deserializing() {
+        assert!(serde_json::from_str::<CrateName>("bad-name").is_err());
+    }
 }
 
 #[derive(Debug, Clone, Default, PartialEq, Eq)]

compiler/wasm/fixtures/deps/lib-a/Nargo.toml

@@ -0,0 +1,8 @@
+[package]
+name="lib_a"
+type="lib"
+authors = [""]
+compiler_version = "0.1"
+
+[dependencies]
+lib_b = { path = "../lib-b" }

compiler/wasm/fixtures/deps/lib-a/src/lib.nr

@@ -0,0 +1,7 @@
+
+use dep::lib_b::assert_non_zero;
+
+pub fn divide(a: u64, b: u64) -> u64 {
+    assert_non_zero(b);
+    a / b
+}

compiler/wasm/fixtures/deps/lib-b/Nargo.toml

@@ -0,0 +1,7 @@
+[package]
+name="lib_b"
+type="lib"
+authors = [""]
+compiler_version = "0.1"
+
+[dependencies]

compiler/wasm/fixtures/deps/lib-b/src/lib.nr

@@ -0,0 +1,4 @@
+
+pub fn assert_non_zero(x: u64) {
+  assert(x != 0);
+}

compiler/wasm/fixtures/deps/noir-script/Nargo.toml

@@ -0,0 +1,8 @@
+[package]
+name="noir_wasm_testing"
+type="bin"
+authors = [""]
+compiler_version = "0.1"
+
+[dependencies]
+lib_a = { path="../lib-a" }

compiler/wasm/fixtures/deps/noir-script/src/main.nr

@@ -0,0 +1,4 @@
+use dep::lib_a::divide;
+fn main(x : u64, y : pub u64) {
+  divide(x, y);
+}

compiler/wasm/src/compile.rs

@@ -1,6 +1,6 @@
 use fm::FileManager;
 use gloo_utils::format::JsValueSerdeExt;
-use js_sys::Array;
+use js_sys::Object;
 use nargo::artifacts::{
     contract::{PreprocessedContract, PreprocessedContractFunction},
     program::PreprocessedProgram,
@@ -9,29 +9,54 @@ use noirc_driver::{
     add_dep, compile_contract, compile_main, prepare_crate, prepare_dependency, CompileOptions,
     CompiledContract, CompiledProgram, NOIR_ARTIFACT_VERSION_STRING,
 };
-use noirc_frontend::{graph::CrateGraph, hir::Context};
-use std::path::Path;
+use noirc_frontend::{
+    graph::{CrateGraph, CrateId, CrateName},
+    hir::Context,
+};
+use serde::Deserialize;
+use std::{collections::HashMap, path::Path};
 use wasm_bindgen::prelude::*;
 
-use crate::errors::JsCompileError;
+use crate::errors::{CompileError, JsCompileError};
 
 const BACKEND_IDENTIFIER: &str = "acvm-backend-barretenberg";
 
+#[wasm_bindgen(typescript_custom_section)]
+const DEPENDENCY_GRAPH: &'static str = r#"
+export type DependencyGraph = {
+    root_dependencies: readonly string[];
+    library_dependencies: Readonly<Record<string, readonly string[]>>;
+}
+"#;
+
 #[wasm_bindgen]
 extern "C" {
-    #[wasm_bindgen(extends = Array, js_name = "StringArray", typescript_type = "string[]")]
+    #[wasm_bindgen(extends = Object, js_name = "DependencyGraph", typescript_type = "DependencyGraph")]
     #[derive(Clone, Debug, PartialEq, Eq)]
-    pub type StringArray;
+    pub type JsDependencyGraph;
+}
+
+#[derive(Deserialize)]
+struct DependencyGraph {
+    root_dependencies: Vec<CrateName>,
+    library_dependencies: HashMap<CrateName, Vec<CrateName>>,
 }
 
 #[wasm_bindgen]
 pub fn compile(
     entry_point: String,
     contracts: Option<bool>,
-    dependencies: Option<StringArray>,
+    dependency_graph: Option<JsDependencyGraph>,
 ) -> Result<JsValue, JsCompileError> {
     console_error_panic_hook::set_once();
 
+    let dependency_graph: DependencyGraph = if let Some(dependency_graph) = dependency_graph {
+        <JsValue as JsValueSerdeExt>::into_serde(&JsValue::from(dependency_graph))
+            .map_err(|err| err.to_string())?
+    } else {
+        DependencyGraph { root_dependencies: vec![], library_dependencies: HashMap::new() }
+    };
+
     let root = Path::new("/");
     let fm = FileManager::new(root, Box::new(get_non_stdlib_asset));
     let graph = CrateGraph::default();
@@ -40,12 +65,7 @@ pub fn compile(
     let path = Path::new(&entry_point);
     let crate_id = prepare_crate(&mut context, path);
 
-    let dependencies: Vec<String> = dependencies
-        .map(|array| array.iter().map(|element| element.as_string().unwrap()).collect())
-        .unwrap_or_default();
-    for dependency in dependencies {
-        add_noir_lib(&mut context, dependency.as_str());
-    }
+    process_dependency_graph(&mut context, dependency_graph);
 
     let compile_options = CompileOptions::default();
 
@@ -57,7 +77,11 @@ pub fn compile(
     if contracts.unwrap_or_default() {
         let compiled_contract = compile_contract(&mut context, crate_id, &compile_options)
             .map_err(|errs| {
-                JsCompileError::new("Failed to compile contract", errs, &context.file_manager)
+                CompileError::with_file_diagnostics(
+                    "Failed to compile contract",
+                    errs,
+                    &context.file_manager,
+                )
             })?
             .0;
 
@@ -71,7 +95,11 @@ pub fn compile(
     } else {
         let compiled_program = compile_main(&mut context, crate_id, &compile_options, None, true)
             .map_err(|errs| {
-                JsCompileError::new("Failed to compile program", errs, &context.file_manager)
+                CompileError::with_file_diagnostics(
+                    "Failed to compile program",
+                    errs,
+                    &context.file_manager,
+                )
             })?
             .0;
 
@@ -85,34 +113,38 @@ pub fn compile(
     }
 }
 
-fn add_noir_lib(context: &mut Context, library_name: &str) {
-    let path_to_lib = Path::new(&library_name).join("lib.nr");
-    let library_crate_id = prepare_dependency(context, &path_to_lib);
-
-    add_dep(context, *context.root_crate_id(), library_crate_id, library_name.parse().unwrap());
-
-    // TODO: Remove this code that attaches every crate to every other crate as a dependency
-    let root_crate_id = context.root_crate_id();
-    let stdlib_crate_id = context.stdlib_crate_id();
-    let other_crate_ids: Vec<_> = context
-        .crate_graph
-        .iter_keys()
-        .filter(|crate_id| {
-            // We don't want to attach this crate to itself or stdlib, nor re-attach it to the root crate
-            crate_id != &library_crate_id
-                && crate_id != root_crate_id
-                && crate_id != stdlib_crate_id
-        })
-        .collect();
+fn process_dependency_graph(context: &mut Context, dependency_graph: DependencyGraph) {
+    let mut crate_names: HashMap<&CrateName, CrateId> = HashMap::new();
 
-    for crate_id in other_crate_ids {
-        context
-            .crate_graph
-            .add_dep(crate_id, library_name.parse().unwrap(), library_crate_id)
-            .unwrap_or_else(|_| panic!("ICE: Cyclic error triggered by {library_name} library"));
+    for lib in &dependency_graph.root_dependencies {
+        let crate_id = add_noir_lib(context, lib);
+        crate_names.insert(lib, crate_id);
+
+        add_dep(context, *context.root_crate_id(), crate_id, lib.clone());
+    }
+
+    for (lib_name, dependencies) in &dependency_graph.library_dependencies {
+        // first create the library crate if needed
+        // this crate might not have been registered yet because of the order of the HashMap
+        // e.g. {root: [lib1], libs: { lib2 -> [lib3], lib1 -> [lib2] }}
+        let crate_id =
+            *crate_names.entry(lib_name).or_insert_with(|| add_noir_lib(context, lib_name));
+
+        for dependency_name in dependencies {
+            let dep_crate_id: &CrateId = crate_names
+                .entry(dependency_name)
+                .or_insert_with(|| add_noir_lib(context, dependency_name));
+
+            add_dep(context, crate_id, *dep_crate_id, dependency_name.clone());
+        }
     }
 }
 
+fn add_noir_lib(context: &mut Context, library_name: &CrateName) -> CrateId {
+    let path_to_lib = Path::new(&library_name.to_string()).join("lib.nr");
+    prepare_dependency(context, &path_to_lib)
+}
+
 fn preprocess_program(program: CompiledProgram) -> PreprocessedProgram {
     PreprocessedProgram {
         hash: program.hash,
@@ -168,3 +200,101 @@ cfg_if::cfg_if! {
         }
     }
 }
+
+#[cfg(test)]
+mod test {
+    use fm::FileManager;
+    use noirc_driver::prepare_crate;
+    use noirc_frontend::{
+        graph::{CrateGraph, CrateName},
+        hir::Context,
+    };
+
+    use super::{process_dependency_graph, DependencyGraph};
+    use std::{collections::HashMap, path::Path};
+
+    fn mock_get_non_stdlib_asset(_path_to_file: &Path) -> std::io::Result<String> {
+        Ok("".to_string())
+    }
+
+    fn setup_test_context() -> Context {
+        let fm = FileManager::new(Path::new("/"), Box::new(mock_get_non_stdlib_asset));
+        let graph = CrateGraph::default();
+        let mut context = Context::new(fm, graph);
+
+        prepare_crate(&mut context, Path::new("/main.nr"));
+
+        context
+    }
+
+    fn crate_name(name: &str) -> CrateName {
+        name.parse().unwrap()
+    }
+
+    #[test]
+    fn test_works_with_empty_dependency_graph() {
+        let mut context = setup_test_context();
+        let dependency_graph =
+            DependencyGraph { root_dependencies: vec![], library_dependencies: HashMap::new() };
+
+        process_dependency_graph(&mut context, dependency_graph);
+
+        // one stdlib + one root crate
+        assert_eq!(context.crate_graph.number_of_crates(), 2);
+    }
+
+    #[test]
+    fn test_works_with_root_dependencies() {
+        let mut context = setup_test_context();
+        let dependency_graph = DependencyGraph {
+            root_dependencies: vec![crate_name("lib1")],
+            library_dependencies: HashMap::new(),
+        };
+
+        process_dependency_graph(&mut context, dependency_graph);
+
+        assert_eq!(context.crate_graph.number_of_crates(), 3);
+    }
+
+    #[test]
+    fn test_works_with_duplicate_root_dependencies() {
+        let mut context = setup_test_context();
+        let dependency_graph = DependencyGraph {
+            root_dependencies: vec![crate_name("lib1"), crate_name("lib1")],
+            library_dependencies: HashMap::new(),
+        };
+
+        process_dependency_graph(&mut context, dependency_graph);
+
+        assert_eq!(context.crate_graph.number_of_crates(), 3);
+    }
+
+    #[test]
+    fn test_works_with_transitive_dependencies() {
+        let mut context = setup_test_context();
+        let dependency_graph = DependencyGraph {
+            root_dependencies: vec![crate_name("lib1")],
+            library_dependencies: HashMap::from([
+                (crate_name("lib1"), vec![crate_name("lib2")]),
+                (crate_name("lib2"), vec![crate_name("lib3")]),
+            ]),
+        };
+
+        process_dependency_graph(&mut context, dependency_graph);
+
+        assert_eq!(context.crate_graph.number_of_crates(), 5);
+    }
+
+    #[test]
+    fn test_works_with_missing_dependencies() {
+        let mut context = setup_test_context();
+        let dependency_graph = DependencyGraph {
+            root_dependencies: vec![crate_name("lib1")],
+            library_dependencies: HashMap::from([(crate_name("lib2"), vec![crate_name("lib3")])]),
+        };
+
+        process_dependency_graph(&mut context, dependency_graph);
+
+        assert_eq!(context.crate_graph.number_of_crates(), 5);
+    }
+}