Repository: tweag/nickel (rust)

Issue:
Non-serializable error on top-level term shows no location
**Describe the bug**
If the top-level term one is trying to serialize (e.g. via `nickel export`) isn't serializable (typically a function), the "non-serializable term" error doesn't show any position, which makes it hard to understand where is the problem. This can typically happen when a function is not fully applied:

**To Reproduce**
```
// gen.ncl
fun param1 param2 => {
  foo = param1,
  bar = param2,
}

$nickel export <<< 'import "gen.ncl" "fooval"'
error: non serializable term
  ┌─ <unknown> (generated by evaluation):1:1
  │
1 │ <func>
  │ ^^^^^^
```

**Expected behavior**
The error should show the correct location of the function inside `gen.ncl`, as it does already for any term that is not the root node of the AST.


Pull request:
Do not discard position after evaluation
Fix #510. The `eval` family previously returned a `Term`, discarding the position information of the root node of the AST. It now returns a `RichTerm`, and the caller is free of using or discarding the position information.

Patch:
diff --git a/src/eval.rs b/src/eval.rs
index 928974b6..fde94d63 100644
--- a/src/eval.rs
+++ b/src/eval.rs
@@ -648,7 +648,11 @@ fn should_update(t: &Term) -> bool {
 
 /// Evaluate a Nickel term. Wrapper around [eval_closure](fn.eval_closure.html) that starts from an
 /// empty local environment and drops the final environment.
-pub fn eval<R>(t0: RichTerm, global_env: &Environment, resolver: &mut R) -> Result<Term, EvalError>
+pub fn eval<R>(
+    t0: RichTerm,
+    global_env: &Environment,
+    resolver: &mut R,
+) -> Result<RichTerm, EvalError>
 where
     R: ImportResolver,
 {
@@ -660,7 +664,7 @@ pub fn eval_full<R>(
     t0: RichTerm,
     global_env: &Environment,
     resolver: &mut R,
-) -> Result<Term, EvalError>
+) -> Result<RichTerm, EvalError>
 where
     R: ImportResolver,
 {
@@ -677,7 +681,7 @@ where
         ),
     );
     eval_closure(Closure::atomic_closure(wrapper), global_env, resolver, true)
-        .map(|(term, env)| subst(term.into(), global_env, &env).into())
+        .map(|(term, env)| subst(term, global_env, &env))
 }
 
 /// Evaluate a Nickel Term, stopping when a meta value is encountered at the top-level without
@@ -689,27 +693,26 @@ pub fn eval_meta<R>(
     t: RichTerm,
     global_env: &Environment,
     resolver: &mut R,
-) -> Result<Term, EvalError>
+) -> Result<RichTerm, EvalError>
 where
     R: ImportResolver,
 {
-    let (term, env) = eval_closure(Closure::atomic_closure(t), global_env, resolver, false)?;
+    let (mut rt, env) = eval_closure(Closure::atomic_closure(t), global_env, resolver, false)?;
 
-    match term {
-        Term::MetaValue(mut meta) => {
+    match *rt.term {
+        Term::MetaValue(ref mut meta) => {
             if let Some(t) = meta.value.take() {
-                let pos = t.pos;
                 let (evaluated, env) =
                     eval_closure(Closure { body: t, env }, global_env, resolver, true)?;
-                let substituted = subst(RichTerm::new(evaluated, pos), global_env, &env);
+                let substituted = subst(evaluated, global_env, &env);
 
-                meta.value.replace(substituted);
+                meta.value = Some(substituted);
             }
-
-            Ok(Term::MetaValue(meta))
         }
-        term => Ok(term),
-    }
+        _ => (),
+    };
+
+    Ok(rt)
 }
 
 /// The main loop of evaluation.
@@ -738,7 +741,7 @@ pub fn eval_closure<R>(
     global_env: &Environment,
     resolver: &mut R,
     mut enriched_strict: bool,
-) -> Result<(Term, Environment), EvalError>
+) -> Result<(RichTerm, Environment), EvalError>
 where
     R: ImportResolver,
 {
@@ -1018,7 +1021,7 @@ where
                 }
             }
             // Unwrapping of enriched terms
