When running --locked, check formatting of store files

This will detect issues like audits being out of order and unrecognized
fields in CI, and should help avoid simple executions of `cargo vet`
causing formatting changes locally.

The implementation isn't particularly efficient for this, in that it
immediately attempts to re-serialize the file after parsing it, and then
compares the results (ignoring trailing newlines). As this checks
formatting, it is a string-based check, and not structural.

The `similar` library, which we already used for our tests, is used to
allow including diffs in the output to make any issues more clear. This
also required some changes to the way we read/write files from disk to
keep information around.

As a side-effect of these changes, we no longer emit an extra
unnecessary newline at the end of store files. This won't be detected as
an error by the checker, but will be a change when run locally.

Fixes #341

Cargo.toml

@@ -34,6 +34,7 @@ nom = "7.1.1"
 reqwest = { version = "0.11.10", default-features = false, features = ["rustls-tls"] }
 serde = "1.0.136"
 serde_json = "1.0.82"
+similar = "2.2.0"
 tar = { version = "0.4.26", default-features = false }
 tempfile = "3.3.0"
 textwrap = { version = "0.15", default-features = false }
@@ -58,4 +59,3 @@ features = [
 
 [dev-dependencies]
 insta = "1.16.0"
-similar = "2.1.0"

src/errors.rs

@@ -1,12 +1,77 @@
-use std::{ffi::OsString, fmt::Display, path::PathBuf, string::FromUtf8Error, sync::Arc};
+use std::{
+    ffi::OsString,
+    fmt::{Debug, Display},
+    path::PathBuf,
+    string::FromUtf8Error,
+    sync::Arc,
+};
 
 use cargo_metadata::Version;
-use miette::{Diagnostic, NamedSource, SourceOffset, SourceSpan};
+use miette::{Diagnostic, MietteSpanContents, SourceCode, SourceOffset, SourceSpan};
 use thiserror::Error;
 
 use crate::format::{CriteriaName, ForeignCriteriaName, ImportName, PackageName};
 
-pub type SourceFile = Arc<NamedSource>;
+#[derive(Eq, PartialEq)]
+struct SourceFileInner {
+    name: String,
+    source: String,
+}
+
+#[derive(Clone, Eq, PartialEq)]
+pub struct SourceFile {
+    inner: Arc<SourceFileInner>,
+}
+
+impl SourceFile {
+    pub fn new_empty(name: &str) -> Self {
+        Self::new(name, String::new())
+    }
+    pub fn new(name: &str, source: String) -> Self {
+        SourceFile {
+            inner: Arc::new(SourceFileInner {
+                name: name.to_owned(),
+                source,
+            }),
+        }
+    }
+    pub fn name(&self) -> &str {
+        &self.inner.name
+    }
+    pub fn source(&self) -> &str {
+        &self.inner.source
+    }
+}
+
+impl SourceCode for SourceFile {
+    fn read_span<'a>(
+        &'a self,
+        span: &SourceSpan,
+        context_lines_before: usize,
+        context_lines_after: usize,
+    ) -> Result<Box<dyn miette::SpanContents<'a> + 'a>, miette::MietteError> {
+        let contents = self
+            .source()
+            .read_span(span, context_lines_before, context_lines_after)?;
+        Ok(Box::new(MietteSpanContents::new_named(
+            self.name().to_owned(),
+            contents.data(),
+            *contents.span(),
+            contents.line(),
+            contents.column(),
+            contents.line_count(),
+        )))
+    }
+}
+
+impl Debug for SourceFile {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.debug_struct("SourceFile")
+            .field("name", &self.name())
+            .field("source", &self.source())
+            .finish()
+    }
+}
 
 ///////////////////////////////////////////////////////////
 // AuditAsErrors
@@ -212,16 +277,12 @@ pub struct CriteriaChangeErrors {
     pub errors: Vec<CriteriaChangeError>,
 }
 #[derive(Debug, Error, Diagnostic)]
