fix(manifest): Show error source to users (#15939)

### What does this PR try to resolve?

Ooch our way towards rustc's quality of error reporting to make up for
the fact that users won't see most frontmatter rustc error messages.

### How to test and review this PR?

Several parts of this are not ideal yet
- Frontmatter close should show the open and show the EOF, instead of
  pointing at the open
- Trailing content on close will usually have a newline
- Multiple frontmatters should show the original frontmatter
- Some content, like supported infostrings, should go in a help
- Ideally we try to recover on no closing and instead point out a
  mismatched open/close

But this is still an improvement over nothing!

Author: weihanglo

src/cargo/util/frontmatter.rs

@@ -1,5 +1,3 @@
-use crate::CargoResult;
-
 type Span = std::ops::Range<usize>;
 
 #[derive(Debug)]
@@ -22,7 +20,7 @@ pub struct ScriptSource<'s> {
 }
 
 impl<'s> ScriptSource<'s> {
-    pub fn parse(raw: &'s str) -> CargoResult<Self> {
+    pub fn parse(raw: &'s str) -> Result<Self, FrontmatterError> {
         use winnow::stream::FindSlice as _;
         use winnow::stream::Location as _;
         use winnow::stream::Offset as _;
@@ -61,24 +59,30 @@ impl<'s> ScriptSource<'s> {
             .char_indices()
             .find_map(|(i, c)| (c != FENCE_CHAR).then_some(i))
             .unwrap_or_else(|| input.eof_offset());
+        let open_start = input.current_token_start();
+        let fence_pattern = input.next_slice(fence_length);
+        let open_end = input.current_token_start();
         match fence_length {
             0 => {
                 return Ok(source);
             }
             1 | 2 => {
                 // either not a frontmatter or invalid frontmatter opening
-                anyhow::bail!(
-                    "found {fence_length} `{FENCE_CHAR}` in rust frontmatter, expected at least 3"
-                )
+                return Err(FrontmatterError::new(
+                    format!(
+                        "found {fence_length} `{FENCE_CHAR}` in rust frontmatter, expected at least 3"
+                    ),
+                    open_start..open_end,
+                ));
             }
             _ => {}
         }
-        let open_start = input.current_token_start();
-        let fence_pattern = input.next_slice(fence_length);
-        let open_end = input.current_token_start();
         source.open = Some(open_start..open_end);
         let Some(info_nl) = input.find_slice("\n") else {
-            anyhow::bail!("no closing `{fence_pattern}` found for frontmatter");
+            return Err(FrontmatterError::new(
+                format!("no closing `{fence_pattern}` found for frontmatter"),
+                open_start..open_end,
+            ));
         };
         let info = input.next_slice(info_nl.start);
         let info = info.trim_matches(is_whitespace);
@@ -91,7 +95,10 @@ impl<'s> ScriptSource<'s> {
         // Ends with a line that starts with a matching number of `-` only followed by whitespace
         let nl_fence_pattern = format!("\n{fence_pattern}");
         let Some(frontmatter_nl) = input.find_slice(nl_fence_pattern.as_str()) else {
-            anyhow::bail!("no closing `{fence_pattern}` found for frontmatter");
+            return Err(FrontmatterError::new(
+                format!("no closing `{fence_pattern}` found for frontmatter"),
+                open_start..open_end,
+            ));
         };
         let frontmatter_start = input.current_token_start() + 1; // skip nl from infostring
         let _ = input.next_slice(frontmatter_nl.start + 1);
@@ -111,14 +118,29 @@ impl<'s> ScriptSource<'s> {
         let after_closing_fence = after_closing_fence.trim_matches(is_whitespace);
         if !after_closing_fence.is_empty() {
             // extra characters beyond the original fence pattern, even if they are extra `-`
-            anyhow::bail!("trailing characters found after frontmatter close");
+            return Err(FrontmatterError::new(
+                format!("trailing characters found after frontmatter close"),
+                close_end..content_start,
+            ));
         }
 
         source.content = content_start..content_end;
 
-        let repeat = Self::parse(source.content())?;
-        if repeat.frontmatter.is_some() {
-            anyhow::bail!("only one frontmatter is supported");
+        if let Some(nl_end) = strip_ws_lines(input.as_ref()) {
+            let _ = input.next_slice(nl_end);
+        }
+        let fence_length = input
+            .as_ref()
+            .char_indices()
+            .find_map(|(i, c)| (c != FENCE_CHAR).then_some(i))
+            .unwrap_or_else(|| input.eof_offset());
+        if 0 < fence_length {
+            let fence_start = input.current_token_start();
+            let fence_end = fence_start + fence_length;
+            return Err(FrontmatterError::new(
+                format!("only one frontmatter is supported"),
+                fence_start..fence_end,
+            ));
         }
 
         Ok(source)
@@ -232,6 +254,37 @@ fn is_whitespace(c: char) -> bool {
     )
 }
 
+#[derive(Debug)]
+pub struct FrontmatterError {
+    message: String,
+    span: Span,
+}
+
+impl FrontmatterError {
+    pub fn new(message: impl Into<String>, span: Span) -> Self {
+        Self {
+            message: message.into(),
+            span,
+        }
+    }
+
+    pub fn message(&self) -> &str {
+        self.message.as_str()
+    }
+
+    pub fn span(&self) -> Span {
+        self.span.clone()
+    }
+}
+
+impl std::fmt::Display for FrontmatterError {
+    fn fmt(&self, fmt: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        self.message.fmt(fmt)
+    }
+}
+
+impl std::error::Error for FrontmatterError {}
+
 #[cfg(test)]
 mod test {
     use snapbox::assert_data_eq;

src/cargo/util/toml/embedded.rs

@@ -1,23 +1,29 @@
 use cargo_util_schemas::manifest::PackageName;
 
-use crate::CargoResult;
+use crate::util::frontmatter::FrontmatterError;
 use crate::util::frontmatter::ScriptSource;
 use crate::util::restricted_names;
 
-pub(super) fn expand_manifest(content: &str) -> CargoResult<String> {
+pub(super) fn expand_manifest(content: &str) -> Result<String, FrontmatterError> {
     let source = ScriptSource::parse(content)?;
     if let Some(span) = source.frontmatter_span() {
         match source.info() {
             Some("cargo") | None => {}
             Some(other) => {
                 if let Some(remainder) = other.strip_prefix("cargo,") {
-                    anyhow::bail!(
-                        "cargo does not support frontmatter infostring attributes like `{remainder}` at this time"
-                    )
+                    return Err(FrontmatterError::new(
+                        format!(
+                            "cargo does not support frontmatter infostring attributes like `{remainder}` at this time"
+                        ),
+                        source.info_span().unwrap(),
+                    ));
                 } else {
-                    anyhow::bail!(
-                        "frontmatter infostring `{other}` is unsupported by cargo; specify `cargo` for embedding a manifest"
-                    )
+                    return Err(FrontmatterError::new(
+                        format!(
+                            "frontmatter infostring `{other}` is unsupported by cargo; specify `cargo` for embedding a manifest"
+                        ),
+                        source.info_span().unwrap(),
+                    ));
                 }
             }
         }

src/cargo/util/toml/mod.rs

@@ -67,12 +67,11 @@ pub fn read_manifest(
     let mut errors = Default::default();
 
     let is_embedded = is_embedded(path);
-    let contents = read_toml_string(path, is_embedded, gctx)
-        .map_err(|err| ManifestError::new(err, path.into()))?;
-    let document =
-        parse_document(&contents).map_err(|e| emit_diagnostic(e.into(), &contents, path, gctx))?;
+    let contents = read_toml_string(path, is_embedded, gctx)?;
+    let document = parse_document(&contents)
+        .map_err(|e| emit_toml_diagnostic(e.into(), &contents, path, gctx))?;
     let original_toml = deserialize_toml(&document)
-        .map_err(|e| emit_diagnostic(e.into(), &contents, path, gctx))?;
+        .map_err(|e| emit_toml_diagnostic(e.into(), &contents, path, gctx))?;
 
     let mut manifest = (|| {
         let empty = Vec::new();
@@ -152,12 +151,13 @@ pub fn read_manifest(
 
 #[tracing::instrument(skip_all)]
 fn read_toml_string(path: &Path, is_embedded: bool, gctx: &GlobalContext) -> CargoResult<String> {
-    let mut contents = paths::read(path)?;
+    let mut contents = paths::read(path).map_err(|err| ManifestError::new(err, path.into()))?;
     if is_embedded {
         if !gctx.cli_unstable().script {
             anyhow::bail!("parsing `{}` requires `-Zscript`", path.display());
         }
-        contents = embedded::expand_manifest(&contents)?;
+        contents = embedded::expand_manifest(&contents)
+            .map_err(|e| emit_frontmatter_diagnostic(e, &contents, path, gctx))?;
     }
     Ok(contents)
 }
@@ -2777,7 +2777,32 @@ fn lints_to_rustflags(lints: &manifest::TomlLints) -> CargoResult<Vec<String>> {
     Ok(rustflags)
 }
 
-fn emit_diagnostic(
+fn emit_frontmatter_diagnostic(
+    e: crate::util::frontmatter::FrontmatterError,
+    contents: &str,
+    manifest_file: &Path,
+    gctx: &GlobalContext,
+) -> anyhow::Error {
+    let span = e.span();
+
+    // Get the path to the manifest, relative to the cwd
+    let manifest_path = diff_paths(manifest_file, gctx.cwd())
+        .unwrap_or_else(|| manifest_file.to_path_buf())
+        .display()
+        .to_string();
+    let group = Group::with_title(Level::ERROR.primary_title(e.message())).element(
+        Snippet::source(contents)
+            .path(manifest_path)
+            .annotation(AnnotationKind::Primary.span(span)),
+    );
+
+    if let Err(err) = gctx.shell().print_report(&[group], true) {
+        return err.into();
+    }
+    return AlreadyPrintedError::new(e.into()).into();
+}
+
+fn emit_toml_diagnostic(
     e: toml::de::Error,
     contents: &str,
     manifest_file: &Path,

tests/testsuite/script/rustc_fixtures/dot-in-infostring-leading.stderr

@@ -1 +1,5 @@
 [ERROR] frontmatter infostring `.toml` is unsupported by cargo; specify `cargo` for embedding a manifest
+ --> script:1:4
+  |
+1 | ---.toml
+  |    ^^^^^

tests/testsuite/script/rustc_fixtures/dot-in-infostring-non-leading.stderr

@@ -1 +1,5 @@
 [ERROR] frontmatter infostring `Cargo.toml` is unsupported by cargo; specify `cargo` for embedding a manifest
+ --> script:1:4
+  |
+1 | ---Cargo.toml
+  |    ^^^^^^^^^^

tests/testsuite/script/rustc_fixtures/extra-after-end.stderr

@@ -1 +1,7 @@
 [ERROR] trailing characters found after frontmatter close
+ --> script:2:4
+  |
+2 |   ---cargo
+  |  ____^
+3 | | //~^ ERROR: extra characters after frontmatter close are not allowed
+  | |_^

tests/testsuite/script/rustc_fixtures/frontmatter-whitespace-2.stderr

@@ -1 +1,5 @@
 [ERROR] no closing `---` found for frontmatter
+ --> script:1:1
+  |
+1 | ---cargo
+  | ^^^

tests/testsuite/script/rustc_fixtures/hyphen-in-infostring-leading.stderr

@@ -1 +1,5 @@
 [ERROR] frontmatter infostring `-toml` is unsupported by cargo; specify `cargo` for embedding a manifest
+ --> script:1:5
+  |
+1 | --- -toml
+  |     ^^^^^

tests/testsuite/script/rustc_fixtures/hyphen-in-infostring-non-leading.stderr

@@ -1 +1,5 @@
 [ERROR] frontmatter infostring `Cargo-toml` is unsupported by cargo; specify `cargo` for embedding a manifest
+ --> script:1:5
+  |
+1 | --- Cargo-toml
+  |     ^^^^^^^^^^

tests/testsuite/script/rustc_fixtures/infostring-fail.stderr

@@ -1 +1,5 @@
 [ERROR] cargo does not support frontmatter infostring attributes like `clippy` at this time
+ --> script:1:4
+  |
+1 | ---cargo,clippy
+  |    ^^^^^^^^^^^^