Store Imports in their own node instead of in Embed

8 files changed, 61 insertions, 33 deletions

diff --git a/dhall/src/phase/binary.rs b/dhall/src/phase/binary.rs
index 3292617..b3a80aa 100644
--- a/dhall/src/phase/binary.rs
+++ b/dhall/src/phase/binary.rs
@@ -19,7 +19,6 @@ pub fn decode(data: &[u8]) -> Result<DecodedSubExpr, DecodeError> {
     }
 }
 
-//TODO: encode normalized expression too
 pub fn encode(expr: &ParsedSubExpr) -> Result<Vec<u8>, EncodeError> {
     serde_cbor::ser::to_vec(&Serialize::Expr(expr))
         .map_err(|e| EncodeError::CBORError(e))
@@ -264,7 +263,7 @@ fn cbor_value_to_dhall(
                             // TODO
                             // Some(x) => {
                             //     match cbor_value_to_dhall(&x)?.as_ref() {
-                            //         Embed(import) => Some(Box::new(
+                            //         Import(import) => Some(Box::new(
                             //             import.location_hashed.clone(),
                             //         )),
                             //         _ => Err(DecodeError::WrongFormatError(
@@ -340,7 +339,7 @@ fn cbor_value_to_dhall(
                         "import/type".to_owned(),
                     ))?,
                 };
-                Embed(Import {
+                Import(dhall_syntax::Import {
                     mode,
                     location_hashed: ImportHashed { hash, location },
                 })
@@ -573,7 +572,10 @@ where
                 .chain(once(expr(x)))
                 .chain(ls.iter().map(label)),
         ),
-        Embed(import) => serialize_import(ser, import),
+        Import(import) => serialize_import(ser, import),
+        Embed(_) => unimplemented!(
+            "An expression with resolved imports cannot be binary-encoded"
+        ),
     }
 }
 
