Skip to content

Commit

Permalink
Add lint_groups_priority lint
Browse files Browse the repository at this point in the history
Warns when a lint group in Cargo.toml's `[lints]` section shares the
same priority as a lint
  • Loading branch information
Alexendoo committed Jan 31, 2024
1 parent 455c07b commit 6619e8c
Show file tree
Hide file tree
Showing 9 changed files with 289 additions and 1 deletion.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5277,6 +5277,7 @@ Released 2018-09-13
[`let_with_type_underscore`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_with_type_underscore
[`lines_filter_map_ok`]: https://rust-lang.github.io/rust-clippy/master/index.html#lines_filter_map_ok
[`linkedlist`]: https://rust-lang.github.io/rust-clippy/master/index.html#linkedlist
[`lint_groups_priority`]: https://rust-lang.github.io/rust-clippy/master/index.html#lint_groups_priority
[`little_endian_bytes`]: https://rust-lang.github.io/rust-clippy/master/index.html#little_endian_bytes
[`logic_bug`]: https://rust-lang.github.io/rust-clippy/master/index.html#logic_bug
[`lossy_float_literal`]: https://rust-lang.github.io/rust-clippy/master/index.html#lossy_float_literal
Expand Down
168 changes: 168 additions & 0 deletions clippy_lints/src/cargo/lint_groups_priority.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
use super::LINT_GROUPS_PRIORITY;
use clippy_utils::diagnostics::span_lint_and_then;
use rustc_data_structures::fx::FxHashSet;
use rustc_errors::Applicability;
use rustc_lint::{unerased_lint_store, LateContext};
use rustc_span::{BytePos, Pos, SourceFile, Span, SyntaxContext};
use serde::{Deserialize, Serialize};
use std::collections::BTreeMap;
use std::ops::Range;
use std::path::Path;
use toml::Spanned;

#[derive(Deserialize, Serialize, Debug)]
struct LintConfigTable {
level: String,
priority: Option<i64>,
}

#[derive(Deserialize, Debug)]
#[serde(untagged)]
enum LintConfig {
Level(String),
Table(LintConfigTable),
}

impl LintConfig {
fn level(&self) -> &str {
match self {
LintConfig::Level(level) => level,
LintConfig::Table(table) => &table.level,
}
}

fn priority(&self) -> i64 {
match self {
LintConfig::Level(_) => 0,
LintConfig::Table(table) => table.priority.unwrap_or(0),
}
}

fn is_implicit(&self) -> bool {
if let LintConfig::Table(table) = self {
table.priority.is_none()
} else {
true
}
}
}

type LintTable = BTreeMap<Spanned<String>, Spanned<LintConfig>>;

#[derive(Deserialize, Debug)]
struct Lints {
#[serde(default)]
rust: LintTable,
#[serde(default)]
clippy: LintTable,
}

#[derive(Deserialize, Debug)]
struct CargoToml {
lints: Lints,
}

#[derive(Default, Debug)]
struct LintsAndGroups {
lints: Vec<Spanned<String>>,
groups: Vec<(Spanned<String>, Spanned<LintConfig>)>,
}

fn toml_span(range: Range<usize>, file: &SourceFile) -> Span {
Span::new(
file.start_pos + BytePos::from_usize(range.start),
file.start_pos + BytePos::from_usize(range.end),
SyntaxContext::root(),
None,
)
}

fn check_table(cx: &LateContext<'_>, table: LintTable, groups: &FxHashSet<&str>, file: &SourceFile) {
let mut by_priority = BTreeMap::<_, LintsAndGroups>::new();
for (name, config) in table {
let lints_and_groups = by_priority.entry(config.as_ref().priority()).or_default();
if groups.contains(name.get_ref().as_str()) {
lints_and_groups.groups.push((name, config));
} else {
lints_and_groups.lints.push(name);
}
}
let low_priority = by_priority
.iter()
.find(|(_, lints_and_groups)| !lints_and_groups.lints.is_empty())
.map_or(-1, |(&lowest_lint_priority, _)| lowest_lint_priority - 1);

for (priority, LintsAndGroups { lints, groups }) in by_priority {
let Some(last_lint_alphabetically) = lints.last() else {
continue;
};

for (group, config) in groups {
span_lint_and_then(
cx,
LINT_GROUPS_PRIORITY,
toml_span(group.span(), file),
&format!(
"lint group `{}` has the same priority ({priority}) as a lint",
group.as_ref()
),
|diag| {
let config_span = toml_span(config.span(), file);
if config.as_ref().is_implicit() {
diag.span_label(config_span, "has an implicit priority of 0");
}
// add the label to next lint after this group that has the same priority
let lint = lints
.iter()
.filter(|lint| lint.span().start > group.span().start)
.min_by_key(|lint| lint.span().start)
.unwrap_or(last_lint_alphabetically);
diag.span_label(toml_span(lint.span(), file), "has the same priority as this lint");
diag.note("the order of the lints in the table is ignored by Cargo");
let mut suggestion = String::new();
Serialize::serialize(
&LintConfigTable {
level: config.as_ref().level().into(),
priority: Some(low_priority),
},
toml::ser::ValueSerializer::new(&mut suggestion),
)
.unwrap();
diag.span_suggestion_verbose(
config_span,
format!(
"to have lints override the group set `{}` to a lower priority",
group.as_ref()
),
suggestion,
Applicability::MaybeIncorrect,
);
},
);
}
}
}

pub fn check(cx: &LateContext<'_>) {
if let Ok(file) = cx.tcx.sess.source_map().load_file(Path::new("Cargo.toml"))
&& let Some(src) = file.src.as_deref()
&& let Ok(cargo_toml) = toml::from_str::<CargoToml>(src)
{
let mut rustc_groups = FxHashSet::default();
let mut clippy_groups = FxHashSet::default();
for (group, ..) in unerased_lint_store(cx.tcx.sess).get_lint_groups() {
match group.split_once("::") {
None => {
rustc_groups.insert(group);
},
Some(("clippy", group)) => {
clippy_groups.insert(group);
},
_ => {},
}
}

check_table(cx, cargo_toml.lints.rust, &rustc_groups, &file);
check_table(cx, cargo_toml.lints.clippy, &clippy_groups, &file);
}
}
43 changes: 42 additions & 1 deletion clippy_lints/src/cargo/mod.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
mod common_metadata;
mod feature_name;
mod lint_groups_priority;
mod multiple_crate_versions;
mod wildcard_dependencies;

Expand Down Expand Up @@ -165,6 +166,43 @@ declare_clippy_lint! {
"wildcard dependencies being used"
}

declare_clippy_lint! {
/// ### What it does
/// Checks for lint groups with the same priority as lints in the `Cargo.toml`
/// [`[lints]` table](https://doc.rust-lang.org/cargo/reference/manifest.html#the-lints-section).
///
/// This lint will be removed once [cargo#12918](https://github.com/rust-lang/cargo/issues/12918)
/// is resolved.
///
/// ### Why is this bad?
/// The order of lints in the `[lints]` is ignored, to have a lint override a group the
/// `priority` field needs to be used, otherwise the sort order is undefined.
///
/// ### Known problems
/// Does not check lints inherited using `lints.workspace = true`
///
/// ### Example
/// ```toml
/// # Passed as `--allow=clippy::similar_names --warn=clippy::pedantic`
/// # which results in `similar_names` being `warn`
/// [lints.clippy]
/// pedantic = "warn"
/// similar_names = "allow"
/// ```
/// Use instead:
/// ```toml
/// # Passed as `--warn=clippy::pedantic --allow=clippy::similar_names`
/// # which results in `similar_names` being `allow`
/// [lints.clippy]
/// pedantic = { level = "warn", priority = -1 }
/// similar_names = "allow"
/// ```
#[clippy::version = "1.76.0"]
pub LINT_GROUPS_PRIORITY,
correctness,
"a lint group in `Cargo.toml` at the same priority as a lint"
}

pub struct Cargo {
pub allowed_duplicate_crates: FxHashSet<String>,
pub ignore_publish: bool,
Expand All @@ -175,7 +213,8 @@ impl_lint_pass!(Cargo => [
REDUNDANT_FEATURE_NAMES,
NEGATIVE_FEATURE_NAMES,
MULTIPLE_CRATE_VERSIONS,
WILDCARD_DEPENDENCIES
WILDCARD_DEPENDENCIES,
LINT_GROUPS_PRIORITY,
]);

impl LateLintPass<'_> for Cargo {
Expand All @@ -188,6 +227,8 @@ impl LateLintPass<'_> for Cargo {
];
static WITH_DEPS_LINTS: &[&Lint] = &[MULTIPLE_CRATE_VERSIONS];

lint_groups_priority::check(cx);

if !NO_DEPS_LINTS
.iter()
.all(|&lint| is_lint_allowed(cx, lint, CRATE_HIR_ID))
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::borrow_deref_ref::BORROW_DEREF_REF_INFO,
crate::box_default::BOX_DEFAULT_INFO,
crate::cargo::CARGO_COMMON_METADATA_INFO,
crate::cargo::LINT_GROUPS_PRIORITY_INFO,
crate::cargo::MULTIPLE_CRATE_VERSIONS_INFO,
crate::cargo::NEGATIVE_FEATURE_NAMES_INFO,
crate::cargo::REDUNDANT_FEATURE_NAMES_INFO,
Expand Down
45 changes: 45 additions & 0 deletions tests/ui-cargo/lint_groups_priority/fail/Cargo.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
error: lint group `rust_2018_idioms` has the same priority (0) as a lint
--> Cargo.toml:7:1
|
7 | rust_2018_idioms = "warn"
| ^^^^^^^^^^^^^^^^ ------ has an implicit priority of 0
8 | bare_trait_objects = "allow"
| ------------------ has the same priority as this lint
|
= note: the order of the lints in the table is ignored by Cargo
= note: `#[deny(clippy::lint_groups_priority)]` on by default
help: to have lints override the group set `rust_2018_idioms` to a lower priority
|
7 | rust_2018_idioms = { level = "warn", priority = -1 }
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

error: lint group `unused` has the same priority (0) as a lint
--> Cargo.toml:10:1
|
10 | unused = { level = "deny" }
| ^^^^^^ ------------------ has an implicit priority of 0
11 | unused_braces = { level = "allow", priority = 1 }
12 | unused_attributes = { level = "allow" }
| ----------------- has the same priority as this lint
|
= note: the order of the lints in the table is ignored by Cargo
help: to have lints override the group set `unused` to a lower priority
|
10 | unused = { level = "deny", priority = -1 }
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

error: lint group `pedantic` has the same priority (-1) as a lint
--> Cargo.toml:19:1
|
19 | pedantic = { level = "warn", priority = -1 }
| ^^^^^^^^
20 | similar_names = { level = "allow", priority = -1 }
| ------------- has the same priority as this lint
|
= note: the order of the lints in the table is ignored by Cargo
help: to have lints override the group set `pedantic` to a lower priority
|
19 | pedantic = { level = "warn", priority = -2 }
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

error: could not compile `fail` (lib) due to 3 previous errors
20 changes: 20 additions & 0 deletions tests/ui-cargo/lint_groups_priority/fail/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
[package]
name = "fail"
version = "0.1.0"
publish = false

[lints.rust]
rust_2018_idioms = "warn"
bare_trait_objects = "allow"

unused = { level = "deny" }
unused_braces = { level = "allow", priority = 1 }
unused_attributes = { level = "allow" }

# `warnings` is not a group so the order it is passed does not matter
warnings = "deny"
deprecated = "allow"

[lints.clippy]
pedantic = { level = "warn", priority = -1 }
similar_names = { level = "allow", priority = -1 }
1 change: 1 addition & 0 deletions tests/ui-cargo/lint_groups_priority/fail/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@

10 changes: 10 additions & 0 deletions tests/ui-cargo/lint_groups_priority/pass/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
[package]
name = "pass"
version = "0.1.0"
publish = false

[lints.clippy]
pedantic = { level = "warn", priority = -1 }
style = { level = "warn", priority = 1 }
similar_names = "allow"
dbg_macro = { level = "warn", priority = 2 }
1 change: 1 addition & 0 deletions tests/ui-cargo/lint_groups_priority/pass/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@

0 comments on commit 6619e8c

Please sign in to comment.