Auto merge of #13172 - Muscraft:diagnostic-system, r=epage

feat: Add `rustc` style errors for manifest parsing #12235 is tracking user control over warnings. One part of that is making `cargo`'s diagnostic system output messages in the style of `rustc`. To make it so that `cargo` doesn't have to manage a formatter and renderer, I decided to use [`annotate-snippets`](https://crates.io/crates/annotate-snippets), which matches `rustc`'s style well (at one time it was meant to be the formatted for `rustc`). To get this work kicked off, it was suggested to me that we should start with styling manifest parsing errors, and that is what his PR does. What manifest parsing errors look like after this change: ![image](https://github.com/rust-lang/cargo/assets/23045215/0eb388b9-8d72-48ad-84a9-585160995078) --- Note: `cargo` does not currently match `rustc` in color (#12740), `rustc` specifies their colors [here](https://github.com/rust-lang/rust/blob/740cea81d6e34fc03c4495890e29f1c5ecb40b96/compiler/rustc_errors/src/lib.rs#L1755-L1768) and [here](https://github.com/rust-lang/rust/blob/740cea81d6e34fc03c4495890e29f1c5ecb40b96/compiler/rustc_errors/src/emitter.rs#L2689-L2723). I used `annotate-snippets` default colors which match what `rustc` currently uses for colors. I did this as a stopgap while I work to unify the colors used between `rustc` and `cargo`. --- Note: the error messages come directly from `toml` and can be quite long. It would be nice if we could put some of the message below the highlight but this would require changes to `toml`. Example: ``` error: invalid type: integer `3` --> Cargo.toml:7:7 | 7 | bar = 3 | ^ expected a version string like "0.9.8" or a detailed dependency like { version = "0.9.8" } | ```
rust-lang · Jan 10, 2024 · 48601a3 · 48601a3
2 parents 187d4cf + 0d62ae2
commit 48601a3
Show file tree

Hide file tree

Showing 19 changed files with 389 additions and 344 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -18,6 +18,7 @@ homepage = "https://github.com/rust-lang/cargo"
 repository = "https://github.com/rust-lang/cargo"
 
 [workspace.dependencies]
+annotate-snippets = "0.10.1"
 anstream = "0.6.5"
 anstyle = "1.0.4"
 anyhow = "1.0.79"
@@ -139,6 +140,7 @@ name = "cargo"
 path = "src/cargo/lib.rs"
 
 [dependencies]
+annotate-snippets.workspace = true
 anstream.workspace = true
 anstyle.workspace = true
 anyhow.workspace = true

diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs
@@ -1,16 +1,19 @@
+use annotate_snippets::{Annotation, AnnotationType, Renderer, Slice, Snippet, SourceAnnotation};
 use std::collections::{BTreeMap, BTreeSet, HashMap};
 use std::ffi::OsStr;
 use std::path::{Path, PathBuf};
 use std::rc::Rc;
 use std::str::{self, FromStr};
 
+use crate::AlreadyPrintedError;
 use anyhow::{anyhow, bail, Context as _};
 use cargo_platform::Platform;
 use cargo_util::paths;
 use cargo_util_schemas::manifest;
 use cargo_util_schemas::manifest::RustVersion;
 use itertools::Itertools;
 use lazycell::LazyCell;
+use pathdiff::diff_paths;
 use tracing::{debug, trace};
 use url::Url;
 
@@ -43,7 +46,7 @@ pub fn read_manifest(
     path: &Path,
     source_id: SourceId,
     config: &Config,
-) -> Result<(EitherManifest, Vec<PathBuf>), ManifestError> {
+) -> CargoResult<(EitherManifest, Vec<PathBuf>)> {
     trace!(
         "read_manifest; path={}; source-id={}",
         path.display(),
@@ -56,15 +59,23 @@ pub fn read_manifest(
             return Err(ManifestError::new(
                 anyhow::anyhow!("parsing `{}` requires `-Zscript`", path.display()),
                 path.into(),
-            ));
+            ))?;
         }
         contents = embedded::expand_manifest(&contents, path, config)
             .map_err(|err| ManifestError::new(err, path.into()))?;
     }
 
-    read_manifest_from_str(&contents, path, embedded, source_id, config)
-        .with_context(|| format!("failed to parse manifest at `{}`", path.display()))
-        .map_err(|err| ManifestError::new(err, path.into()))
+    read_manifest_from_str(&contents, path, embedded, source_id, config).map_err(|err| {
+        if err.is::<AlreadyPrintedError>() {
+            err
+        } else {
+            ManifestError::new(
+                err.context(format!("failed to parse manifest at `{}`", path.display())),
+                path.into(),
+            )
+            .into()
+        }
+    })
 }
 
 /// See also `bin/cargo/commands/run.rs`s `is_manifest_command`
