From c1d3f4d1b96939e75aea9997848c99cbe9208b3c Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 8 Jan 2024 15:34:38 -0600 Subject: [PATCH] refactor(toml): Make it more obvious to update package-dependent fields Inspired by my having forgotten to add `[lints]` to the if sequence. Previously, we added a comment to suggest this but the further the code is, the harder it is to track. I considered a custom `Deserialize` impl, possibly through a new type, that would error. This would be the more "pure" solution. Unfortunately, this would also have worse errors because the errors would be reported to the `Deserializer` at the document-level, rather than directly on the individual fields. Well, we don't do on individual fields now but it is something we will soon be exploring. --- crates/cargo-util-schemas/src/manifest.rs | 23 +++++++++++- src/cargo/util/toml/mod.rs | 45 +---------------------- tests/testsuite/workspaces.rs | 9 ++--- 3 files changed, 28 insertions(+), 49 deletions(-) diff --git a/crates/cargo-util-schemas/src/manifest.rs b/crates/cargo-util-schemas/src/manifest.rs index c17b47d3c6ff..1591ec30197e 100644 --- a/crates/cargo-util-schemas/src/manifest.rs +++ b/crates/cargo-util-schemas/src/manifest.rs @@ -49,10 +49,31 @@ pub struct TomlManifest { pub workspace: Option, pub badges: Option, pub lints: Option, - // when adding new fields, be sure to check whether `to_virtual_manifest` should disallow them + // when adding new fields, be sure to check whether `requires_package` should disallow them } impl TomlManifest { + pub fn requires_package(&self) -> impl Iterator { + [ + self.lib.as_ref().map(|_| "lib"), + self.bin.as_ref().map(|_| "bin"), + self.example.as_ref().map(|_| "example"), + self.test.as_ref().map(|_| "test"), + self.bench.as_ref().map(|_| "bench"), + self.dependencies.as_ref().map(|_| "dependencies"), + self.dev_dependencies().as_ref().map(|_| "dev-dependencies"), + self.build_dependencies() + .as_ref() + .map(|_| "build-dependencies"), + self.features.as_ref().map(|_| "features"), + self.target.as_ref().map(|_| "target"), + self.badges.as_ref().map(|_| "badges"), + self.lints.as_ref().map(|_| "lints"), + ] + .into_iter() + .flatten() + } + pub fn has_profiles(&self) -> bool { self.profile.is_some() } diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 39b0e6e7faf7..44237b6e0feb 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -1073,49 +1073,8 @@ fn to_virtual_manifest( root: &Path, config: &Config, ) -> CargoResult<(VirtualManifest, Vec)> { - if me.project.is_some() { - bail!("this virtual manifest specifies a [project] section, which is not allowed"); - } - if me.package.is_some() { - bail!("this virtual manifest specifies a [package] section, which is not allowed"); - } - if me.lib.is_some() { - bail!("this virtual manifest specifies a [lib] section, which is not allowed"); - } - if me.bin.is_some() { - bail!("this virtual manifest specifies a [[bin]] section, which is not allowed"); - } - if me.example.is_some() { - bail!("this virtual manifest specifies a [[example]] section, which is not allowed"); - } - if me.test.is_some() { - bail!("this virtual manifest specifies a [[test]] section, which is not allowed"); - } - if me.bench.is_some() { - bail!("this virtual manifest specifies a [[bench]] section, which is not allowed"); - } - if me.dependencies.is_some() { - bail!("this virtual manifest specifies a [dependencies] section, which is not allowed"); - } - if me.dev_dependencies().is_some() { - bail!("this virtual manifest specifies a [dev-dependencies] section, which is not allowed"); - } - if me.build_dependencies().is_some() { - bail!( - "this virtual manifest specifies a [build-dependencies] section, which is not allowed" - ); - } - if me.features.is_some() { - bail!("this virtual manifest specifies a [features] section, which is not allowed"); - } - if me.target.is_some() { - bail!("this virtual manifest specifies a [target] section, which is not allowed"); - } - if me.badges.is_some() { - bail!("this virtual manifest specifies a [badges] section, which is not allowed"); - } - if me.lints.is_some() { - bail!("this virtual manifest specifies a [lints] section, which is not allowed"); + for field in me.requires_package() { + bail!("this virtual manifest specifies a `{field}` section, which is not allowed"); } let mut nested_paths = Vec::new(); diff --git a/tests/testsuite/workspaces.rs b/tests/testsuite/workspaces.rs index 53ddc1616060..0465266e043c 100644 --- a/tests/testsuite/workspaces.rs +++ b/tests/testsuite/workspaces.rs @@ -2186,7 +2186,7 @@ fn ws_rustc_err() { #[cargo_test] fn ws_err_unused() { - for key in &[ + for table in &[ "[lib]", "[[bin]]", "[[example]]", @@ -2200,6 +2200,7 @@ fn ws_err_unused() { "[badges]", "[lints]", ] { + let key = table.trim_start_matches('[').trim_end_matches(']'); let p = project() .file( "Cargo.toml", @@ -2208,9 +2209,8 @@ fn ws_err_unused() { [workspace] members = ["a"] - {} + {table} "#, - key ), ) .file("a/Cargo.toml", &basic_lib_manifest("a")) @@ -2223,9 +2223,8 @@ fn ws_err_unused() { [ERROR] failed to parse manifest at `[..]/foo/Cargo.toml` Caused by: - this virtual manifest specifies a {} section, which is not allowed + this virtual manifest specifies a `{key}` section, which is not allowed ", - key )) .run(); }