Skip to content

Commit

Permalink
refactor(toml): Make it more obvious to update package-dependent fields
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
epage committed Jan 8, 2024
1 parent 90d1ea8 commit 62eab55
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 49 deletions.
23 changes: 22 additions & 1 deletion crates/cargo-util-schemas/src/manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,31 @@ pub struct TomlManifest {
pub workspace: Option<TomlWorkspace>,
pub badges: Option<InheritableBtreeMap>,
pub lints: Option<InheritableLints>,
// 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<Item = &'static str> {
[
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()
}
Expand Down
45 changes: 2 additions & 43 deletions src/cargo/util/toml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1073,49 +1073,8 @@ fn to_virtual_manifest(
root: &Path,
config: &Config,
) -> CargoResult<(VirtualManifest, Vec<PathBuf>)> {
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();
Expand Down
9 changes: 4 additions & 5 deletions tests/testsuite/workspaces.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2186,7 +2186,7 @@ fn ws_rustc_err() {

#[cargo_test]
fn ws_err_unused() {
for key in &[
for table in &[
"[lib]",
"[[bin]]",
"[[example]]",
Expand All @@ -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",
Expand All @@ -2208,9 +2209,8 @@ fn ws_err_unused() {
[workspace]
members = ["a"]
{}
{table}
"#,
key
),
)
.file("a/Cargo.toml", &basic_lib_manifest("a"))
Expand All @@ -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();
}
Expand Down

0 comments on commit 62eab55

Please sign in to comment.