Format some errors with codespan-reporting

src/files.rs

@@ -10,8 +10,9 @@ use codespan_reporting::term::{self, Config};
 use termcolor::StandardStream;
 use tzfile::Tz;
 
-use self::commands::{Command, Done, File};
+use self::commands::{Command, File};
 pub use self::error::{Error, Result};
+use self::primitives::Spanned;
 
 pub mod arguments;
 pub mod commands;
@@ -52,10 +53,18 @@ pub struct Source {
     command: usize,
 }
 
+// TODO Rename to `SourceFile`?
+#[derive(Debug, Clone, Copy)]
+pub struct FileSource(usize);
+
 impl Source {
     pub fn new(file: usize, command: usize) -> Self {
         Self { file, command }
     }
+
+    pub fn file(&self) -> FileSource {
+        FileSource(self.file)
+    }
 }
 
 #[derive(Debug)]
@@ -69,37 +78,47 @@ pub struct Files {
     files: Vec<LoadedFile>,
     /// Codespan-reporting file database.
     cs_files: SimpleFiles<String, String>,
-    timezone: Tz,
+    timezone: Option<Tz>,
     logs: HashMap<NaiveDate, Source>,
 }
 
 impl Files {
     /* Loading */
 
-    pub fn load(path: &Path) -> Result<Self> {
+    pub fn new() -> Self {
+        Self {
+            files: vec![],
+            cs_files: SimpleFiles::new(),
+            timezone: None,
+            logs: HashMap::new(),
+        }
+    }
+
+    /// Load a file and all its includes.
+    ///
+    /// # Warning
+    ///
+    /// - This function must be called before all other functions.
+    /// - This function must only be called once.
+    /// - If this function fails,
+    ///   - it is safe to print the error with [`Files::eprint_diagnostic`] and
+    ///   - no other function must be called.
+    pub fn load(&mut self, path: &Path) -> Result<()> {
+        if !self.files.is_empty() {
+            panic!("Files::load called multiple times");
+        }
+
         // Track already loaded files by their normalized paths
         let mut loaded = HashSet::new();
 
-        let mut files = vec![];
+        self.load_file(&mut loaded, path)?;
-        let mut cs_files = SimpleFiles::new();
+        self.determine_timezone()?;
-        Self::load_file(&mut loaded, &mut files, &mut cs_files, path)?;
+        self.collect_logs()?;
 
-        let timezone = Self::determine_timezone(&files)?;
+        Ok(())
-        let logs = Self::collect_logs(&files)?;
-        Ok(Self {
-            files,
-            cs_files,
-            timezone,
-            logs,
-        })
     }
 
-    fn load_file(
+    fn load_file(&mut self, loaded: &mut HashSet<PathBuf>, name: &Path) -> Result<()> {
-        loaded: &mut HashSet<PathBuf>,
-        files: &mut Vec<LoadedFile>,
-        cs_files: &mut SimpleFiles<String, String>,
-        name: &Path,
-    ) -> Result<()> {
         let path = name.canonicalize().map_err(|e| Error::ResolvePath {
             path: name.to_path_buf(),
             error: e,
@@ -127,58 +146,87 @@ impl Files {
             .collect::<Vec<_>>();
 
         loaded.insert(path.clone());
-        let cs_id = cs_files.add(path.to_string_lossy().to_string(), content);
+        let cs_id = self
-        files.push(LoadedFile::new(path, name.to_owned(), cs_id, file));
+            .cs_files
+            .add(path.to_string_lossy().to_string(), content);
+        self.files
+            .push(LoadedFile::new(path, name.to_owned(), cs_id, file));
 
         for include in includes {
             // Since we've successfully opened the file, its name can't be the
             // root directory or empty string and it must thus have a parent.
-            let include_path = name.parent().unwrap().join(include);
+            let include_path = name.parent().unwrap().join(include.value);
-            Self::load_file(loaded, files, cs_files, &include_path)?;
+            self.load_file(loaded, &include_path)?;
         }
 
         Ok(())
     }
 
-    fn determine_timezone(files: &[LoadedFile]) -> Result<Tz> {
+    fn determine_timezone(&mut self) -> Result<()> {
-        let mut found: Option<String> = None;
+        assert_eq!(self.timezone, None);
 
-        for command in Self::commands_of_files(files) {
+        let mut found: Option<(Source, Spanned<String>)> = None;
+
+        for command in self.commands() {
             if let Command::Timezone(tz) = command.command {
-                if let Some(found_tz) = &found {
+                if let Some((found_source, found_tz)) = &found {
-                    if tz != found_tz {
+                    if tz.value != found_tz.value {
                         return Err(Error::TzConflict {
-                            tz1: found_tz.clone(),
+                            file1: found_source.file(),
-                            tz2: tz.clone(),
+                            span1: found_tz.span,
+                            tz1: found_tz.value.clone(),
+                            file2: command.source.file(),
+                            span2: tz.span,
+                            tz2: tz.value.clone(),
                         });
                     }
                 } else {
-                    found = Some(tz.clone());
+                    found = Some((command.source, tz.clone()));
                 }
             }
         }
 
-        Ok(if let Some(timezone) = found {
+        let timezone = if let Some((source, tz)) = found {
-            Tz::named(&timezone).map_err(|error| Error::ResolveTz { timezone, error })?
+            Tz::named(&tz.value).map_err(|error| Error::ResolveTz {
+                file: source.file(),
+                span: tz.span,
+                tz: tz.value,
+                error,
+            })?
         } else {
             Tz::local().map_err(|error| Error::LocalTz { error })?
-        })
+        };
+        self.timezone = Some(timezone);
+
+        Ok(())
     }
 
-    fn collect_logs(files: &[LoadedFile]) -> Result<HashMap<NaiveDate, Source>> {
+    fn collect_logs(&mut self) -> Result<()> {
-        let mut logs = HashMap::new();
+        for command in Self::commands_of_files(&self.files) {
-
-        for command in Self::commands_of_files(files) {
             if let Command::Log(log) = command.command {
-                if let Entry::Vacant(e) = logs.entry(log.date) {
+                match self.logs.entry(log.date.value) {
+                    Entry::Vacant(e) => {
                         e.insert(command.source);
-                } else {
+                    }
-                    return Err(Error::LogConflict(log.date));
+                    Entry::Occupied(e) => {
+                        let other_cmd = Self::command_of_files(&self.files, *e.get());
+                        let other_span = match &other_cmd.command {
+                            Command::Log(log) => log.date.span,
+                            _ => unreachable!(),
+                        };
+                        return Err(Error::LogConflict {
+                            file1: other_cmd.source.file(),
+                            span1: other_span,
+                            file2: command.source.file(),
+                            span2: log.date.span,
+                            date: log.date.value,
+                        });
+                    }
                 }
             }
         }
 
-        Ok(logs)
+        Ok(())
     }
 
     /* Saving */
@@ -231,12 +279,21 @@ impl Files {
         Self::commands_of_files(&self.files)
     }
 
-    pub fn command(&self, source: Source) -> &Command {
+    fn command_of_files(files: &[LoadedFile], source: Source) -> SourcedCommand<'_> {
-        &self.files[source.file].file.commands[source.command]
+        let command = &files[source.file].file.commands[source.command];
+        SourcedCommand { source, command }
+    }
+
+    pub fn command(&self, source: Source) -> SourcedCommand<'_> {
+        Self::command_of_files(&self.files, source)
     }
 
     pub fn now(&self) -> DateTime<&Tz> {
-        Utc::now().with_timezone(&&self.timezone)
+        if let Some(tz) = &self.timezone {
+            Utc::now().with_timezone(&tz)
+        } else {
+            panic!("Called Files::now before Files::load");
+        }
     }
 
     /* Updating */
@@ -266,6 +323,10 @@ impl Files {
 
     /* Errors */
 
+    pub fn cs_id(&self, file: FileSource) -> usize {
+        self.files[file.0].cs_id
+    }
+
     pub fn eprint_diagnostic(&self, diagnostic: &Diagnostic<usize>) {
         let mut out = StandardStream::stderr(termcolor::ColorChoice::Auto);
         let config = Config::default();

src/files/commands.rs

@@ -339,14 +339,14 @@ pub struct Note {
 
 #[derive(Debug)]
 pub struct Log {
-    pub date: NaiveDate,
+    pub date: Spanned<NaiveDate>,
     pub desc: Vec<String>,
 }
 
 #[derive(Debug)]
 pub enum Command {
-    Include(String),
+    Include(Spanned<String>),
-    Timezone(String),
+    Timezone(Spanned<String>),
     Task(Task),
     Note(Note),
     Log(Log),

src/files/error.rs

@@ -2,10 +2,10 @@ use std::path::PathBuf;
 use std::{io, result};
 
 use chrono::NaiveDate;
+use codespan_reporting::diagnostic::{Diagnostic, Label};
 
-use super::parse;
+use super::primitives::Span;
-
+use super::{parse, FileSource, Files};
-// TODO Format TzConflict and LogConflict errors better
 
 #[derive(Debug, thiserror::Error)]
 pub enum Error {
@@ -15,20 +15,38 @@ pub enum Error {
     ReadFile { file: PathBuf, error: io::Error },
     #[error("Could not write {file}: {error}")]
     WriteFile { file: PathBuf, error: io::Error },
-    #[error("Could not resolve timezone {timezone}: {error}")]
+    #[error("Could not resolve timezone {tz}: {error}")]
-    ResolveTz { timezone: String, error: io::Error },
+    ResolveTz {
+        file: FileSource,
+        span: Span,
+        tz: String,
+        error: io::Error,
+    },
     #[error("Could not determine local timezone: {error}")]
     LocalTz { error: io::Error },
     #[error("{0}")]
     Parse(#[from] parse::Error),
     #[error("Conflicting time zones {tz1} and {tz2}")]
-    TzConflict { tz1: String, tz2: String },
+    TzConflict {
-    #[error("Duplicate logs for {0}")]
+        file1: FileSource,
-    LogConflict(NaiveDate),
+        span1: Span,
+        tz1: String,
+        file2: FileSource,
+        span2: Span,
+        tz2: String,
+    },
+    #[error("Duplicate logs for {date}")]
+    LogConflict {
+        file1: FileSource,
+        span1: Span,
+        file2: FileSource,
+        span2: Span,
+        date: NaiveDate,
+    },
 }
 
 impl Error {
-    pub fn print(&self) {
+    pub fn print(&self, files: &Files) {
         match self {
             Error::ResolvePath { path, error } => {
                 eprintln!("Could not resolve path {:?}:", path);
@@ -42,22 +60,61 @@ impl Error {
                 eprintln!("Could not write file {:?}:", file);
                 eprintln!("  {}", error);
             }
-            Error::ResolveTz { timezone, error } => {
+            Error::ResolveTz {
-                eprintln!("Could not resolve time zone {}:", timezone);
+                file,
-                eprintln!("  {}", error);
+                span,
+                tz,
+                error,
+            } => {
+                let diagnostic = Diagnostic::error()
+                    .with_message(format!("Could not resolve time zone {}", tz))
+                    .with_labels(vec![Label::primary(files.cs_id(*file), span)
+                        .with_message("Time zone defined here")])
+                    .with_notes(vec![format!("{}", error)]);
+                files.eprint_diagnostic(&diagnostic);
             }
             Error::LocalTz { error } => {
                 eprintln!("Could not determine local timezone:");
                 eprintln!("  {}", error);
             }
+            // TODO Format using codespan-reporting as well
             Error::Parse(error) => eprintln!("{}", error),
-            Error::TzConflict { tz1, tz2 } => {
+            Error::TzConflict {
-                eprintln!("Time zone conflict:");
+                file1,
-                eprintln!("  Both {} and {} are specified", tz1, tz2);
+                span1,
+                tz1,
+                file2,
+                span2,
+                tz2,
+            } => {
+                let diagnostic = Diagnostic::error()
+                    .with_message(format!("Time zone conflict between {} and {}", tz1, tz2))
+                    .with_labels(vec![
+                        Label::primary(files.cs_id(*file1), span1)
+                            .with_message("Time zone defined here"),
+                        Label::primary(files.cs_id(*file2), span2)
+                            .with_message("Time zone defined here"),
+                    ])
+                    .with_notes(vec![
+                        "All TIMEZONE commands must set the same time zone.".to_string()
+                    ]);
+                files.eprint_diagnostic(&diagnostic);
             }
-            Error::LogConflict(date) => {
+            Error::LogConflict {
-                eprintln!("Log conflict:");
+                file1,
-                eprintln!("  More than one entry exists for {}", date);
+                span1,
+                file2,
+                span2,
+                date,
+            } => {
+                let diagnostic = Diagnostic::error()
+                    .with_message(format!("Duplicate log entries for {}", date))
+                    .with_labels(vec![
+                        Label::primary(files.cs_id(*file1), span1).with_message("Log defined here"),
+                        Label::primary(files.cs_id(*file2), span2).with_message("Log defined here"),
+                    ])
+                    .with_notes(vec!["A day can have at most one LOG entry.".to_string()]);
+                files.eprint_diagnostic(&diagnostic);
             }
         }
     }

src/files/parse.rs

@@ -33,14 +33,20 @@ fn fail<S: Into<String>, T>(span: Span<'_>, message: S) -> Result<T> {
     Err(error(span, message))
 }
 
-fn parse_include(p: Pair<'_, Rule>) -> String {
+fn parse_include(p: Pair<'_, Rule>) -> Spanned<String> {
     assert_eq!(p.as_rule(), Rule::include);
-    p.into_inner().next().unwrap().as_str().to_string()
+    let p = p.into_inner().next().unwrap();
+    let span = (&p.as_span()).into();
+    let name = p.as_str().to_string();
+    Spanned::new(span, name)
 }
 
-fn parse_timezone(p: Pair<'_, Rule>) -> String {
+fn parse_timezone(p: Pair<'_, Rule>) -> Spanned<String> {
     assert_eq!(p.as_rule(), Rule::timezone);
-    p.into_inner().next().unwrap().as_str().trim().to_string()
+    let p = p.into_inner().next().unwrap();
+    let span = (&p.as_span()).into();
+    let name = p.as_str().to_string();
+    Spanned::new(span, name)
 }
 
 fn parse_number(p: Pair<'_, Rule>) -> i32 {
@@ -795,9 +801,9 @@ fn parse_note(p: Pair<'_, Rule>) -> Result<Note> {
     })
 }
 
-fn parse_log_head(p: Pair<'_, Rule>) -> Result<NaiveDate> {
+fn parse_log_head(p: Pair<'_, Rule>) -> Result<Spanned<NaiveDate>> {
     assert_eq!(p.as_rule(), Rule::log_head);
-    Ok(parse_datum(p.into_inner().next().unwrap())?.value)
+    parse_datum(p.into_inner().next().unwrap())
 }
 
 fn parse_log(p: Pair<'_, Rule>) -> Result<Log> {

src/files/primitives.rs

@@ -1,5 +1,5 @@
 use std::cmp::{self, Ordering};
-use std::fmt;
+use std::{fmt, ops};
 
 use chrono::{NaiveTime, Timelike};
 
@@ -18,6 +18,12 @@ impl<'a> From<&pest::Span<'a>> for Span {
     }
 }
 
+impl From<&Span> for ops::Range<usize> {
+    fn from(span: &Span) -> Self {
+        span.start..span.end
+    }
+}
+
 impl Span {
     pub fn join(self, other: Self) -> Self {
         Self {