From d35baaee1c90bcd82e7881c3e55fd9f94f265705 Mon Sep 17 00:00:00 2001 From: Yann Hamdaoui Date: Tue, 19 Dec 2023 15:36:57 +0000 Subject: [PATCH] Add --error-format flag to serialize err diagnostics (#1740) * Add --error-format flag to serialize err diagnostics * Make other CLI errors follow --error-format as well --- Cargo.lock | 2 + Cargo.toml | 4 +- cli/src/cli.rs | 6 ++ cli/src/error.rs | 40 ++++++++-- cli/src/main.rs | 4 +- cli/src/query.rs | 2 +- core/src/error/mod.rs | 81 +++----------------- core/src/error/report.rs | 111 ++++++++++++++++++++++++++++ core/src/program.rs | 9 ++- core/src/repl/mod.rs | 9 ++- core/src/repl/rustyline_frontend.rs | 20 ++--- core/src/serialize.rs | 9 ++- core/src/term/mod.rs | 1 - core/tests/manual/main.rs | 4 +- utils/src/bench.rs | 6 +- 15 files changed, 202 insertions(+), 106 deletions(-) create mode 100644 core/src/error/report.rs diff --git a/Cargo.lock b/Cargo.lock index 3df46661..30f100a0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -405,6 +405,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3362992a0d9f1dd7c3d0e89e0ab2bb540b7a95fea8cd798090e758fda2899b5e" dependencies = [ "codespan-reporting", + "serde", ] [[package]] @@ -424,6 +425,7 @@ version = "0.11.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3538270d33cc669650c4b093848450d380def10c331d38c768e34cac80576e6e" dependencies = [ + "serde", "termcolor", "unicode-width", ] diff --git a/Cargo.toml b/Cargo.toml index 90179569..499d4abd 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -39,9 +39,9 @@ assert_cmd = "2.0.11" assert_matches = "1.5.0" clap = "4.3" clap_complete = "4.3.2" -codespan = "0.11" +codespan = { version = "0.11", features = ["serialization"] } codespan-lsp = "0.11" -codespan-reporting = "0.11" +codespan-reporting = { version = "0.11", features = ["serialization"] } comrak = "0.17.0" criterion = "0.4" csv = "1" diff --git a/cli/src/cli.rs b/cli/src/cli.rs index 98c05644..cb1bd7c9 100644 --- a/cli/src/cli.rs +++ b/cli/src/cli.rs @@ -7,6 +7,8 @@ use crate::{ pprint_ast::PprintAstCommand, query::QueryCommand, typecheck::TypecheckCommand, }; +use nickel_lang_core::error::report::ErrorFormat; + #[cfg(feature = "repl")] use crate::repl::ReplCommand; @@ -47,6 +49,10 @@ pub struct GlobalOptions { #[arg(long, global = true, value_enum, default_value_t)] pub color: clap::ColorChoice, + /// Output error messages in a specific format. + #[arg(long, global = true, value_enum, default_value_t)] + pub error_format: ErrorFormat, + #[cfg(feature = "metrics")] /// Print all recorded metrics at the very end of the program #[arg(long, global = true, default_value_t = false)] diff --git a/cli/src/error.rs b/cli/src/error.rs index 29d3769a..ea0fea03 100644 --- a/cli/src/error.rs +++ b/cli/src/error.rs @@ -1,7 +1,10 @@ //! Error handling for the CLI. use nickel_lang_core::{ - error::{Diagnostic, FileId, Files, IntoDiagnostics, ParseError}, + error::{ + report::{ColorOpt, ErrorFormat}, + Diagnostic, FileId, Files, IntoDiagnostics, ParseError, + }, eval::cache::lazy::CBNCache, program::{FieldOverride, FieldPath, Program}, }; @@ -215,25 +218,46 @@ impl ResultErrorExt for Result { } impl Error { - pub fn report(self) { + /// Report this error on the standard error stream. + pub fn report(self, format: ErrorFormat, color: ColorOpt) { + // Report a standalone error which doesn't actually refer to any source code. + let report_standalone = |main_label: &str, msg: Option| { + use nickel_lang_core::{ + cache::{Cache, ErrorTolerance}, + error::report::report as core_report, + }; + + let mut dummy_cache = Cache::new(ErrorTolerance::Tolerant); + let diagnostic = Diagnostic::error() + .with_message(main_label) + .with_notes(msg.into_iter().collect()); + + core_report(&mut dummy_cache, diagnostic, format, color); + }; + + // We try to fit every error in a diagnostic. This makes sure all errors are rendered using + // the same format set (potentitally by default) by the `--error-format` flag. This also + // makes error styling more consistent. match self { - Error::Program { mut program, error } => program.report(error), + Error::Program { mut program, error } => program.report(error, format), Error::Io { error } => { - eprintln!("{error}") + report_standalone("IO error", Some(error.to_string())); } #[cfg(feature = "repl")] Error::Repl { error } => { use nickel_lang_core::repl::InitError; match error { - InitError::Stdlib => eprintln!("Failed to load the Nickel standard library"), + InitError::Stdlib => { + report_standalone("failed to initialize the standard library", None) + } InitError::ReadlineError(msg) => { - eprintln!("Readline intialization failed: {msg}") + report_standalone("failed to initialize the terminal interface", Some(msg)) } } } #[cfg(feature = "format")] - Error::Format { error } => eprintln!("{error}"), - Error::CliUsage { error, mut program } => program.report(error), + Error::Format { error } => report_standalone("format error", Some(error.to_string())), + Error::CliUsage { error, mut program } => program.report(error, format), Error::CustomizeInfoPrinted => { // Nothing to do, the caller should simply exit. } diff --git a/cli/src/main.rs b/cli/src/main.rs index ff668c1b..18df7356 100644 --- a/cli/src/main.rs +++ b/cli/src/main.rs @@ -30,6 +30,8 @@ fn main() -> ExitCode { let opts = ::parse(); + let error_format = opts.global.error_format; + let color = opts.global.color; #[cfg(feature = "metrics")] let report_metrics = opts.global.metrics; @@ -61,7 +63,7 @@ fn main() -> ExitCode { // user's point of view. Ok(()) | Err(error::Error::CustomizeInfoPrinted) => ExitCode::SUCCESS, Err(error) => { - error.report(); + error.report(error_format, color.into()); ExitCode::FAILURE } } diff --git a/cli/src/query.rs b/cli/src/query.rs index 21ee16f4..f25b03ac 100644 --- a/cli/src/query.rs +++ b/cli/src/query.rs @@ -52,7 +52,7 @@ impl QueryCommand { let mut program = self.inputs.prepare(&global)?; if self.inputs.customize_mode.field().is_none() { - program.report(Warning::EmptyQueryPath) + program.report(Warning::EmptyQueryPath, global.error_format); } let found = program diff --git a/core/src/error/mod.rs b/core/src/error/mod.rs index bfe72d2b..ab7bcd1e 100644 --- a/core/src/error/mod.rs +++ b/core/src/error/mod.rs @@ -32,6 +32,7 @@ use crate::{ typ::{Type, TypeF, VarKindDiscriminant}, }; +pub mod report; pub mod suggest; /// A general error occurring during either parsing or evaluation. @@ -820,6 +821,17 @@ pub trait IntoDiagnostics { ) -> Vec>; } +// Allow the use of a single `Diagnostic` directly as an error that can be reported by Nickel. +impl IntoDiagnostics for Diagnostic { + fn into_diagnostics( + self, + _files: &mut Files, + _stdlib_ids: Option<&Vec>, + ) -> Vec> { + vec![self] + } +} + // Helpers for the creation of codespan `Label`s /// Create a primary label from a span. @@ -2282,72 +2294,3 @@ impl IntoDiagnostics for ReplError { } } } - -#[derive(Copy, Clone, Debug, PartialEq, Eq)] -pub struct ColorOpt(pub(crate) clap::ColorChoice); - -impl From for ColorOpt { - fn from(color_choice: clap::ColorChoice) -> Self { - Self(color_choice) - } -} - -impl From for ColorChoice { - fn from(c: ColorOpt) -> Self { - use std::io::{stdout, IsTerminal}; - - match c.0 { - clap::ColorChoice::Auto => { - if stdout().is_terminal() { - ColorChoice::Auto - } else { - ColorChoice::Never - } - } - clap::ColorChoice::Always => ColorChoice::Always, - clap::ColorChoice::Never => ColorChoice::Never, - } - } -} - -impl Default for ColorOpt { - fn default() -> Self { - Self(clap::ColorChoice::Auto) - } -} - -/// Pretty-print an error on stderr. -/// -/// # Arguments -/// -/// - `cache` is the file cache used during the evaluation, which is required by the reporting -/// infrastructure to point at specific locations and print snippets when needed. -pub fn report>(cache: &mut Cache, error: E, color_opt: ColorOpt) { - let stdlib_ids = cache.get_all_stdlib_modules_file_id(); - report_with( - &mut StandardStream::stderr(color_opt.into()).lock(), - cache.files_mut(), - stdlib_ids.as_ref(), - error, - ) -} - -/// Report an error on `stderr`, provided a file database and a list of stdlib file ids. -pub fn report_with>( - writer: &mut dyn WriteColor, - files: &mut Files, - stdlib_ids: Option<&Vec>, - error: E, -) { - let config = codespan_reporting::term::Config::default(); - let diagnostics = error.into_diagnostics(files, stdlib_ids); - - let result = diagnostics - .iter() - .try_for_each(|d| codespan_reporting::term::emit(writer, &config, files, d)); - - match result { - Ok(()) => (), - Err(err) => panic!("error::report_with(): could not print an error on stderr: {err}"), - }; -} diff --git a/core/src/error/report.rs b/core/src/error/report.rs new file mode 100644 index 00000000..28a9e7b0 --- /dev/null +++ b/core/src/error/report.rs @@ -0,0 +1,111 @@ +//! Error diagnostics reporting and serialization. +use super::*; + +/// Serializable wrapper type to export diagnostics with a top-level attribute. +#[derive(serde::Serialize)] +pub struct DiagnosticsWrapper { + pub diagnostics: Vec>, +} + +impl From>> for DiagnosticsWrapper { + fn from(diagnostics: Vec>) -> Self { + Self { diagnostics } + } +} + +/// Available export formats for error diagnostics. +#[derive(Copy, Clone, Eq, PartialEq, Debug, Default, clap::ValueEnum)] +pub enum ErrorFormat { + #[default] + Text, + Json, + Yaml, + Toml, +} + +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub struct ColorOpt(pub(crate) clap::ColorChoice); + +impl From for ColorOpt { + fn from(color_choice: clap::ColorChoice) -> Self { + Self(color_choice) + } +} + +impl From for ColorChoice { + fn from(c: ColorOpt) -> Self { + use std::io::{stdout, IsTerminal}; + + match c.0 { + clap::ColorChoice::Auto => { + if stdout().is_terminal() { + ColorChoice::Auto + } else { + ColorChoice::Never + } + } + clap::ColorChoice::Always => ColorChoice::Always, + clap::ColorChoice::Never => ColorChoice::Never, + } + } +} + +impl Default for ColorOpt { + fn default() -> Self { + Self(clap::ColorChoice::Auto) + } +} + +/// Pretty-print an error on stderr. +/// +/// # Arguments +/// +/// - `cache` is the file cache used during the evaluation, which is required by the reporting +/// infrastructure to point at specific locations and print snippets when needed. +pub fn report>( + cache: &mut Cache, + error: E, + format: ErrorFormat, + color_opt: ColorOpt, +) { + let stdlib_ids = cache.get_all_stdlib_modules_file_id(); + report_with( + &mut StandardStream::stderr(color_opt.into()).lock(), + cache.files_mut(), + stdlib_ids.as_ref(), + error, + format, + ) +} + +/// Report an error on `stderr`, provided a file database and a list of stdlib file ids. +pub fn report_with>( + writer: &mut dyn WriteColor, + files: &mut Files, + stdlib_ids: Option<&Vec>, + error: E, + format: ErrorFormat, +) { + let config = codespan_reporting::term::Config::default(); + let diagnostics = error.into_diagnostics(files, stdlib_ids); + let stderr = std::io::stderr(); + + let result = match format { + ErrorFormat::Text => diagnostics.iter().try_for_each(|d| { + codespan_reporting::term::emit(writer, &config, files, d).map_err(|err| err.to_string()) + }), + ErrorFormat::Json => serde_json::to_writer(stderr, &DiagnosticsWrapper::from(diagnostics)) + .map(|_| eprintln!()) + .map_err(|err| err.to_string()), + ErrorFormat::Yaml => serde_yaml::to_writer(stderr, &DiagnosticsWrapper::from(diagnostics)) + .map_err(|err| err.to_string()), + ErrorFormat::Toml => toml::to_string(&DiagnosticsWrapper::from(diagnostics)) + .map(|repr| eprint!("{}", repr)) + .map_err(|err| err.to_string()), + }; + + match result { + Ok(()) => (), + Err(err) => panic!("error::report_with(): could not print an error on stderr: {err}"), + }; +} diff --git a/core/src/program.rs b/core/src/program.rs index 30116a56..ce238755 100644 --- a/core/src/program.rs +++ b/core/src/program.rs @@ -22,7 +22,10 @@ //! Each such value is added to the initial environment before the evaluation of the program. use crate::{ cache::*, - error::{report, ColorOpt, Error, EvalError, IOError, IntoDiagnostics, ParseError}, + error::{ + report::{report, ColorOpt, ErrorFormat}, + Error, EvalError, IOError, IntoDiagnostics, ParseError, + }, eval::{cache::Cache as EvalCache, Closure, VirtualMachine}, identifier::LocIdent, label::Label, @@ -454,11 +457,11 @@ impl Program { } /// Wrapper for [`report`]. - pub fn report(&mut self, error: E) + pub fn report(&mut self, error: E, format: ErrorFormat) where E: IntoDiagnostics, { - report(self.vm.import_resolver_mut(), error, self.color_opt) + report(self.vm.import_resolver_mut(), error, format, self.color_opt) } /// Build an error report as a string and return it. diff --git a/core/src/repl/mod.rs b/core/src/repl/mod.rs index 06686714..a06bc6aa 100644 --- a/core/src/repl/mod.rs +++ b/core/src/repl/mod.rs @@ -7,7 +7,10 @@ //! jupyter-kernel (which is not exactly user-facing, but still manages input/output and //! formatting), etc. use crate::cache::{Cache, Envs, ErrorTolerance, SourcePath}; -use crate::error::{Error, EvalError, IOError, ParseError, ParseErrors, ReplError}; +use crate::error::{ + report::{self, ColorOpt, ErrorFormat}, + Error, EvalError, IOError, IntoDiagnostics, ParseError, ParseErrors, ReplError, +}; use crate::eval::cache::Cache as EvalCache; use crate::eval::{Closure, VirtualMachine}; use crate::identifier::LocIdent; @@ -208,6 +211,10 @@ impl ReplImpl { } } } + + fn report(&mut self, err: impl IntoDiagnostics, color_opt: ColorOpt) { + report::report(self.cache_mut(), err, ErrorFormat::Text, color_opt); + } } impl Repl for ReplImpl { diff --git a/core/src/repl/rustyline_frontend.rs b/core/src/repl/rustyline_frontend.rs index 0d0533ea..4260ad56 100644 --- a/core/src/repl/rustyline_frontend.rs +++ b/core/src/repl/rustyline_frontend.rs @@ -3,10 +3,7 @@ use std::path::PathBuf; use super::{command::Command, *}; -use crate::{ - error::{self, ColorOpt}, - eval::cache::CacheImpl, -}; +use crate::{error::report::ColorOpt, eval::cache::CacheImpl}; use ansi_term::Style; use rustyline::{error::ReadlineError, Config, EditMode, Editor}; @@ -39,7 +36,7 @@ pub fn repl(histfile: PathBuf, color_opt: ColorOpt) -> Result<(), InitError> { match repl.load_stdlib() { Ok(()) => (), Err(err) => { - error::report(repl.cache_mut(), err, color_opt); + repl.report(err, color_opt); return Err(InitError::Stdlib); } } @@ -101,7 +98,7 @@ pub fn repl(histfile: PathBuf, color_opt: ColorOpt) -> Result<(), InitError> { match repl.eval_full(&exp) { Ok(EvalResult::Evaluated(rt)) => println!("{rt}"), Ok(EvalResult::Bound(_)) => (), - Err(err) => error::report(repl.cache_mut(), err, color_opt), + Err(err) => repl.report(err, color_opt), }; Ok(()) } @@ -117,7 +114,7 @@ pub fn repl(histfile: PathBuf, color_opt: ColorOpt) -> Result<(), InitError> { }; if let Err(err) = result { - error::report(repl.cache_mut(), err, color_opt); + repl.report(err, color_opt); } else { println!(); } @@ -126,7 +123,7 @@ pub fn repl(histfile: PathBuf, color_opt: ColorOpt) -> Result<(), InitError> { match repl.eval_full(&line) { Ok(EvalResult::Evaluated(rt)) => println!("{rt}\n"), Ok(EvalResult::Bound(_)) => (), - Err(err) => error::report(repl.cache_mut(), err, color_opt), + Err(err) => repl.report(err, color_opt), }; } Err(ReadlineError::Eof) => { @@ -136,14 +133,11 @@ pub fn repl(histfile: PathBuf, color_opt: ColorOpt) -> Result<(), InitError> { Err(ReadlineError::Interrupted) => (), Err(err) => { let _ = editor.save_history(&histfile); - error::report( - repl.cache_mut(), - Error::IOError(IOError(format!("{err}"))), - color_opt, - ); + repl.report(Error::IOError(IOError(format!("{err}"))), color_opt); } } }; + let _ = editor.save_history(&histfile); result } diff --git a/core/src/serialize.rs b/core/src/serialize.rs index fd192f36..063f739e 100644 --- a/core/src/serialize.rs +++ b/core/src/serialize.rs @@ -1,7 +1,4 @@ //! Serialization of an evaluated program to various data format. -use malachite::{num::conversion::traits::RoundingFrom, rounding_modes::RoundingMode}; -use once_cell::sync::Lazy; - use crate::{ error::ExportError, identifier::LocIdent, @@ -17,7 +14,11 @@ use serde::{ ser::{Serialize, SerializeMap, SerializeSeq, Serializer}, }; -use malachite::num::conversion::traits::IsInteger; +use malachite::{ + num::conversion::traits::{IsInteger, RoundingFrom}, + rounding_modes::RoundingMode, +}; +use once_cell::sync::Lazy; use std::{fmt, io, rc::Rc}; diff --git a/core/src/term/mod.rs b/core/src/term/mod.rs index 973d8bc7..9216d31d 100644 --- a/core/src/term/mod.rs +++ b/core/src/term/mod.rs @@ -60,7 +60,6 @@ use std::{ /// /// Parsed terms also need to store their position in the source for error reporting. This is why /// this type is nested with [`RichTerm`]. -/// #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(untagged)] pub enum Term { diff --git a/core/tests/manual/main.rs b/core/tests/manual/main.rs index 766831c2..78acc9c0 100644 --- a/core/tests/manual/main.rs +++ b/core/tests/manual/main.rs @@ -138,6 +138,8 @@ fn check_error_report(actual: impl AsRef, expected: MessageExpectation) { } fn check_repl(content: String) { + use error::report::{report_with, ErrorFormat}; + let mut repl = ReplImpl::::new(std::io::sink()); repl.load_stdlib().unwrap(); for piece in content.split("\n\n") { @@ -156,7 +158,7 @@ fn check_repl(content: String) { let mut error = NoColor::new(Vec::::new()); let stdlib_ids = repl.cache_mut().get_all_stdlib_modules_file_id(); let files = repl.cache_mut().files_mut(); - error::report_with(&mut error, files, stdlib_ids.as_ref(), e); + report_with(&mut error, files, stdlib_ids.as_ref(), e, ErrorFormat::Text); check_error_report(String::from_utf8(error.into_inner()).unwrap(), expected); } diff --git a/utils/src/bench.rs b/utils/src/bench.rs index 40f57ce4..5aff8e52 100644 --- a/utils/src/bench.rs +++ b/utils/src/bench.rs @@ -174,6 +174,7 @@ macro_rules! ncl_bench_group { cache::{Envs, Cache, ErrorTolerance, ImportResolver}, eval::{VirtualMachine, cache::{CacheImpl, Cache as EvalCache}}, transform::import_resolution::strict::resolve_imports, + error::report::{report, ColorOpt, ErrorFormat}, }; let mut c: criterion::Criterion<_> = $config @@ -212,10 +213,11 @@ macro_rules! ncl_bench_group { .with_initial_env(eval_env.clone()); if let Err(e) = vm.eval(t) { - nickel_lang_core::error::report( + report( vm.import_resolver_mut(), e, - nickel_lang_core::error::ColorOpt::default() + ErrorFormat::Text, + ColorOpt::default(), ); panic!("Error during bench evaluation"); }