Merge #854

854: fix: Gracefully error on concurrently loaded cyclic modules r=Marwes a=Marwes

bors r+

Co-authored-by: Markus Westerlind <[email protected]>

Author: bors[bot]

Cargo.lock

@@ -43,7 +43,7 @@ futures = { version = "0.3.1", default-features = false }
 codespan = "0.9"
 codespan-reporting = "0.9"
 pin-project-lite = { version = "0.1", optional = true }
-salsa = { version = "0.14.0", package = "gluon-salsa" }
+salsa = { version = "0.14.2", package = "gluon-salsa" }
 
 serde = { version = "1.0.0", optional = true }
 serde_state = { version = "0.4", optional = true }
@@ -85,7 +85,7 @@ pretty_assertions = "0.6"
 structopt = "0.3"
 tempfile = "3.0.4"
 tensile = { version = "0.6", features = ["tokio"] }
-tokio = { version = "0.2", features = ["macros", "rt-core", "rt-threaded"] }
+tokio = { version = "0.2", features = ["macros", "rt-core", "rt-threaded", "fs"] }
 walkdir = "2"
 
 serde = "1.0.0"

Cargo.toml

@@ -183,7 +183,7 @@ pub(crate) trait ImportApi: Send + Sync {
         compiler: &mut ModuleCompiler<'_>,
         vm: &Thread,
         module_id: &Symbol,
-    ) -> Result<ArcType, (Option<ArcType>, MacroError)>;
+    ) -> Result<ArcType, (Option<ArcType>, crate::Error)>;
     fn snapshot(&self, thread: RootedThread) -> DatabaseSnapshot;
     fn fork(
         &mut self,
@@ -210,14 +210,14 @@ where
         compiler: &mut ModuleCompiler<'_>,
         vm: &Thread,
         module_id: &Symbol,
-    ) -> Result<ArcType, (Option<ArcType>, MacroError)> {
+    ) -> Result<ArcType, (Option<ArcType>, crate::Error)> {
         assert!(module_id.is_global());
         let modulename = module_id.name().definition_name();
 
         self.importer
             .import(compiler, vm, &modulename)
             .await
-            .map_err(|(t, err)| (t, MacroError::new(err)))
+            .map_err(|(t, err)| (t, err))
     }
     fn snapshot(&self, thread: RootedThread) -> DatabaseSnapshot {
         Self::snapshot(self, thread)
@@ -542,9 +542,7 @@ where
                         .map_err(|err| Error::String(err.to_string()))?;
                     modulename
                 }
