diff --git a/cli/tests/snapshot/inputs/errors/non_serializable_print_path.ncl b/cli/tests/snapshot/inputs/errors/non_serializable_print_path.ncl new file mode 100644 index 000000000..8bc409585 --- /dev/null +++ b/cli/tests/snapshot/inputs/errors/non_serializable_print_path.ncl @@ -0,0 +1,22 @@ +# capture = 'stderr' +# command = ['export'] + +# Check that a non-serializable error prints the path in the notes. This is +# particularly useful in the case tested below, when we haven't fully applied a +# contract, because the error points to the contract definition site (at the +# time of writing), and not the usage site, which is not very useful. +let SomeParametricContract = fun parameter label value => value +in +{ + foo.bar.baz = + [0, 1] + @ [ + 2, + ( + { + inner = { qux_miss_param | SomeParametricContract = {} }, + } + & { inner.qux_without_error = 1 } + ), + ], +} diff --git a/cli/tests/snapshot/snapshots/snapshot__export_stderr_non_serializable_print_path.ncl.snap b/cli/tests/snapshot/snapshots/snapshot__export_stderr_non_serializable_print_path.ncl.snap new file mode 100644 index 000000000..24433abbb --- /dev/null +++ b/cli/tests/snapshot/snapshots/snapshot__export_stderr_non_serializable_print_path.ncl.snap @@ -0,0 +1,14 @@ +--- +source: cli/tests/snapshot/main.rs +expression: err +--- +error: non serializable term + ┌─ [INPUTS_PATH]/errors/non_serializable_print_path.ncl:8:30 + │ +8 │ let SomeParametricContract = fun parameter label value => value + │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + │ + = When exporting field `foo.bar.baz[3].inner.qux_miss_param` + = Nickel only supports serializing to and from strings, booleans, numbers, enum tags, `null` (depending on the format), as well as records and arrays of serializable values. + = Functions and special values (such as contract labels) aren't serializable. + = If you want serialization to ignore a specific value, please use the `not_exported` metadata. diff --git a/core/src/error/mod.rs b/core/src/error/mod.rs index 928f5d976..70afaef9e 100644 --- a/core/src/error/mod.rs +++ b/core/src/error/mod.rs @@ -26,7 +26,7 @@ use crate::{ }, position::{RawSpan, TermPos}, repl, - serialize::ExportFormat, + serialize::{ExportFormat, NickelPointer}, term::{record::FieldMetadata, Number, RichTerm, Term}, typ::{EnumRow, RecordRow, Type, TypeF, VarKindDiscriminant}, }; @@ -571,9 +571,18 @@ pub enum ImportError { ), } -/// An error occurred during serialization. #[derive(Debug, PartialEq, Clone)] -pub enum ExportError { +pub struct ExportError { + /// The path to the field that contains a non-serializable value. This might be empty if the + /// error occurred before entering any record. + pub path: NickelPointer, + /// The cause of the error. + pub data: ExportErrorData, +} + +/// The type of error occurring during serialization. +#[derive(Debug, PartialEq, Clone)] +pub enum ExportErrorData { /// Encountered a null value for a format that doesn't support them. UnsupportedNull(ExportFormat, RichTerm), /// Tried exporting something else than a `String` to raw format. @@ -590,6 +599,15 @@ pub enum ExportError { Other(String), } +impl From for ExportError { + fn from(data: ExportErrorData) -> ExportError { + ExportError { + path: NickelPointer::new(), + data, + } + } +} + /// A general I/O error, occurring when reading a source file or writing an export. #[derive(Debug, PartialEq, Eq, Clone)] pub struct IOError(pub String); @@ -2544,22 +2562,28 @@ impl IntoDiagnostics for ExportError { files: &mut Files, _stdlib_ids: Option<&Vec>, ) -> Vec> { - match self { - ExportError::NotAString(rt) => vec![Diagnostic::error() + let mut notes = if !self.path.0.is_empty() { + vec![format!("When exporting field `{}`", self.path)] + } else { + vec![] + }; + + match self.data { + ExportErrorData::NotAString(rt) => vec![Diagnostic::error() .with_message(format!( "raw export expects a String value, but got {}", rt.as_ref() .type_of() .unwrap_or_else(|| String::from("")) )) - .with_labels(vec![primary_term(&rt, files)])], - ExportError::UnsupportedNull(format, rt) => vec![Diagnostic::error() + .with_labels(vec![primary_term(&rt, files)]) + .with_notes(notes)], + ExportErrorData::UnsupportedNull(format, rt) => vec![Diagnostic::error() .with_message(format!("{format} format doesn't support null values")) - .with_labels(vec![primary_term(&rt, files)])], - ExportError::NonSerializable(rt) => vec![Diagnostic::error() - .with_message("non serializable term") .with_labels(vec![primary_term(&rt, files)]) - .with_notes(vec![ + .with_notes(notes)], + ExportErrorData::NonSerializable(rt) => { + notes.extend([ "Nickel only supports serializing to and from strings, booleans, numbers, \ enum tags, `null` (depending on the format), as well as records and arrays \ of serializable values." @@ -2570,27 +2594,43 @@ impl IntoDiagnostics for ExportError { "If you want serialization to ignore a specific value, please use the \ `not_exported` metadata." .into(), - ])], - ExportError::NoDocumentation(rt) => vec![Diagnostic::error() - .with_message("no documentation found") - .with_labels(vec![primary_term(&rt, files)]) - .with_notes(vec![ - "documentation can only be collected from a record.".to_owned() - ])], - ExportError::NumberOutOfRange { term, value } => vec![Diagnostic::error() - .with_message(format!( - "The number {} is too large (in absolute value) to be serialized.", - value.to_sci() - )) - .with_labels(vec![primary_term(&term, files)]) - .with_notes(vec![format!( + ]); + + vec![Diagnostic::error() + .with_message("non serializable term") + .with_labels(vec![primary_term(&rt, files)]) + .with_notes(notes)] + } + ExportErrorData::NoDocumentation(rt) => { + notes.push("documentation can only be collected from a record.".to_owned()); + + vec![Diagnostic::error() + .with_message("no documentation found") + .with_labels(vec![primary_term(&rt, files)]) + .with_notes(notes)] + } + ExportErrorData::NumberOutOfRange { term, value } => { + notes.push(format!( "Only numbers in the range {:e} to {:e} can be portably serialized", f64::MIN, f64::MAX - )])], - ExportError::Other(msg) => vec![Diagnostic::error() - .with_message("serialization failed") - .with_notes(vec![msg])], + )); + + vec![Diagnostic::error() + .with_message(format!( + "The number {} is too large (in absolute value) to be serialized.", + value.to_sci() + )) + .with_labels(vec![primary_term(&term, files)]) + .with_notes(notes)] + } + ExportErrorData::Other(msg) => { + notes.push(msg); + + vec![Diagnostic::error() + .with_message("serialization failed") + .with_notes(notes)] + } } } } diff --git a/core/src/program.rs b/core/src/program.rs index dd185ff84..40c2aa876 100644 --- a/core/src/program.rs +++ b/core/src/program.rs @@ -622,11 +622,11 @@ impl Program { /// Extract documentation from the program #[cfg(feature = "doc")] pub fn extract_doc(&mut self) -> Result { - use crate::error::ExportError; + use crate::error::ExportErrorData; let term = self.eval_record_spine()?; doc::ExtractedDocumentation::extract_from_term(&term).ok_or(Error::ExportError( - ExportError::NoDocumentation(term.clone()), + ExportErrorData::NoDocumentation(term.clone()).into(), )) } @@ -664,7 +664,7 @@ impl Program { #[cfg(feature = "doc")] mod doc { - use crate::error::{Error, ExportError, IOError}; + use crate::error::{Error, ExportErrorData, IOError}; use crate::term::{RichTerm, Term}; use comrak::arena_tree::NodeEdge; use comrak::nodes::{ @@ -744,7 +744,7 @@ mod doc { pub fn write_json(&self, out: &mut dyn Write) -> Result<(), Error> { serde_json::to_writer(out, self) - .map_err(|e| Error::ExportError(ExportError::Other(e.to_string()))) + .map_err(|e| Error::ExportError(ExportErrorData::Other(e.to_string()).into())) } pub fn write_markdown(&self, out: &mut dyn Write) -> Result<(), Error> { diff --git a/core/src/serialize.rs b/core/src/serialize.rs index 063f739ed..d9f7d4d30 100644 --- a/core/src/serialize.rs +++ b/core/src/serialize.rs @@ -1,7 +1,7 @@ //! Serialization of an evaluated program to various data format. use crate::{ - error::ExportError, - identifier::LocIdent, + error::{ExportError, ExportErrorData}, + identifier::{Ident, LocIdent}, term::{ array::{Array, ArrayAttrs}, record::RecordData, @@ -187,58 +187,170 @@ impl<'de> Deserialize<'de> for RichTerm { } } +/// Element of a path to a specific value within a serialized term. See [NickelPointer]. +#[derive(Debug, PartialEq, Clone)] +pub enum NickelPointerElem { + Field(Ident), + Index(usize), +} + +impl fmt::Display for NickelPointerElem { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + NickelPointerElem::Field(id) => write!(f, "{}", id), + NickelPointerElem::Index(i) => write!(f, "[{}]", i), + } + } +} + +/// A pointer to a specific value within a serialized term. The name is inspired from [JSON +/// pointer](https://datatracker.ietf.org/doc/html/rfc6901), which is an equivalent notion. +/// +/// In a serialized term, there can only be constants (numbers, strings, booleans, null) and two +/// kinds of containers: records and lists. To locate a particular value within a serialized term, +/// we can use a path of field names and indices. +/// +/// # Example +/// +/// In the following full evaluated program: +/// +/// ```nickel +/// { +/// foo = { +/// bar = ["hello", "world"], +/// other = null, +/// } +/// } +/// ``` +/// +/// The path to the string `"world"` is `[Field("foo"), Field("bar"), Index(1)]`. This is +/// represented (e.g. in error messages) as `foo.bar[1]`. +#[derive(Debug, PartialEq, Clone, Default)] +pub struct NickelPointer(pub Vec); + +impl NickelPointer { + pub fn new() -> Self { + Self::default() + } +} + +impl fmt::Display for NickelPointer { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let mut it = self.0.iter(); + let Some(first) = it.next() else { + return Ok(()); + }; + + write!(f, "{first}")?; + + for elem in it { + if let NickelPointerElem::Field(_) = elem { + write!(f, ".{elem}")? + } else { + write!(f, "{elem}")? + } + } + + Ok(()) + } +} + /// Check that a term is serializable. Serializable terms are booleans, numbers, strings, enum, /// arrays of serializable terms or records of serializable terms. pub fn validate(format: ExportFormat, t: &RichTerm) -> Result<(), ExportError> { use Term::*; + // The max and min value that we accept to serialize as a number. Because Nickel uses arbitrary + // precision rationals, we could actually support a wider range of numbers, but we expect that + // implementations consuming the resulting JSON (or similar formats) won't necessary be able to + // handle values that don't fit in a 64 bits float. static NUMBER_MIN: Lazy = Lazy::new(|| Number::try_from(f64::MIN).unwrap()); static NUMBER_MAX: Lazy = Lazy::new(|| Number::try_from(f64::MAX).unwrap()); - if format == ExportFormat::Raw { - if let Term::Str(_) = t.term.as_ref() { - Ok(()) - } else { - Err(ExportError::NotAString(t.clone())) - } - } else { - match t.term.as_ref() { + // Push an NickelPoinerElem to the end of the path of an ExportError + fn with_elem(mut err: ExportError, elem: NickelPointerElem) -> ExportError { + err.path.0.push(elem); + err + } + + // We need to build a field path locating a potential export error. One way would be to pass a + // context storing the current path to recursive calls of `validate`. However, representing + // this context isn't entirely trivial: using an owned `Vec` will incur a lot of copying, even + // in the happy path (no error), because the current path needs to be shared among all sibling + // fields of a record. What we want is persistent linked list (cheap to clone and to extend), + // but Rust doesn't have a built-in type for that. It's not very hard to implement manually, or + // to use an external crate, but it's still more code. + // + // Instead, what we do is to return the current field in case of an error. Then, recursive + // calls to `do_validate` can unwrap the error and add their own field to the path, so that + // when the chain of recursive calls finally returns, we have reconstructed the full path (in + // some sense, we're encoding the list in the OS stack). Not only this doesn't require any + // additional data structure, but we expect that it's performant, as in the happy path branch + // prediction should be able to negate the error code path. + // + // `do_validate` is the method doing the actual validation. The only reason this code is put in + // a separate subfunction is that since we reconstruct the path bottom-up, it needs to be + // reversed before finally returning from validate. + fn do_validate(format: ExportFormat, t: &RichTerm) -> Result<(), ExportError> { + match t.as_ref() { // TOML doesn't support null values Null if format == ExportFormat::Json || format == ExportFormat::Yaml => Ok(()), - Null => Err(ExportError::UnsupportedNull(format, t.clone())), + Null => Err(ExportErrorData::UnsupportedNull(format, t.clone()).into()), Bool(_) | Str(_) | Enum(_) => Ok(()), Num(n) => { if *n >= *NUMBER_MIN && *n <= *NUMBER_MAX { Ok(()) } else { - Err(ExportError::NumberOutOfRange { + Err(ExportErrorData::NumberOutOfRange { term: t.clone(), value: n.clone(), - }) + } + .into()) } } Record(record) => { record.iter_serializable().try_for_each(|binding| { // unwrap(): terms must be fully evaluated before being validated for // serialization. Otherwise, it's an internal error. - let (_, rt) = binding.unwrap_or_else(|err| { + let (id, rt) = binding.unwrap_or_else(|err| { panic!( "encountered field without definition `{}` \ during pre-serialization validation", err.id ) }); - validate(format, rt) + + do_validate(format, rt) + .map_err(|err| with_elem(err, NickelPointerElem::Field(id))) })?; Ok(()) } Array(array, _) => { - array.iter().try_for_each(|t| validate(format, t))?; + array.iter().enumerate().try_for_each(|(index, t)| { + do_validate(format, t) + .map_err(|err| with_elem(err, NickelPointerElem::Index(index))) + })?; Ok(()) } - _ => Err(ExportError::NonSerializable(t.clone())), + _ => Err(ExportErrorData::NonSerializable(t.clone()).into()), } } + + if format == ExportFormat::Raw { + if let Term::Str(_) = t.term.as_ref() { + Ok(()) + } else { + Err(ExportErrorData::NotAString(t.clone()).into()) + } + } else { + let mut result = do_validate(format, t); + + if let Err(ExportError { path, .. }) = &mut result { + path.0.reverse(); + } + + result + } } pub fn to_writer(mut writer: W, format: ExportFormat, rt: &RichTerm) -> Result<(), ExportError> @@ -247,29 +359,30 @@ where { match format { ExportFormat::Json => serde_json::to_writer_pretty(writer, &rt) - .map_err(|err| ExportError::Other(err.to_string())), - ExportFormat::Yaml => { - serde_yaml::to_writer(writer, &rt).map_err(|err| ExportError::Other(err.to_string())) - } + .map_err(|err| ExportErrorData::Other(err.to_string())), + ExportFormat::Yaml => serde_yaml::to_writer(writer, &rt) + .map_err(|err| ExportErrorData::Other(err.to_string())), ExportFormat::Toml => toml::to_string_pretty(rt) - .map_err(|err| ExportError::Other(err.to_string())) + .map_err(|err| ExportErrorData::Other(err.to_string())) .and_then(|s| { writer .write_all(s.as_bytes()) - .map_err(|err| ExportError::Other(err.to_string())) + .map_err(|err| ExportErrorData::Other(err.to_string())) }), ExportFormat::Raw => match rt.as_ref() { Term::Str(s) => writer .write_all(s.as_bytes()) - .map_err(|err| ExportError::Other(err.to_string())), - t => Err(ExportError::Other(format!( + .map_err(|err| ExportErrorData::Other(err.to_string())), + t => Err(ExportErrorData::Other(format!( "raw export requires a `String`, got {}", // unwrap(): terms must be fully evaluated before serialization, // and fully evaluated terms have a definite type. t.type_of().unwrap() ))), }, - } + }?; + + Ok(()) } pub fn to_string(format: ExportFormat, rt: &RichTerm) -> Result { diff --git a/core/tests/integration/main.rs b/core/tests/integration/main.rs index 2a6899364..bebcb71dd 100644 --- a/core/tests/integration/main.rs +++ b/core/tests/integration/main.rs @@ -1,7 +1,9 @@ use std::{io::Cursor, thread}; use nickel_lang_core::{ - error::{Error, EvalError, ExportError, ImportError, ParseError, TypecheckError}, + error::{ + Error, EvalError, ExportError, ExportErrorData, ImportError, ParseError, TypecheckError, + }, term::Term, }; use nickel_lang_utils::{ @@ -252,7 +254,8 @@ impl PartialEq for ErrorExpectation { | (ImportIoError, Error::ImportError(ImportError::IOError(..))) | ( SerializeNumberOutOfRange, - Error::EvalError(EvalError::SerializationError(ExportError::NumberOutOfRange { + Error::EvalError(EvalError::SerializationError(ExportError { + data: ExportErrorData::NumberOutOfRange { .. }, .. })), ) => true,