Auto merge of #13589 - epage:toml, r=Muscraft

refactor(toml): Flatten manifest parsing

### What does this PR try to resolve?

This is just a clean up but the goals are
- Support diagnostics that show source by tracking `&str`, `ImDocument`, etc in `Manifest` by making each accessible in the creation of a `Manifest`
- Defer warning analysis until we know what is a local vs non-local workspace by refactoring warnings out into a dedicated step
- Centralize the logic for `cargo publish` stripping of dev-dependencies and their feature activations by allowing a `Summary` to be created from any "resolved" `TomlManifest`
- Enumerate all build targets in the "resolved" `TomlManifest` so they get included in `cargo publish`, reducing the work done on registry dependencies and resolving problems like #13456

Along the way, this fixed a bug where we were not reporting warnings from virtual manifests

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

### Additional information

Author: bors

Cargo.lock

@@ -33,7 +33,7 @@ cargo-platform = { path = "crates/cargo-platform", version = "0.1.5" }
 cargo-test-macro = { path = "crates/cargo-test-macro" }
 cargo-test-support = { path = "crates/cargo-test-support" }
 cargo-util = { version = "0.2.9", path = "crates/cargo-util" }
-cargo-util-schemas = { version = "0.2.1", path = "crates/cargo-util-schemas" }
+cargo-util-schemas = { version = "0.3.0", path = "crates/cargo-util-schemas" }
 cargo_metadata = "0.18.1"
 clap = "4.5.1"
 color-print = "0.3.5"
@@ -97,7 +97,7 @@ tempfile = "3.10.1"
 thiserror = "1.0.57"
 time = { version = "0.3", features = ["parsing", "formatting", "serde"] }
 toml = "0.8.10"
-toml_edit = { version = "0.22.6", features = ["serde"] }
+toml_edit = { version = "0.22.7", features = ["serde"] }
 tracing = "0.1.40" # be compatible with rustc_log: https://github.com/rust-lang/rust/blob/e51e98dde6a/compiler/rustc_log/Cargo.toml#L9
 tracing-chrome = "0.7.1"
 tracing-subscriber = { version = "0.3.18", features = ["env-filter"] }

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "cargo-util-schemas"
-version = "0.2.1"
+version = "0.3.0"
 rust-version = "1.76.0"  # MSRV:1
 edition.workspace = true
 license.workspace = true

crates/cargo-util-schemas/Cargo.toml

@@ -6,6 +6,7 @@
 //! - Keys that exist for bookkeeping but don't correspond to the schema have a `_` prefix
 
 use std::collections::BTreeMap;
+use std::collections::BTreeSet;
 use std::fmt::{self, Display, Write};
 use std::path::PathBuf;
 use std::str;