-                Expr::Literal(Literal::String(ref filename)) => {
-                    format!("@{}", filename_to_module(filename))
-                }
+                Expr::Literal(Literal::String(ref filename)) => filename_to_module(filename),
                 _ => {
                     return Err(Error::String(
                         "Expected a string literal or path to import".into(),

src/import.rs

@@ -630,7 +630,7 @@ pub trait ThreadExt: Send + Sync {
             db.add_module(module_name.clone(), input.into());
         }
         let mut db = vm.get_database();
-        db.global(module_name).await.map(|_| ())
+        db.import(module_name).await.map(|_| ())
     }
 
     /// Loads `filename` and compiles and runs its input by calling `load_script`

src/lib.rs

@@ -439,18 +439,20 @@ fn recover_cycle<T>(
     cycle: &[String],
     module: &String,
 ) -> StdResult<T, Error> {
+    let mut cycle: Vec<_> = cycle
+        .iter()
+        .filter(|k| k.contains("CompileStorage(import("))
+        .map(|k| {
+            k.trim_matches(|c: char| c != '"')
+                .trim_matches('"')
+                .trim_start_matches('@')
+                .to_string()
+        })
+        .collect();
+    cycle.pop();
     Err(macros::Error::new(crate::import::Error::CyclicDependency(
         module.to_string(),
-        cycle
-            .iter()
-            .filter(|k| k.contains("import"))
-            .map(|k| {
-                k.trim_matches(|c: char| c != '"')
-                    .trim_matches('"')
-                    .trim_start_matches('@')
-                    .to_string()
-            })
-            .collect(),
+        cycle,
     ))
     .into())
 }
@@ -638,14 +640,11 @@ async fn import(
     db: &mut dyn Compilation,
     modulename: String,
 ) -> StdResult<TypedIdent<Symbol>, Error> {
+    assert!(!modulename.starts_with('@'));
     let thread = db.thread().root_thread();
     let compiler = db.compiler();
 
-    let name = Symbol::from(if modulename.starts_with('@') {
-        modulename.clone()
-    } else {
-        format!("@{}", modulename)
-    });
+    let name = Symbol::from(format!("@{}", modulename));
     let result = crate::get_import(&thread)
         .load_module(&mut ModuleCompiler::new(compiler), &thread, &name)
         .await

src/query.rs

@@ -1,2 +1,3 @@
+// ERROR Module 'tests.fail.cyclic_dependency' occurs in a cyclic dependency: `tests.fail.cyclic_dependency -> tests.fail.deps.cyclic_dependency2 -> tests.fail.cyclic_dependency`
 let c  = import! "tests/fail/deps/cyclic_dependency2.glu"
 in 1

tests/fail/cyclic_dependency.glu

@@ -1,3 +1,4 @@
+// ERROR Option was None
 let { Option, unwrap }  = import! std.option
 
 unwrap None

tests/fail/unwrap.glu

@@ -1,15 +1,16 @@
 use std::{
-    fs::File,
-    io::{self, Read},
+    io,
     path::{Path, PathBuf},
 };
 
 use {
+    anyhow::anyhow,
     collect_mac::collect,
     futures::{join, prelude::*, stream, task::SpawnExt},
     serde_derive::Deserialize,
     structopt::StructOpt,
     thiserror::Error,
+    tokio::fs,
 };
 
 use gluon::{
@@ -182,22 +183,36 @@ impl TestCase {
 }
 
 async fn make_test<'t>(vm: &'t Thread, name: &str, filename: &Path) -> Result<TestCase, Error> {
-    let mut file = File::open(&filename)?;
-    let mut text = String::new();
-    file.read_to_string(&mut text)?;
+    let text = fs::read_to_string(filename).await?;
     let (De(test), _) = vm.run_expr_async(&name, &text).await?;
     Ok(test)
 }
 
-async fn run_file<'t>(
-    vm: &'t Thread,
-    name: &str,
-    filename: &Path,
-) -> Result<(OpaqueValue<RootedThread, Hole>, ArcType), Error> {
-    let mut file = File::open(&filename)?;
-    let mut text = String::new();
-    file.read_to_string(&mut text)?;
-    Ok(vm.run_expr_async(&name, &text).await?)
+async fn run_fail_test<'t>(vm: &'t Thread, name: &str, filename: &Path) -> Result<(), Error> {
+    let source = fs::read_to_string(&filename).await?;
+    let error_pattern = {
+        let pat = "// ERROR";
+        assert!(
+            source.starts_with(pat),
+            "Failure tests must have an error pattern to match against"
+        );
+        &source.lines().next().unwrap()[pat.len()..].trim()
+    };
+    match vm.load_script_async(&name, &source).await {
+        Ok(err) => Err(anyhow!(
+            "Expected test '{}' to fail\n{:?}",
+            filename.to_str().unwrap(),
+            err
+        )
+        .into()),
+        Err(ref err) if !err.to_string().contains(error_pattern) => Err(anyhow!(
+            "Error does not match the error pattern.\nExpected:\n\t`{}`\nActual:\n\t`{}`",
+            error_pattern,
+            err
+        )
+        .into()),
+        Err(_) => Ok(()),
+    }
 }
 
 fn gather_doc_tests(expr: &SpannedExpr<Symbol>) -> Vec<(String, String)> {
@@ -279,9 +294,7 @@ async fn run_doc_tests<'t>(
     name: &str,
     filename: &Path,
 ) -> Result<Vec<tensile::Test<Error>>, Error> {
-    let mut file = File::open(&filename)?;
-    let mut text = String::new();
-    file.read_to_string(&mut text)?;
+    let text = fs::read_to_string(filename).await?;
 
     let (expr, _, _) = vm.extract_metadata(&name, &text).await?;
 
@@ -386,15 +399,7 @@ async fn main_(options: &Opt) -> Result<(), Error> {
             tensile::test(
                 name.clone(),
                 tensile::Future(std::panic::AssertUnwindSafe(async move {
-                    match run_file(&vm, &name, &filename).await {
-                        Ok(err) => Err(format!(
-                            "Expected test '{}' to fail\n{:?}",
-                            filename.to_str().unwrap(),
-                            err.0,
-                        )
-                        .into()),
-                        Err(_) => Ok(()),
-                    }
+                    run_fail_test(&vm, &name, &filename).await
                 })),
             )
         })