Fix and improve

- Set context location to resolved path during module evaluation.
- Dump module diagnostics on import
- Use same-file for more robustness than fs::canonicalize

5 files changed, 65 insertions, 46 deletions

diff --git a/Cargo.toml b/Cargo.toml
index fa3668a2..58f5c937 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -7,7 +7,7 @@ edition = "2018"
 [features]
 default = ["cli", "fs"]
 cli = ["anyhow", "fs"]
-fs = ["dirs", "memmap2", "walkdir"]
+fs = ["dirs", "memmap2", "same-file", "walkdir"]
 
 [workspace]
 members = ["bench"]
@@ -35,6 +35,7 @@ xi-unicode = "0.3"
 anyhow = { version = "1", optional = true }
 dirs = { version = "3", optional = true }
 memmap2 = { version = "0.2", optional = true }
+same-file = { version = "1", optional = true }
 walkdir = { version = "2", optional = true }
 
 [dev-dependencies]
diff --git a/src/eval/mod.rs b/src/eval/mod.rs
index e40f91da..38c6263e 100644
--- a/src/eval/mod.rs
+++ b/src/eval/mod.rs
@@ -11,6 +11,7 @@ pub use scope::*;
 pub use value::*;
 
 use std::collections::HashMap;
+use std::mem;
 use std::path::{Path, PathBuf};
 use std::rc::Rc;
 
@@ -65,10 +66,10 @@ pub struct EvalContext<'a> {
     pub scopes: Scopes<'a>,
     /// Evaluation diagnostics.
     pub diags: DiagSet,
-    /// The stack of imported files that led to evaluation of the current file.
-    pub route: Vec<FileHash>,
     /// The location of the currently evaluated file.
     pub path: PathBuf,
+    /// The stack of imported files that led to evaluation of the current file.
+    pub route: Vec<FileHash>,
     /// A map of loaded module.
     pub modules: HashMap<FileHash, Module>,
 }
@@ -91,8 +92,8 @@ impl<'a> EvalContext<'a> {
             cache,
             scopes: Scopes::with_base(Some(base)),
             diags: DiagSet::new(),
-            route,
             path: path.to_owned(),
+            route,
             modules: HashMap::new(),
         }
     }