@@ -94,11 +105,61 @@ fn read_manifest_from_str(
 
     let mut unused = BTreeSet::new();
     let deserializer = toml::de::Deserializer::new(contents);
-    let manifest: manifest::TomlManifest = serde_ignored::deserialize(deserializer, |path| {
+    let manifest: manifest::TomlManifest = match serde_ignored::deserialize(deserializer, |path| {
         let mut key = String::new();
         stringify(&mut key, &path);
         unused.insert(key);
-    })?;
+    }) {
+        Ok(manifest) => manifest,
+        Err(e) => {
+            let Some(span) = e.span() else {
+                return Err(e.into());
+            };
+
+            let (line_num, column) = translate_position(&contents, span.start);
+            let source_start = contents[0..span.start]
+                .rfind('\n')
+                .map(|s| s + 1)
+                .unwrap_or(0);
+            let source_end = contents[span.end - 1..]
+                .find('\n')
+                .map(|s| s + span.end)
+                .unwrap_or(contents.len());
+            let source = &contents[source_start..source_end];
+            // Make sure we don't try to highlight past the end of the line,
+            // but also make sure we are highlighting at least one character
+            let highlight_end = (column + contents[span].chars().count())
+                .min(source.len())
+                .max(column + 1);
+            // Get the path to the manifest, relative to the cwd
+            let manifest_path = diff_paths(manifest_file, config.cwd())
+                .unwrap_or_else(|| manifest_file.to_path_buf())
+                .display()
+                .to_string();
+            let snippet = Snippet {
+                title: Some(Annotation {
+                    id: None,
+                    label: Some(e.message()),
+                    annotation_type: AnnotationType::Error,
+                }),
+                footer: vec![],
+                slices: vec![Slice {
+                    source: &source,
+                    line_start: line_num + 1,
+                    origin: Some(manifest_path.as_str()),
+                    annotations: vec![SourceAnnotation {
+                        range: (column, highlight_end),
+                        label: "",
+                        annotation_type: AnnotationType::Error,
+                    }],
+                    fold: false,
+                }],
+            };
+            let renderer = Renderer::styled();
+            writeln!(config.shell().err(), "{}", renderer.render(snippet))?;
+            return Err(AlreadyPrintedError::new(e.into()).into());
+        }
+    };
     let add_unused = |warnings: &mut Warnings| {
         for key in unused {
             warnings.add_warning(format!("unused manifest key: {}", key));
@@ -2113,3 +2174,33 @@ impl ResolveToPath for ConfigRelativePath {
         self.resolve_path(c)
     }
 }
+
+fn translate_position(input: &str, index: usize) -> (usize, usize) {
+    if input.is_empty() {
+        return (0, index);
+    }
+
+    let safe_index = index.min(input.len() - 1);
+    let column_offset = index - safe_index;
+
+    let nl = input[0..safe_index]
+        .as_bytes()
+        .iter()
+        .rev()
+        .enumerate()
+        .find(|(_, b)| **b == b'\n')
+        .map(|(nl, _)| safe_index - nl - 1);
+    let line_start = match nl {
+        Some(nl) => nl + 1,
+        None => 0,
+    };
+    let line = input[0..line_start]
+        .as_bytes()
+        .iter()
+        .filter(|c| **c == b'\n')
+        .count();
+    let column = input[line_start..=safe_index].chars().count() - 1;
+    let column = column + column_offset;
+
+    (line, column)
+}
diff --git a/tests/testsuite/alt_registry.rs b/tests/testsuite/alt_registry.rs
@@ -712,16 +712,17 @@ fn bad_registry_name() {
         .with_status(101)
         .with_stderr(
             "\
-[ERROR] failed to parse manifest at `[CWD]/Cargo.toml`
-
-Caused by:
-  TOML parse error at line 7, column 17
-    |
-  7 |                 [dependencies.bar]
-    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-  invalid character ` ` in registry name: `bad name`, [..]
+[ERROR] invalid character ` ` in registry name: `bad name`, characters must be Unicode XID characters (numbers, `-`, `_`, or most letters)
 
 
+ --> Cargo.toml:7:17
+  |
+7 |                   [dependencies.bar]
+  |  _________________^
+8 | |                 version = \"0.0.1\"
+9 | |                 registry = \"bad name\"
+  | |_____________________________________^
+  |
 ",
         )
         .run();
@@ -1622,16 +1623,14 @@ fn empty_dependency_registry() {
         .with_status(101)
         .with_stderr(
             "\
-[ERROR] failed to parse manifest at `[CWD]/Cargo.toml`
-
-Caused by:
-  TOML parse error at line 7, column 23
-    |
-  7 |                 bar = { version = \"0.1.0\", registry = \"\" }
-    |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-  registry name cannot be empty
+[ERROR] registry name cannot be empty
 
 
+ --> Cargo.toml:7:23
+  |
+7 |                 bar = { version = \"0.1.0\", registry = \"\" }
+  |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+  |
 ",
         )
         .run();

diff --git a/tests/testsuite/bad_config.rs b/tests/testsuite/bad_config.rs
@@ -449,15 +449,13 @@ fn malformed_override() {
         .with_status(101)
         .with_stderr(
             "\
-[ERROR] failed to parse manifest at `[..]`
-
-Caused by:
-  TOML parse error at line 8, column 27
-    |
-  8 |                 native = {
-    |                           ^
-  invalid inline table
-  expected `}`
+[ERROR] invalid inline table
+expected `}`
+ --> Cargo.toml:8:27
+  |
+8 |                 native = {
+  |                           ^
+  |
 ",
         )
         .run();
@@ -1282,14 +1280,12 @@ fn bad_dependency() {
         .with_status(101)
         .with_stderr(
             "\
-error: failed to parse manifest at `[..]`
-
-Caused by:
-  TOML parse error at line 8, column 23
-    |
-  8 |                 bar = 3
-    |                       ^
-  invalid type: integer `3`, expected a version string like [..]
+[ERROR] invalid type: integer `3`, expected a version string like [..]
+ --> Cargo.toml:8:23
+  |
+8 |                 bar = 3
+  |                       ^
+  |
 ",
         )
         .run();
@@ -1317,14 +1313,12 @@ fn bad_debuginfo() {
         .with_status(101)
         .with_stderr(
             "\
-error: failed to parse manifest [..]
-
-Caused by:
-  TOML parse error at line 8, column 25
-    |
-  8 |                 debug = 'a'
-    |                         ^^^
-  invalid value: string \"a\", expected a boolean, 0, 1, 2, \"line-tables-only\", or \"line-directives-only\"
+[ERROR] invalid value: string \"a\", expected a boolean, 0, 1, 2, \"line-tables-only\", or \"line-directives-only\"
+ --> Cargo.toml:8:25
+  |
+8 |                 debug = 'a'
+  |                         ^^^
+  |
 ",
         )
         .run();
@@ -1352,14 +1346,12 @@ fn bad_debuginfo2() {
         .with_status(101)
         .with_stderr(
             "\
-error: failed to parse manifest at `[..]`
-
-Caused by:
-  TOML parse error at line 8, column 25
-    |
-  8 |                 debug = 3.6
-    |                         ^^^
-  invalid type: floating point `3.6`, expected a boolean, 0, 1, 2, \"line-tables-only\", or \"line-directives-only\"
+[ERROR] invalid type: floating point `3.6`, expected a boolean, 0, 1, 2, \"line-tables-only\", or \"line-directives-only\"
+ --> Cargo.toml:8:25
+  |
+8 |                 debug = 3.6
+  |                         ^^^
+  |
 ",
         )
         .run();
@@ -1385,14 +1377,12 @@ fn bad_opt_level() {
         .with_status(101)
         .with_stderr(
             "\
-error: failed to parse manifest at `[..]`
-
-Caused by:
-  TOML parse error at line 6, column 25
-    |
-  6 |                 build = 3
-    |                         ^
-  invalid type: integer `3`, expected a boolean or string
+[ERROR] invalid type: integer `3`, expected a boolean or string
+ --> Cargo.toml:6:25
+  |
+6 |                 build = 3
+  |                         ^
+  |
 ",
         )
         .run();
@@ -1685,16 +1675,14 @@ fn bad_trim_paths() {
     p.cargo("check -Ztrim-paths")
         .masquerade_as_nightly_cargo(&["trim-paths"])
         .with_status(101)
-        .with_stderr(
-            r#"error: failed to parse manifest at `[..]`
-
-Caused by:
-  TOML parse error at line 7, column 30
-    |
-  7 |                 trim-paths = "split-debuginfo"
-    |                              ^^^^^^^^^^^^^^^^^
-  expected a boolean, "none", "diagnostics", "macro", "object", "all", or an array with these options
-"#,
+        .with_stderr("\
+[ERROR] expected a boolean, \"none\", \"diagnostics\", \"macro\", \"object\", \"all\", or an array with these options
+ --> Cargo.toml:7:30
+  |
+7 |                 trim-paths = \"split-debuginfo\"
+  |                              ^^^^^^^^^^^^^^^^^
+  |
+",
         )
         .run();
 }