Add lint_groups_priority lint

Alexendoo · Alexendoo · commit 6619e8c27da8 · 2024-01-31T18:33:41.000Z
Warns when a lint group in Cargo.toml's `[lints]` section shares the
same priority as a lint
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -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
diff --git a/clippy_lints/src/cargo/lint_groups_priority.rs b/clippy_lints/src/cargo/lint_groups_priority.rs
@@ -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);
+    }
+}
diff --git a/clippy_lints/src/cargo/mod.rs b/clippy_lints/src/cargo/mod.rs
@@ -1,5 +1,6 @@
 mod common_metadata;
 mod feature_name;
+mod lint_groups_priority;
 mod multiple_crate_versions;
 mod wildcard_dependencies;
 
@@ -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,
@@ -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 {
@@ -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))
diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs
@@ -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,
diff --git a/tests/ui-cargo/lint_groups_priority/fail/Cargo.stderr b/tests/ui-cargo/lint_groups_priority/fail/Cargo.stderr
@@ -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
diff --git a/tests/ui-cargo/lint_groups_priority/fail/Cargo.toml b/tests/ui-cargo/lint_groups_priority/fail/Cargo.toml
@@ -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 }
diff --git a/tests/ui-cargo/lint_groups_priority/fail/src/lib.rs b/tests/ui-cargo/lint_groups_priority/fail/src/lib.rs
@@ -0,0 +1 @@
+
diff --git a/tests/ui-cargo/lint_groups_priority/pass/Cargo.toml b/tests/ui-cargo/lint_groups_priority/pass/Cargo.toml
@@ -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 }
diff --git a/tests/ui-cargo/lint_groups_priority/pass/src/lib.rs b/tests/ui-cargo/lint_groups_priority/pass/src/lib.rs
@@ -0,0 +1 @@
+
