Merge pull request #31 from jerel/borrow-check

Add borrow checking and improve the error messaging

membrane/src/lib.rs

@@ -161,7 +161,17 @@ impl std::fmt::Debug for DeferredEnumTrace {
 inventory::collect!(DeferredTrace);
 inventory::collect!(DeferredEnumTrace);
 
+macro_rules! return_if_error {
+  ( $e:expr ) => {
+    if !$e.errors.is_empty() {
+      return $e;
+    }
+  };
+}
+
+#[derive(Debug)]
 pub struct Membrane {
+  errors: Vec<String>,
   package_name: String,
   destination: PathBuf,
   library: String,
@@ -206,6 +216,7 @@ impl<'a> Membrane {
   where
     P: ?Sized + AsRef<Path> + std::fmt::Debug,
   {
+    let mut errors = vec![];
     let mut input_libs = vec![];
 
     std::env::set_var(
@@ -229,8 +240,9 @@ impl<'a> Membrane {
           ) {
             Ok(symbols) => symbols,
             Err(msg) => {
-              tracing::error!("{}", msg);
-              exit(1);
+              errors.push(msg);
+              // panic to terminate without having a compatible branch type
+              panic!();
             }
           };
 
@@ -324,6 +336,7 @@ impl<'a> Membrane {
     });
 
     Self {
+      errors,
       package_name: match std::env::var_os("MEMBRANE_PACKAGE_NAME") {
         Some(name) => name.into_string().unwrap(),
         None => "".to_string(),
@@ -364,6 +377,7 @@ impl<'a> Membrane {
   ///
   /// Can be overridden with the environment variable `MEMBRANE_DESTINATION`.
   pub fn package_destination_dir<P: ?Sized + AsRef<Path>>(&mut self, path: &'a P) -> &mut Self {
+    return_if_error!(self);
     // allowing an empty path could result in data loss in a directory named `lib`
     assert!(
       !path.as_ref().to_str().unwrap().is_empty(),
@@ -380,6 +394,7 @@ impl<'a> Membrane {
   ///
   /// Can be overridden with the environment variable `MEMBRANE_PACKAGE_NAME`.
   pub fn package_name(&mut self, name: &str) -> &mut Self {
+    return_if_error!(self);
     if self.package_name.is_empty() {
       self.package_name = name.to_string();
     }
@@ -391,6 +406,7 @@ impl<'a> Membrane {
   ///
   /// Can be overridden with the environment variable `MEMBRANE_LLVM_PATHS`. Takes a comma or space separated list.
   pub fn llvm_paths(&mut self, paths: Vec<&str>) -> &mut Self {
+    return_if_error!(self);
     assert!(
       !paths.is_empty(),
       "llvm_paths() cannot be called with no paths"
@@ -407,6 +423,7 @@ impl<'a> Membrane {
   ///
   /// Can be overridden with the environment variable `MEMBRANE_LIBRARY`.
   pub fn using_lib(&mut self, name: &str) -> &mut Self {
+    return_if_error!(self);
     if self.library == "libmembrane" {
       self.library = name.to_string();
     }
@@ -418,6 +435,7 @@ impl<'a> Membrane {
   /// Existing Dart files in this directory may be deleted during this operation.
   #[allow(unreachable_code)]
   pub fn create_pub_package(&mut self) -> &mut Self {
+    return_if_error!(self);
     use serde_generate::SourceInstaller;
 
     #[cfg(all(
@@ -444,14 +462,18 @@ impl<'a> Membrane {
       let registry = match self.namespaced_registry.get(namespace).unwrap() {
         Ok(reg) => reg,
         Err(Error::MissingVariants(names)) => {
-          tracing::error!(
+          self.errors.push(format!(
             "An enum was used that has not had the membrane::dart_enum macro applied for the consuming namespace. Please add #[dart_enum(namespace = \"{}\")] to the {} enum.",
             namespace,
             names.first().unwrap()
-          );
-          exit(1);
+          ));
+
+          return self;
+        }
+        Err(err) => {
+          self.errors.push(format!("{}", err));
+          return self;
         }
-        Err(err) => panic!("{}", err),
       };
       let generator = serde_generate::dart::CodeGenerator::new(&config);
       generator
@@ -474,7 +496,9 @@ impl<'a> Membrane {
     if pub_get.status.code() != Some(0) {
       std::io::stderr().write_all(&pub_get.stderr).unwrap();
       std::io::stdout().write_all(&pub_get.stdout).unwrap();
-      tracing::error!("'dart pub get' returned an error");
+      self
+        .errors
+        .push("'dart pub get' returned an error".to_string());
     }
 
     self
@@ -484,6 +508,7 @@ impl<'a> Membrane {
   /// When set to `true` (the default) we generate basic Dart enums. When set to `false`
   /// Dart classes are generated (one for the base case and one for each variant).
   pub fn with_c_style_enums(&mut self, val: bool) -> &mut Self {
+    return_if_error!(self);
     self.c_style_enums = val;
     self
   }
@@ -496,6 +521,7 @@ impl<'a> Membrane {
   ///
   /// Default: 1000ms
   pub fn timeout(&mut self, val: i32) -> &mut Self {
+    return_if_error!(self);
     self.timeout = Some(val);
     self
   }
@@ -504,6 +530,7 @@ impl<'a> Membrane {
   /// Write a header file for each namespace that provides the C types
   /// needed by ffigen to generate the FFI bindings.
   pub fn write_c_headers(&mut self) -> &mut Self {
+    return_if_error!(self);
     let head = r#"/*
  * AUTO GENERATED FILE, DO NOT EDIT
  *
@@ -540,8 +567,9 @@ uint8_t membrane_free_membrane_string(char *ptr);
 
     let path = self.destination.join("lib/src/membrane_types.h");
     std::fs::write(&path, head).unwrap_or_else(|_| {
-      tracing::error!("unable to write {}", path.to_str().unwrap());
-      exit(1);
+      self
+        .errors
+        .push(format!("unable to write {}", path.to_str().unwrap()));
     });
 
     let namespaces = self.namespaces.clone();
@@ -555,6 +583,7 @@ uint8_t membrane_free_membrane_string(char *ptr);
   ///
   /// Write all Dart classes needed by the Dart application.
   pub fn write_api(&mut self) -> &mut Self {
+    return_if_error!(self);
     let namespaces = self.namespaces.clone();
     namespaces.iter().for_each(|x| {
       self.create_ffi_impl(x);
@@ -575,6 +604,7 @@ uint8_t membrane_free_membrane_string(char *ptr);
   ///
   /// Invokes `dart run ffigen` with the appropriate config to generate FFI bindings.
   pub fn write_bindings(&mut self) -> &mut Self {
+    return_if_error!(self);
     if !self.generated {
       return self;
     }
@@ -594,13 +624,27 @@ uint8_t membrane_free_membrane_string(char *ptr);
     if ffigen.status.code() != Some(0) {
       std::io::stderr().write_all(&ffigen.stderr).unwrap();
       std::io::stdout().write_all(&ffigen.stdout).unwrap();
-      tracing::error!("dart ffigen returned an error");
-      exit(1);
+      self
+        .errors
+        .push("dart ffigen returned an error".to_string());
     }
 
     self
   }
 
+  ///
+  /// Whether any errors were found during code generation.
+  pub fn is_err(&mut self) -> bool {
+    !self.errors.is_empty()
+  }
+
+  ///
+  /// Returns all codegen errors and empties the error queue. This will prevent Membrane from
+  /// automatically exiting `1` and allow you to implement your own CLI exit handling if needed.
+  pub fn drain_errors(&mut self) -> Vec<String> {
+    self.errors.drain(..).collect()
+  }
+
   ///
   /// Private implementations
   ///
@@ -701,8 +745,10 @@ headers:
 
     let path = self.destination.join("ffigen.yaml");
     std::fs::write(&path, config).unwrap_or_else(|_| {
-      tracing::error!("unable to write ffigen config {}", path.to_str().unwrap());
-      exit(1);
+      self.errors.push(format!(
+        "unable to write ffigen config {}",
+        path.to_str().unwrap()
+      ));
     });
 
     self
@@ -730,10 +776,13 @@ headers:
 
     let mut buffer =
       std::fs::File::create(path.clone()).expect("header could not be written at namespace path");
-    buffer.write_all(head.as_bytes()).unwrap_or_else(|_| {
-      tracing::error!("unable to write C header file {}", path.to_str().unwrap());
-      exit(1);
-    });
+
+    if buffer.write_all(head.as_bytes()).is_err() {
+      self.errors.push(format!(
+        "unable to write C header file {}",
+        path.to_str().unwrap()
+      ));
+    }
 
     fns.iter().for_each(|x| {
       generators::functions::C::new(x).build(self).write(&buffer);
@@ -962,17 +1011,20 @@ class {class_name}Api {{
   }
 
   fn create_imports(&mut self) -> &mut Self {
-    self.borrows.iter().for_each(|(namespace, imports)| {
+    let mut owned_types: Vec<String> = vec![];
+    let mut non_owned_types: Vec<String> = vec![];
+    let borrows = self.borrows.clone();
+
+    borrows.iter().for_each(|(namespace, imports)| {
       imports
         .iter()
         // sort the imports in reverse order so that we can append them to existing
-        // lines and up with a descending order
+        // lines and end up with a descending order
         .rev()
         .for_each(|(from_namespace, borrowed_types)| {
           let mut borrowed_types: Vec<String> = borrowed_types.iter().flat_map(|r#type| {
             if namespace == from_namespace {
-              tracing::error!("{ns}::{import} was borrowed by {ns} which is a circular reference", ns = namespace, import = r#type);
-              exit(1);
+              self.errors.push(format!("`{ns}::{import}` was borrowed by `{ns}` which is a self reference", ns = namespace, import = r#type));
             }
 
             let auto_import = self.with_child_borrows(from_namespace, r#type);
@@ -989,6 +1041,11 @@ class {class_name}Api {{
           borrowed_types.sort();
           borrowed_types.dedup();
 
+          // this is the path that is owned (IE the namespace that holds the Rust source type)
+          owned_types.extend(borrowed_types.iter().map(|ty| format!("{}::{}", namespace, ty)));
+          // and this is the borrowed path
+          non_owned_types.extend(borrowed_types.iter().map(|ty| format!("{}::{}", from_namespace, ty)));
+
           let file_name = format!("{ns}.dart", ns = namespace);
           let namespace_path = self.destination.join("lib/src").join(namespace);
           let barrel_file_path = namespace_path.join(file_name);
@@ -1038,10 +1095,36 @@ class {class_name}Api {{
         });
     });
 
+    // if we already have an error about borrows above then lets exit with that
+    return_if_error!(self);
+
+    let mut reborrows: Vec<String> = non_owned_types
+      .iter()
+      .filter(|path| owned_types.contains(path))
+      .cloned()
+      .collect();
+
+    reborrows.sort();
+
+    if !reborrows.is_empty() {
+      self.errors.push(format!("The following `borrows` were found which attempt to reborrow a type which is not owned by the target namespace: `{}`", reborrows.join(", ")));
+    }
+
     self
   }
 }
 
+impl Drop for Membrane {
+  fn drop(&mut self) {
+    if self.is_err() {
+      for err in self.errors.iter() {
+        tracing::error!("{:?}", err);
+      }
+      exit(1);
+    }
+  }
+}
+
 #[doc(hidden)]
 #[repr(u8)]
 #[derive(serde::Serialize)]

membrane/tests/codegen_reborrow_test.rs

@@ -0,0 +1,51 @@
+mod mock;
+use crate::mock::RUNTIME;
+
+mod test {
+  use membrane::Membrane;
+  use pretty_assertions::assert_eq;
+  use std::path::Path;
+
+  #[test]
+  fn test_borrow_errors() {
+    mod app {
+      use membrane::async_dart;
+
+      mod data {
+        #[derive(serde::Deserialize, serde::Serialize)]
+        pub struct Location(pub String);
+      }
+
+      #[async_dart(
+        namespace = "a",
+        // circular reference
+        borrow = "b::Location",
+      )]
+      pub async fn reborrow_one() -> Result<data::Location, String> {
+        todo!()
+      }
+
+      #[async_dart(
+        namespace = "b",
+        // circular reference
+        borrow = "a::Location",
+      )]
+      pub async fn reborrow_two() -> Result<data::Location, String> {
+        todo!()
+      }
+    }
+
+    // gather metadata types and ensure that the above circular dependency is caught
+    let mut membrane = Membrane::new();
+
+    membrane
+      .package_destination_dir(Path::new("../dart_example"))
+      .create_pub_package()
+      .write_api();
+
+    assert_eq!(
+      membrane.drain_errors(),
+      vec!["The following `borrows` were found which attempt to reborrow a type which is not owned by the target namespace: `a::Location, b::Location`"]
+    );
+  }
+}