Merge pull request #203 from Nadrieril/fix-import-bug

6 files changed, 145 insertions, 108 deletions

diff --git a/dhall/src/semantics/parse.rs b/dhall/src/semantics/parse.rs
index a770c15..5870d47 100644
--- a/dhall/src/semantics/parse.rs
+++ b/dhall/src/semantics/parse.rs
@@ -3,39 +3,38 @@ use url::Url;
 
 use crate::error::Error;
 use crate::semantics::resolve::{download_http_text, ImportLocation};
-use crate::syntax::binary;
-use crate::syntax::parse_expr;
+use crate::syntax::{binary, parse_expr};
 use crate::Parsed;
 
 pub fn parse_file(f: &Path) -> Result<Parsed, Error> {
     let text = std::fs::read_to_string(f)?;
     let expr = parse_expr(&text)?;
-    let root = ImportLocation::Local(f.to_owned());
+    let root = ImportLocation::local_dhall_code(f.to_owned());
     Ok(Parsed(expr, root))
 }
 
 pub fn parse_remote(url: Url) -> Result<Parsed, Error> {
     let body = download_http_text(url.clone())?;
     let expr = parse_expr(&body)?;
-    let root = ImportLocation::Remote(url);
+    let root = ImportLocation::remote_dhall_code(url);
     Ok(Parsed(expr, root))
 }
 
 pub fn parse_str(s: &str) -> Result<Parsed, Error> {
     let expr = parse_expr(s)?;
-    let root = ImportLocation::Missing;
+    let root = ImportLocation::dhall_code_of_unknown_origin();
     Ok(Parsed(expr, root))
 }
 
 pub fn parse_binary(data: &[u8]) -> Result<Parsed, Error> {
     let expr = binary::decode(data)?;
-    let root = ImportLocation::Missing;
+    let root = ImportLocation::dhall_code_of_unknown_origin();
     Ok(Parsed(expr, root))
 }
 
 pub fn parse_binary_file(f: &Path) -> Result<Parsed, Error> {
     let data = crate::utils::read_binary_file(f)?;
     let expr = binary::decode(&data)?;
-    let root = ImportLocation::Local(f.to_owned());
+    let root = ImportLocation::local_dhall_code(f.to_owned());
     Ok(Parsed(expr, root))
 }