@@ -631,12 +633,14 @@ where
         ImportLocation::Remote(url) => {
             match &url.headers {
                 None => ser_seq.serialize_element(&Null)?,
-                Some(location_hashed) => ser_seq.serialize_element(
-                    &self::Serialize::Expr(&rc(ExprF::Embed(Import {
-                        mode: ImportMode::Code,
-                        location_hashed: location_hashed.as_ref().clone(),
-                    }))),
-                )?,
+                Some(location_hashed) => {
+                    ser_seq.serialize_element(&self::Serialize::Expr(&rc(
+                        ExprF::Import(dhall_syntax::Import {
+                            mode: ImportMode::Code,
+                            location_hashed: location_hashed.as_ref().clone(),
+                        }),
+                    )))?
+                }
             };
             ser_seq.serialize_element(&url.authority)?;
             for p in &url.path {
diff --git a/dhall/src/phase/mod.rs b/dhall/src/phase/mod.rs
index 7364949..a1e3f29 100644
--- a/dhall/src/phase/mod.rs
+++ b/dhall/src/phase/mod.rs
@@ -2,7 +2,7 @@ use std::borrow::Cow;
 use std::fmt::Display;
 use std::path::Path;
 
-use dhall_syntax::{Const, Import, SubExpr};
+use dhall_syntax::{Const, SubExpr, Void};
 
 use crate::core::thunk::{Thunk, TypedThunk};
 use crate::core::value::Value;
@@ -17,8 +17,8 @@ pub(crate) mod parse;
 pub(crate) mod resolve;
 pub(crate) mod typecheck;
 
-pub type ParsedSubExpr = SubExpr<Import>;
-pub type DecodedSubExpr = SubExpr<Import>;
+pub type ParsedSubExpr = SubExpr<Void>;
+pub type DecodedSubExpr = SubExpr<Void>;
 pub type ResolvedSubExpr = SubExpr<Normalized>;
 pub type NormalizedSubExpr = SubExpr<Normalized>;
 
@@ -26,6 +26,8 @@ pub type NormalizedSubExpr = SubExpr<Normalized>;
 pub struct Parsed(ParsedSubExpr, ImportRoot);
 
 /// An expression where all imports have been resolved
+///
+/// Invariant: there must be no `Import` nodes or `ImportAlt` operations left.
 #[derive(Debug, Clone)]
 pub struct Resolved(ResolvedSubExpr);
 
diff --git a/dhall/src/phase/normalize.rs b/dhall/src/phase/normalize.rs
index 2970f5f..adbcb02 100644
--- a/dhall/src/phase/normalize.rs
+++ b/dhall/src/phase/normalize.rs
@@ -644,6 +644,9 @@ pub fn normalize_one_layer(expr: ExprF<Thunk, Normalized>) -> Value {
     };
 
     let ret = match expr {
+        ExprF::Import(_) => unreachable!(
+            "There should remain no imports in a resolved expression"
+        ),
         ExprF::Embed(_) => unreachable!(),
         ExprF::Var(_) => unreachable!(),
         ExprF::Annot(x, _) => Ret::Thunk(x),
diff --git a/dhall/src/phase/typecheck.rs b/dhall/src/phase/typecheck.rs
index e8b2544..89e2da8 100644
--- a/dhall/src/phase/typecheck.rs
+++ b/dhall/src/phase/typecheck.rs
@@ -407,6 +407,9 @@ fn type_last_layer(
     let mkerr = |msg: TypeMessage| TypeError::new(ctx, msg);
 
     match e {
+        Import(_) => unreachable!(
+            "There should remain no imports in a resolved expression"
+        ),
         Lam(_, _, _) | Pi(_, _, _) | Let(_, _, _, _) | Embed(_) | Var(_) => {
             unreachable!()
         }
diff --git a/dhall_syntax/src/core/expr.rs b/dhall_syntax/src/core/expr.rs
index 74eb993..4c809f8 100644
--- a/dhall_syntax/src/core/expr.rs
+++ b/dhall_syntax/src/core/expr.rs
@@ -12,6 +12,12 @@ pub type Double = NaiveDouble;
 #[derive(Debug, Copy, Clone, PartialEq, Eq)]
 pub enum Void {}
 
+impl std::fmt::Display for Void {
+    fn fmt(&self, _f: &mut std::fmt::Formatter) -> Result<(), std::fmt::Error> {
+        match *self {}
+    }
+}
+
 pub fn trivial_result<T>(x: Result<T, Void>) -> T {
     match x {
         Ok(x) => x,
@@ -233,7 +239,9 @@ pub enum ExprF<SubExpr, Embed> {
     Field(SubExpr, Label),
     ///  `e.{ x, y, z }`
     Projection(SubExpr, DupTreeSet<Label>),
-    /// Embeds an import or the result of resolving the import
+    /// `./some/path`
+    Import(Import),
+    /// Embeds the result of resolving an import
     Embed(Embed),
 }
 
@@ -294,7 +302,7 @@ impl<SE, E> ExprF<SE, E> {
     }
 }
 
-impl Expr<Import> {
+impl<E> Expr<E> {
     pub fn traverse_resolve<E2, Err>(
         &self,
         visit_import: impl FnMut(&Import) -> Result<E2, Err>,
@@ -316,6 +324,7 @@ impl Expr<Import> {
                 .as_ref()
                 .traverse_resolve_with_visitor(visitor)
                 .or(r.as_ref().traverse_resolve_with_visitor(visitor)),
+            ExprF::Import(import) => Ok(ExprF::Embed((visitor.0)(import)?)),
             _ => self.visit(visitor),
         }
     }
@@ -343,7 +352,7 @@ impl<E> SubExpr<E> {
     }
 }
 
-impl SubExpr<Import> {
+impl<E> SubExpr<E> {
     pub fn traverse_resolve<E2, Err>(
         &self,
         visit_import: impl FnMut(&Import) -> Result<E2, Err>,
diff --git a/dhall_syntax/src/core/visitor.rs b/dhall_syntax/src/core/visitor.rs
index f2f3b32..f3d9194 100644
--- a/dhall_syntax/src/core/visitor.rs
+++ b/dhall_syntax/src/core/visitor.rs
@@ -153,6 +153,7 @@ where
             Field(e, l) => Field(v.visit_subexpr(e)?, l.clone()),
             Projection(e, ls) => Projection(v.visit_subexpr(e)?, ls.clone()),
             Assert(e) => Assert(v.visit_subexpr(e)?),
+            Import(a) => Import(a.clone()),
             Embed(a) => Embed(v.visit_embed(a)?),
         })
     }
@@ -246,8 +247,8 @@ where
 
 pub struct ResolveVisitor<F1>(pub F1);
 
-impl<'a, 'b, E2, Err, F1>
-    ExprFFallibleVisitor<'a, SubExpr<Import>, SubExpr<E2>, Import, E2>
+impl<'a, 'b, E, E2, Err, F1>
+    ExprFFallibleVisitor<'a, SubExpr<E>, SubExpr<E2>, E, E2>
     for &'b mut ResolveVisitor<F1>
 where
     F1: FnMut(&Import) -> Result<E2, Err>,
@@ -256,7 +257,7 @@ where
 
     fn visit_subexpr(
         &mut self,
-        subexpr: &'a SubExpr<Import>,
+        subexpr: &'a SubExpr<E>,
     ) -> Result<SubExpr<E2>, Self::Error> {
         Ok(subexpr.rewrap(
             subexpr
@@ -264,7 +265,7 @@ where
                 .traverse_resolve_with_visitor(&mut **self)?,
         ))
     }
-    fn visit_embed(self, embed: &'a Import) -> Result<E2, Self::Error> {
-        (self.0)(embed)
+    fn visit_embed(self, _embed: &'a E) -> Result<E2, Self::Error> {
+        unimplemented!()
     }
 }
diff --git a/dhall_syntax/src/parser.rs b/dhall_syntax/src/parser.rs
index 1ab6e6d..a8dd359 100644
--- a/dhall_syntax/src/parser.rs
+++ b/dhall_syntax/src/parser.rs
@@ -15,10 +15,10 @@ use crate::*;
 // their own crate because they are quite general and useful. For now they
 // are here and hopefully you can figure out how they work.
 
-pub(crate) type ParsedExpr = Expr<Import>;
-pub(crate) type ParsedSubExpr = SubExpr<Import>;
-type ParsedText = InterpolatedText<SubExpr<Import>>;
-type ParsedTextContents = InterpolatedTextContents<SubExpr<Import>>;
+pub(crate) type ParsedExpr = Expr<Void>;
+pub(crate) type ParsedSubExpr = SubExpr<Void>;
+type ParsedText = InterpolatedText<ParsedSubExpr>;
+type ParsedTextContents = InterpolatedTextContents<ParsedSubExpr>;
 
 pub type ParseError = pest::error::Error<Rule>;
 
@@ -691,19 +691,19 @@ make_parser! {
 
     rule!(import<ParsedSubExpr> as expression; span; children!(
         [import_hashed(location_hashed)] => {
-            spanned(span, Embed(Import {
+            spanned(span, Import(crate::Import {
                 mode: ImportMode::Code,
                 location_hashed
             }))
         },
         [import_hashed(location_hashed), Text(_)] => {
-            spanned(span, Embed(Import {
+            spanned(span, Import(crate::Import {
                 mode: ImportMode::RawText,
                 location_hashed
             }))
         },
         [import_hashed(location_hashed), Location(_)] => {
-            spanned(span, Embed(Import {
+            spanned(span, Import(crate::Import {
                 mode: ImportMode::Location,
                 location_hashed
             }))
diff --git a/dhall_syntax/src/printer.rs b/dhall_syntax/src/printer.rs
index 48e56be..5f5f99b 100644
--- a/dhall_syntax/src/printer.rs
+++ b/dhall_syntax/src/printer.rs
@@ -88,6 +88,7 @@ impl<SE: Display + Clone, E: Display> Display for ExprF<SE, E> {
                 }
                 Ok(())
             })?,
+            Import(a) => a.fmt(f)?,
             Embed(a) => a.fmt(f)?,
         }
         Ok(())
@@ -151,7 +152,9 @@ impl<A: Display + Clone> Expr<A> {
             // Precedence is magically handled by the ordering of BinOps.
             ExprF::BinOp(op, _, _) if phase > PrintPhase::BinOp(*op) => true,
             ExprF::App(_, _) if phase > PrintPhase::App => true,
-            Field(_, _) | Projection(_, _) if phase > Import => true,
+            Field(_, _) | Projection(_, _) if phase > PrintPhase::Import => {
+                true
+            }
             _ => false,
         };
 
@@ -165,8 +168,8 @@ impl<A: Display + Clone> Expr<A> {
                 }
             }
             Merge(a, b, c) => Merge(
-                a.phase(Import),
-                b.phase(Import),
+                a.phase(PrintPhase::Import),
+                b.phase(PrintPhase::Import),
                 c.map(|x| x.phase(PrintPhase::App)),
             ),
             Annot(a, b) => Annot(a.phase(Operator), b),
@@ -175,8 +178,11 @@ impl<A: Display + Clone> Expr<A> {
                 a.phase(PrintPhase::BinOp(op)),
                 b.phase(PrintPhase::BinOp(op)),
             ),
-            SomeLit(e) => SomeLit(e.phase(Import)),
-            ExprF::App(f, a) => ExprF::App(f.phase(Import), a.phase(Import)),
+            SomeLit(e) => SomeLit(e.phase(PrintPhase::Import)),
+            ExprF::App(f, a) => ExprF::App(
+                f.phase(PrintPhase::Import),
+                a.phase(PrintPhase::Import),
+            ),
             Field(a, b) => Field(a.phase(Primitive), b),
             Projection(e, ls) => Projection(e.phase(Primitive), ls),
             e => e,