Repository: tweag/nickel
Pull request #1059 (2023-01-20 17:52:52)

Make export snapshot tests capture stderr
Add standard error capture in the snapshot tests for `nickel export`. This is done by calling `assert_snapshot!` twice, once for `stdout` and once for `stderr`, so that the snapshots are reasonably reviewable in `cargo insta review`.

Combining both into one call would be feasible, but only at the cost of presenting the output as raw string literals during snapshot review, I think. If someone knows of a better way, I'd love to see it!

Depends on #1055 


Patch:
diff --git a/.github/workflows/update-flake-lock.yml b/.github/workflows/update-flake-lock.yml
index 6be77b47..e3da5b7d 100644
--- a/.github/workflows/update-flake-lock.yml
+++ b/.github/workflows/update-flake-lock.yml
@@ -16,7 +16,7 @@ jobs:
           extra_nix_config: |
             access-tokens = github.com=${{ secrets.GITHUB_TOKEN }}
       - name: Update flake.lock
-        uses: DeterminateSystems/update-flake-lock@v16
+        uses: DeterminateSystems/update-flake-lock@v15
         with:
           # Providing a secret token is necessary for the PRs to run CI
           # See https://github.com/DeterminateSystems/update-flake-lock#with-a-personal-authentication-token
diff --git a/Cargo.lock b/Cargo.lock
index 69319d62..842c6f44 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -200,9 +200,9 @@ dependencies = [
 
 [[package]]
 name = "bumpalo"
-version = "3.12.0"
+version = "3.10.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "0d261e256854913907f67ed06efbc3338dfe6179796deefc1ff763fc1aee5535"
+checksum = "37ccbd214614c6783386c1af30caf03192f17891059cecc394b4fb119e363de3"
 
 [[package]]
 name = "byte-tools"
@@ -1705,7 +1705,6 @@ dependencies = [
 name = "pyckel"
 version = "0.3.1"
 dependencies = [
- "codespan-reporting",
  "nickel-lang",
  "pyo3",
 ]
diff --git a/pyckel/Cargo.toml b/pyckel/Cargo.toml
index 7eeb6e0d..9869bfcc 100644
--- a/pyckel/Cargo.toml
+++ b/pyckel/Cargo.toml
@@ -13,7 +13,6 @@ edition = "2018"
 [dependencies]
 nickel-lang = {default-features = false, path = "../", version = "0.3.1" }
 pyo3 = { version = "0.17.3", features = ["extension-module"] }
-codespan-reporting = "0.11"
 
 [lib]
 crate-type = ["cdylib", "rlib"]
diff --git a/pyckel/src/lib.rs b/pyckel/src/lib.rs
index b2d44eee..0fb18093 100644
--- a/pyckel/src/lib.rs
+++ b/pyckel/src/lib.rs
@@ -1,8 +1,8 @@
 use std::io::Cursor;
 
 use nickel_lang::{
-    error::Error,
-    eval::cache::{CBNCache, Cache},
+    error::{Error, SerializationError},
+    eval::cache::CBNCache,
     program::Program,
     serialize,
 };
@@ -11,9 +11,38 @@ use pyo3::{create_exception, exceptions::PyException, prelude::*};
 
 create_exception!(pyckel, NickelException, PyException);
 
-/// Turn an internal Nickel error into a PyErr with a fancy diagnostic message
-fn error_to_exception<E: Into<Error>, EC: Cache>(error: E, program: &mut Program<EC>) -> PyErr {
-    NickelException::new_err(program.report_as_str(error.into()))
+// see https://pyo3.rs/v0.17.3/function/error_handling.html#foreign-rust-error-types
+struct NickelError(Error);
+struct NickelSerializationError(SerializationError);
+
+impl From<Error> for NickelError {
+    fn from(value: Error) -> Self {
+        Self(value)
+    }
+}
+
+impl std::convert::From<NickelError> for PyErr {
+    fn from(err: NickelError) -> PyErr {
+        match err {
+            // TODO better exceptions
+            NickelError(error) => NickelException::new_err(format!("{error:?}")),
+        }
+    }
+}
+
+impl From<SerializationError> for NickelSerializationError {
+    fn from(value: SerializationError) -> Self {
+        Self(value)
+    }
+}
+
+impl std::convert::From<NickelSerializationError> for PyErr {
+    fn from(err: NickelSerializationError) -> PyErr {
+        match err {
+            // TODO better exceptions
+            NickelSerializationError(error) => NickelException::new_err(format!("{error:?}")),
+        }
+    }
 }
 
 /// Evaluate from a Python str of a Nickel expression to a Python str of the resulting JSON.
@@ -22,15 +51,12 @@ pub fn run(s: String) -> PyResult<String> {
     let mut program: Program<CBNCache> =
         Program::new_from_source(Cursor::new(s.to_string()), "python")?;
 
-    let term = program
-        .eval_full()
-        .map_err(|error| error_to_exception(error, &mut program))?;
+    let term = program.eval_full().map_err(NickelError)?;
 
-    serialize::validate(serialize::ExportFormat::Json, &term)
-        .map_err(|error| error_to_exception(error, &mut program))?;
+    serialize::validate(serialize::ExportFormat::Json, &term).map_err(NickelSerializationError)?;
 
     let json_string = serialize::to_string(serialize::ExportFormat::Json, &term)
-        .map_err(|error| error_to_exception(error, &mut program))?;
+        .map_err(NickelSerializationError)?;
 
     Ok(json_string)
 }
diff --git a/src/parser/uniterm.rs b/src/parser/uniterm.rs
index 43e693dc..96b61578 100644
--- a/src/parser/uniterm.rs
+++ b/src/parser/uniterm.rs
@@ -167,40 +167,6 @@ impl UniRecord {
     /// doesn't support the field path syntax: `{foo.bar.baz : Type}.into_type_strict()` returns an
     /// `Err`.
     pub fn into_type_strict(self) -> Result<Types, InvalidRecordTypeError> {
-        fn term_to_record_rows(
-            id: Ident,
-            rt: RichTerm,
-            tail: RecordRows,
-        ) -> Result<RecordRows, InvalidRecordTypeError> {
-            // At parsing stage, all `Rc`s must be 1-counted. We can thus call
-            // `into_owned()` without risking to actually clone anything.
-            match rt.term.into_owned() {
-                Term::MetaValue(MetaValue {
-                    doc: None,
-                    types: Some(ctrt),
-                    contracts,
-                    opt: false,
-                    priority: MergePriority::Neutral,
-                    value: None,
-                }) if contracts.is_empty() => Ok(RecordRows(RecordRowsF::Extend {
-                    row: RecordRow {
-                        id,
-                        types: Box::new(ctrt.types),
-                    },
-                    tail: Box::new(tail),
-                })),
-                _ => {
-                    // Position of identifiers must always be set at this stage
-                    // (parsing)
-                    let span_id = id.pos.unwrap();
-                    let term_pos = rt.pos.into_opt().unwrap_or(span_id);
-                    Err(InvalidRecordTypeError(TermPos::Original(
-                        RawSpan::fuse(span_id, term_pos).unwrap(),
-                    )))
-                }
-            }
-        }
-
         // An open record (with an ellipsis `..` at the end) can't be translated to a record type.
         // `pos_ellipsis` should be set iff `attrs.open` is true.
         debug_assert!((self.pos_ellipsis == TermPos::None) != self.attrs.open);
@@ -241,14 +207,38 @@ impl UniRecord {
                     } else {
                         let elem = path.pop().unwrap();
                         match elem {
-                            FieldPathElem::Ident(id) => term_to_record_rows(id, rt, acc),
-                            FieldPathElem::Expr(expr) => {
-                                let Some(id) = expr.term.as_ref().try_str_chunk_as_static_str() else {
-                                        return Err(InvalidRecordTypeError(rt.pos))
-                                };
-                                let id = Ident::new_with_pos(id, expr.pos);
-                                term_to_record_rows(id, rt, acc)
+                            FieldPathElem::Ident(id) => {
+                                // At parsing stage, all `Rc`s must be 1-counted. We can thus call
+                                // `into_owned()` without risking to actually clone anything.
+                                match rt.term.into_owned() {
+                                    Term::MetaValue(MetaValue {
+                                        doc: None,
+                                        types: Some(ctrt),
+                                        contracts,
+                                        opt: false,
+                                        priority: MergePriority::Neutral,
+                                        value: None,
+                                    }) if contracts.is_empty() => {
+                                        Ok(RecordRows(RecordRowsF::Extend {
+                                            row: RecordRow {
+                                                id,
+                                                types: Box::new(ctrt.types),
+                                            },
+                                            tail: Box::new(acc),
+                                        }))
+                                    }
+                                    _ => {
+                                        // Position of identifiers must always be set at this stage
+                                        // (parsing)
+                                        let span_id = id.pos.unwrap();
+                                        let term_pos = rt.pos.into_opt().unwrap_or(span_id);
+                                        Err(InvalidRecordTypeError(TermPos::Original(
+                                            RawSpan::fuse(span_id, term_pos).unwrap(),
+                                        )))
+                                    }
+                                }
                             }
+                            FieldPathElem::Expr(rt) => Err(InvalidRecordTypeError(rt.pos)),
                         }
                     }
                 },
diff --git a/src/parser/utils.rs b/src/parser/utils.rs
index 5cde50db..337d0c83 100644
--- a/src/parser/utils.rs
+++ b/src/parser/utils.rs
@@ -299,7 +299,20 @@ pub fn elaborate_field_path(
                 RichTerm::new(Term::Record(RecordData::with_fields(fields)), pos)
             }
             FieldPathElem::Expr(exp) => {
-                let static_access = exp.term.as_ref().try_str_chunk_as_static_str();
+                let static_access = match exp.term.as_ref() {
+                    Term::StrChunks(chunks) => {
+                        chunks
+                            .iter()
+                            .fold(Some(String::new()), |acc, next| match (acc, next) {
+                                (Some(mut acc), StrChunk::Literal(lit)) => {
+                                    acc.push_str(lit);
+                                    Some(acc)
+                                }
+                                _ => None,
+                            })
+                    }
+                    _ => None,
+                };
                 if let Some(static_access) = static_access {
                     let id = Ident::new_with_pos(static_access, exp.pos);
                     let mut fields = HashMap::new();
diff --git a/src/program.rs b/src/program.rs
index 726cc679..64775129 100644
--- a/src/program.rs
+++ b/src/program.rs
@@ -29,9 +29,9 @@ use crate::parser::lexer::Lexer;
 use crate::term::{RichTerm, Term};
 use crate::{eval, parser};
 use codespan::FileId;
-use codespan_reporting::term::termcolor::{Ansi, ColorChoice, StandardStream};
+use codespan_reporting::term::termcolor::{ColorChoice, StandardStream};
 use std::ffi::OsString;
-use std::io::{self, Cursor, Read};
+use std::io::{self, Read};
 use std::result::Result;
 
 #[derive(Copy, Clone, Debug, PartialEq, Eq)]
@@ -163,28 +163,6 @@ impl<EC: EvalCache> Program<EC> {
         report(self.vm.import_resolver_mut(), error, self.color_opt)
     }
 
-    /// Build an error report as a string and return it.
-    pub fn report_as_str<E>(&mut self, error: E) -> String
-    where
-        E: ToDiagnostic<FileId>,
-    {
-        let cache = self.vm.import_resolver_mut();
-        let contracts_id = cache.id_of("<stdlib/contract.ncl>");
-        let diagnostics = error.to_diagnostic(cache.files_mut(), contracts_id);
-        let mut buffer = Ansi::new(Cursor::new(Vec::new()));
-        let config = codespan_reporting::term::Config::default();
-        // write to `buffer`
-        diagnostics
-            .iter()
-            .try_for_each(|d| {
-                codespan_reporting::term::emit(&mut buffer, &config, cache.files_mut(), d)
-            })
-            // safe because writing to a cursor in memory
-            .unwrap();
-        // unwrap(): emit() should only print valid utf8 to the the buffer
-        String::from_utf8(buffer.into_inner().into_inner()).unwrap()
-    }
-
     /// Create a markdown file with documentation for the specified program in `.nickel/doc/program_main_file_name.md`
     #[cfg(feature = "doc")]
     pub fn output_doc(&mut self, out: &mut dyn std::io::Write) -> Result<(), Error> {
diff --git a/src/term/mod.rs b/src/term/mod.rs
index 599e2b45..1cc11be4 100644
--- a/src/term/mod.rs
+++ b/src/term/mod.rs
@@ -792,25 +792,6 @@ impl Term {
             | Term::ParseError(_) => false,
         }
     }
-
-    /// Extract the static literal from string chunk. It only returns a `Some(..)`
-    /// when the term is a `Term::StrChunk` and all the chunks are `StrChunk::Literal(..)`
-    pub fn try_str_chunk_as_static_str(&self) -> Option<String> {
-        match self {
-            Term::StrChunks(chunks) => {
-                chunks
-                    .iter()
-                    .fold(Some(String::new()), |acc, next| match (acc, next) {
-                        (Some(mut acc), StrChunk::Literal(lit)) => {
-                            acc.push_str(lit);
-                            Some(acc)
-                        }
-                        _ => None,
-                    })
-            }
-            _ => None,
-        }
-    }
 }
 
 #[derive(Debug, Clone, PartialEq)]


Test patch:
diff --git a/tests/integration/pass/typechecking.ncl b/tests/integration/pass/typechecking.ncl
index 4fe5213c..5c040ba4 100644
--- a/tests/integration/pass/typechecking.ncl
+++ b/tests/integration/pass/typechecking.ncl
@@ -91,9 +91,6 @@ let typecheck = [
   ({bla = true, blo = 1}.blo : Num),
   let r : {bla : Bool, blo : Num} = {blo = 1, bla = true} in
     ((if r.bla then r.blo else 2) : Num),
-  # Regression tests for https://github.com/tweag/nickel/issues/888
-  {"fo京o" = "bar"} : {"fo京o" : Str},
-  {foo = 1} : { "foo" : Num},
 
   let f : forall a r. {bla : Bool, blo : a, ble : a ; r} -> a =
       fun r => if r.bla then r.blo else r.ble in
diff --git a/tests/snapshot/inputs/errors/trace_not_saturated.ncl b/tests/snapshot/inputs/errors/trace_not_saturated.ncl
new file mode 100644
index 00000000..948f9250
--- /dev/null
+++ b/tests/snapshot/inputs/errors/trace_not_saturated.ncl
@@ -0,0 +1,1 @@
+%trace% "too few arguments"
diff --git a/tests/snapshot/main.rs b/tests/snapshot/main.rs
index b56608aa..018b4df8 100644
--- a/tests/snapshot/main.rs
+++ b/tests/snapshot/main.rs
@@ -18,7 +18,7 @@ fn check_pretty_print_snapshots(file: &str) {
 }
 
 #[test_resources("tests/snapshot/inputs/export/*.ncl")]
-fn check_export_snapshots(file: &str) {
+fn check_export_stdout_snapshots(file: &str) {
     let file = TestFile::from_project_path(file);
 
     let snapshot = NickelInvocation::new()
@@ -26,7 +26,19 @@ fn check_export_snapshots(file: &str) {
         .file(&file)
         .snapshot_stdout();
 
-    insta::assert_snapshot!(file.prefixed_test_name("export"), snapshot)
+    insta::assert_snapshot!(file.prefixed_test_name("export_stdout"), snapshot);
+}
+
+#[test_resources("tests/snapshot/inputs/export/*.ncl")]
+fn check_export_stderr_snapshots(file: &str) {
+    let file = TestFile::from_project_path(file);
+
+    let snapshot = NickelInvocation::new()
+        .subcommand("export")
+        .file(&file)
+        .snapshot_stderr();
+
+    insta::assert_snapshot!(file.prefixed_test_name("export_stderr"), snapshot);
 }
 
 #[test_resources("tests/snapshot/inputs/errors/*.ncl")]
diff --git a/tests/snapshot/snapshots/snapshot__error_trace_not_saturated.ncl.snap b/tests/snapshot/snapshots/snapshot__error_trace_not_saturated.ncl.snap
new file mode 100644
index 00000000..234043e7
--- /dev/null
+++ b/tests/snapshot/snapshots/snapshot__error_trace_not_saturated.ncl.snap
@@ -0,0 +1,12 @@
+---
+source: tests/snapshot/main.rs
+expression: snapshot
+---
+builtin.trace: too few arguments
+error: not enough arguments
+  ┌─ [INPUTS_PATH]/errors/trace_not_saturated.ncl:1:1
+  │
+1 │ %trace% "too few arguments"
+  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^ trace expects 2 arguments, but not enough were provided
+
+
diff --git a/tests/snapshot/snapshots/snapshot__export_stderr_nested_record.ncl.snap b/tests/snapshot/snapshots/snapshot__export_stderr_nested_record.ncl.snap
new file mode 100644
index 00000000..776aa3f8
--- /dev/null
+++ b/tests/snapshot/snapshots/snapshot__export_stderr_nested_record.ncl.snap
@@ -0,0 +1,5 @@
+---
+source: tests/snapshot/main.rs
+expression: snapshot.stderr
+---
+
diff --git a/tests/snapshot/snapshots/snapshot__export_stderr_trace.ncl.snap b/tests/snapshot/snapshots/snapshot__export_stderr_trace.ncl.snap
new file mode 100644
index 00000000..99814a27
--- /dev/null
+++ b/tests/snapshot/snapshots/snapshot__export_stderr_trace.ncl.snap
@@ -0,0 +1,6 @@
+---
+source: tests/snapshot/main.rs
+expression: snapshot.stderr
+---
+builtin.trace: Hello, world!
+
diff --git a/tests/snapshot/snapshots/snapshot__export_nested_record.ncl.snap b/tests/snapshot/snapshots/snapshot__export_stdout_nested_record.ncl.snap
similarity index 92%
rename from tests/snapshot/snapshots/snapshot__export_nested_record.ncl.snap
rename to tests/snapshot/snapshots/snapshot__export_stdout_nested_record.ncl.snap
index 9d686f59..53605e44 100644
--- a/tests/snapshot/snapshots/snapshot__export_nested_record.ncl.snap
+++ b/tests/snapshot/snapshots/snapshot__export_stdout_nested_record.ncl.snap
@@ -1,6 +1,6 @@
 ---
 source: tests/snapshot/main.rs
-expression: snapshot
+expression: snapshot.stdout
 ---
 {
   "a_num": 1,
diff --git a/tests/snapshot/snapshots/snapshot__export_trace.ncl.snap b/tests/snapshot/snapshots/snapshot__export_stdout_trace.ncl.snap
similarity index 61%
rename from tests/snapshot/snapshots/snapshot__export_trace.ncl.snap
rename to tests/snapshot/snapshots/snapshot__export_stdout_trace.ncl.snap
index 109fa87b..a40e60e5 100644
--- a/tests/snapshot/snapshots/snapshot__export_trace.ncl.snap
+++ b/tests/snapshot/snapshots/snapshot__export_stdout_trace.ncl.snap
@@ -1,5 +1,5 @@
 ---
 source: tests/snapshot/main.rs
-expression: snapshot
+expression: snapshot.stdout
 ---
 true