From 60d04f6c886741d8a0d538fc51b023d74b5f2ec1 Mon Sep 17 00:00:00 2001 From: Yann Hamdaoui Date: Fri, 3 May 2024 13:55:15 +0200 Subject: [PATCH] Include field path in non serializable error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When failing to serialize a value, the error sometimes doesn't point to a usable location (that is, it points to the definition site of some function somewhere, but it's impossible to know where this function was actually used, which is usually where the issue is). This commit enriches the error message of non-serializable errors with a path, à la JSON pointer (such that it also integrates array indices), whenever possible. This should make debugging non serializable error much nicer. --- .../errors/non_serializable_print_path.ncl | 22 +++ ...tderr_non_serializable_print_path.ncl.snap | 14 ++ core/src/error/mod.rs | 98 ++++++++--- core/src/program.rs | 8 +- core/src/serialize.rs | 165 +++++++++++++++--- core/tests/integration/main.rs | 7 +- 6 files changed, 253 insertions(+), 61 deletions(-) create mode 100644 cli/tests/snapshot/inputs/errors/non_serializable_print_path.ncl create mode 100644 cli/tests/snapshot/snapshots/snapshot__export_stderr_non_serializable_print_path.ncl.snap 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 0000000000..8bc4095852 --- /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 0000000000..24433abbb4 --- /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 928f5d976b..70afaef9ed 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 dd185ff846..40c2aa8768 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 063f739ed2..d9f7d4d308 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 2a68993648..bebcb71dd7 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,