From c8fa7096d46e172c3cd50a4515c9d2fae9be2c17 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 14 Mar 2024 15:37:32 -0500 Subject: [PATCH 01/19] refactor: Group alt Manifest formats This is prep for adding more --- src/cargo/core/manifest.rs | 11 ++++++++--- src/cargo/util/toml/mod.rs | 2 +- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/cargo/core/manifest.rs b/src/cargo/core/manifest.rs index 9f4eb29ada5..5301be1c512 100644 --- a/src/cargo/core/manifest.rs +++ b/src/cargo/core/manifest.rs @@ -41,7 +41,11 @@ impl EitherManifest { /// This is deserialized using the [`TomlManifest`] type. #[derive(Clone, Debug)] pub struct Manifest { + // alternate forms of manifests: + original: Rc, summary: Summary, + + // this form of manifest: targets: Vec, default_kind: Option, forced_kind: Option, @@ -56,7 +60,6 @@ pub struct Manifest { replace: Vec<(PackageIdSpec, Dependency)>, patch: HashMap>, workspace: WorkspaceConfig, - original: Rc, unstable_features: Features, edition: Edition, rust_version: Option, @@ -386,7 +389,9 @@ compact_debug! { impl Manifest { pub fn new( + original: Rc, summary: Summary, + default_kind: Option, forced_kind: Option, targets: Vec, @@ -405,14 +410,15 @@ impl Manifest { rust_version: Option, im_a_teapot: Option, default_run: Option, - original: Rc, metabuild: Option>, resolve_behavior: Option, lint_rustflags: Vec, embedded: bool, ) -> Manifest { Manifest { + original, summary, + default_kind, forced_kind, targets, @@ -430,7 +436,6 @@ impl Manifest { unstable_features, edition, rust_version, - original, im_a_teapot, default_run, metabuild, diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 7470c333db5..b5db008c97b 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -1214,6 +1214,7 @@ pub fn to_real_manifest( }), }; let mut manifest = Manifest::new( + Rc::new(resolved_toml), summary, default_kind, forced_kind, @@ -1233,7 +1234,6 @@ pub fn to_real_manifest( rust_version, package.im_a_teapot, package.default_run.clone(), - Rc::new(resolved_toml), package.metabuild.clone().map(|sov| sov.0), resolve_behavior, rustflags, From 89f1cce439ecc218d177f602d1f367e702a246f1 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 14 Mar 2024 15:40:02 -0500 Subject: [PATCH 02/19] refactor: Clarify what Manifest::original means I plan to add a true `original` --- src/cargo/core/manifest.rs | 10 +++++----- src/cargo/core/package.rs | 2 +- src/cargo/core/workspace.rs | 2 +- src/cargo/ops/cargo_package.rs | 3 ++- src/cargo/ops/registry/publish.rs | 2 +- src/cargo/ops/vendor.rs | 2 +- 6 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/cargo/core/manifest.rs b/src/cargo/core/manifest.rs index 5301be1c512..0deda834bbf 100644 --- a/src/cargo/core/manifest.rs +++ b/src/cargo/core/manifest.rs @@ -42,7 +42,7 @@ impl EitherManifest { #[derive(Clone, Debug)] pub struct Manifest { // alternate forms of manifests: - original: Rc, + resolved_toml: Rc, summary: Summary, // this form of manifest: @@ -389,7 +389,7 @@ compact_debug! { impl Manifest { pub fn new( - original: Rc, + resolved_toml: Rc, summary: Summary, default_kind: Option, @@ -416,7 +416,7 @@ impl Manifest { embedded: bool, ) -> Manifest { Manifest { - original, + resolved_toml, summary, default_kind, @@ -500,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> { &self.patch diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs index d9151e1a9a6..59b342c53be 100644 --- a/src/cargo/core/package.rs +++ b/src/cargo/core/package.rs @@ -198,7 +198,7 @@ impl Package { } pub fn to_registry_toml(&self, ws: &Workspace<'_>) -> CargoResult { - 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)) } diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index d4be9a43a11..5291cbdbd4b 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -1006,7 +1006,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() { diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index 4e6c0585dd3..06b0d1366eb 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -453,7 +453,8 @@ fn build_lock(ws: &Workspace<'_>, orig_pkg: &Package) -> CargoResult { 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 toml_manifest = + prepare_for_publish(orig_pkg.manifest().resolved_toml(), ws, orig_pkg.root())?; let package_root = orig_pkg.root(); let source_id = orig_pkg.package_id().source_id(); let (manifest, _nested_paths) = diff --git a/src/cargo/ops/registry/publish.rs b/src/cargo/ops/registry/publish.rs index 65b9a960730..5615825f3fc 100644 --- a/src/cargo/ops/registry/publish.rs +++ b/src/cargo/ops/registry/publish.rs @@ -420,7 +420,7 @@ fn transmit( .map(|dep| dep.name.clone()) .collect::>(); - let string_features = match manifest.original().features() { + let string_features = match manifest.resolved_toml().features() { Some(features) => features .iter() .map(|(feat, values)| { diff --git a/src/cargo/ops/vendor.rs b/src/cargo/ops/vendor.rs index e785c6c4096..4413786feb2 100644 --- a/src/cargo/ops/vendor.rs +++ b/src/cargo/ops/vendor.rs @@ -363,7 +363,7 @@ fn cp_sources( let cksum = if dst.file_name() == Some(OsStr::new("Cargo.toml")) && pkg.package_id().source_id().is_git() { - let original_toml = toml::to_string_pretty(pkg.manifest().original())?; + let original_toml = toml::to_string_pretty(pkg.manifest().resolved_toml())?; let contents = format!("{}\n{}", MANIFEST_PREAMBLE, original_toml); copy_and_checksum( &dst, From 69ee34a274012929f2bd03c46a2e29035c5ed6f0 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 14 Mar 2024 15:46:11 -0500 Subject: [PATCH 03/19] refactor(toml): Pull fs out as explicit step So diagnostics can use this for reporting. --- src/cargo/util/toml/mod.rs | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index b5db008c97b..842a47499a1 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -54,20 +54,9 @@ pub fn read_manifest( path.display(), source_id ); - let mut contents = paths::read(path).map_err(|err| ManifestError::new(err, path.into()))?; - let embedded = is_embedded(path); - if embedded { - if !gctx.cli_unstable().script { - return Err(ManifestError::new( - anyhow::anyhow!("parsing `{}` requires `-Zscript`", path.display()), - path.into(), - ))?; - } - contents = embedded::expand_manifest(&contents, path, gctx) - .map_err(|err| ManifestError::new(err, path.into()))?; - } + let contents = read_toml_string(path, gctx)?; - read_manifest_from_str(&contents, path, embedded, source_id, gctx).map_err(|err| { + read_manifest_from_str(&contents, path, source_id, gctx).map_err(|err| { if err.is::() { err } else { @@ -80,6 +69,22 @@ pub fn read_manifest( }) } +#[tracing::instrument(skip_all)] +fn read_toml_string(path: &Path, gctx: &GlobalContext) -> CargoResult { + let mut contents = paths::read(path).map_err(|err| ManifestError::new(err, path.into()))?; + if is_embedded(path) { + if !gctx.cli_unstable().script { + return Err(ManifestError::new( + anyhow::anyhow!("parsing `{}` requires `-Zscript`", path.display()), + path.into(), + ))?; + } + contents = embedded::expand_manifest(&contents, path, gctx) + .map_err(|err| ManifestError::new(err, path.into()))?; + } + Ok(contents) +} + /// See also `bin/cargo/commands/run.rs`s `is_manifest_command` pub fn is_embedded(path: &Path) -> bool { let ext = path.extension(); @@ -99,7 +104,6 @@ pub fn is_embedded(path: &Path) -> bool { fn read_manifest_from_str( contents: &str, manifest_file: &Path, - embedded: bool, source_id: SourceId, gctx: &GlobalContext, ) -> CargoResult<(EitherManifest, Vec)> { @@ -186,6 +190,7 @@ fn read_manifest_from_str( } } return if manifest.project.is_some() || manifest.package.is_some() { + let embedded = is_embedded(manifest_file); let (mut manifest, paths) = to_real_manifest(manifest, embedded, source_id, package_root, gctx)?; add_unused(manifest.warnings_mut()); From 490583a573c32b0ab79c8e2e92b4c61270939c21 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 14 Mar 2024 15:53:29 -0500 Subject: [PATCH 04/19] refactor(toml): Pull out diagnostic emitting --- src/cargo/util/toml/mod.rs | 111 ++++++++++++++++++++----------------- 1 file changed, 59 insertions(+), 52 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 842a47499a1..288cde63c43 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -93,6 +93,62 @@ pub fn is_embedded(path: &Path) -> bool { (ext.is_none() && path.is_file()) } +fn emit_diagnostic( + e: toml::de::Error, + contents: &str, + manifest_file: &Path, + gctx: &GlobalContext, +) -> anyhow::Error { + let Some(span) = e.span() else { + return 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.saturating_sub(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, gctx.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(); + if let Err(err) = writeln!(gctx.shell().err(), "{}", renderer.render(snippet)) { + return err.into(); + } + return AlreadyPrintedError::new(e.into()).into(); +} + /// Parse an already-loaded `Cargo.toml` as a Cargo manifest. /// /// This could result in a real or virtual manifest being returned. @@ -111,61 +167,12 @@ fn read_manifest_from_str( let mut unused = BTreeSet::new(); let deserializer = toml::de::Deserializer::new(contents); - let manifest: manifest::TomlManifest = match serde_ignored::deserialize(deserializer, |path| { + let manifest: manifest::TomlManifest = 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.saturating_sub(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, gctx.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!(gctx.shell().err(), "{}", renderer.render(snippet))?; - return Err(AlreadyPrintedError::new(e.into()).into()); - } - }; + }) + .map_err(|e| emit_diagnostic(e, contents, manifest_file, gctx))?; let add_unused = |warnings: &mut Warnings| { for key in unused { warnings.add_warning(format!("unused manifest key: {}", key)); From 66b19adaa00b1a59cd153468df01cfb58b814755 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 14 Mar 2024 15:57:28 -0500 Subject: [PATCH 05/19] refactor(toml): Pull out deserialize code - So we can eventually track the `original` - So we can track spans for diagnostics --- src/cargo/util/toml/mod.rs | 62 ++++++++++++++++++++++---------------- 1 file changed, 36 insertions(+), 26 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 288cde63c43..7bb1eb4acfc 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -85,6 +85,22 @@ fn read_toml_string(path: &Path, gctx: &GlobalContext) -> CargoResult { Ok(contents) } +#[tracing::instrument(skip_all)] +fn deserialize_toml( + contents: &str, + manifest_file: &Path, + unused: &mut BTreeSet, + gctx: &GlobalContext, +) -> CargoResult { + let deserializer = toml::de::Deserializer::new(contents); + serde_ignored::deserialize(deserializer, |path| { + let mut key = String::new(); + stringify(&mut key, &path); + unused.insert(key); + }) + .map_err(|e| emit_diagnostic(e, contents, manifest_file, gctx)) +} + /// See also `bin/cargo/commands/run.rs`s `is_manifest_command` pub fn is_embedded(path: &Path) -> bool { let ext = path.extension(); @@ -166,13 +182,7 @@ fn read_manifest_from_str( let package_root = manifest_file.parent().unwrap(); let mut unused = BTreeSet::new(); - let deserializer = toml::de::Deserializer::new(contents); - let manifest: manifest::TomlManifest = serde_ignored::deserialize(deserializer, |path| { - let mut key = String::new(); - stringify(&mut key, &path); - unused.insert(key); - }) - .map_err(|e| emit_diagnostic(e, contents, manifest_file, gctx))?; + let manifest = deserialize_toml(contents, manifest_file, &mut unused, gctx)?; let add_unused = |warnings: &mut Warnings| { for key in unused { warnings.add_warning(format!("unused manifest key: {}", key)); @@ -214,30 +224,30 @@ fn read_manifest_from_str( add_unused(m.warnings_mut()); Ok((EitherManifest::Virtual(m), paths)) }; +} - fn stringify(dst: &mut String, path: &serde_ignored::Path<'_>) { - use serde_ignored::Path; +fn stringify(dst: &mut String, path: &serde_ignored::Path<'_>) { + use serde_ignored::Path; - match *path { - Path::Root => {} - Path::Seq { parent, index } => { - stringify(dst, parent); - if !dst.is_empty() { - dst.push('.'); - } - dst.push_str(&index.to_string()); + match *path { + Path::Root => {} + Path::Seq { parent, index } => { + stringify(dst, parent); + if !dst.is_empty() { + dst.push('.'); } - Path::Map { parent, ref key } => { - stringify(dst, parent); - if !dst.is_empty() { - dst.push('.'); - } - dst.push_str(key); + dst.push_str(&index.to_string()); + } + Path::Map { parent, ref key } => { + stringify(dst, parent); + if !dst.is_empty() { + dst.push('.'); } - Path::Some { parent } - | Path::NewtypeVariant { parent } - | Path::NewtypeStruct { parent } => stringify(dst, parent), + dst.push_str(key); } + Path::Some { parent } + | Path::NewtypeVariant { parent } + | Path::NewtypeStruct { parent } => stringify(dst, parent), } } From c272cd7e37d7837feae66be89663924c2aeae379 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 14 Mar 2024 16:01:51 -0500 Subject: [PATCH 06/19] refactor(toml): Resolve toml_edit deprecations --- Cargo.lock | 4 ++-- Cargo.toml | 2 +- crates/xtask-stale-label/src/main.rs | 4 ++-- src/bin/cargo/commands/remove.rs | 4 ++-- src/cargo/ops/cargo_new.rs | 12 ++++++------ src/cargo/util/context/mod.rs | 4 ++-- src/cargo/util/toml_mut/manifest.rs | 4 ++-- 7 files changed, 17 insertions(+), 17 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ea8e4354359..58e46e5f203 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3418,9 +3418,9 @@ dependencies = [ [[package]] name = "toml_edit" -version = "0.22.6" +version = "0.22.7" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2c1b5fd4128cc8d3e0cb74d4ed9a9cc7c7284becd4df68f5f940e1ad123606f6" +checksum = "18769cd1cec395d70860ceb4d932812a0b4d06b1a4bb336745a4d21b9496e992" dependencies = [ "indexmap", "serde", diff --git a/Cargo.toml b/Cargo.toml index b5b508c7b7f..c4d04a527e4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -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"] } diff --git a/crates/xtask-stale-label/src/main.rs b/crates/xtask-stale-label/src/main.rs index efcb52d01c1..34590995c69 100644 --- a/crates/xtask-stale-label/src/main.rs +++ b/crates/xtask-stale-label/src/main.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::().expect("a toml"); + let doc = toml.parse::().expect("a toml"); let autolabel = doc["autolabel"].as_table().expect("a toml table"); for (label, value) in autolabel.iter() { diff --git a/src/bin/cargo/commands/remove.rs b/src/bin/cargo/commands/remove.rs index 34e31e9fae4..25179487c93 100644 --- a/src/bin/cargo/commands/remove.rs +++ b/src/bin/cargo/commands/remove.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 { - let mut manifest: toml_edit::Document = + let mut manifest: toml_edit::DocumentMut = cargo_util::paths::read(workspace.root_manifest())?.parse()?; let mut modified = false; diff --git a/src/cargo/ops/cargo_new.rs b/src/cargo/ops/cargo_new.rs index b0bcf6daca4..de3aa052bfa 100644 --- a/src/cargo/ops/cargo_new.rs +++ b/src/cargo/ops/cargo_new.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::() { + if let Ok(mut workspace_document) = root_manifest.parse::() { 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 { // 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 { if let Some(exclude) = workspace_document .get("workspace") diff --git a/src/cargo/util/context/mod.rs b/src/cargo/util/context/mod.rs index 92b6e18279a..da89f40f2dc 100644 --- a/src/cargo/util/context/mod.rs +++ b/src/cargo/util/context/mod.rs @@ -1365,9 +1365,9 @@ impl GlobalContext { // We only want to allow "dotted key" (see https://toml.io/en/v1.0.0#keys) // expressions followed by a value that's not an "inline table" // (https://toml.io/en/v1.0.0#inline-table). Easiest way to check for that is to - // parse the value as a toml_edit::Document, and check that the (single) + // parse the value as a toml_edit::DocumentMut, and check that the (single) // inner-most table is set via dotted keys. - let doc: toml_edit::Document = arg.parse().with_context(|| { + let doc: toml_edit::DocumentMut = arg.parse().with_context(|| { format!("failed to parse value from --config argument `{arg}` as a dotted key expression") })?; fn non_empty(d: Option<&toml_edit::RawString>) -> bool { diff --git a/src/cargo/util/toml_mut/manifest.rs b/src/cargo/util/toml_mut/manifest.rs index 3e3b4e69ae3..1fbfb9decca 100644 --- a/src/cargo/util/toml_mut/manifest.rs +++ b/src/cargo/util/toml_mut/manifest.rs @@ -82,7 +82,7 @@ impl From for DepTable { #[derive(Debug, Clone)] pub struct Manifest { /// Manifest contents as TOML data. - pub data: toml_edit::Document, + pub data: toml_edit::DocumentMut, } impl Manifest { @@ -225,7 +225,7 @@ impl str::FromStr for Manifest { /// Read manifest data from string fn from_str(input: &str) -> ::std::result::Result { - let d: toml_edit::Document = input.parse().context("Manifest not valid TOML")?; + let d: toml_edit::DocumentMut = input.parse().context("Manifest not valid TOML")?; Ok(Manifest { data: d }) } From b1743463bc02af9909cdf116a7a65f61eec3c6ad Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 14 Mar 2024 16:09:40 -0500 Subject: [PATCH 07/19] refactor(toml): Pull out toml parsing So we can track spans for diagnostics --- src/cargo/util/toml/mod.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 7bb1eb4acfc..2de7af3234b 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -85,6 +85,16 @@ fn read_toml_string(path: &Path, gctx: &GlobalContext) -> CargoResult { Ok(contents) } +#[tracing::instrument(skip_all)] +fn parse_document( + contents: &str, + manifest_file: &Path, + gctx: &GlobalContext, +) -> CargoResult> { + toml_edit::ImDocument::parse(contents.to_owned()) + .map_err(|e| emit_diagnostic(e.into(), contents, manifest_file, gctx)) +} + #[tracing::instrument(skip_all)] fn deserialize_toml( contents: &str, @@ -92,7 +102,8 @@ fn deserialize_toml( unused: &mut BTreeSet, gctx: &GlobalContext, ) -> CargoResult { - let deserializer = toml::de::Deserializer::new(contents); + let document = parse_document(contents, manifest_file, gctx)?; + let deserializer = toml_edit::de::Deserializer::from(document); serde_ignored::deserialize(deserializer, |path| { let mut key = String::new(); stringify(&mut key, &path); @@ -110,7 +121,7 @@ pub fn is_embedded(path: &Path) -> bool { } fn emit_diagnostic( - e: toml::de::Error, + e: toml_edit::de::Error, contents: &str, manifest_file: &Path, gctx: &GlobalContext, From 069c67a9aa6367843fd1241fc82daebf54e4948b Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 14 Mar 2024 16:29:15 -0500 Subject: [PATCH 08/19] refactor(toml): Move steps to top-level Before, we split things up. This makes it so everything has access to every step so we can reap the benefits - use `&str` and `ImDocument` for diagnostics - access `original` --- src/cargo/util/toml/mod.rs | 48 ++++++++++++++++---------------------- 1 file changed, 20 insertions(+), 28 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 2de7af3234b..3bc78302631 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -55,17 +55,18 @@ pub fn read_manifest( source_id ); let contents = read_toml_string(path, gctx)?; + let document = + parse_document(&contents).map_err(|e| emit_diagnostic(e.into(), &contents, path, gctx))?; + let mut unused = BTreeSet::new(); + let toml = deserialize_toml(&document, &mut unused) + .map_err(|e| emit_diagnostic(e.into(), &contents, path, gctx))?; - read_manifest_from_str(&contents, path, source_id, gctx).map_err(|err| { - if err.is::() { - err - } else { - ManifestError::new( - err.context(format!("failed to parse manifest at `{}`", path.display())), - path.into(), - ) - .into() - } + convert_toml(toml, unused, path, source_id, gctx).map_err(|err| { + ManifestError::new( + err.context(format!("failed to parse manifest at `{}`", path.display())), + path.into(), + ) + .into() }) } @@ -86,30 +87,21 @@ fn read_toml_string(path: &Path, gctx: &GlobalContext) -> CargoResult { } #[tracing::instrument(skip_all)] -fn parse_document( - contents: &str, - manifest_file: &Path, - gctx: &GlobalContext, -) -> CargoResult> { - toml_edit::ImDocument::parse(contents.to_owned()) - .map_err(|e| emit_diagnostic(e.into(), contents, manifest_file, gctx)) +fn parse_document(contents: &str) -> Result, toml_edit::de::Error> { + toml_edit::ImDocument::parse(contents.to_owned()).map_err(Into::into) } #[tracing::instrument(skip_all)] fn deserialize_toml( - contents: &str, - manifest_file: &Path, + document: &toml_edit::ImDocument, unused: &mut BTreeSet, - gctx: &GlobalContext, -) -> CargoResult { - let document = parse_document(contents, manifest_file, gctx)?; - let deserializer = toml_edit::de::Deserializer::from(document); +) -> Result { + let deserializer = toml_edit::de::Deserializer::from(document.clone()); serde_ignored::deserialize(deserializer, |path| { let mut key = String::new(); stringify(&mut key, &path); unused.insert(key); }) - .map_err(|e| emit_diagnostic(e, contents, manifest_file, gctx)) } /// See also `bin/cargo/commands/run.rs`s `is_manifest_command` @@ -184,16 +176,16 @@ fn emit_diagnostic( /// within the manifest. For virtual manifests, these paths can only /// come from patched or replaced dependencies. These paths are not /// canonicalized. -fn read_manifest_from_str( - contents: &str, +#[tracing::instrument(skip_all)] +fn convert_toml( + manifest: manifest::TomlManifest, + unused: BTreeSet, manifest_file: &Path, source_id: SourceId, gctx: &GlobalContext, ) -> CargoResult<(EitherManifest, Vec)> { let package_root = manifest_file.parent().unwrap(); - let mut unused = BTreeSet::new(); - let manifest = deserialize_toml(contents, manifest_file, &mut unused, gctx)?; let add_unused = |warnings: &mut Warnings| { for key in unused { warnings.add_warning(format!("unused manifest key: {}", key)); From 8cd94e4cc47ff77e38da696d8888938e7c6433e5 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 14 Mar 2024 16:36:09 -0500 Subject: [PATCH 09/19] refactor(toml): Simplify virtual manifest check --- src/cargo/util/toml/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 3bc78302631..d1cb58d7b8a 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -209,7 +209,7 @@ fn convert_toml( } } } - return if manifest.project.is_some() || manifest.package.is_some() { + return if manifest.package().is_some() { let embedded = is_embedded(manifest_file); let (mut manifest, paths) = to_real_manifest(manifest, embedded, source_id, package_root, gctx)?; From a7b70e98f9f8294e8cb3570ee37968edb3054967 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 14 Mar 2024 16:39:21 -0500 Subject: [PATCH 10/19] refactor(toml): Pull out unused warning code --- src/cargo/util/toml/mod.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index d1cb58d7b8a..e23cceac8e9 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -186,15 +186,6 @@ fn convert_toml( ) -> CargoResult<(EitherManifest, Vec)> { let package_root = manifest_file.parent().unwrap(); - let add_unused = |warnings: &mut Warnings| { - for key in unused { - warnings.add_warning(format!("unused manifest key: {}", key)); - if key == "profiles.debug" { - warnings.add_warning("use `[profile.dev]` to configure debug builds".to_string()); - } - } - }; - if let Some(deps) = manifest .workspace .as_ref() @@ -213,7 +204,7 @@ fn convert_toml( let embedded = is_embedded(manifest_file); let (mut manifest, paths) = to_real_manifest(manifest, embedded, source_id, package_root, gctx)?; - add_unused(manifest.warnings_mut()); + warn_on_unused(&unused, manifest.warnings_mut()); if manifest.targets().iter().all(|t| t.is_custom_build()) { bail!( "no targets specified in the manifest\n\ @@ -224,7 +215,7 @@ fn convert_toml( Ok((EitherManifest::Real(manifest), paths)) } else { let (mut m, paths) = to_virtual_manifest(manifest, source_id, package_root, gctx)?; - add_unused(m.warnings_mut()); + warn_on_unused(&unused, m.warnings_mut()); Ok((EitherManifest::Virtual(m), paths)) }; } @@ -263,6 +254,15 @@ fn warn_on_deprecated(new_path: &str, name: &str, kind: &str, warnings: &mut Vec )) } +fn warn_on_unused(unused: &BTreeSet, warnings: &mut Warnings) { + for key in unused { + warnings.add_warning(format!("unused manifest key: {}", key)); + if key == "profiles.debug" { + warnings.add_warning("use `[profile.dev]` to configure debug builds".to_string()); + } + } +} + /// Prepares the manifest for publishing. // - Path and git components of dependency specifications are removed. // - License path is updated to point within the package. From 401c502e6a3c7e80db7e487c78b5af75fe520887 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 14 Mar 2024 20:19:45 -0500 Subject: [PATCH 11/19] refactor(toml): Consolidate no-target errors Moving this out for collapsing `convert_toml` --- src/cargo/util/toml/mod.rs | 17 +++++++---------- tests/testsuite/build.rs | 1 + 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index e23cceac8e9..4eeeb6df833 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -14,7 +14,7 @@ use cargo_util_schemas::manifest::{self, TomlManifest}; use itertools::Itertools; use lazycell::LazyCell; use pathdiff::diff_paths; -use tracing::{debug, trace}; +use tracing::trace; use url::Url; use crate::core::compiler::{CompileKind, CompileTarget}; @@ -205,13 +205,6 @@ fn convert_toml( let (mut manifest, paths) = to_real_manifest(manifest, embedded, source_id, package_root, gctx)?; warn_on_unused(&unused, manifest.warnings_mut()); - if manifest.targets().iter().all(|t| t.is_custom_build()) { - bail!( - "no targets specified in the manifest\n\ - either src/lib.rs, src/main.rs, a [lib] section, or \ - [[bin]] section must be present" - ) - } Ok((EitherManifest::Real(manifest), paths)) } else { let (mut m, paths) = to_virtual_manifest(manifest, source_id, package_root, gctx)?; @@ -747,8 +740,12 @@ pub fn to_real_manifest( &mut errors, )?; - if targets.is_empty() { - debug!("manifest has no build targets"); + if targets.iter().all(|t| t.is_custom_build()) { + bail!( + "no targets specified in the manifest\n\ + either src/lib.rs, src/main.rs, a [lib] section, or \ + [[bin]] section must be present" + ) } if let Err(conflict_targets) = unique_build_targets(&targets, package_root) { diff --git a/tests/testsuite/build.rs b/tests/testsuite/build.rs index c7c751b5052..d506605a853 100644 --- a/tests/testsuite/build.rs +++ b/tests/testsuite/build.rs @@ -711,6 +711,7 @@ fn cargo_compile_with_invalid_non_numeric_dep_version() { crossbeam = "y" "#, ) + .file("src/lib.rs", "") .build(); p.cargo("build") From 65bf96a10bc56f06889f8e7c4b641d75360578fd Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 14 Mar 2024 20:32:04 -0500 Subject: [PATCH 12/19] fix(toml): Remove redundant trace --- src/cargo/util/toml/mod.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 4eeeb6df833..fc1a2922c0e 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -14,7 +14,6 @@ use cargo_util_schemas::manifest::{self, TomlManifest}; use itertools::Itertools; use lazycell::LazyCell; use pathdiff::diff_paths; -use tracing::trace; use url::Url; use crate::core::compiler::{CompileKind, CompileTarget}; @@ -49,11 +48,6 @@ pub fn read_manifest( source_id: SourceId, gctx: &GlobalContext, ) -> CargoResult<(EitherManifest, Vec)> { - trace!( - "read_manifest; path={}; source-id={}", - path.display(), - source_id - ); let contents = read_toml_string(path, gctx)?; let document = parse_document(&contents).map_err(|e| emit_diagnostic(e.into(), &contents, path, gctx))?; From fcc6981d007f4a9f341e5f31837aa666d67b183e Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 14 Mar 2024 20:35:53 -0500 Subject: [PATCH 13/19] refactor(toml): Centralize error wrapping --- src/cargo/util/toml/mod.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index fc1a2922c0e..ef72de67adc 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -48,7 +48,8 @@ pub fn read_manifest( source_id: SourceId, gctx: &GlobalContext, ) -> CargoResult<(EitherManifest, Vec)> { - let contents = read_toml_string(path, gctx)?; + let contents = + read_toml_string(path, 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 mut unused = BTreeSet::new(); @@ -66,16 +67,12 @@ pub fn read_manifest( #[tracing::instrument(skip_all)] fn read_toml_string(path: &Path, gctx: &GlobalContext) -> CargoResult { - let mut contents = paths::read(path).map_err(|err| ManifestError::new(err, path.into()))?; + let mut contents = paths::read(path)?; if is_embedded(path) { if !gctx.cli_unstable().script { - return Err(ManifestError::new( - anyhow::anyhow!("parsing `{}` requires `-Zscript`", path.display()), - path.into(), - ))?; + anyhow::bail!("parsing `{}` requires `-Zscript`", path.display()); } - contents = embedded::expand_manifest(&contents, path, gctx) - .map_err(|err| ManifestError::new(err, path.into()))?; + contents = embedded::expand_manifest(&contents, path, gctx)?; } Ok(contents) } From 714359289d8c66c5f652127db2d1b8292e9cda57 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 14 Mar 2024 20:55:36 -0500 Subject: [PATCH 14/19] fix(toml): Produce warnings for virtual manifests This is prep for shifting unused keys warnings out of `convert_toml` --- src/cargo/util/toml/mod.rs | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index ef72de67adc..bb42e6613d6 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -1345,17 +1345,19 @@ fn to_virtual_manifest( bail!("virtual manifests must be configured with [workspace]"); } }; - Ok(( - VirtualManifest::new( - replace, - patch, - workspace_config, - profiles, - features, - resolve_behavior, - ), - nested_paths, - )) + let mut manifest = VirtualManifest::new( + replace, + patch, + workspace_config, + profiles, + features, + resolve_behavior, + ); + for warning in warnings { + manifest.warnings_mut().add_warning(warning); + } + + Ok((manifest, nested_paths)) } fn replace( From ff454fd452dfee28fac0f18242c4956205b0db79 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 14 Mar 2024 20:56:06 -0500 Subject: [PATCH 15/19] refactor(toml): Consolidate how we track unused keys This makes it act more like everything else, making this easier to evolve over time. --- Cargo.lock | 2 +- Cargo.toml | 2 +- crates/cargo-util-schemas/Cargo.toml | 2 +- crates/cargo-util-schemas/src/manifest/mod.rs | 8 ++++- src/cargo/util/toml/mod.rs | 34 +++++++++++-------- 5 files changed, 29 insertions(+), 19 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 58e46e5f203..df841251d86 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -469,7 +469,7 @@ dependencies = [ [[package]] name = "cargo-util-schemas" -version = "0.2.1" +version = "0.3.0" dependencies = [ "semver", "serde", diff --git a/Cargo.toml b/Cargo.toml index c4d04a527e4..28c475e6eb9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -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" diff --git a/crates/cargo-util-schemas/Cargo.toml b/crates/cargo-util-schemas/Cargo.toml index 1fe4d1a3907..0352997ff23 100644 --- a/crates/cargo-util-schemas/Cargo.toml +++ b/crates/cargo-util-schemas/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 diff --git a/crates/cargo-util-schemas/src/manifest/mod.rs b/crates/cargo-util-schemas/src/manifest/mod.rs index 75f7f628bef..aa438c54c8d 100644 --- a/crates/cargo-util-schemas/src/manifest/mod.rs +++ b/crates/cargo-util-schemas/src/manifest/mod.rs @@ -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>, pub package: Option>, pub project: Option>, @@ -51,7 +53,11 @@ pub struct TomlManifest { pub workspace: Option, pub badges: Option, pub lints: Option, - // 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, } impl TomlManifest { diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index bb42e6613d6..d05b9612e25 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -18,7 +18,7 @@ use url::Url; use crate::core::compiler::{CompileKind, CompileTarget}; use crate::core::dependency::{Artifact, ArtifactTarget, DepKind}; -use crate::core::manifest::{ManifestMetadata, TargetSourcePath, Warnings}; +use crate::core::manifest::{ManifestMetadata, TargetSourcePath}; use crate::core::resolver::ResolveBehavior; use crate::core::{find_workspace_root, resolve_relative_path, CliUnstable, FeatureValue}; use crate::core::{Dependency, Manifest, PackageId, Summary, Target}; @@ -52,11 +52,10 @@ pub fn read_manifest( read_toml_string(path, 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 mut unused = BTreeSet::new(); - let toml = deserialize_toml(&document, &mut unused) + let toml = deserialize_toml(&document) .map_err(|e| emit_diagnostic(e.into(), &contents, path, gctx))?; - convert_toml(toml, unused, path, source_id, gctx).map_err(|err| { + convert_toml(toml, path, source_id, gctx).map_err(|err| { ManifestError::new( err.context(format!("failed to parse manifest at `{}`", path.display())), path.into(), @@ -85,14 +84,16 @@ fn parse_document(contents: &str) -> Result, toml_ #[tracing::instrument(skip_all)] fn deserialize_toml( document: &toml_edit::ImDocument, - unused: &mut BTreeSet, ) -> Result { + let mut unused = BTreeSet::new(); let deserializer = toml_edit::de::Deserializer::from(document.clone()); - serde_ignored::deserialize(deserializer, |path| { + let mut document: manifest::TomlManifest = serde_ignored::deserialize(deserializer, |path| { let mut key = String::new(); stringify(&mut key, &path); unused.insert(key); - }) + })?; + document._unused_keys = unused; + Ok(document) } /// See also `bin/cargo/commands/run.rs`s `is_manifest_command` @@ -170,7 +171,6 @@ fn emit_diagnostic( #[tracing::instrument(skip_all)] fn convert_toml( manifest: manifest::TomlManifest, - unused: BTreeSet, manifest_file: &Path, source_id: SourceId, gctx: &GlobalContext, @@ -193,13 +193,11 @@ fn convert_toml( } return if manifest.package().is_some() { let embedded = is_embedded(manifest_file); - let (mut manifest, paths) = + let (manifest, paths) = to_real_manifest(manifest, embedded, source_id, package_root, gctx)?; - warn_on_unused(&unused, manifest.warnings_mut()); Ok((EitherManifest::Real(manifest), paths)) } else { - let (mut m, paths) = to_virtual_manifest(manifest, source_id, package_root, gctx)?; - warn_on_unused(&unused, m.warnings_mut()); + let (m, paths) = to_virtual_manifest(manifest, source_id, package_root, gctx)?; Ok((EitherManifest::Virtual(m), paths)) }; } @@ -238,11 +236,11 @@ fn warn_on_deprecated(new_path: &str, name: &str, kind: &str, warnings: &mut Vec )) } -fn warn_on_unused(unused: &BTreeSet, warnings: &mut Warnings) { +fn warn_on_unused(unused: &BTreeSet, warnings: &mut Vec) { for key in unused { - warnings.add_warning(format!("unused manifest key: {}", key)); + warnings.push(format!("unused manifest key: {}", key)); if key == "profiles.debug" { - warnings.add_warning("use `[profile.dev]` to configure debug builds".to_string()); + warnings.push("use `[profile.dev]` to configure debug builds".to_string()); } } } @@ -375,6 +373,7 @@ pub fn prepare_for_publish( badges: me.badges.clone(), cargo_features: me.cargo_features.clone(), lints: me.lints.clone(), + _unused_keys: Default::default(), }; strip_features(&mut manifest); return Ok(manifest); @@ -523,6 +522,8 @@ pub fn to_real_manifest( let mut warnings = vec![]; let mut errors = vec![]; + warn_on_unused(&me._unused_keys, &mut warnings); + // Parse features first so they will be available when parsing other parts of the TOML. let empty = Vec::new(); let cargo_features = me.cargo_features.as_ref().unwrap_or(&empty); @@ -1225,6 +1226,7 @@ pub fn to_real_manifest( workspace: false, lints, }), + _unused_keys: Default::default(), }; let mut manifest = Manifest::new( Rc::new(resolved_toml), @@ -1292,6 +1294,8 @@ fn to_virtual_manifest( let cargo_features = me.cargo_features.as_ref().unwrap_or(&empty); let features = Features::new(cargo_features, gctx, &mut warnings, source_id.is_path())?; + warn_on_unused(&me._unused_keys, &mut warnings); + let (replace, patch) = { let mut manifest_ctx = ManifestContext { deps: &mut deps, From 37c3080bea8fdf03e98f3bfa7e11c80cdae57c2e Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 15 Mar 2024 08:00:40 -0500 Subject: [PATCH 16/19] refactor(toml): Move path processing out of convert_toml This is in an effort to remove `convert_toml` --- src/cargo/ops/cargo_package.rs | 3 +-- src/cargo/util/toml/mod.rs | 17 ++++++++--------- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index 06b0d1366eb..a4070900df9 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -455,10 +455,9 @@ fn build_lock(ws: &Workspace<'_>, orig_pkg: &Package) -> CargoResult { // Convert Package -> TomlManifest -> Manifest -> Package let toml_manifest = prepare_for_publish(orig_pkg.manifest().resolved_toml(), ws, orig_pkg.root())?; - let package_root = 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)?; + 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. diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index d05b9612e25..11acf910468 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -175,8 +175,6 @@ fn convert_toml( source_id: SourceId, gctx: &GlobalContext, ) -> CargoResult<(EitherManifest, Vec)> { - let package_root = manifest_file.parent().unwrap(); - if let Some(deps) = manifest .workspace .as_ref() @@ -192,12 +190,10 @@ fn convert_toml( } } return if manifest.package().is_some() { - let embedded = is_embedded(manifest_file); - let (manifest, paths) = - to_real_manifest(manifest, embedded, source_id, package_root, gctx)?; + let (manifest, paths) = to_real_manifest(manifest, source_id, manifest_file, gctx)?; Ok((EitherManifest::Real(manifest), paths)) } else { - let (m, paths) = to_virtual_manifest(manifest, source_id, package_root, gctx)?; + let (m, paths) = to_virtual_manifest(manifest, source_id, manifest_file, gctx)?; Ok((EitherManifest::Virtual(m), paths)) }; } @@ -479,9 +475,8 @@ pub fn prepare_for_publish( #[tracing::instrument(skip_all)] pub fn to_real_manifest( me: manifest::TomlManifest, - embedded: bool, source_id: SourceId, - package_root: &Path, + manifest_file: &Path, gctx: &GlobalContext, ) -> CargoResult<(Manifest, Vec)> { fn get_ws( @@ -511,6 +506,8 @@ pub fn to_real_manifest( } } + let embedded = is_embedded(manifest_file); + let package_root = manifest_file.parent().unwrap(); if !package_root.is_dir() { bail!( "package root '{}' is not a directory", @@ -1280,9 +1277,11 @@ pub fn to_real_manifest( fn to_virtual_manifest( me: manifest::TomlManifest, source_id: SourceId, - root: &Path, + manifest_file: &Path, gctx: &GlobalContext, ) -> CargoResult<(VirtualManifest, Vec)> { + let root = manifest_file.parent().unwrap(); + for field in me.requires_package() { bail!("this virtual manifest specifies a `{field}` section, which is not allowed"); } From 954142e582ee3ccb7728acd62047e2babd881d49 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 15 Mar 2024 08:04:54 -0500 Subject: [PATCH 17/19] refactor(toml): Move path workspace dep validation out of convert_toml This is part of an effort to remove `convert_toml` --- src/cargo/util/toml/mod.rs | 44 ++++++++++++++++++++++++++------------ 1 file changed, 30 insertions(+), 14 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 11acf910468..79571b814db 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -175,20 +175,6 @@ fn convert_toml( source_id: SourceId, gctx: &GlobalContext, ) -> CargoResult<(EitherManifest, Vec)> { - if let Some(deps) = manifest - .workspace - .as_ref() - .and_then(|ws| ws.dependencies.as_ref()) - { - for (name, dep) in deps { - if dep.is_optional() { - bail!("{name} is optional, but workspace dependencies cannot be optional",); - } - if dep.is_public() { - bail!("{name} is public, but workspace dependencies cannot be public",); - } - } - } return if manifest.package().is_some() { let (manifest, paths) = to_real_manifest(manifest, source_id, manifest_file, gctx)?; Ok((EitherManifest::Real(manifest), paths)) @@ -515,6 +501,21 @@ pub fn to_real_manifest( ); }; + if let Some(deps) = me + .workspace + .as_ref() + .and_then(|ws| ws.dependencies.as_ref()) + { + for (name, dep) in deps { + if dep.is_optional() { + bail!("{name} is optional, but workspace dependencies cannot be optional",); + } + if dep.is_public() { + bail!("{name} is public, but workspace dependencies cannot be public",); + } + } + } + let mut nested_paths = vec![]; let mut warnings = vec![]; let mut errors = vec![]; @@ -1282,6 +1283,21 @@ fn to_virtual_manifest( ) -> CargoResult<(VirtualManifest, Vec)> { let root = manifest_file.parent().unwrap(); + if let Some(deps) = me + .workspace + .as_ref() + .and_then(|ws| ws.dependencies.as_ref()) + { + for (name, dep) in deps { + if dep.is_optional() { + bail!("{name} is optional, but workspace dependencies cannot be optional",); + } + if dep.is_public() { + bail!("{name} is public, but workspace dependencies cannot be public",); + } + } + } + for field in me.requires_package() { bail!("this virtual manifest specifies a `{field}` section, which is not allowed"); } From dd56d79c83d71e68cca54665588d9139d5550f26 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 15 Mar 2024 08:18:03 -0500 Subject: [PATCH 18/19] refactor(toml): Only collect nested paths when needed This simplifies the interface for `toml/mod.rs` and reduces the work we do. --- src/cargo/core/workspace.rs | 8 ++--- src/cargo/ops/cargo_package.rs | 3 +- src/cargo/ops/cargo_read_manifest.rs | 49 +++++++++++++++++++++++++--- src/cargo/sources/path.rs | 2 +- src/cargo/util/toml/mod.rs | 30 +++++++---------- 5 files changed, 60 insertions(+), 32 deletions(-) diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index 5291cbdbd4b..e689933492a 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.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, @@ -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)) diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index a4070900df9..1bc8f4309f0 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -456,8 +456,7 @@ fn build_lock(ws: &Workspace<'_>, orig_pkg: &Package) -> CargoResult { 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, source_id, orig_pkg.manifest_path(), 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. diff --git a/src/cargo/ops/cargo_read_manifest.rs b/src/cargo/ops/cargo_read_manifest.rs index 0cfc7bb2b00..e8eaf1b43f9 100644 --- a/src/cargo/ops/cargo_read_manifest.rs +++ b/src/cargo/ops/cargo_read_manifest.rs @@ -3,7 +3,7 @@ use std::fs; use std::io; use std::path::{Path, PathBuf}; -use crate::core::{EitherManifest, Package, PackageId, SourceId}; +use crate::core::{EitherManifest, Manifest, Package, PackageId, SourceId}; use crate::util::errors::CargoResult; use crate::util::important_paths::find_project_manifest_exact; use crate::util::toml::read_manifest; @@ -15,13 +15,13 @@ pub fn read_package( path: &Path, source_id: SourceId, gctx: &GlobalContext, -) -> CargoResult<(Package, Vec)> { +) -> CargoResult { trace!( "read_package; path={}; source-id={}", path.display(), source_id ); - let (manifest, nested) = read_manifest(path, source_id, gctx)?; + let manifest = read_manifest(path, source_id, gctx)?; let manifest = match manifest { EitherManifest::Real(manifest) => manifest, EitherManifest::Virtual(..) => anyhow::bail!( @@ -31,7 +31,7 @@ pub fn read_package( ), }; - Ok((Package::new(manifest, path), nested)) + Ok(Package::new(manifest, path)) } pub fn read_packages( @@ -107,6 +107,44 @@ pub fn read_packages( } } +fn nested_paths(manifest: &Manifest) -> Vec { + let mut nested_paths = Vec::new(); + let resolved = manifest.resolved_toml(); + let dependencies = resolved + .dependencies + .iter() + .chain(resolved.build_dependencies()) + .chain(resolved.dev_dependencies()) + .chain( + resolved + .target + .as_ref() + .into_iter() + .flat_map(|t| t.values()) + .flat_map(|t| { + t.dependencies + .iter() + .chain(t.build_dependencies()) + .chain(t.dev_dependencies()) + }), + ); + for dep_table in dependencies { + for dep in dep_table.values() { + let cargo_util_schemas::manifest::InheritableDependency::Value(dep) = dep else { + continue; + }; + let cargo_util_schemas::manifest::TomlDependency::Detailed(dep) = dep else { + continue; + }; + let Some(path) = dep.path.as_ref() else { + continue; + }; + nested_paths.push(PathBuf::from(path.as_str())); + } + } + nested_paths +} + fn walk(path: &Path, callback: &mut dyn FnMut(&Path) -> CargoResult) -> CargoResult<()> { if !callback(path)? { trace!("not processing {}", path.display()); @@ -151,7 +189,7 @@ fn read_nested_packages( let manifest_path = find_project_manifest_exact(path, "Cargo.toml")?; - let (manifest, nested) = match read_manifest(&manifest_path, source_id, gctx) { + let manifest = match read_manifest(&manifest_path, source_id, gctx) { Err(err) => { // Ignore malformed manifests found on git repositories // @@ -174,6 +212,7 @@ fn read_nested_packages( EitherManifest::Real(manifest) => manifest, EitherManifest::Virtual(..) => return Ok(()), }; + let nested = nested_paths(&manifest); let pkg = Package::new(manifest, &manifest_path); let pkg_id = pkg.package_id(); diff --git a/src/cargo/sources/path.rs b/src/cargo/sources/path.rs index 415298eb7c7..2e4d9042038 100644 --- a/src/cargo/sources/path.rs +++ b/src/cargo/sources/path.rs @@ -110,7 +110,7 @@ impl<'gctx> PathSource<'gctx> { ops::read_packages(&self.path, self.source_id, self.gctx) } else { let path = self.path.join("Cargo.toml"); - let (pkg, _) = ops::read_package(&path, self.source_id, self.gctx)?; + let pkg = ops::read_package(&path, self.source_id, self.gctx)?; Ok(vec![pkg]) } } diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 79571b814db..a6be13aec97 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -47,7 +47,7 @@ pub fn read_manifest( path: &Path, source_id: SourceId, gctx: &GlobalContext, -) -> CargoResult<(EitherManifest, Vec)> { +) -> CargoResult { let contents = read_toml_string(path, gctx).map_err(|err| ManifestError::new(err, path.into()))?; let document = @@ -174,13 +174,13 @@ fn convert_toml( manifest_file: &Path, source_id: SourceId, gctx: &GlobalContext, -) -> CargoResult<(EitherManifest, Vec)> { +) -> CargoResult { return if manifest.package().is_some() { - let (manifest, paths) = to_real_manifest(manifest, source_id, manifest_file, gctx)?; - Ok((EitherManifest::Real(manifest), paths)) + let manifest = to_real_manifest(manifest, source_id, manifest_file, gctx)?; + Ok(EitherManifest::Real(manifest)) } else { - let (m, paths) = to_virtual_manifest(manifest, source_id, manifest_file, gctx)?; - Ok((EitherManifest::Virtual(m), paths)) + let manifest = to_virtual_manifest(manifest, source_id, manifest_file, gctx)?; + Ok(EitherManifest::Virtual(manifest)) }; } @@ -464,7 +464,7 @@ pub fn to_real_manifest( source_id: SourceId, manifest_file: &Path, gctx: &GlobalContext, -) -> CargoResult<(Manifest, Vec)> { +) -> CargoResult { fn get_ws( gctx: &GlobalContext, resolved_path: &Path, @@ -516,7 +516,6 @@ pub fn to_real_manifest( } } - let mut nested_paths = vec![]; let mut warnings = vec![]; let mut errors = vec![]; @@ -770,7 +769,6 @@ pub fn to_real_manifest( let mut manifest_ctx = ManifestContext { deps: &mut deps, source_id, - nested_paths: &mut nested_paths, gctx, warnings: &mut warnings, features: &features, @@ -1272,7 +1270,7 @@ pub fn to_real_manifest( manifest.feature_gate()?; - Ok((manifest, nested_paths)) + Ok(manifest) } fn to_virtual_manifest( @@ -1280,7 +1278,7 @@ fn to_virtual_manifest( source_id: SourceId, manifest_file: &Path, gctx: &GlobalContext, -) -> CargoResult<(VirtualManifest, Vec)> { +) -> CargoResult { let root = manifest_file.parent().unwrap(); if let Some(deps) = me @@ -1302,7 +1300,6 @@ fn to_virtual_manifest( bail!("this virtual manifest specifies a `{field}` section, which is not allowed"); } - let mut nested_paths = Vec::new(); let mut warnings = Vec::new(); let mut deps = Vec::new(); let empty = Vec::new(); @@ -1315,7 +1312,6 @@ fn to_virtual_manifest( let mut manifest_ctx = ManifestContext { deps: &mut deps, source_id, - nested_paths: &mut nested_paths, gctx, warnings: &mut warnings, platform: None, @@ -1376,7 +1372,7 @@ fn to_virtual_manifest( manifest.warnings_mut().add_warning(warning); } - Ok((manifest, nested_paths)) + Ok(manifest) } fn replace( @@ -1467,7 +1463,6 @@ fn patch( struct ManifestContext<'a, 'b> { deps: &'a mut Vec, source_id: SourceId, - nested_paths: &'a mut Vec, gctx: &'b GlobalContext, warnings: &'a mut Vec, platform: Option, @@ -1563,7 +1558,7 @@ fn inheritable_from_path( }; let source_id = SourceId::for_path(workspace_path_root)?; - let (man, _) = read_manifest(&workspace_path, source_id, gctx)?; + let man = read_manifest(&workspace_path, source_id, gctx)?; match man.workspace_config() { WorkspaceConfig::Root(root) => { gctx.ws_roots @@ -1885,7 +1880,6 @@ pub(crate) fn to_dependency( dep: &manifest::TomlDependency

, name: &str, source_id: SourceId, - nested_paths: &mut Vec, gctx: &GlobalContext, warnings: &mut Vec, platform: Option, @@ -1899,7 +1893,6 @@ pub(crate) fn to_dependency( &mut ManifestContext { deps: &mut Vec::new(), source_id, - nested_paths, gctx, warnings, platform, @@ -2068,7 +2061,6 @@ fn detailed_dep_to_dependency( } (None, Some(path), _, _) => { let path = path.resolve(manifest_ctx.gctx); - manifest_ctx.nested_paths.push(path.clone()); // If the source ID for the package we're parsing is a path // source, then we normalize the path here to get rid of // components like `..`. From 4ed67e3a330f75e5df3bb6970a60e24764dcca0d Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 15 Mar 2024 09:17:44 -0500 Subject: [PATCH 19/19] refactor(toml): Flatten convert_toml This is to work towards tracking everything needed for diagnostics in `Manifest` --- src/cargo/util/toml/mod.rs | 33 ++++++++------------------------- 1 file changed, 8 insertions(+), 25 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index a6be13aec97..9873e9f2a53 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -55,7 +55,14 @@ pub fn read_manifest( let toml = deserialize_toml(&document) .map_err(|e| emit_diagnostic(e.into(), &contents, path, gctx))?; - convert_toml(toml, path, source_id, gctx).map_err(|err| { + (|| { + if toml.package().is_some() { + to_real_manifest(toml, source_id, path, gctx).map(EitherManifest::Real) + } else { + to_virtual_manifest(toml, source_id, path, gctx).map(EitherManifest::Virtual) + } + })() + .map_err(|err| { ManifestError::new( err.context(format!("failed to parse manifest at `{}`", path.display())), path.into(), @@ -160,30 +167,6 @@ fn emit_diagnostic( return AlreadyPrintedError::new(e.into()).into(); } -/// Parse an already-loaded `Cargo.toml` as a Cargo manifest. -/// -/// This could result in a real or virtual manifest being returned. -/// -/// A list of nested paths is also returned, one for each path dependency -/// within the manifest. For virtual manifests, these paths can only -/// come from patched or replaced dependencies. These paths are not -/// canonicalized. -#[tracing::instrument(skip_all)] -fn convert_toml( - manifest: manifest::TomlManifest, - manifest_file: &Path, - source_id: SourceId, - gctx: &GlobalContext, -) -> CargoResult { - return if manifest.package().is_some() { - let manifest = to_real_manifest(manifest, source_id, manifest_file, gctx)?; - Ok(EitherManifest::Real(manifest)) - } else { - let manifest = to_virtual_manifest(manifest, source_id, manifest_file, gctx)?; - Ok(EitherManifest::Virtual(manifest)) - }; -} - fn stringify(dst: &mut String, path: &serde_ignored::Path<'_>) { use serde_ignored::Path;