Skip to content

Commit

Permalink
chore(ci): Check for clippy correctness (#14796)
Browse files Browse the repository at this point in the history
### What does this PR try to resolve?

The fact that we don't check for `derive_ord_xor_partial_ord ` almost
bit us in #14663. If we used clippy's default lint levels, this would
have been caught automatically.

Instead of just enabling this one lint, I figured it'd be good to audit
all of the `deny` by default lints.
That is long enough that I was concerned about maintaining it (or
bikeshedding which to enable or disable).
All `deny` by default lints are `correctness` lints and I figure we
could just enable the group.

We normally opt-in to individual clippy lints.
From what I remember of that conversation, it mostly stems from how
liberal clippy is with making a lint `warn` by default.
It also makes an unpinned CI more brittle.
I figured clippy is more conservative about `deny` by default lints
and slower to add them that this is unlikely to be a problem.

As for what existing problems this found,
- Some missing serde functions that would be useful for formats besides
toml (since `toml` never uses borrowed strings)
- Code that behaves differently than the syntax says (a 0-1 iteration
loop)
- A redundant assignment (wasn't even removing `mut`ness

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

### Additional information
  • Loading branch information
ehuss authored Nov 12, 2024
2 parents 31c96be + fd54406 commit 2e7fc43
Show file tree
Hide file tree
Showing 6 changed files with 27 additions and 5 deletions.
3 changes: 2 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,8 @@ rust_2018_idioms = "warn" # TODO: could this be removed?
private_intra_doc_links = "allow"

[workspace.lints.clippy]
all = { level = "allow", priority = -1 }
all = { level = "allow", priority = -2 }
correctness = { level = "warn", priority = -1 }
dbg_macro = "warn"
disallowed_methods = "warn"
print_stderr = "warn"
Expand Down
21 changes: 21 additions & 0 deletions crates/cargo-util-schemas/src/manifest/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,13 @@ impl<'de> de::Deserialize<'de> for InheritableString {
Ok(InheritableString::Value(value))
}

fn visit_str<E>(self, value: &str) -> Result<Self::Value, E>
where
E: de::Error,
{
self.visit_string(value.to_owned())
}

fn visit_map<V>(self, map: V) -> Result<Self::Value, V::Error>
where
V: de::MapAccess<'de>,
Expand Down Expand Up @@ -454,6 +461,13 @@ impl<'de> de::Deserialize<'de> for InheritableRustVersion {
Ok(InheritableRustVersion::Value(value))
}

fn visit_str<E>(self, value: &str) -> Result<Self::Value, E>
where
E: de::Error,
{
self.visit_string(value.to_owned())
}

fn visit_map<V>(self, map: V) -> Result<Self::Value, V::Error>
where
V: de::MapAccess<'de>,
Expand Down Expand Up @@ -533,6 +547,13 @@ impl<'de> de::Deserialize<'de> for InheritableStringOrBool {
StringOrBool::deserialize(string).map(InheritableField::Value)
}

fn visit_str<E>(self, value: &str) -> Result<Self::Value, E>
where
E: de::Error,
{
self.visit_string(value.to_owned())
}

fn visit_map<V>(self, map: V) -> Result<Self::Value, V::Error>
where
V: de::MapAccess<'de>,
Expand Down
1 change: 1 addition & 0 deletions crates/rustfix/src/replace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,7 @@ mod tests {
}

#[test]
#[allow(clippy::reversed_empty_ranges)]
fn replace_invalid_range() {
let mut d = Data::new(b"foo!");

Expand Down
1 change: 0 additions & 1 deletion src/cargo/core/resolver/dep_cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,6 @@ impl<'a> RegistryQueryer<'a> {
}
}

let first_version = first_version;
self.version_prefs.sort_summaries(&mut ret, first_version);

let out = Poll::Ready(Rc::new(ret));
Expand Down
4 changes: 2 additions & 2 deletions src/cargo/util/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -176,8 +176,8 @@ mod test {
);
assert_eq!(human_readable_bytes(1024 * 1024 * 1024), (1., "GiB"));
assert_eq!(
human_readable_bytes((1024. * 1024. * 1024. * 3.1415) as u64),
(3.1415, "GiB")
human_readable_bytes((1024. * 1024. * 1024. * 1.2345) as u64),
(1.2345, "GiB")
);
assert_eq!(human_readable_bytes(1024 * 1024 * 1024 * 1024), (1., "TiB"));
assert_eq!(
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/util/toml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -507,7 +507,7 @@ fn normalize_toml(

normalized_toml.badges = original_toml.badges.clone();
} else {
for field in original_toml.requires_package() {
if let Some(field) = original_toml.requires_package().next() {
bail!("this virtual manifest specifies a `{field}` section, which is not allowed");
}
}
Expand Down

0 comments on commit 2e7fc43

Please sign in to comment.