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

Warn instead of error when removed rules are used in ignore #14435

Merged
merged 2 commits into from
Nov 19, 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
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 has no effect:
- 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 has no effect:
- 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,40 @@
use ruff_diagnostics::Violation;
use ruff_macros::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
AlexWaygood marked this conversation as resolved.
Show resolved Hide resolved
/// 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 {
fn message(&self) -> String {
unreachable!("UP027 has been removed")
}

fn message_formats() -> &'static [&'static str] {
&["Replace unpacked list comprehension with a generator expression"]
}
}
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 has no effect:{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
Loading