-// FIXME: it would be rad if this was a diff!
-#[error(
-    "{import_name}'s '{criteria_name}' criteria changed from\n\n{old_desc}\n\nto\n\n{new_desc}\n"
-)]
+#[error("{import_name}'s '{criteria_name}' criteria changed:\n\n{unified_diff}")]
 #[diagnostic(help("Run `cargo vet regenerate imports` to accept this new definition"))]
 pub struct CriteriaChangeError {
     pub import_name: ImportName,
     pub criteria_name: ForeignCriteriaName,
-    pub old_desc: String,
-    pub new_desc: String,
+    pub unified_diff: String,
 }
 
 ////////////////////////////////////////////////////////////
@@ -241,6 +302,9 @@ pub enum StoreValidateError {
     #[diagnostic(transparent)]
     #[error(transparent)]
     InvalidCriteria(InvalidCriteriaError),
+    #[diagnostic(transparent)]
+    #[error(transparent)]
+    BadFormat(BadFormatError),
     #[error("imports.lock is out-of-date with respect to configuration")]
     #[diagnostic(help("run `cargo vet` without --locked to update imports"))]
     ImportsLockOutdated,
@@ -258,6 +322,13 @@ pub struct InvalidCriteriaError {
     pub valid_names: Arc<Vec<String>>,
 }
 
+#[derive(Debug, Error, Diagnostic)]
+#[error("A file in the store is not correctly formatted:\n\n{unified_diff}")]
+#[diagnostic(help("run `cargo vet` without --locked to reformat files in the store"))]
+pub struct BadFormatError {
+    pub unified_diff: String,
+}
+
 //////////////////////////////////////////////////////////
 // CacheErrors
 /////////////////////////////////////////////////////////
@@ -691,28 +762,6 @@ pub enum LoadJsonError {
     ),
 }
 
-#[derive(Debug, Error, Diagnostic)]
-#[non_exhaustive]
-pub enum StoreJsonError {
-    #[error(transparent)]
-    JsonSerialize(#[from] serde_json::Error),
-    #[error("couldn't store json")]
-    IoError(
-        #[from]
-        #[source]
-        std::io::Error,
-    ),
-}
+pub type StoreJsonError = serde_json::Error;
 
-#[derive(Debug, Error, Diagnostic)]
-#[non_exhaustive]
-pub enum StoreTomlError {
-    #[error(transparent)]
-    TomlSerialize(#[from] toml_edit::ser::Error),
-    #[error("couldn't store toml")]
-    IoError(
-        #[from]
-        #[source]
-        std::io::Error,
-    ),
-}
+pub type StoreTomlError = toml_edit::ser::Error;

src/main.rs

@@ -20,7 +20,7 @@ use format::{CriteriaName, CriteriaStr, PackageName, PolicyEntry};
 use futures_util::future::{join_all, try_join_all};
 use indicatif::ProgressDrawTarget;
 use lazy_static::lazy_static;
-use miette::{miette, Context, Diagnostic, IntoDiagnostic, NamedSource, SourceOffset};
+use miette::{miette, Context, Diagnostic, IntoDiagnostic, SourceOffset};
 use network::Network;
 use out::{progress_bar, IncProgressOnDrop};
 use reqwest::Url;
@@ -30,7 +30,7 @@ use thiserror::Error;
 use tracing::{error, info, trace, warn};
 
 use crate::cli::*;
-use crate::errors::{CommandError, DownloadError, RegenerateExemptionsError};
+use crate::errors::{CommandError, DownloadError, RegenerateExemptionsError, SourceFile};
 use crate::format::{
     AuditEntry, AuditKind, AuditsFile, ConfigFile, CriteriaEntry, DependencyCriteria,
     ExemptedDependency, FetchCommand, ImportsFile, MetaConfig, MetaConfigInstance, PackageStr,
@@ -1610,7 +1610,7 @@ fn cmd_aggregate(
             let url_string = url.to_string();
             let audit_bytes = network.download(url).await?;
             let audit_string = String::from_utf8(audit_bytes).map_err(LoadTomlError::from)?;
-            let audit_source = Arc::new(NamedSource::new(url_string.clone(), audit_string.clone()));
+            let audit_source = SourceFile::new(&url_string, audit_string.clone());
             let audit_file: AuditsFile = toml::de::from_str(&audit_string)
                 .map_err(|error| {
                     let (line, col) = error.line_col().unwrap_or((0, 0));