Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lint_groups_priority: ignore lints & groups at the same level #12827

Merged
merged 1 commit into from
Jun 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 28 additions & 32 deletions clippy_lints/src/cargo/lint_groups_priority.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,12 +71,6 @@ struct CargoToml {
workspace: Workspace,
}

#[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),
Expand All @@ -86,27 +80,28 @@ fn toml_span(range: Range<usize>, file: &SourceFile) -> Span {
)
}

fn check_table(cx: &LateContext<'_>, table: LintTable, groups: &FxHashSet<&str>, file: &SourceFile) {
let mut by_priority = BTreeMap::<_, LintsAndGroups>::new();
fn check_table(cx: &LateContext<'_>, table: LintTable, known_groups: &FxHashSet<&str>, file: &SourceFile) {
let mut lints = Vec::new();
let mut groups = Vec::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));
if name.get_ref() == "warnings" {
continue;
}

if known_groups.contains(name.get_ref().as_str()) {
groups.push((name, config));
} else {
lints_and_groups.lints.push(name);
lints.push((name, config.into_inner()));
}
}
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 {
for (group, group_config) in groups {
let priority = group_config.get_ref().priority();
let level = group_config.get_ref().level();
if let Some((conflict, _)) = lints
.iter()
.rfind(|(_, lint_config)| lint_config.priority() == priority && lint_config.level() != level)
Comment on lines +101 to +103
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to only lint one conflict instead of linting on all conflicts here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to keep the number of warnings reasonable, e.g. with 2 groups and 5 lints you get 2 warnings, one for each group, rather than 10

{
span_lint_and_then(
cx,
LINT_GROUPS_PRIORITY,
Expand All @@ -116,22 +111,23 @@ fn check_table(cx: &LateContext<'_>, table: LintTable, groups: &FxHashSet<&str>,
group.as_ref()
),
|diag| {
let config_span = toml_span(config.span(), file);
if config.as_ref().is_implicit() {
let config_span = toml_span(group_config.span(), file);

if group_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.span_label(toml_span(conflict.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();
let low_priority = lints
.iter()
.map(|(_, config)| config.priority().saturating_sub(1))
.min()
.unwrap_or(-1);
Serialize::serialize(
&LintConfigTable {
level: config.as_ref().level().into(),
level: level.into(),
priority: Some(low_priority),
},
toml::ser::ValueSerializer::new(&mut suggestion),
Expand Down
49 changes: 25 additions & 24 deletions tests/ui-cargo/lint_groups_priority/fail/Cargo.stderr
Original file line number Diff line number Diff line change
@@ -1,17 +1,18 @@
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
--> Cargo.toml:7:1
|
7 | rust_2018_idioms = "warn"
| ^^^^^^^^^^^^^^^^ ------ has an implicit priority of 0
...
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
= 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 }
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
7 | rust_2018_idioms = { level = "warn", priority = -1 }
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

error: lint group `unused` has the same priority (0) as a lint
--> Cargo.toml:10:1
Expand All @@ -29,45 +30,45 @@ help: to have lints override the group set `unused` to a lower priority
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

error: lint group `pedantic` has the same priority (-1) as a lint
--> Cargo.toml:19:1
--> Cargo.toml:15:1
|
19 | pedantic = { level = "warn", priority = -1 }
15 | pedantic = { level = "warn", priority = -1 }
| ^^^^^^^^
20 | similar_names = { level = "allow", priority = -1 }
16 | 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 }
15 | pedantic = { level = "warn", priority = -2 }
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

error: lint group `rust_2018_idioms` has the same priority (0) as a lint
--> Cargo.toml:23:1
--> Cargo.toml:19:1
|
23 | rust_2018_idioms = "warn"
19 | rust_2018_idioms = "warn"
| ^^^^^^^^^^^^^^^^ ------ has an implicit priority of 0
24 | bare_trait_objects = "allow"
20 | bare_trait_objects = "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 `rust_2018_idioms` to a lower priority
|
23 | rust_2018_idioms = { level = "warn", priority = -1 }
19 | rust_2018_idioms = { level = "warn", priority = -1 }
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

error: lint group `pedantic` has the same priority (0) as a lint
--> Cargo.toml:27:1
--> Cargo.toml:23:1
|
27 | pedantic = "warn"
23 | pedantic = "warn"
| ^^^^^^^^ ------ has an implicit priority of 0
28 | similar_names = "allow"
24 | similar_names = "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 `pedantic` to a lower priority
|
27 | pedantic = { level = "warn", priority = -1 }
23 | pedantic = { level = "warn", priority = -1 }
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

error: could not compile `fail` (lib) due to 5 previous errors
4 changes: 0 additions & 4 deletions tests/ui-cargo/lint_groups_priority/fail/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,6 @@ 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 }
Expand Down
7 changes: 7 additions & 0 deletions tests/ui-cargo/lint_groups_priority/pass/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,13 @@ name = "pass"
version = "0.1.0"
publish = false

[lints.rust]
# Warnings does not conflict with any group or lint
warnings = "deny"
# Groups & lints at the same level do not conflict
rust_2018_idioms = "warn"
unsafe_code = "warn"

[lints.clippy]
pedantic = { level = "warn", priority = -1 }
style = { level = "warn", priority = 1 }
Expand Down