@@ -116,12 +117,13 @@ impl<'a> EvalContext<'a> {
     pub fn import(&mut self, path: &str, span: Span) -> Option<FileHash> {
         let (resolved, hash) = self.resolve(path, span)?;
 
-        // Prevent cycling importing.
+        // Prevent cyclic importing.
         if self.route.contains(&hash) {
             self.diag(error!(span, "cyclic import"));
             return None;
         }
 
+        // Check whether the module was already loaded.
         if self.modules.get(&hash).is_some() {
             return Some(hash);
         }
@@ -136,19 +138,33 @@ impl<'a> EvalContext<'a> {
             None
         })?;
 
+        // Parse the file.
+        let parsed = parse(string);
+
         // Prepare the new context.
-        self.route.push(hash);
         let new_scopes = Scopes::with_base(self.scopes.base);
-        let old_scopes = std::mem::replace(&mut self.scopes, new_scopes);
+        let old_scopes = mem::replace(&mut self.scopes, new_scopes);
+        let old_diags = mem::replace(&mut self.diags, parsed.diags);
+        let old_path = mem::replace(&mut self.path, resolved);
+        self.route.push(hash);
 
         // Evaluate the module.
-        let tree = Rc::new(parse(string).output);
+        let tree = Rc::new(parsed.output);
         let map = tree.eval(self);
 
         // Restore the old context.
-        let new_scopes = std::mem::replace(&mut self.scopes, old_scopes);
+        let new_scopes = mem::replace(&mut self.scopes, old_scopes);
+        let new_diags = mem::replace(&mut self.diags, old_diags);
+        self.path = old_path;
         self.route.pop();
 
+        // Put all diagnostics from the module on the import.
+        for mut diag in new_diags {
+            diag.span = span;
+            self.diag(diag);
+        }
+
+        // Save the evaluated module.
         self.modules.insert(hash, Module {
             scope: new_scopes.top,
             template: vec![TemplateNode::Tree { tree, map }],
@@ -430,7 +446,7 @@ impl BinaryExpr {
             }
         };
 
-        let lhs = std::mem::take(&mut *mutable);
+        let lhs = mem::take(&mut *mutable);
         let types = (lhs.type_name(), rhs.type_name());
         *mutable = op(lhs, rhs);
 
@@ -513,7 +529,7 @@ impl Eval for ClosureExpr {
         Value::Func(FuncValue::new(name, move |ctx, args| {
             // Don't leak the scopes from the call site. Instead, we use the
             // scope of captured variables we collected earlier.
-            let prev = std::mem::take(&mut ctx.scopes);
+            let prev = mem::take(&mut ctx.scopes);
             ctx.scopes.top = captured.clone();
 
             for param in params.iter() {
@@ -657,11 +673,9 @@ impl Eval for ImportExpr {
     type Output = Value;
 
     fn eval(&self, ctx: &mut EvalContext) -> Self::Output {
-        let span = self.path.span();
         let path = self.path.eval(ctx);
-
-        if let Some(path) = ctx.cast::<String>(path, span) {
-            if let Some(hash) = ctx.import(&path, span) {
+        if let Some(path) = ctx.cast::<String>(path, self.path.span()) {
+            if let Some(hash) = ctx.import(&path, self.path.span()) {
                 let mut module = &ctx.modules[&hash];
                 match &self.imports {
                     Imports::Wildcard => {
@@ -695,11 +709,9 @@ impl Eval for IncludeExpr {
     type Output = Value;
 
     fn eval(&self, ctx: &mut EvalContext) -> Self::Output {
-        let span = self.path.span();
         let path = self.path.eval(ctx);
-
-        if let Some(path) = ctx.cast::<String>(path, span) {
-            if let Some(hash) = ctx.import(&path, span) {
+        if let Some(path) = ctx.cast::<String>(path, self.path.span()) {
+            if let Some(hash) = ctx.import(&path, self.path.span()) {
                 return Value::Template(ctx.modules[&hash].template.clone());
             }
         }
diff --git a/src/library/lang.rs b/src/library/lang.rs
index 9e163755..d88d23c0 100644
--- a/src/library/lang.rs
+++ b/src/library/lang.rs
@@ -16,20 +16,19 @@ use super::*;
 ///   - `ltr`
 ///   - `rtl`
 pub fn lang(ctx: &mut EvalContext, args: &mut FuncArgs) -> Value {
-    let iso = args.eat::<String>(ctx).map(|s| s.to_ascii_lowercase());
-    let dir = args.eat_named::<Spanned<Dir>>(ctx, "dir");
-
-    Value::template("lang", move |ctx| {
-        if let Some(iso) = &iso {
-            ctx.state.lang.dir = lang_dir(iso);
+    let iso = args.eat::<String>(ctx).map(|s| lang_dir(&s));
+    let dir = match args.eat_named::<Spanned<Dir>>(ctx, "dir") {
+        Some(dir) if dir.v.axis() == SpecAxis::Horizontal => Some(dir.v),
+        Some(dir) => {
+            ctx.diag(error!(dir.span, "must be horizontal"));
+            None
         }
+        None => None,
+    };
 
-        if let Some(dir) = dir {
-            if dir.v.axis() == SpecAxis::Horizontal {
-                ctx.state.lang.dir = dir.v;
-            } else {
-                ctx.diag(error!(dir.span, "must be horizontal"));
-            }
+    Value::template("lang", move |ctx| {
+        if let Some(dir) = dir.or(iso) {
+            ctx.state.lang.dir = dir;
         }
 
         ctx.parbreak();
@@ -38,7 +37,7 @@ pub fn lang(ctx: &mut EvalContext, args: &mut FuncArgs) -> Value {
 
 /// The default direction for the language identified by `iso`.
 fn lang_dir(iso: &str) -> Dir {
-    match iso {
+    match iso.to_ascii_lowercase().as_str() {
         "ar" | "he" | "fa" | "ur" | "ps" | "yi" => Dir::RTL,
         "en" | "fr" | "de" | _ => Dir::LTR,
     }
diff --git a/src/loading/fs.rs b/src/loading/fs.rs
index bf768bd5..e2654a2e 100644
--- a/src/loading/fs.rs
+++ b/src/loading/fs.rs
@@ -5,6 +5,7 @@ use std::path::{Path, PathBuf};
 use std::rc::Rc;
 
 use memmap2::Mmap;
+use same_file::Handle;
 use serde::{Deserialize, Serialize};
 use ttf_parser::{name_id, Face};
 use walkdir::WalkDir;
@@ -167,10 +168,6 @@ impl Loader for FsLoader {
         &self.faces
     }
 
-    fn resolve(&self, path: &Path) -> Option<FileHash> {
-        hash(path)
-    }
-
     fn load_face(&mut self, idx: usize) -> Option<Buffer> {
         load(&mut self.cache, &self.files[idx])
     }
@@ -178,6 +175,10 @@ impl Loader for FsLoader {
     fn load_file(&mut self, path: &Path) -> Option<Buffer> {
         load(&mut self.cache, path)
     }
+
+    fn resolve(&self, path: &Path) -> Option<FileHash> {
+        hash(path)
+    }
 }
 
 /// Load from the file system using a cache.
@@ -191,8 +192,11 @@ fn load(cache: &mut FileCache, path: &Path) -> Option<Buffer> {
     })
 }
 
+/// Create a hash that is the same for all paths pointing to the same file.
 fn hash(path: &Path) -> Option<FileHash> {
-    path.canonicalize().ok().map(|p| FileHash(fxhash::hash64(&p)))
+    Handle::from_path(path)
+        .map(|handle| FileHash(fxhash::hash64(&handle)))
+        .ok()
 }
 
 #[cfg(test)]
diff --git a/src/loading/mod.rs b/src/loading/mod.rs
index b4e5d160..f57b5c73 100644
--- a/src/loading/mod.rs
+++ b/src/loading/mod.rs
@@ -19,19 +19,22 @@ pub trait Loader {
     /// Descriptions of all font faces this loader serves.
     fn faces(&self) -> &[FaceInfo];
 
-    /// Resolve a hash that is the same for all paths pointing to the same file.
-    ///
-    /// Should return `None` if the file does not exist.
-    fn resolve(&self, path: &Path) -> Option<FileHash>;
-
     /// Load the font face with the given index in [`faces()`](Self::faces).
     fn load_face(&mut self, idx: usize) -> Option<Buffer>;
 
     /// Load a file from a path.
     fn load_file(&mut self, path: &Path) -> Option<Buffer>;
+
+    /// Resolve a hash for the file the path points to.
+    ///
+    /// This should return the same hash for all paths pointing to the same file
+    /// and `None` if the file does not exist.
+    fn resolve(&self, path: &Path) -> Option<FileHash>;
 }
 
-/// A hash that must be the same for all paths pointing to the same file.
+/// A file hash that can be [resolved](Loader::resolve) from a path.
+///
+/// Should be the same for all paths pointing to the same file.
 #[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)]
 pub struct FileHash(pub u64);
 
@@ -43,15 +46,15 @@ impl Loader for BlankLoader {
         &[]
     }
 
-    fn resolve(&self, _: &Path) -> Option<FileHash> {
+    fn load_face(&mut self, _: usize) -> Option<Buffer> {
         None
     }
 
-    fn load_face(&mut self, _: usize) -> Option<Buffer> {
+    fn load_file(&mut self, _: &Path) -> Option<Buffer> {
         None
     }
 
-    fn load_file(&mut self, _: &Path) -> Option<Buffer> {
+    fn resolve(&self, _: &Path) -> Option<FileHash> {
         None
     }
 }