From badc0d7a9f5905d13248f0651d7e16c0b9b7f738 Mon Sep 17 00:00:00 2001 From: Joscha Date: Fri, 7 Jan 2022 21:42:37 +0100 Subject: [PATCH] Print parse errors with codespan-reporting --- src/cli.rs | 42 ++++++++++++--------- src/files.rs | 26 ++++++++++--- src/files/arguments.rs | 22 ++++++----- src/files/commands.rs | 11 ++++++ src/files/error.rs | 85 ++++++++++++++++++++++++++++++++++++++++-- 5 files changed, 150 insertions(+), 36 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index 4664fcc..df1624f 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -7,9 +7,9 @@ use codespan_reporting::files::SimpleFile; use directories::ProjectDirs; use structopt::StructOpt; -use crate::eval::{DateRange, Entry, EntryMode}; +use crate::eval::{self, DateRange, Entry, EntryMode}; use crate::files::arguments::CliRange; -use crate::files::{self, FileSource, Files}; +use crate::files::{self, FileSource, Files, ParseError}; use self::error::Error; use self::layout::line::LineLayout; @@ -95,6 +95,24 @@ fn find_layout( layout::layout(files, entries, range, now) } +fn parse_eval_arg( + name: &str, + text: &str, + eval: impl FnOnce(T) -> Result>, +) -> Option +where + T: FromStr>, +{ + match T::from_str(text) { + Ok(value) => match eval(value) { + Ok(result) => return Some(result), + Err(e) => crate::error::eprint_error(&SimpleFile::new(name, text), &e), + }, + Err(e) => crate::error::eprint_error(&SimpleFile::new(name, text), &e), + } + None +} + fn run_command( opt: &Opt, files: &mut Files, @@ -144,21 +162,11 @@ pub fn run() { let now = find_now(&opt, &files); - // Kinda ugly, but it can stay for now (until it grows at least). - let range = match CliRange::from_str(&opt.range) { - Ok(range) => match range.eval((), now.date()) { - Ok(range) => range, - Err(e) => { - eprintln!("Failed to evaluate --range:"); - let file = SimpleFile::new("--range", &opt.range); - crate::error::eprint_error(&file, &e); - process::exit(1) - } - }, - Err(e) => { - eprintln!("Failed to parse --range:\n{}", e.with_path("--range")); - process::exit(1) - } + let range = match parse_eval_arg("--range", &opt.range, |range: CliRange| { + range.eval((), now.date()) + }) { + Some(range) => range, + None => process::exit(1), }; if let Err(e) = run_command(&opt, &mut files, range, now) { diff --git a/src/files.rs b/src/files.rs index 105010c..f1e170c 100644 --- a/src/files.rs +++ b/src/files.rs @@ -8,7 +8,7 @@ use codespan_reporting::files::SimpleFiles; use tzfile::Tz; use self::commands::{Command, Done, File, Log}; -pub use self::error::{Error, Result}; +pub use self::error::{Error, ParseError, Result}; use self::primitives::Spanned; pub mod arguments; @@ -162,9 +162,26 @@ impl Files { file: path.clone(), error: e, })?; + let cs_id = self + .cs_files + .add(name.to_string_lossy().to_string(), content.clone()); // Using `name` instead of `path` for the unwrap below. - let file = parse::parse(name, &content)?; + let file = match parse::parse(name, &content) { + Ok(file) => file, + Err(error) => { + // Using a dummy file. This should be fine since we return an + // error immediately after and the user must never call `load` + // twice. Otherwise, we run the danger of overwriting a file + // with empty content. + self.files + .push(LoadedFile::new(name.to_owned(), cs_id, File::dummy())); + return Err(Error::Parse { + file: FileSource(self.files.len() - 1), + error, + }); + } + }; let includes = file .commands @@ -175,10 +192,7 @@ impl Files { }) .collect::>(); - loaded.insert(path.clone()); - let cs_id = self - .cs_files - .add(path.to_string_lossy().to_string(), content); + loaded.insert(path); self.files .push(LoadedFile::new(name.to_owned(), cs_id, file)); diff --git a/src/files/arguments.rs b/src/files/arguments.rs index c04f991..6ff4b7d 100644 --- a/src/files/arguments.rs +++ b/src/files/arguments.rs @@ -1,3 +1,4 @@ +use std::result; use std::str::FromStr; use chrono::NaiveDate; @@ -5,7 +6,8 @@ use pest::iterators::Pair; use pest::Parser; use super::commands::Delta; -use super::parse::{self, Error, Result, Rule, TodayfileParser}; +use super::parse::{self, Result, Rule, TodayfileParser}; +use super::ParseError; #[derive(Debug)] pub enum CliDatum { @@ -61,14 +63,15 @@ fn parse_cli_ident(p: Pair<'_, Rule>) -> Result { } impl FromStr for CliIdent { - type Err = Error; + type Err = ParseError<()>; - fn from_str(s: &str) -> Result { - let mut pairs = TodayfileParser::parse(Rule::cli_ident, s)?; + fn from_str(s: &str) -> result::Result> { + let mut pairs = + TodayfileParser::parse(Rule::cli_ident, s).map_err(|e| ParseError::new((), e))?; let p = pairs.next().unwrap(); assert_eq!(pairs.next(), None); - parse_cli_ident(p) + parse_cli_ident(p).map_err(|e| ParseError::new((), e)) } } @@ -132,13 +135,14 @@ fn parse_cli_range(p: Pair<'_, Rule>) -> Result { } impl FromStr for CliRange { - type Err = Error; + type Err = ParseError<()>; - fn from_str(s: &str) -> Result { - let mut pairs = TodayfileParser::parse(Rule::cli_range, s)?; + fn from_str(s: &str) -> result::Result> { + let mut pairs = + TodayfileParser::parse(Rule::cli_range, s).map_err(|e| ParseError::new((), e))?; let p = pairs.next().unwrap(); assert_eq!(pairs.next(), None); - parse_cli_range(p) + parse_cli_range(p).map_err(|e| ParseError::new((), e)) } } diff --git a/src/files/commands.rs b/src/files/commands.rs index bda9ce7..82c6b51 100644 --- a/src/files/commands.rs +++ b/src/files/commands.rs @@ -357,3 +357,14 @@ pub struct File { pub contents: String, pub commands: Vec, } + +impl File { + /// Create an empty dummy file. This file should only be used as a + /// placeholder value. + pub fn dummy() -> Self { + Self { + contents: String::new(), + commands: vec![], + } + } +} diff --git a/src/files/error.rs b/src/files/error.rs index 14f2a8c..3133221 100644 --- a/src/files/error.rs +++ b/src/files/error.rs @@ -4,12 +4,85 @@ use std::{io, result}; use chrono::NaiveDate; use codespan_reporting::diagnostic::{Diagnostic, Label}; use codespan_reporting::term::Config; +use pest::error::{ErrorVariant, InputLocation}; use crate::error::Eprint; use super::primitives::Span; use super::{parse, FileSource, Files}; +#[derive(Debug, thiserror::Error)] +#[error("{error}")] +pub struct ParseError { + file: S, + error: parse::Error, +} + +impl ParseError { + pub fn new(file: S, error: parse::Error) -> Self { + Self { file, error } + } + + fn rule_name(rule: parse::Rule) -> String { + // TODO Rename rules to be more readable? + format!("{:?}", rule) + } + + fn enumerate(rules: &[parse::Rule]) -> String { + match rules.len() { + 0 => "something".to_string(), + 1 => Self::rule_name(rules[0]), + n => { + let except_last = rules + .iter() + .take(n - 1) + .map(|rule| Self::rule_name(*rule)) + .collect::>() + .join(", "); + let last = Self::rule_name(rules[n - 1]); + format!("{} or {}", except_last, last) + } + } + } + + fn notes(&self) -> Vec { + match &self.error.variant { + ErrorVariant::ParsingError { + positives, + negatives, + } => { + let mut notes = vec![]; + if !positives.is_empty() { + notes.push(format!("expected {}", Self::enumerate(positives))) + } + if !negatives.is_empty() { + notes.push(format!("unexpected {}", Self::enumerate(negatives))) + } + notes + } + ErrorVariant::CustomError { message } => vec![message.clone()], + } + } +} + +impl<'a, F> Eprint<'a, F> for ParseError +where + F: codespan_reporting::files::Files<'a>, +{ + fn eprint<'f: 'a>(&self, files: &'f F, config: &Config) { + let range = match self.error.location { + InputLocation::Pos(at) => at..at, + InputLocation::Span((from, to)) => from..to, + }; + let name = files.name(self.file).expect("file exists"); + let diagnostic = Diagnostic::error() + .with_message(format!("Could not parse {}", name)) + .with_labels(vec![Label::primary(self.file, range)]) + .with_notes(self.notes()); + Self::eprint_diagnostic(files, config, &diagnostic); + } +} + #[derive(Debug, thiserror::Error)] pub enum Error { #[error("Could not resolve {path}: {error}")] @@ -27,8 +100,11 @@ pub enum Error { }, #[error("Could not determine local timezone: {error}")] LocalTz { error: io::Error }, - #[error("{0}")] - Parse(#[from] parse::Error), + #[error("{error}")] + Parse { + file: FileSource, + error: parse::Error, + }, #[error("Conflicting time zones {tz1} and {tz2}")] TzConflict { file1: FileSource, @@ -81,8 +157,9 @@ impl<'a> Eprint<'a, Files> for Error { eprintln!("Could not determine local timezone:"); eprintln!(" {}", error); } - // TODO Format using codespan-reporting as well - Error::Parse(error) => eprintln!("{}", error), + Error::Parse { file, error } => { + ParseError::new(*file, error.clone()).eprint(files, config) + } Error::TzConflict { file1, span1,