Auto merge of #12556 - epage:serde, r=arlosi

fix(toml): Improve parse errors

### What does this PR try to resolve?

When we adopted `toml_edit`, we got TOML syntax errors that showed the context for where the error occurred.  However, the work was not done to extend this to semantic errors reported by serde.

This updates `Cargo.toml` and `Cargo.lock` code to provide that context on semantic errors.  `config.toml` is not done because the schema is decentralized.

In theory, this will also improve performance because we aren't having to allocate a lot of intermediate data to then throw away for every `Cargo.toml` we read.

### How should we test and review this PR?

Check by commit to see this change gradually.
- The `package.cargo-features` change was made to drop out dependence on `toml::Table` so we could do the direct deserialization

Author: bors

src/cargo/ops/lockfile.rs

@@ -2,7 +2,6 @@ use std::io::prelude::*;
 
 use crate::core::{resolver, Resolve, ResolveVersion, Workspace};
 use crate::util::errors::CargoResult;
-use crate::util::toml as cargo_toml;
 use crate::util::Filesystem;
 
 use anyhow::Context as _;
@@ -20,8 +19,7 @@ pub fn load_pkg_lockfile(ws: &Workspace<'_>) -> CargoResult<Option<Resolve>> {
         .with_context(|| format!("failed to read file: {}", f.path().display()))?;
 
     let resolve = (|| -> CargoResult<Option<Resolve>> {
-        let resolve: toml::Table = cargo_toml::parse_document(&s, f.path(), ws.config())?;
-        let v: resolver::EncodableResolve = resolve.try_into()?;
+        let v: resolver::EncodableResolve = toml::from_str(&s)?;
         Ok(Some(v.into_resolve(&s, ws)?))
     })()
     .with_context(|| format!("failed to parse lock file at: {}", f.path().display()))?;

src/cargo/util/toml/mod.rs

@@ -12,7 +12,6 @@ use cargo_util::paths;
 use itertools::Itertools;
 use lazycell::LazyCell;
 use semver::{self, VersionReq};
-use serde::de::IntoDeserializer as _;
 use serde::de::{self, Unexpected};
 use serde::ser;
 use serde::{Deserialize, Serialize};
@@ -99,26 +98,9 @@ fn read_manifest_from_str(
 ) -> CargoResult<(EitherManifest, Vec<PathBuf>)> {
     let package_root = manifest_file.parent().unwrap();
 
-    let toml = {
-        let pretty_filename = manifest_file
-            .strip_prefix(config.cwd())
-            .unwrap_or(manifest_file);
-        parse_document(contents, pretty_filename, config)?
-    };
-
-    // Provide a helpful error message for a common user error.
-    if let Some(package) = toml.get("package").or_else(|| toml.get("project")) {
-        if let Some(feats) = package.get("cargo-features") {
-            bail!(
-                "cargo-features = {} was found in the wrong location: it \
-                 should be set at the top of Cargo.toml before any tables",
-                feats
-            );
-        }
-    }
-
     let mut unused = BTreeSet::new();
-    let manifest: TomlManifest = serde_ignored::deserialize(toml.into_deserializer(), |path| {
+    let deserializer = toml::de::Deserializer::new(contents);
+    let manifest: TomlManifest = serde_ignored::deserialize(deserializer, |path| {
         let mut key = String::new();
         stringify(&mut key, &path);
         unused.insert(key);
@@ -194,8 +176,7 @@ fn read_manifest_from_str(
 
 pub fn parse_document(toml: &str, _file: &Path, _config: &Config) -> CargoResult<toml::Table> {
     // At the moment, no compatibility checks are needed.
-    toml.parse()
-        .map_err(|e| anyhow::Error::from(e).context("could not parse input as TOML"))
+    toml.parse().map_err(Into::into)
 }
 
 /// Warn about paths that have been deprecated and may conflict.
@@ -1537,6 +1518,10 @@ pub struct TomlPackage {
     repository: Option<MaybeWorkspaceString>,
     resolver: Option<String>,
 
+    // Provide a helpful error message for a common user error.
+    #[serde(rename = "cargo-features", skip_serializing)]
+    _invalid_cargo_features: Option<InvalidCargoFeatures>,
+
     // Note that this field must come last due to the way toml serialization
     // works which requires tables to be emitted after all values.
     metadata: Option<toml::Value>,
@@ -3547,3 +3532,20 @@ impl TomlLintLevel {
         }
     }
 }
+
+#[derive(Copy, Clone, Debug)]
+#[non_exhaustive]
+struct InvalidCargoFeatures {}
+
+impl<'de> de::Deserialize<'de> for InvalidCargoFeatures {
+    fn deserialize<D>(_d: D) -> Result<Self, D::Error>
+    where
+        D: de::Deserializer<'de>,
+    {
+        use serde::de::Error as _;
+
+        Err(D::Error::custom(
+            "the field `cargo-features` should be set at the top of Cargo.toml before any tables",
+        ))
+    }
+}

tests/testsuite/bad_config.rs

@@ -171,9 +171,6 @@ fn invalid_global_config() {
 Caused by:
   could not parse TOML configuration in `[..]`
 
-Caused by:
-  could not parse input as TOML
-
 Caused by:
   TOML parse error at line 1, column 2
     |
@@ -199,8 +196,11 @@ fn bad_cargo_lock() {
 [ERROR] failed to parse lock file at: [..]Cargo.lock
 
 Caused by:
+  TOML parse error at line 1, column 1
+    |
+  1 | [[package]]
+    | ^^^^^^^^^^^
   missing field `name`
-  in `package`
 ",
         )
         .run();
@@ -303,8 +303,11 @@ fn bad_source_in_cargo_lock() {
 [ERROR] failed to parse lock file at: [..]
 
 Caused by:
+  TOML parse error at line 12, column 26
+     |
+  12 |                 source = \"You shall not parse\"
+     |                          ^^^^^^^^^^^^^^^^^^^^^
   invalid source `You shall not parse`
-  in `package.source`
 ",
         )
         .run();
@@ -448,9 +451,6 @@ fn malformed_override() {
             "\
 [ERROR] failed to parse manifest at `[..]`
 
-Caused by:
-  could not parse input as TOML
-
 Caused by:
   TOML parse error at line 8, column 27
     |
@@ -803,9 +803,6 @@ error: could not load Cargo configuration
 Caused by:
   could not parse TOML configuration in `[..]`
 
-Caused by:
-  could not parse input as TOML
-
 Caused by:
   TOML parse error at line 1, column 7
     |
@@ -1288,8 +1285,11 @@ fn bad_dependency() {
 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 [..]
-  in `dependencies.bar`
 ",
         )
         .run();
@@ -1320,8 +1320,11 @@ fn bad_debuginfo() {
 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\"
-  in `profile.dev.debug`
 ",
         )
         .run();
@@ -1352,8 +1355,11 @@ fn bad_debuginfo2() {
 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\"
-  in `profile.dev.debug`
 ",
         )
         .run();
@@ -1382,8 +1388,11 @@ fn bad_opt_level() {
 error: failed to parse manifest at `[..]`
 
 Caused by:
+  TOML parse error at line 6, column 25
+    |
+  6 |                 build = 3
+    |                         ^
   expected a boolean or a string
-  in `package.build`
 ",
         )
         .run();

tests/testsuite/build.rs

@@ -258,9 +258,6 @@ fn cargo_compile_with_invalid_manifest2() {
             "\
 [ERROR] failed to parse manifest at `[..]`
 
-Caused by:
-  could not parse input as TOML
-
 Caused by:
   TOML parse error at line 3, column 23
     |
@@ -283,9 +280,6 @@ fn cargo_compile_with_invalid_manifest3() {
             "\
 [ERROR] failed to parse manifest at `[..]`
 
-Caused by:
-  could not parse input as TOML
-
 Caused by:
   TOML parse error at line 1, column 5
     |
@@ -346,8 +340,11 @@ fn cargo_compile_with_invalid_version() {
 [ERROR] failed to parse manifest at `[..]`
 
 Caused by:
+  TOML parse error at line 4, column 19
+    |
+  4 |         version = \"1.0\"
+    |                   ^^^^^
   unexpected end of input while parsing minor version number
-  in `package.version`
 ",
         )
         .run();
@@ -3035,9 +3032,6 @@ fn bad_cargo_config() {
 Caused by:
   could not parse TOML configuration in `[..]`
 
-Caused by:
-  could not parse input as TOML
-
 Caused by:
   TOML parse error at line 1, column 6
     |

tests/testsuite/cargo_add/invalid_manifest/stderr.log

@@ -1,8 +1,5 @@
 error: failed to parse manifest at `[ROOT]/case/Cargo.toml`
 
-Caused by:
-  could not parse input as TOML
-
 Caused by:
   TOML parse error at line 8, column 7
     |

tests/testsuite/cargo_alias_config.rs

@@ -52,9 +52,6 @@ fn alias_malformed_config_string() {
 Caused by:
   could not parse TOML configuration in `[..]/config`
 
-Caused by:
-  [..]
-
 Caused by:
   TOML parse error at line [..]
     |

tests/testsuite/cargo_features.rs

@@ -676,8 +676,11 @@ fn wrong_position() {
 error: failed to parse manifest at [..]
 
 Caused by:
-  cargo-features = [\"test-dummy-unstable\"] was found in the wrong location: it \
-  should be set at the top of Cargo.toml before any tables
+  TOML parse error at line 5, column 34
+    |
+  5 |                 cargo-features = [\"test-dummy-unstable\"]
+    |                                  ^^^^^^^^^^^^^^^^^^^^^^^
+  the field `cargo-features` should be set at the top of Cargo.toml before any tables
 ",
         )
         .run();

tests/testsuite/config.rs

@@ -721,9 +721,6 @@ could not load Cargo configuration
 Caused by:
   could not parse TOML configuration in `[..]/.cargo/config`
 
-Caused by:
-  could not parse input as TOML
-
 Caused by:
   TOML parse error at line 1, column 5
   |
@@ -1090,9 +1087,6 @@ could not load Cargo configuration
 Caused by:
   could not parse TOML configuration in `[..]/.cargo/config`
 
-Caused by:
-  could not parse input as TOML
-
 Caused by:
   TOML parse error at line 3, column 1
   |

tests/testsuite/git.rs

@@ -2577,9 +2577,6 @@ Caused by:
 Caused by:
   failed to parse manifest at `[..]`
 
-Caused by:
-  could not parse input as TOML
-
 Caused by:
   TOML parse error at line 8, column 21
     |

tests/testsuite/inheritable_workspace_fields.rs

@@ -1234,8 +1234,11 @@ fn error_workspace_false() {
 [ERROR] failed to parse manifest at `[CWD]/Cargo.toml`
 
 Caused by:
+  TOML parse error at line 7, column 41
+    |
+  7 |             description = { workspace = false }
+    |                                         ^^^^^
   `workspace` cannot be false
-  in `package.description.workspace`
 ",
         )
         .run();
@@ -1321,9 +1324,6 @@ fn error_malformed_workspace_root() {
             "\
 [ERROR] failed to parse manifest at `[..]/foo/Cargo.toml`
 
-Caused by:
-  [..]
-
 Caused by:
   [..]
     |

tests/testsuite/metadata.rs

@@ -1821,8 +1821,11 @@ fn cargo_metadata_with_invalid_authors_field() {
             r#"[ERROR] failed to parse manifest at `[..]`
 
 Caused by:
-  invalid type: string "", expected a vector of strings or workspace
-  in `package.authors`"#,
+  TOML parse error at line 3, column 27
+    |
+  3 |                 authors = ""
+    |                           ^^
+  invalid type: string "", expected a vector of strings or workspace"#,
         )
         .run();
 }
@@ -1846,8 +1849,11 @@ fn cargo_metadata_with_invalid_version_field() {
             r#"[ERROR] failed to parse manifest at `[..]`
 
 Caused by:
-  invalid type: integer `1`, expected SemVer version
-  in `package.version`"#,
+  TOML parse error at line 3, column 27
+    |
+  3 |                 version = 1
+    |                           ^
+  invalid type: integer `1`, expected SemVer version"#,
         )
         .run();
 }
@@ -1871,8 +1877,11 @@ fn cargo_metadata_with_invalid_publish_field() {
             r#"[ERROR] failed to parse manifest at `[..]`
 
 Caused by:
-  invalid type: string "foo", expected a boolean, a vector of strings, or workspace
-  in `package.publish`"#,
+  TOML parse error at line 3, column 27
+    |
+  3 |                 publish = "foo"
+    |                           ^^^^^
+  invalid type: string "foo", expected a boolean, a vector of strings, or workspace"#,
         )
         .run();
 }

tests/testsuite/workspaces.rs

@@ -1090,9 +1090,6 @@ fn new_warning_with_corrupt_ws() {
 
 failed to parse manifest at `[..]foo/Cargo.toml`
 
-Caused by:
-  could not parse input as TOML
-
 Caused by:
   TOML parse error at line 1, column 5
     |