diff --git a/dhall/src/semantics/resolve/resolve.rs b/dhall/src/semantics/resolve/resolve.rs
index dc1951e..16987de 100644
--- a/dhall/src/semantics/resolve/resolve.rs
+++ b/dhall/src/semantics/resolve/resolve.rs
@@ -22,7 +22,7 @@ pub type Import = syntax::Import<()>;
 
 /// The location of some data, usually some dhall code.
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
-pub enum ImportLocation {
+enum ImportLocationKind {
     /// Local file
     Local(PathBuf),
     /// Remote file
@@ -33,63 +33,29 @@ pub enum ImportLocation {
     Missing,
 }
 
-impl ImportLocation {
-    /// Given an import pointing to `target` found in the current location, compute the next
-    /// location, or error if not allowed.
-    /// `sanity_check` indicates whether to check if that location is allowed to be referenced,
-    /// for example to prevent a remote file from reading an environment variable.
-    fn chain(
-        &self,
-        target: &ImportTarget<()>,
-        sanity_check: bool,
-    ) -> Result<ImportLocation, Error> {
-        Ok(match target {
-            ImportTarget::Local(prefix, path) => {
-                self.chain_local(*prefix, path)?
-            }
-            ImportTarget::Remote(remote) => {
-                if sanity_check {
-                    if let ImportLocation::Remote(..) = self {
-                        // TODO: allow if CORS check passes
-                        return Err(ImportError::SanityCheck.into());
-                    }
-                }
-                let mut url = Url::parse(&format!(
-                    "{}://{}",
-                    remote.scheme, remote.authority
-                ))?;
-                url.set_path(&remote.path.file_path.iter().join("/"));
-                url.set_query(remote.query.as_ref().map(String::as_ref));
-                ImportLocation::Remote(url)
-            }
-            ImportTarget::Env(var_name) => {
-                if sanity_check {
-                    if let ImportLocation::Remote(..) = self {
-                        return Err(ImportError::SanityCheck.into());
-                    }
-                }
-                ImportLocation::Env(var_name.clone())
-            }
-            ImportTarget::Missing => ImportLocation::Missing,
-        })
-    }
+/// The location of some data.
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+pub struct ImportLocation {
+    kind: ImportLocationKind,
+    mode: ImportMode,
+}
 
+impl ImportLocationKind {
     fn chain_local(
         &self,
         prefix: FilePrefix,
         path: &FilePath,
-    ) -> Result<ImportLocation, Error> {
+    ) -> Result<Self, Error> {
         Ok(match self {
-            ImportLocation::Local(..)
-            | ImportLocation::Env(..)
-            | ImportLocation::Missing => {
+            ImportLocationKind::Local(..)
+            | ImportLocationKind::Env(..)
+            | ImportLocationKind::Missing => {
                 let dir = match self {
-                    ImportLocation::Local(path) => {
+                    ImportLocationKind::Local(path) => {
                         path.parent().unwrap().to_owned()
                     }
-                    ImportLocation::Env(..) | ImportLocation::Missing => {
-                        std::env::current_dir()?
-                    }
+                    ImportLocationKind::Env(..)
+                    | ImportLocationKind::Missing => std::env::current_dir()?,
                     _ => unreachable!(),
                 };
                 let mut dir: Vec<String> = dir
@@ -120,9 +86,9 @@ impl ImportLocation {
                 };
                 let path =
                     Some(prefix.to_string()).into_iter().chain(path).collect();
-                ImportLocation::Local(path)
+                ImportLocationKind::Local(path)
             }
-            ImportLocation::Remote(url) => {
+            ImportLocationKind::Remote(url) => {
                 let mut url = url.clone();
                 match prefix {
                     FilePrefix::Here => {}
@@ -133,46 +99,56 @@ impl ImportLocation {
                     FilePrefix::Home => panic!("error"),
                 }
                 url = url.join(&path.file_path.join("/"))?;
-                ImportLocation::Remote(url)
+                ImportLocationKind::Remote(url)
             }
         })
     }
 
-    fn fetch_dhall(self) -> Result<Parsed, Error> {
+    fn fetch_dhall(&self) -> Result<Parsed, Error> {
         Ok(match self {
-            ImportLocation::Local(path) => Parsed::parse_file(&path)?,
-            ImportLocation::Remote(url) => Parsed::parse_remote(url)?,
-            ImportLocation::Env(var_name) => {
+            ImportLocationKind::Local(path) => Parsed::parse_file(path)?,
+            ImportLocationKind::Remote(url) => {
+                Parsed::parse_remote(url.clone())?
+            }
+            ImportLocationKind::Env(var_name) => {
                 let val = match env::var(var_name) {
                     Ok(val) => val,
                     Err(_) => return Err(ImportError::MissingEnvVar.into()),
                 };
                 Parsed::parse_str(&val)?
             }
-            ImportLocation::Missing => return Err(ImportError::Missing.into()),
+            ImportLocationKind::Missing => {
+                return Err(ImportError::Missing.into())
+            }
         })
     }
 
-    fn fetch_text(self) -> Result<String, Error> {
+    fn fetch_text(&self) -> Result<String, Error> {
         Ok(match self {
-            ImportLocation::Local(path) => std::fs::read_to_string(&path)?,
-            ImportLocation::Remote(url) => download_http_text(url)?,
-            ImportLocation::Env(var_name) => match env::var(var_name) {
+            ImportLocationKind::Local(path) => std::fs::read_to_string(path)?,
+            ImportLocationKind::Remote(url) => download_http_text(url.clone())?,
+            ImportLocationKind::Env(var_name) => match env::var(var_name) {
                 Ok(val) => val,
                 Err(_) => return Err(ImportError::MissingEnvVar.into()),
             },
-            ImportLocation::Missing => return Err(ImportError::Missing.into()),
+            ImportLocationKind::Missing => {
+                return Err(ImportError::Missing.into())
+            }
         })
     }
 
-    fn into_location(self) -> Expr {
+    fn to_location(&self) -> Expr {
         let (field_name, arg) = match self {
-            ImportLocation::Local(path) => {
+            ImportLocationKind::Local(path) => {
                 ("Local", Some(path.to_string_lossy().into_owned()))
             }
-            ImportLocation::Remote(url) => ("Remote", Some(url.into_string())),
-            ImportLocation::Env(name) => ("Environment", Some(name)),
-            ImportLocation::Missing => ("Missing", None),
+            ImportLocationKind::Remote(url) => {
+                ("Remote", Some(url.to_string()))
+            }
+            ImportLocationKind::Env(name) => {
+                ("Environment", Some(name.clone()))
+            }
+            ImportLocationKind::Missing => ("Missing", None),
         };
 
         let asloc_ty = make_aslocation_uniontype();
@@ -188,6 +164,96 @@ impl ImportLocation {
     }
 }
 
+impl ImportLocation {
+    pub fn dhall_code_of_unknown_origin() -> Self {
+        ImportLocation {
+            kind: ImportLocationKind::Missing,
+            mode: ImportMode::Code,
+        }
+    }
+    pub fn local_dhall_code(path: PathBuf) -> Self {
+        ImportLocation {
+            kind: ImportLocationKind::Local(path),
+            mode: ImportMode::Code,
+        }
+    }
+    pub fn remote_dhall_code(url: Url) -> Self {
+        ImportLocation {
+            kind: ImportLocationKind::Remote(url),
+            mode: ImportMode::Code,
+        }
+    }
+
+    /// Given an import pointing to `target` found in the current location, compute the next
+    /// location, or error if not allowed.
+    /// `sanity_check` indicates whether to check if that location is allowed to be referenced,
+    /// for example to prevent a remote file from reading an environment variable.
+    fn chain(&self, import: &Import) -> Result<ImportLocation, Error> {
+        // Makes no sense to chain an import if the current file is not a dhall file.
+        assert!(matches!(self.mode, ImportMode::Code));
+        let kind = match &import.location {
+            ImportTarget::Local(prefix, path) => {
+                self.kind.chain_local(*prefix, path)?
+            }
+            ImportTarget::Remote(remote) => {
+                if matches!(self.kind, ImportLocationKind::Remote(..))
+                    && !matches!(import.mode, ImportMode::Location)
+                {
+                    // TODO: allow if CORS check passes
+                    return Err(ImportError::SanityCheck.into());
+                }
+                let mut url = Url::parse(&format!(
+                    "{}://{}",
+                    remote.scheme, remote.authority
+                ))?;
+                url.set_path(&remote.path.file_path.iter().join("/"));
+                url.set_query(remote.query.as_ref().map(String::as_ref));
+                ImportLocationKind::Remote(url)
+            }
+            ImportTarget::Env(var_name) => {
+                if matches!(self.kind, ImportLocationKind::Remote(..))
+                    && !matches!(import.mode, ImportMode::Location)
+                {
+                    return Err(ImportError::SanityCheck.into());
+                }
+                ImportLocationKind::Env(var_name.clone())
+            }
+            ImportTarget::Missing => ImportLocationKind::Missing,
+        };
+        Ok(ImportLocation {
+            kind,
+            mode: import.mode,
+        })
+    }
+
+    /// Fetches the expression corresponding to this location.
+    fn fetch(&self, env: &mut ImportEnv, span: Span) -> Result<Typed, Error> {
+        let (hir, ty) = match self.mode {
+            ImportMode::Code => {
+                let parsed = self.kind.fetch_dhall()?;
+                let typed = resolve_with_env(env, parsed)?.typecheck()?;
+                let hir = typed.normalize().to_hir();
+                (hir, typed.ty)
+            }
+            ImportMode::RawText => {
+                let text = self.kind.fetch_text()?;
+                let hir = Hir::new(
+                    HirKind::Expr(ExprKind::TextLit(text.into())),
+                    span,
+                );
+                (hir, Type::from_builtin(Builtin::Text))
+            }
+            ImportMode::Location => {
+                let expr = self.kind.to_location();
+                let hir = skip_resolve_expr(&expr)?;
+                let ty = hir.typecheck_noenv()?.ty().clone();
+                (hir, ty)
+            }
+        };
+        Ok(Typed { hir, ty })
+    }
+}
+
 fn mkexpr(kind: UnspannedExpr) -> Expr {
     Expr::new(kind, Span::Artificial)
 }
@@ -237,35 +303,6 @@ fn check_hash(import: &Import, typed: &Typed, span: Span) -> Result<(), Error> {
     Ok(())
 }
 
-fn resolve_one_import(
-    env: &mut ImportEnv,
-    import: &Import,
-    location: ImportLocation,
-    span: Span,
-) -> Result<Typed, Error> {
-    let (hir, ty) = match import.mode {
-        ImportMode::Code => {
-            let parsed = location.fetch_dhall()?;
-            let typed = resolve_with_env(env, parsed)?.typecheck()?;
-            let hir = typed.normalize().to_hir();
-            (hir, typed.ty)
-        }
-        ImportMode::RawText => {
-            let text = location.fetch_text()?;
-            let hir =
-                Hir::new(HirKind::Expr(ExprKind::TextLit(text.into())), span);
-            (hir, Type::from_builtin(Builtin::Text))
-        }
-        ImportMode::Location => {
-            let expr = location.into_location();
-            let hir = skip_resolve_expr(&expr)?;
-            let ty = hir.typecheck_noenv()?.ty().clone();
-            (hir, ty)
-        }
-    };
-    Ok(Typed { hir, ty })
-}
-
 /// Desugar the first level of the expression.
 fn desugar(expr: &Expr) -> Cow<'_, Expr> {
     match expr.kind() {
@@ -358,9 +395,7 @@ fn resolve_with_env(
         &mut NameEnv::new(),
         &expr,
         &mut |import, span| {
-            let do_sanity_check = import.mode != ImportMode::Location;
-            let location =
-                base_location.chain(&import.location, do_sanity_check)?;
+            let location = base_location.chain(&import)?;
 
             // If the import is in the in-memory cache, or the hash is in the on-disk cache, return
             // the cached contents.
@@ -383,7 +418,7 @@ fn resolve_with_env(
             // Resolve this import, making sure that recursive imports don't cycle back to the
             // current one.
             let res = env.with_cycle_detection(location.clone(), |env| {
-                resolve_one_import(env, &import, location.clone(), span.clone())
+                location.fetch(env, span.clone())
             });
             let typed = match res {
                 Ok(typed) => typed,
diff --git a/dhall/tests/import/failure/cycle.txt b/dhall/tests/import/failure/cycle.txt
index 899484b..641667f 100644
--- a/dhall/tests/import/failure/cycle.txt
+++ b/dhall/tests/import/failure/cycle.txt
@@ -10,7 +10,7 @@ Type error: error: error
  --> <current file>:1:1
   |
 1 | ../data/cycle.dhall
-  | ^^^^^^^^^^^^^^^^^^^ ImportCycle([Local("./dhall-lang/tests/import/data/cycle.dhall"), Local("./dhall-lang/tests/import/failure/cycle.dhall")], Local("./dhall-lang/tests/import/data/cycle.dhall"))
+  | ^^^^^^^^^^^^^^^^^^^ ImportCycle([ImportLocation { kind: Local("./dhall-lang/tests/import/data/cycle.dhall"), mode: Code }, ImportLocation { kind: Local("./dhall-lang/tests/import/failure/cycle.dhall"), mode: Code }], ImportLocation { kind: Local("./dhall-lang/tests/import/data/cycle.dhall"), mode: Code })
   |
   |
   |
diff --git a/dhall/tests/import/success/unit/MixImportModesA.dhall b/dhall/tests/import/success/unit/MixImportModesA.dhall
new file mode 100644
index 0000000..8946ff8
--- /dev/null
+++ b/dhall/tests/import/success/unit/MixImportModesA.dhall
@@ -0,0 +1 @@
+{ n = ../../data/simple.dhall, txt = ../../data/simple.dhall as Text, loc = ../../data/simple.dhall as Location }
diff --git a/dhall/tests/import/success/unit/MixImportModesB.dhall b/dhall/tests/import/success/unit/MixImportModesB.dhall
new file mode 100644
index 0000000..430e0bc
--- /dev/null
+++ b/dhall/tests/import/success/unit/MixImportModesB.dhall
@@ -0,0 +1 @@
+{ loc = < Environment: Text | Local: Text | Missing | Remote: Text >.Local "./dhall/tests/import/data/simple.dhall", n = 3, txt = "3\n" }
diff --git a/dhall/tests/spec.rs b/dhall/tests/spec.rs
index 6b156f6..646083a 100644
--- a/dhall/tests/spec.rs
+++ b/dhall/tests/spec.rs
@@ -519,6 +519,7 @@ fn ignore_test(variant: SpecTestKind, path: &str) -> bool {
         // Paths on windows have backslashes; this breaks many things. This is undefined in the
         // spec; see https://github.com/dhall-lang/dhall-lang/issues/1032
         || (variant == ImportSuccess && path.contains("asLocation"))
+        || path == "import/success/unit/MixImportModes"
         || variant == ImportFailure;
 
     // Only include in release tests.