@@ -28,6 +29,7 @@ pub use rust_version::RustVersionError;
 #[derive(Debug, Deserialize, Serialize)]
 #[serde(rename_all = "kebab-case")]
 pub struct TomlManifest {
+    // when adding new fields, be sure to check whether `requires_package` should disallow them
     pub cargo_features: Option<Vec<String>>,
     pub package: Option<Box<TomlPackage>>,
     pub project: Option<Box<TomlPackage>>,
@@ -51,7 +53,11 @@ pub struct TomlManifest {
     pub workspace: Option<TomlWorkspace>,
     pub badges: Option<InheritableBtreeMap>,
     pub lints: Option<InheritableLints>,
-    // when adding new fields, be sure to check whether `requires_package` should disallow them
+
+    /// Report unused keys (see also nested `_unused_keys`)
+    /// Note: this is populated by the caller, rather than automatically
+    #[serde(skip)]
+    pub _unused_keys: BTreeSet<String>,
 }
 
 impl TomlManifest {

crates/cargo-util-schemas/src/manifest/mod.rs

@@ -15,7 +15,7 @@
 use std::fmt::Write as _;
 use std::path::PathBuf;
 use std::process;
-use toml_edit::Document;
+use toml_edit::DocumentMut;
 
 fn main() {
     let pkg_root = std::env!("CARGO_MANIFEST_DIR");
@@ -31,7 +31,7 @@ fn main() {
     let mut passed = 0;
 
     let toml = std::fs::read_to_string(path).expect("read from file");
-    let doc = toml.parse::<Document>().expect("a toml");
+    let doc = toml.parse::<DocumentMut>().expect("a toml");
     let autolabel = doc["autolabel"].as_table().expect("a toml table");
 
     for (label, value) in autolabel.iter() {

crates/xtask-stale-label/src/main.rs

@@ -160,7 +160,7 @@ fn parse_section(args: &ArgMatches) -> DepTable {
 /// Clean up the workspace.dependencies, profile, patch, and replace sections of the root manifest
 /// by removing dependencies which no longer have a reference to them.
 fn gc_workspace(workspace: &Workspace<'_>) -> CargoResult<()> {
-    let mut manifest: toml_edit::Document =
+    let mut manifest: toml_edit::DocumentMut =
         cargo_util::paths::read(workspace.root_manifest())?.parse()?;
     let mut is_modified = true;
 
@@ -315,7 +315,7 @@ fn spec_has_match(
 
 /// Removes unused patches from the manifest
 fn gc_unused_patches(workspace: &Workspace<'_>, resolve: &Resolve) -> CargoResult<bool> {
-    let mut manifest: toml_edit::Document =
+    let mut manifest: toml_edit::DocumentMut =
         cargo_util::paths::read(workspace.root_manifest())?.parse()?;
     let mut modified = false;
 

src/bin/cargo/commands/remove.rs

@@ -41,7 +41,11 @@ impl EitherManifest {
 /// This is deserialized using the [`TomlManifest`] type.
 #[derive(Clone, Debug)]
 pub struct Manifest {
+    // alternate forms of manifests:
+    resolved_toml: Rc<TomlManifest>,
     summary: Summary,
+
+    // this form of manifest:
     targets: Vec<Target>,
     default_kind: Option<CompileKind>,
     forced_kind: Option<CompileKind>,
@@ -56,7 +60,6 @@ pub struct Manifest {
     replace: Vec<(PackageIdSpec, Dependency)>,
     patch: HashMap<Url, Vec<Dependency>>,
     workspace: WorkspaceConfig,
-    original: Rc<TomlManifest>,
     unstable_features: Features,
     edition: Edition,
     rust_version: Option<RustVersion>,
@@ -386,7 +389,9 @@ compact_debug! {
 
 impl Manifest {
     pub fn new(
+        resolved_toml: Rc<TomlManifest>,
         summary: Summary,
+
         default_kind: Option<CompileKind>,
         forced_kind: Option<CompileKind>,
         targets: Vec<Target>,
@@ -405,14 +410,15 @@ impl Manifest {
         rust_version: Option<RustVersion>,
         im_a_teapot: Option<bool>,
         default_run: Option<String>,
-        original: Rc<TomlManifest>,
         metabuild: Option<Vec<String>>,
         resolve_behavior: Option<ResolveBehavior>,
         lint_rustflags: Vec<String>,
         embedded: bool,
     ) -> Manifest {
         Manifest {
+            resolved_toml,
             summary,
+
             default_kind,
             forced_kind,
             targets,
@@ -430,7 +436,6 @@ impl Manifest {
             unstable_features,
             edition,
             rust_version,
-            original,
             im_a_teapot,
             default_run,
             metabuild,
@@ -495,8 +500,8 @@ impl Manifest {
     pub fn replace(&self) -> &[(PackageIdSpec, Dependency)] {
         &self.replace
     }
-    pub fn original(&self) -> &TomlManifest {
-        &self.original
+    pub fn resolved_toml(&self) -> &TomlManifest {
+        &self.resolved_toml
     }
     pub fn patch(&self) -> &HashMap<Url, Vec<Dependency>> {
         &self.patch

src/cargo/core/manifest.rs

@@ -198,7 +198,7 @@ impl Package {
     }
 
     pub fn to_registry_toml(&self, ws: &Workspace<'_>) -> CargoResult<String> {
-        let manifest = prepare_for_publish(self.manifest().original(), ws, self.root())?;
+        let manifest = prepare_for_publish(self.manifest().resolved_toml(), ws, self.root())?;
         let toml = toml::to_string_pretty(&manifest)?;
         Ok(format!("{}\n{}", MANIFEST_PREAMBLE, toml))
     }

src/cargo/core/package.rs

@@ -420,7 +420,6 @@ impl<'gctx> Workspace<'gctx> {
         let source = SourceId::for_path(self.root())?;
 
         let mut warnings = Vec::new();
-        let mut nested_paths = Vec::new();
 
         let mut patch = HashMap::new();
         for (url, deps) in config_patch.into_iter().flatten() {
@@ -442,7 +441,6 @@ impl<'gctx> Workspace<'gctx> {
                             dep,
                             name,
                             source,
-                            &mut nested_paths,
                             self.gctx,
                             &mut warnings,
                             /* platform */ None,
@@ -1006,7 +1004,7 @@ impl<'gctx> Workspace<'gctx> {
                     );
                     self.gctx.shell().warn(&msg)
                 };
-                if manifest.original().has_profiles() {
+                if manifest.resolved_toml().has_profiles() {
                     emit_warning("profiles")?;
                 }
                 if !manifest.replace().is_empty() {
@@ -1063,7 +1061,7 @@ impl<'gctx> Workspace<'gctx> {
             return Ok(p);
         }
         let source_id = SourceId::for_path(manifest_path.parent().unwrap())?;
-        let (package, _nested_paths) = ops::read_package(manifest_path, source_id, self.gctx)?;
+        let package = ops::read_package(manifest_path, source_id, self.gctx)?;
         loaded.insert(manifest_path.to_path_buf(), package.clone());
         Ok(package)
     }
@@ -1596,7 +1594,7 @@ impl<'gctx> Packages<'gctx> {
             Entry::Occupied(e) => Ok(e.into_mut()),
             Entry::Vacant(v) => {
                 let source_id = SourceId::for_path(key)?;
-                let (manifest, _nested_paths) = read_manifest(manifest_path, source_id, self.gctx)?;
+                let manifest = read_manifest(manifest_path, source_id, self.gctx)?;
                 Ok(v.insert(match manifest {
                     EitherManifest::Real(manifest) => {
                         MaybePackage::Package(Package::new(manifest, manifest_path))
@@ -1746,7 +1744,7 @@ pub fn find_workspace_root(
     find_workspace_root_with_loader(manifest_path, gctx, |self_path| {
         let key = self_path.parent().unwrap();
         let source_id = SourceId::for_path(key)?;
-        let (manifest, _nested_paths) = read_manifest(self_path, source_id, gctx)?;
+        let manifest = read_manifest(self_path, source_id, gctx)?;
         Ok(manifest
             .workspace_config()
             .get_ws_root(self_path, manifest_path))

src/cargo/core/workspace.rs

@@ -768,7 +768,7 @@ fn mk(gctx: &GlobalContext, opts: &MkOptions<'_>) -> CargoResult<()> {
     write_ignore_file(path, &ignore, vcs)?;
 
     // Create `Cargo.toml` file with necessary `[lib]` and `[[bin]]` sections, if needed.
-    let mut manifest = toml_edit::Document::new();
+    let mut manifest = toml_edit::DocumentMut::new();
     manifest["package"] = toml_edit::Item::Table(toml_edit::Table::new());
     manifest["package"]["name"] = toml_edit::value(name);
     manifest["package"]["version"] = toml_edit::value("0.1.0");
@@ -814,7 +814,7 @@ fn mk(gctx: &GlobalContext, opts: &MkOptions<'_>) -> CargoResult<()> {
         // Sometimes the root manifest is not a valid manifest, so we only try to parse it if it is.
         // This should not block the creation of the new project. It is only a best effort to
         // inherit the workspace package keys.
-        if let Ok(mut workspace_document) = root_manifest.parse::<toml_edit::Document>() {
+        if let Ok(mut workspace_document) = root_manifest.parse::<toml_edit::DocumentMut>() {
             let display_path = get_display_path(&root_manifest_path, &path)?;
             let can_be_a_member = can_be_workspace_member(&display_path, &workspace_document)?;
             // Only try to inherit the workspace stuff if the new package can be a member of the workspace.
@@ -933,14 +933,14 @@ mod tests {
 // If the option is set, keep the value from the manifest.
 fn update_manifest_with_inherited_workspace_package_keys(
     opts: &MkOptions<'_>,
-    manifest: &mut toml_edit::Document,
+    manifest: &mut toml_edit::DocumentMut,
     workspace_package_keys: &toml_edit::Table,
 ) {
     if workspace_package_keys.is_empty() {
         return;
     }
 
-    let try_remove_and_inherit_package_key = |key: &str, manifest: &mut toml_edit::Document| {
+    let try_remove_and_inherit_package_key = |key: &str, manifest: &mut toml_edit::DocumentMut| {
         let package = manifest["package"]
             .as_table_mut()
             .expect("package is a table");
@@ -974,7 +974,7 @@ fn update_manifest_with_inherited_workspace_package_keys(
 /// with the new package in it.
 fn update_manifest_with_new_member(
     root_manifest_path: &Path,
-    workspace_document: &mut toml_edit::Document,
+    workspace_document: &mut toml_edit::DocumentMut,
     display_path: &str,
 ) -> CargoResult<bool> {
     // If the members element already exist, check if one of the patterns
@@ -1048,7 +1048,7 @@ fn get_display_path(root_manifest_path: &Path, package_path: &Path) -> CargoResu
 // Check if the package can be a member of the workspace.
 fn can_be_workspace_member(
     display_path: &str,
-    workspace_document: &toml_edit::Document,
+    workspace_document: &toml_edit::DocumentMut,
 ) -> CargoResult<bool> {
     if let Some(exclude) = workspace_document
         .get("workspace")

src/cargo/ops/cargo_new.rs

@@ -453,11 +453,10 @@ fn build_lock(ws: &Workspace<'_>, orig_pkg: &Package) -> CargoResult<String> {
     let orig_resolve = ops::load_pkg_lockfile(ws)?;
 
     // Convert Package -> TomlManifest -> Manifest -> Package
-    let toml_manifest = prepare_for_publish(orig_pkg.manifest().original(), ws, orig_pkg.root())?;
-    let package_root = orig_pkg.root();
+    let toml_manifest =
+        prepare_for_publish(orig_pkg.manifest().resolved_toml(), ws, orig_pkg.root())?;
     let source_id = orig_pkg.package_id().source_id();
-    let (manifest, _nested_paths) =
-        to_real_manifest(toml_manifest, false, source_id, package_root, gctx)?;
+    let manifest = to_real_manifest(toml_manifest, source_id, orig_pkg.manifest_path(), gctx)?;
     let new_pkg = Package::new(manifest, orig_pkg.manifest_path());
 
     // Regenerate Cargo.lock using the old one as a guide.