Skip to content

Commit

Permalink
Auto merge of #12584 - epage:lints, r=arlosi
Browse files Browse the repository at this point in the history
fix(lints): Fail when overriding inherited lints

### What does this PR try to resolve?

Overriding of inherited lints was reserved for the future but as pointed out in #12115 (comment), we aren't failing on these when we should but silently ignoring the overrides.

This turns it into a hard error.

In fixing this, I had to add a `#[serde(expecting)]` attribute to maintain behavior on an error case (otherwise it would say "expecting struct WorkspaceLints").  Since this drew the error message to my attention, I also tweaked it to make it more specific.

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

Commits are broken down by the relevant tests and fixes to make the intended behavior changes obvious.
  • Loading branch information
bors committed Aug 29, 2023
2 parents 333ca23 + 6568e2c commit 96fe1c9
Show file tree
Hide file tree
Showing 2 changed files with 79 additions and 27 deletions.
60 changes: 34 additions & 26 deletions src/cargo/util/toml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1475,29 +1475,6 @@ impl<'de> de::Deserialize<'de> for MaybeWorkspaceBtreeMap {
}
}

type MaybeWorkspaceLints = MaybeWorkspace<TomlLints, TomlWorkspaceField>;

impl<'de> de::Deserialize<'de> for MaybeWorkspaceLints {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: de::Deserializer<'de>,
{
let value = serde_value::Value::deserialize(deserializer)?;

if let Ok(w) = TomlWorkspaceField::deserialize(
serde_value::ValueDeserializer::<D::Error>::new(value.clone()),
) {
return if w.workspace() {
Ok(MaybeWorkspace::Workspace(w))
} else {
Err(de::Error::custom("`workspace` cannot be false"))
};
}
TomlLints::deserialize(serde_value::ValueDeserializer::<D::Error>::new(value))
.map(MaybeWorkspace::Defined)
}
}

#[derive(Deserialize, Serialize, Copy, Clone, Debug)]
pub struct TomlWorkspaceField {
#[serde(deserialize_with = "bool_no_false")]
Expand Down Expand Up @@ -2277,7 +2254,7 @@ impl TomlManifest {

let lints =
parse_unstable_lints::<MaybeWorkspaceLints>(me.lints.clone(), config, cx.warnings)?
.map(|mw| mw.resolve("lints", || inherit()?.lints()))
.map(|mw| mw.resolve(|| inherit()?.lints()))
.transpose()?;
let lints = verify_lints(lints)?;
let default = TomlLints::default();
Expand Down Expand Up @@ -2579,8 +2556,7 @@ impl TomlManifest {
.badges
.as_ref()
.map(|_| MaybeWorkspace::Defined(metadata.badges.clone())),
lints: lints
.map(|lints| toml::Value::try_from(MaybeWorkspaceLints::Defined(lints)).unwrap()),
lints: lints.map(|lints| toml::Value::try_from(lints).unwrap()),
};
let mut manifest = Manifest::new(
summary,
Expand Down Expand Up @@ -3522,6 +3498,38 @@ impl fmt::Debug for PathValue {
}
}

#[derive(Deserialize, Serialize, Debug, Clone)]
#[serde(expecting = "a lints table")]
pub struct MaybeWorkspaceLints {
#[serde(skip_serializing_if = "is_false")]
#[serde(deserialize_with = "bool_no_false", default)]
workspace: bool,
#[serde(flatten)]
lints: TomlLints,
}

fn is_false(b: &bool) -> bool {
!b
}

impl MaybeWorkspaceLints {
fn resolve<'a>(
self,
get_ws_inheritable: impl FnOnce() -> CargoResult<TomlLints>,
) -> CargoResult<TomlLints> {
if self.workspace {
if !self.lints.is_empty() {
anyhow::bail!("cannot override `workspace.lints` in `lints`, either remove the overrides or `lints.workspace = true` and manually specify the lints");
}
get_ws_inheritable().with_context(|| {
"error inheriting `lints` from workspace root manifest's `workspace.lints`"
})
} else {
Ok(self.lints)
}
}
}

pub type TomlLints = BTreeMap<String, TomlToolLints>;

pub type TomlToolLints = BTreeMap<String, TomlLint>;
Expand Down
46 changes: 45 additions & 1 deletion tests/testsuite/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ fn malformed_on_nightly() {
error: failed to parse manifest[..]
Caused by:
invalid type: integer `20`, expected a map
invalid type: integer `20`, expected a lints table
",
)
.run();
Expand Down Expand Up @@ -413,6 +413,50 @@ error: usage of an `unsafe` block
.run();
}

#[cargo_test]
fn workspace_and_package_lints() {
let foo = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.0.1"
authors = []
[lints]
workspace = true
[lints.rust]
"unsafe_code" = "allow"
[workspace.lints.rust]
"unsafe_code" = "deny"
"#,
)
.file(
"src/lib.rs",
"
pub fn foo(num: i32) -> u32 {
unsafe { std::mem::transmute(num) }
}
",
)
.build();

foo.cargo("check -Zlints")
.masquerade_as_nightly_cargo(&["lints"])
.with_status(101)
.with_stderr(
"\
[ERROR] failed to parse manifest at `[CWD]/Cargo.toml`
Caused by:
cannot override `workspace.lints` in `lints`, either remove the overrides or `lints.workspace = true` and manually specify the lints
",
)
.run();
}

#[cargo_test]
fn attribute_has_precedence() {
let foo = project()
Expand Down

0 comments on commit 96fe1c9

Please sign in to comment.