Skip to content

Commit

Permalink
Warn instead of error when removed rules are used in ignore
Browse files Browse the repository at this point in the history
  • Loading branch information
MichaReiser committed Nov 18, 2024
1 parent 1d1a14a commit 2a831b9
Show file tree
Hide file tree
Showing 5 changed files with 122 additions and 1 deletion.
62 changes: 62 additions & 0 deletions crates/ruff/tests/integration_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1250,6 +1250,68 @@ fn removed_indirect() {
");
}

#[test]
fn removed_ignore_direct() {
let mut cmd = RuffCheck::default().args(["--ignore", "UP027"]).build();
assert_cmd_snapshot!(cmd, @r"
success: true
exit_code: 0
----- stdout -----
All checks passed!
----- stderr -----
warning: The following rules have been removed and ignoring them is useless:
- UP027
");
}

#[test]
fn removed_ignore_multiple_direct() {
let mut cmd = RuffCheck::default()
.args(["--ignore", "UP027", "--ignore", "PLR1706"])
.build();
assert_cmd_snapshot!(cmd, @r"
success: true
exit_code: 0
----- stdout -----
All checks passed!
----- stderr -----
warning: The following rules have been removed and ignoring them is useless:
- PLR1706
- UP027
");
}

#[test]
fn removed_ignore_remapped_direct() {
let mut cmd = RuffCheck::default().args(["--ignore", "PGH001"]).build();
assert_cmd_snapshot!(cmd, @r"
success: true
exit_code: 0
----- stdout -----
All checks passed!
----- stderr -----
warning: `PGH001` has been remapped to `S307`.
");
}

#[test]
fn removed_ignore_indirect() {
// `PLR170` includes removed rules but should not select or warn
// since it is not a "direct" selection
let mut cmd = RuffCheck::default().args(["--ignore", "PLR170"]).build();
assert_cmd_snapshot!(cmd, @r"
success: true
exit_code: 0
----- stdout -----
All checks passed!
----- stderr -----
");
}

#[test]
fn redirect_direct() {
// Selection of a redirected rule directly should use the new rule and warn
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Pyupgrade, "024") => (RuleGroup::Stable, rules::pyupgrade::rules::OSErrorAlias),
(Pyupgrade, "025") => (RuleGroup::Stable, rules::pyupgrade::rules::UnicodeKindPrefix),
(Pyupgrade, "026") => (RuleGroup::Stable, rules::pyupgrade::rules::DeprecatedMockImport),
(Pyupgrade, "027") => (RuleGroup::Removed, rules::pyupgrade::rules::UnpackedListComprehension),
(Pyupgrade, "028") => (RuleGroup::Stable, rules::pyupgrade::rules::YieldInForLoop),
(Pyupgrade, "029") => (RuleGroup::Stable, rules::pyupgrade::rules::UnnecessaryBuiltinImport),
(Pyupgrade, "030") => (RuleGroup::Stable, rules::pyupgrade::rules::FormatLiterals),
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/pyupgrade/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ pub(crate) use unnecessary_coding_comment::*;
pub(crate) use unnecessary_default_type_args::*;
pub(crate) use unnecessary_encode_utf8::*;
pub(crate) use unnecessary_future_import::*;
pub(crate) use unpacked_list_comprehension::*;
pub(crate) use use_pep585_annotation::*;
pub(crate) use use_pep604_annotation::*;
pub(crate) use use_pep604_isinstance::*;
Expand Down Expand Up @@ -73,6 +74,7 @@ mod unnecessary_coding_comment;
mod unnecessary_default_type_args;
mod unnecessary_encode_utf8;
mod unnecessary_future_import;
mod unpacked_list_comprehension;
mod use_pep585_annotation;
mod use_pep604_annotation;
mod use_pep604_isinstance;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
use ruff_diagnostics::Violation;
use ruff_macros::{derive_message_formats, violation};

/// ## Removed
/// There's no [evidence](https://github.com/astral-sh/ruff/issues/12754) that generators are
/// meaningfully faster than list comprehensions when combined with unpacking.
///
/// ## What it does
/// Checks for list comprehensions that are immediately unpacked.
///
/// ## Why is this bad?
/// There is no reason to use a list comprehension if the result is immediately
/// unpacked. Instead, use a generator expression, which avoids allocating
/// an intermediary list.
///
/// ## Example
/// ```python
/// a, b, c = [foo(x) for x in items]
/// ```
///
/// Use instead:
/// ```python
/// a, b, c = (foo(x) for x in items)
/// ```
///
/// ## References
/// - [Python documentation: Generator expressions](https://docs.python.org/3/reference/expressions.html#generator-expressions)
/// - [Python documentation: List comprehensions](https://docs.python.org/3/tutorial/datastructures.html#list-comprehensions)
#[violation]
pub struct UnpackedListComprehension;

impl Violation for UnpackedListComprehension {
#[derive_message_formats]
fn message(&self) -> String {
"Replace unpacked list comprehension with a generator expression".to_string()
}
}
21 changes: 20 additions & 1 deletion crates/ruff_workspace/src/configuration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -775,6 +775,7 @@ impl LintConfiguration {
let mut redirects = FxHashMap::default();
let mut deprecated_selectors = FxHashSet::default();
let mut removed_selectors = FxHashSet::default();
let mut removed_ignored_rules = FxHashSet::default();
let mut ignored_preview_selectors = FxHashSet::default();

// Track which docstring rules are specifically enabled
Expand Down Expand Up @@ -926,7 +927,11 @@ impl LintConfiguration {
// Removed rules
if selector.is_exact() {
if selector.all_rules().all(|rule| rule.is_removed()) {
removed_selectors.insert(selector);
if kind.is_disable() {
removed_ignored_rules.insert(selector);
} else {
removed_selectors.insert(selector);
}
}
}

Expand Down Expand Up @@ -968,6 +973,20 @@ impl LintConfiguration {
}
}

if !removed_ignored_rules.is_empty() {
let mut rules = String::new();
for selection in removed_ignored_rules.iter().sorted() {
let (prefix, code) = selection.prefix_and_code();
rules.push_str("\n - ");
rules.push_str(prefix);
rules.push_str(code);
}
rules.push('\n');
warn_user_once_by_message!(
"The following rules have been removed and ignoring them is useless:{rules}"
);
}

for (from, target) in redirects.iter().sorted_by_key(|item| item.0) {
// TODO(martin): This belongs into the ruff crate.
warn_user_once_by_id!(
Expand Down

0 comments on commit 2a831b9

Please sign in to comment.