-            Term::MetaValue(mut meta) if enriched_strict => {
+            Term::MetaValue(meta) if enriched_strict => {
                 if meta.value.is_some() {
                     /* Since we are forcing a metavalue, we are morally evaluating `force t` rather
                      * than `t` iteself.  Updating a thunk after having performed this forcing may
@@ -1091,7 +1094,7 @@ where
                     env.insert(x, thunk);
                     Closure { body: t, env }
                 } else {
-                    return Ok((Term::Fun(x, t), env));
+                    return Ok((RichTerm::new(Term::Fun(x, t), pos), env));
                 }
             }
             // Otherwise, this is either an ill-formed application, or we are done
@@ -1106,7 +1109,7 @@ where
                         pos_app,
                     ));
                 } else {
-                    return Ok((t, env));
+                    return Ok((RichTerm::new(t, pos), env));
                 }
             }
         }
@@ -1336,7 +1339,7 @@ mod tests {
 
     /// Evaluate a term without import support.
     fn eval_no_import(t: RichTerm) -> Result<Term, EvalError> {
-        eval(t, &Environment::new(), &mut DummyResolver {})
+        eval(t, &Environment::new(), &mut DummyResolver {}).map(Term::from)
     }
 
     fn parse(s: &str) -> Option<RichTerm> {
@@ -1344,10 +1347,7 @@ mod tests {
 
         grammar::TermParser::new()
             .parse_term(id, lexer::Lexer::new(&s))
-            .map(|mut t| {
-                t.clean_pos();
-                t
-            })
+            .map(RichTerm::without_pos)
             .map_err(|err| println!("{:?}", err))
             .ok()
     }
@@ -1525,6 +1525,7 @@ mod tests {
                 &Environment::new(),
                 &mut resolver
             )
+            .map(Term::from)
             .unwrap(),
             Term::Num(2.0)
         );
@@ -1542,6 +1543,7 @@ mod tests {
                 &Environment::new(),
                 &mut resolver
             )
+            .map(Term::from)
             .unwrap(),
             Term::Bool(true)
         );
@@ -1623,14 +1625,23 @@ mod tests {
         );
 
         let t = mk_term::let_in("x", Term::Num(2.0), mk_term::var("x"));
-        assert_eq!(eval(t, &global_env, &mut resolver), Ok(Term::Num(2.0)));
+        assert_eq!(
+            eval(t, &global_env, &mut resolver).map(Term::from),
+            Ok(Term::Num(2.0))
+        );
 
         let t = mk_term::let_in("x", Term::Num(2.0), mk_term::var("g"));
-        assert_eq!(eval(t, &global_env, &mut resolver), Ok(Term::Num(1.0)));
+        assert_eq!(
+            eval(t, &global_env, &mut resolver).map(Term::from),
+            Ok(Term::Num(1.0))
+        );
 
         // Shadowing of global environment
         let t = mk_term::let_in("g", Term::Num(2.0), mk_term::var("g"));
-        assert_eq!(eval(t, &global_env, &mut resolver), Ok(Term::Num(2.0)));
+        assert_eq!(
+            eval(t, &global_env, &mut resolver).map(Term::from),
+            Ok(Term::Num(2.0))
+        );
     }
 
     fn mk_env(bindings: Vec<(&str, RichTerm)>) -> Environment {
diff --git a/src/program.rs b/src/program.rs
index 072b64f9..cacb5d7a 100644
--- a/src/program.rs
+++ b/src/program.rs
@@ -78,13 +78,13 @@ impl Program {
     }
 
     /// Parse if necessary, typecheck and then evaluate the program.
-    pub fn eval(&mut self) -> Result<Term, Error> {
+    pub fn eval(&mut self) -> Result<RichTerm, Error> {
         let (t, global_env) = self.prepare_eval()?;
         eval::eval(t, &global_env, &mut self.cache).map_err(|e| e.into())
     }
 
     /// Same as `eval`, but proceeds to a full evaluation.
-    pub fn eval_full(&mut self) -> Result<Term, Error> {
+    pub fn eval_full(&mut self) -> Result<RichTerm, Error> {
         let (t, global_env) = self.prepare_eval()?;
         eval::eval_full(t, &global_env, &mut self.cache).map_err(|e| e.into())
     }
@@ -167,7 +167,7 @@ pub fn query(
         cache.get_owned(file_id).unwrap()
     };
 
-    Ok(eval::eval_meta(t, &global_env.eval_env, cache)?)
+    Ok(eval::eval_meta(t, &global_env.eval_env, cache)?.into())
 }
 
 /// Pretty-print an error.
@@ -203,6 +203,7 @@ mod tests {
     use crate::error::EvalError;
     use crate::parser::{grammar, lexer};
     use crate::position::TermPos;
+    use crate::term::Term;
     use codespan::Files;
     use std::io::Cursor;
 
@@ -211,15 +212,12 @@ mod tests {
 
         grammar::TermParser::new()
             .parse_term(id, lexer::Lexer::new(&s))
-            .map(|mut t| {
-                t.clean_pos();
-                t
-            })
+            .map(RichTerm::without_pos)
             .map_err(|err| println!("{:?}", err))
             .ok()
     }
 
-    fn eval_full(s: &str) -> Result<Term, Error> {
+    fn eval_full(s: &str) -> Result<RichTerm, Error> {
         let src = Cursor::new(s);
 
         let mut p = Program::new_from_source(src, "<test>").map_err(|io_err| {
@@ -236,14 +234,7 @@ mod tests {
         use crate::mk_record;
         use crate::term::make as mk_term;
 
-        // Clean all the position information in a term.
-        fn clean_pos(t: Term) -> Term {
-            let mut tmp = RichTerm::new(t, TermPos::None);
-            tmp.clean_pos();
-            *tmp.term
-        }
-
-        let t = clean_pos(eval_full("[(1 + 1), (\"a\" ++ \"b\"), ([ 1, [1 + 2] ])]").unwrap());
+        let t = eval_full("[(1 + 1), (\"a\" ++ \"b\"), ([ 1, [1 + 2] ])]").unwrap();
         let mut expd = parse("[2, \"ab\", [1, [3]]]").unwrap();
 
         // String are parsed as StrChunks, but evaluated to Str, so we need to hack list a bit
@@ -253,17 +244,15 @@ mod tests {
             panic!();
         }
 
-        assert_eq!(t, *expd.term);
+        assert_eq!(t.without_pos(), expd.without_pos());
 
-        let t = clean_pos(
-            eval_full("let x = 1 in let y = 1 + x in let z = {foo.bar.baz = y} in z").unwrap(),
-        );
+        let t = eval_full("let x = 1 in let y = 1 + x in let z = {foo.bar.baz = y} in z").unwrap();
         // Records are parsed as RecRecords, so we need to build one by hand
         let expd = mk_record!((
             "foo",
             mk_record!(("bar", mk_record!(("baz", Term::Num(2.0)))))
         ));
-        assert_eq!(t, *expd.term);
+        assert_eq!(t.without_pos(), expd);
 
         // /!\ [MAY OVERFLOW STACK]
         // Check that substitution do not replace bound variables. Before the fixing commit, this
diff --git a/src/repl/mod.rs b/src/repl/mod.rs
index bc3870bd..984388f6 100644
--- a/src/repl/mod.rs
+++ b/src/repl/mod.rs
@@ -37,13 +37,13 @@ pub mod wasm_frontend;
 /// Result of the evaluation of an input.
 pub enum EvalResult {
     /// The input has been evaluated to a term.
-    Evaluated(Term),
+    Evaluated(RichTerm),
     /// The input was a toplevel let, which has been bound in the environment.
     Bound(Ident),
 }
 
-impl From<Term> for EvalResult {
-    fn from(t: Term) -> Self {
+impl From<RichTerm> for EvalResult {
+    fn from(t: RichTerm) -> Self {
         EvalResult::Evaluated(t)
     }
 }
diff --git a/src/repl/rustyline_frontend.rs b/src/repl/rustyline_frontend.rs
index 377e9af9..23dd65f2 100644
--- a/src/repl/rustyline_frontend.rs
+++ b/src/repl/rustyline_frontend.rs
@@ -78,7 +78,7 @@ pub fn repl(histfile: PathBuf) -> Result<(), InitError> {
                     }),
                     Ok(Command::Print(exp)) => {
                         match repl.eval_full(&exp) {
-                            Ok(EvalResult::Evaluated(t)) => println!("{}\n", t.deep_repr()),
+                            Ok(EvalResult::Evaluated(rt)) => println!("{}\n", rt.as_ref().deep_repr()),
                             Ok(EvalResult::Bound(_)) => (),
                             Err(err) => program::report(repl.cache_mut(), err),
                         };
@@ -103,7 +103,7 @@ pub fn repl(histfile: PathBuf) -> Result<(), InitError> {
             }
             Ok(line) => {
                 match repl.eval(&line) {
-                    Ok(EvalResult::Evaluated(t)) => println!("{}\n", t.shallow_repr()),
+                    Ok(EvalResult::Evaluated(rt)) => println!("{}\n", rt.as_ref().shallow_repr()),
                     Ok(EvalResult::Bound(_)) => (),
                     Err(err) => program::report(repl.cache_mut(), err),
                 };
diff --git a/src/repl/simple_frontend.rs b/src/repl/simple_frontend.rs
index ec4b5183..0268a6b7 100644
--- a/src/repl/simple_frontend.rs
+++ b/src/repl/simple_frontend.rs
@@ -65,7 +65,7 @@ pub fn input<R: REPL>(repl: &mut R, line: &str) -> Result<InputResult, InputErro
             Ok(Command::Print(exp)) => repl
                 .eval_full(&exp)
                 .map(|res| match res {
-                    EvalResult::Evaluated(t) => InputResult::Success(t.deep_repr()),
+                    EvalResult::Evaluated(rt) => InputResult::Success(rt.as_ref().deep_repr()),
                     EvalResult::Bound(_) => InputResult::Blank,
                 })
                 .map_err(InputError::from),
@@ -82,7 +82,9 @@ pub fn input<R: REPL>(repl: &mut R, line: &str) -> Result<InputResult, InputErro
     } else {
         repl.eval(&line)
             .map(|eval_res| match eval_res {
-                EvalResult::Evaluated(t) => InputResult::Success(format!("{}\n", t.shallow_repr())),
+                EvalResult::Evaluated(rt) => {
+                    InputResult::Success(format!("{}\n", rt.as_ref().shallow_repr()))
+                }
                 EvalResult::Bound(_) => InputResult::Success(String::new()),
             })
             .map_err(InputError::from)
diff --git a/src/serialize.rs b/src/serialize.rs
index 7cfd7713..25e4386e 100644
--- a/src/serialize.rs
+++ b/src/serialize.rs
@@ -299,7 +299,8 @@ mod tests {
                     mk_term::op2(BinaryOp::Eq(), from_json, evaluated.clone()),
                     &Environment::new(),
                     &mut $crate::cache::resolvers::DummyResolver {}
-                ),
+                )
+                .map(Term::from),
                 Ok(Term::Bool(true))
             );
             assert_eq!(
@@ -307,7 +308,8 @@ mod tests {
                     mk_term::op2(BinaryOp::Eq(), from_yaml, evaluated.clone()),
                     &Environment::new(),
                     &mut $crate::cache::resolvers::DummyResolver {}
-                ),
+                )
+                .map(Term::from),
                 Ok(Term::Bool(true))
             );
             assert_eq!(
@@ -315,7 +317,8 @@ mod tests {
                     mk_term::op2(BinaryOp::Eq(), from_toml, evaluated),
                     &Environment::new(),
                     &mut $crate::cache::resolvers::DummyResolver {}
-                ),
+                )
+                .map(Term::from),
                 Ok(Term::Bool(true))
             );
         };
diff --git a/src/term.rs b/src/term.rs
index 1d786e47..cf53c3fe 100644
--- a/src/term.rs
+++ b/src/term.rs
@@ -873,10 +873,15 @@ impl RichTerm {
     ///
     /// It allows to use rust `Eq` trait to compare the values of the underlying terms.
     #[cfg(test)]
-    pub fn clean_pos(&mut self) {
-        self.pos = TermPos::None;
-        self.term
-            .apply_to_rich_terms(|rt: &mut Self| rt.clean_pos());
+    pub fn without_pos(mut self) -> Self {
+        fn clean_pos(rt: &mut RichTerm) {
+            rt.pos = TermPos::None;
+            rt.term
+                .apply_to_rich_terms(|rt: &mut RichTerm| clean_pos(rt));
+        }
+
+        clean_pos(&mut self);
+        self
     }
 
     /// Set the position and return the term updated.


Test patch:
diff --git a/src/parser/tests.rs b/src/parser/tests.rs
index e2a5a6c6..61b23ab1 100644
--- a/src/parser/tests.rs
+++ b/src/parser/tests.rs
@@ -18,9 +18,7 @@ fn parse(s: &str) -> Result<RichTerm, ParseError> {
 }
 
 fn parse_without_pos(s: &str) -> RichTerm {
-    let mut result = parse(s).unwrap();
-    result.clean_pos();
-    result
+    parse(s).unwrap().without_pos()
 }
 
 fn lex(s: &str) -> Result<Vec<(usize, Token, usize)>, InternalParseError> {
diff --git a/tests/common.rs b/tests/common.rs
index 6b67d04e..ffdcbbc8 100644
--- a/tests/common.rs
+++ b/tests/common.rs
@@ -5,11 +5,11 @@ use std::io::Cursor;
 
 pub fn eval(s: impl std::string::ToString) -> Result<Term, Error> {
     let mut p = Program::new_from_source(Cursor::new(s.to_string()), "test").unwrap();
-    p.eval()
+    p.eval().map(Term::from)
 }
 
 pub fn eval_file(f: &str) -> Result<Term, Error> {
     let mut p =
         Program::new_from_file(format!("{}/tests/{}", env!("CARGO_MANIFEST_DIR"), f)).unwrap();
-    p.eval()
+    p.eval().map(Term::from)
 }
diff --git a/tests/destructuring.rs b/tests/destructuring.rs
index c839de58..3372fee3 100644
--- a/tests/destructuring.rs
+++ b/tests/destructuring.rs
@@ -1,5 +1,5 @@
 use assert_matches::assert_matches;
-use nickel::error::{Error, EvalError, TypecheckError};
+use nickel::error::{Error, EvalError};
 use nickel::term::Term;
 
 #[path = "./common.rs"]
diff --git a/tests/examples.rs b/tests/examples.rs
index b36a956d..4b5ed2e7 100644
--- a/tests/examples.rs
+++ b/tests/examples.rs
@@ -1,10 +1,10 @@
 use assert_matches::assert_matches;
 use nickel::error::{Error, EvalError};
 use nickel::program::Program;
-use nickel::term::Term;
+use nickel::term::RichTerm;
 use std::path::PathBuf;
 
-fn eval_file(file: &str) -> Result<Term, Error> {
+fn eval_file(file: &str) -> Result<RichTerm, Error> {
     let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
     path.push(format!("examples/{}", file));
     let mut p = Program::new_from_file(path).expect("could not load file as a program");
diff --git a/tests/imports.rs b/tests/imports.rs
index 19aa3ba5..34a2ba70 100644
--- a/tests/imports.rs
+++ b/tests/imports.rs
@@ -21,7 +21,7 @@ fn nested() {
         "should_be = 3",
     )
     .unwrap();
-    assert_eq!(prog.eval(), Ok(Term::Num(3.)));
+    assert_eq!(prog.eval().map(Term::from), Ok(Term::Num(3.)));
 }
 
 #[test]
@@ -31,7 +31,7 @@ fn root_path() {
         "should_be = 44",
     )
     .unwrap();
-    assert_eq!(prog.eval(), Ok(Term::Num(44.)));
+    assert_eq!(prog.eval().map(Term::from), Ok(Term::Num(44.)));
 }
 
 #[test]
@@ -41,7 +41,7 @@ fn multi_imports() {
         "should_be = 5",
     )
     .unwrap();
-    assert_eq!(prog.eval(), Ok(Term::Num(5.)));
+    assert_eq!(prog.eval().map(Term::from), Ok(Term::Num(5.)));
 }
 
 #[test]
@@ -97,7 +97,7 @@ fn serialize() {
         "shoud success",
     )
     .unwrap();
-    assert_eq!(prog.eval(), Ok(Term::Bool(true)));
+    assert_eq!(prog.eval().map(Term::from), Ok(Term::Bool(true)));
 }
 
 #[test]
@@ -107,5 +107,8 @@ fn circular_imports_fail() {
         "should_fail",
     )
     .unwrap();
-    assert_matches!(prog.eval(), Ok(Term::RecRecord(..)) | Ok(Term::Record(..)));
+    assert_matches!(
+        prog.eval().map(Term::from),
+        Ok(Term::RecRecord(..)) | Ok(Term::Record(..))
+    );
 }
diff --git a/tests/pass.rs b/tests/pass.rs
index fd30db70..df1970cd 100644
--- a/tests/pass.rs
+++ b/tests/pass.rs
@@ -12,7 +12,7 @@ fn run(path: impl Into<OsString>) {
     let path = path.into();
     let mut p = Program::new_from_file(path.clone()).expect("could not load file as a program");
     assert_eq!(
-        p.eval(),
+        p.eval().map(Term::from),
         Ok(Term::Bool(true)),
         "error evaluating {}",
         path.to_string_lossy(),