Skip to content

Commit

Permalink
Auto merge of rust-lang#13262 - epage:unused, r=ehuss
Browse files Browse the repository at this point in the history
fix(manifest): Provide unused key warnings for lints table

### What does this PR try to resolve?

The use of `flatten` was getting in the way of `serde_ignored`.
A common workaround is to add our own `unused` tracking but that would
cause duplicates with `workspace.lints` (or we'd just ignore it).

Since the manual deserializer was relatively simple, I went that route.

Fixes rust-lang#12917

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

Per commit

A test was added for the issue.  I then was worried about regressions in `workspace = false` errors (and I was right) so I added a test for that.  To get `workspace = false` to work nicely, I made it share code with other `workspace: bool` fields.

### Additional information
  • Loading branch information
bors committed Jan 8, 2024
2 parents bde42f1 + 6e86530 commit 484f0f2
Show file tree
Hide file tree
Showing 2 changed files with 149 additions and 18 deletions.
91 changes: 73 additions & 18 deletions crates/cargo-util-schemas/src/manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -410,11 +410,7 @@ impl<'de> de::Deserialize<'de> for InheritableBtreeMap {
if let Ok(w) = TomlInheritedField::deserialize(
serde_value::ValueDeserializer::<D::Error>::new(value.clone()),
) {
return if w.workspace {
Ok(InheritableField::Inherit(w))
} else {
Err(de::Error::custom("`workspace` cannot be false"))
};
return Ok(InheritableField::Inherit(w));
}
BTreeMap::deserialize(serde_value::ValueDeserializer::<D::Error>::new(value))
.map(InheritableField::Value)
Expand All @@ -424,13 +420,14 @@ impl<'de> de::Deserialize<'de> for InheritableBtreeMap {
#[derive(Deserialize, Serialize, Copy, Clone, Debug)]
#[serde(rename_all = "kebab-case")]
pub struct TomlInheritedField {
#[serde(deserialize_with = "bool_no_false")]
workspace: bool,
workspace: WorkspaceValue,
}

impl TomlInheritedField {
pub fn new() -> Self {
TomlInheritedField { workspace: true }
TomlInheritedField {
workspace: WorkspaceValue,
}
}
}

Expand All @@ -440,12 +437,25 @@ impl Default for TomlInheritedField {
}
}

fn bool_no_false<'de, D: de::Deserializer<'de>>(deserializer: D) -> Result<bool, D::Error> {
let b: bool = Deserialize::deserialize(deserializer)?;
if b {
Ok(b)
} else {
Err(de::Error::custom("`workspace` cannot be false"))
#[derive(Deserialize, Serialize, Copy, Clone, Debug)]
#[serde(try_from = "bool")]
#[serde(into = "bool")]
struct WorkspaceValue;

impl TryFrom<bool> for WorkspaceValue {
type Error = String;
fn try_from(other: bool) -> Result<WorkspaceValue, Self::Error> {
if other {
Ok(WorkspaceValue)
} else {
Err("`workspace` cannot be false".to_owned())
}
}
}

impl From<WorkspaceValue> for bool {
fn from(_: WorkspaceValue) -> bool {
true
}
}

Expand Down Expand Up @@ -1250,12 +1260,9 @@ impl TomlPlatform {
}
}

#[derive(Deserialize, Serialize, Debug, Clone)]
#[serde(expecting = "a lints table")]
#[serde(rename_all = "kebab-case")]
#[derive(Serialize, Debug, Clone)]
pub struct InheritableLints {
#[serde(skip_serializing_if = "is_false")]
#[serde(deserialize_with = "bool_no_false", default)]
pub workspace: bool,
#[serde(flatten)]
pub lints: TomlLints,
Expand All @@ -1265,6 +1272,54 @@ fn is_false(b: &bool) -> bool {
!b
}

impl<'de> Deserialize<'de> for InheritableLints {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: de::Deserializer<'de>,
{
struct InheritableLintsVisitor;

impl<'de> de::Visitor<'de> for InheritableLintsVisitor {
// The type that our Visitor is going to produce.
type Value = InheritableLints;

// Format a message stating what data this Visitor expects to receive.
fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
formatter.write_str("a lints table")
}

// Deserialize MyMap from an abstract "map" provided by the
// Deserializer. The MapAccess input is a callback provided by
// the Deserializer to let us see each entry in the map.
fn visit_map<M>(self, mut access: M) -> Result<Self::Value, M::Error>
where
M: de::MapAccess<'de>,
{
let mut lints = TomlLints::new();
let mut workspace = false;

// While there are entries remaining in the input, add them
// into our map.
while let Some(key) = access.next_key()? {
if key == "workspace" {
workspace = match access.next_value()? {
Some(WorkspaceValue) => true,
None => false,
};
} else {
let value = access.next_value()?;
lints.insert(key, value);
}
}

Ok(InheritableLints { workspace, lints })
}
}

deserializer.deserialize_map(InheritableLintsVisitor)
}
}

pub type TomlLints = BTreeMap<String, TomlToolLints>;

pub type TomlToolLints = BTreeMap<String, TomlLint>;
Expand Down
76 changes: 76 additions & 0 deletions tests/testsuite/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,37 @@ Caused by:
.run();
}

#[cargo_test]
fn warn_on_unused_key() {
let foo = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.0.1"
[workspace.lints.rust]
rust-2018-idioms = { level = "allow", unused = true }
[lints.rust]
rust-2018-idioms = { level = "allow", unused = true }
"#,
)
.file("src/lib.rs", "")
.build();

foo.cargo("check")
.with_stderr(
"\
[WARNING] [CWD]/Cargo.toml: unused manifest key: lints.rust.rust-2018-idioms.unused
[WARNING] [CWD]/Cargo.toml: unused manifest key: workspace.lints.rust.rust-2018-idioms.unused
[CHECKING] foo v0.0.1 ([CWD])
[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]s
",
)
.run();
}

#[cargo_test]
fn fail_on_tool_injection() {
let foo = project()
Expand Down Expand Up @@ -276,6 +307,51 @@ error: usage of an `unsafe` block
.run();
}

#[cargo_test]
fn workspace_cant_be_false() {
let foo = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.0.1"
authors = []
[lints]
workspace = false
[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")
.with_status(101)
.with_stderr_contains(
"\
error: failed to parse manifest at `[CWD]/Cargo.toml`
Caused by:
TOML parse error at line 8, column 29
|
8 | workspace = false
| ^^^^^
`workspace` cannot be false
",
)
.run();
}

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

0 comments on commit 484f0f2

Please sign in to comment.