Skip to content

Commit

Permalink
Improve error messages and docs for flake8-comprehensions rules
Browse files Browse the repository at this point in the history
  • Loading branch information
AlexWaygood committed Dec 2, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
1 parent 76d2e56 commit a7f42c8
Showing 36 changed files with 691 additions and 577 deletions.
Original file line number Diff line number Diff line change
@@ -295,7 +295,7 @@ pub(crate) fn fix_unnecessary_collection_call(

/// Re-formats the given expression for use within a formatted string.
///
/// For example, when converting a `dict` call to a dictionary literal within
/// For example, when converting a `dict()` call to a dictionary literal within
/// a formatted string, we might naively generate the following code:
///
/// ```python
Original file line number Diff line number Diff line change
@@ -8,18 +8,18 @@ use crate::checkers::ast::Checker;
use crate::rules::flake8_comprehensions::fixes;

/// ## What it does
/// Checks for unnecessary `list` or `reversed` calls around `sorted`
/// Checks for unnecessary `list()` or `reversed()` calls around `sorted()`
/// calls.
///
/// ## Why is this bad?
/// It is unnecessary to use `list` around `sorted`, as the latter already
/// It is unnecessary to use `list()` around `sorted()`, as the latter already
/// returns a list.
///
/// It is also unnecessary to use `reversed` around `sorted`, as the latter
/// It is also unnecessary to use `reversed()` around `sorted()`, as the latter
/// has a `reverse` argument that can be used in lieu of an additional
/// `reversed` call.
/// `reversed()` call.
///
/// In both cases, it's clearer to avoid the redundant call.
/// In both cases, it's clearer and more efficient to avoid the redundant call.
///
/// ## Examples
/// ```python
@@ -32,27 +32,27 @@ use crate::rules::flake8_comprehensions::fixes;
/// ```
///
/// ## Fix safety
/// This rule's fix is marked as unsafe, as `reversed` and `reverse=True` will
/// This rule's fix is marked as unsafe, as `reversed()` and `reverse=True` will
/// yield different results in the event of custom sort keys or equality
/// functions. Specifically, `reversed` will reverse the order of the
/// collection, while `sorted` with `reverse=True` will perform a stable
/// functions. Specifically, `reversed()` will reverse the order of the
/// collection, while `sorted()` with `reverse=True` will perform a stable
/// reverse sort, which will preserve the order of elements that compare as
/// equal.
#[derive(ViolationMetadata)]
pub(crate) struct UnnecessaryCallAroundSorted {
func: String,
func: UnnecessaryFunction,
}

impl AlwaysFixableViolation for UnnecessaryCallAroundSorted {
#[derive_message_formats]
fn message(&self) -> String {
let UnnecessaryCallAroundSorted { func } = self;
format!("Unnecessary `{func}` call around `sorted()`")
format!("Unnecessary `{func}()` call around `sorted()`")
}

fn fix_title(&self) -> String {
let UnnecessaryCallAroundSorted { func } = self;
format!("Remove unnecessary `{func}` call")
format!("Remove unnecessary `{func}()` call")
}
}

@@ -73,27 +73,55 @@ pub(crate) fn unnecessary_call_around_sorted(
let Some(outer_func_name) = semantic.resolve_builtin_symbol(outer_func) else {
return;
};
if !matches!(outer_func_name, "list" | "reversed") {
let Some(unnecessary_function) = UnnecessaryFunction::try_from_str(outer_func_name) else {
return;
}
};
if !semantic.match_builtin_expr(inner_func, "sorted") {
return;
}
let mut diagnostic = Diagnostic::new(
UnnecessaryCallAroundSorted {
func: outer_func_name.to_string(),
func: unnecessary_function,
},
expr.range(),
);
diagnostic.try_set_fix(|| {
Ok(Fix::applicable_edit(
fixes::fix_unnecessary_call_around_sorted(expr, checker.locator(), checker.stylist())?,
if outer_func_name == "reversed" {
Applicability::Unsafe
} else {
Applicability::Safe
},
))
let edit =
fixes::fix_unnecessary_call_around_sorted(expr, checker.locator(), checker.stylist())?;
let applicability = match unnecessary_function {
UnnecessaryFunction::List => Applicability::Safe,
UnnecessaryFunction::Reversed => Applicability::Unsafe,
};
Ok(Fix::applicable_edit(edit, applicability))
});
checker.diagnostics.push(diagnostic);
}

#[derive(Debug, Copy, Clone, PartialEq, Eq)]
enum UnnecessaryFunction {
List,
Reversed,
}

impl UnnecessaryFunction {
fn try_from_str(name: &str) -> Option<Self> {
match name {
"list" => Some(Self::List),
"reversed" => Some(Self::Reversed),
_ => None,
}
}

const fn as_str(self) -> &'static str {
match self {
Self::List => "list",
Self::Reversed => "reversed",
}
}
}

impl std::fmt::Display for UnnecessaryFunction {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.write_str(self.as_str())
}
}
Original file line number Diff line number Diff line change
@@ -9,7 +9,7 @@ use crate::rules::flake8_comprehensions::fixes::{pad_end, pad_start};
use crate::rules::flake8_comprehensions::settings::Settings;

/// ## What it does
/// Checks for unnecessary `dict`, `list` or `tuple` calls that can be
/// Checks for unnecessary `dict()`, `list()` or `tuple()` calls that can be
/// rewritten as empty literals.
///
/// ## Why is this bad?
@@ -41,14 +41,14 @@ use crate::rules::flake8_comprehensions::settings::Settings;
/// - `lint.flake8-comprehensions.allow-dict-calls-with-keyword-arguments`
#[derive(ViolationMetadata)]
pub(crate) struct UnnecessaryCollectionCall {
obj_type: String,
kind: Collection,
}

impl AlwaysFixableViolation for UnnecessaryCollectionCall {
#[derive_message_formats]
fn message(&self) -> String {
let UnnecessaryCollectionCall { obj_type } = self;
format!("Unnecessary `{obj_type}` call (rewrite as a literal)")
let UnnecessaryCollectionCall { kind } = self;
format!("Unnecessary `{kind}()` call (rewrite as a literal)")
}

fn fix_title(&self) -> String {
@@ -88,12 +88,8 @@ pub(crate) fn unnecessary_collection_call(
_ => return,
};

let mut diagnostic = Diagnostic::new(
UnnecessaryCollectionCall {
obj_type: builtin.to_string(),
},
call.range(),
);
let mut diagnostic =
Diagnostic::new(UnnecessaryCollectionCall { kind: collection }, call.range());

// Convert `dict()` to `{}`.
if call.arguments.keywords.is_empty() {
@@ -136,8 +132,25 @@ pub(crate) fn unnecessary_collection_call(
checker.diagnostics.push(diagnostic);
}

#[derive(Debug, Copy, Clone, PartialEq, Eq)]
enum Collection {
Tuple,
List,
Dict,
}

impl Collection {
const fn as_str(self) -> &'static str {
match self {
Self::Dict => "dict",
Self::List => "list",
Self::Tuple => "tuple",
}
}
}

impl std::fmt::Display for Collection {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.write_str(self.as_str())
}
}
Original file line number Diff line number Diff line change
@@ -9,10 +9,10 @@ use crate::checkers::ast::Checker;
use crate::rules::flake8_comprehensions::fixes;

/// ## What it does
/// Checks for unnecessary `dict`, `list`, and `set` comprehension.
/// Checks for unnecessary dict, list, and set comprehension.
///
/// ## Why is this bad?
/// It's unnecessary to use a `dict`/`list`/`set` comprehension to build a data structure if the
/// It's unnecessary to use a dict/list/set comprehension to build a data structure if the
/// elements are unchanged. Wrap the iterable with `dict()`, `list()`, or `set()` instead.
///
/// ## Examples
@@ -32,9 +32,9 @@ use crate::rules::flake8_comprehensions::fixes;
/// ## Known problems
///
/// This rule may produce false positives for dictionary comprehensions that iterate over a mapping.
/// The `dict` constructor behaves differently depending on if it receives a sequence (e.g., a
/// `list`) or a mapping (e.g., a `dict`). When a comprehension iterates over the keys of a mapping,
/// replacing it with a `dict` constructor call will give a different result.
/// The dict constructor behaves differently depending on if it receives a sequence (e.g., a
/// list) or a mapping (e.g., a dict). When a comprehension iterates over the keys of a mapping,
/// replacing it with a `dict()` constructor call will give a different result.
///
/// For example:
///
@@ -58,36 +58,36 @@ use crate::rules::flake8_comprehensions::fixes;
/// Additionally, this fix may drop comments when rewriting the comprehension.
#[derive(ViolationMetadata)]
pub(crate) struct UnnecessaryComprehension {
obj_type: String,
kind: ComprehensionKind,
}

impl AlwaysFixableViolation for UnnecessaryComprehension {
#[derive_message_formats]
fn message(&self) -> String {
let UnnecessaryComprehension { obj_type } = self;
format!("Unnecessary `{obj_type}` comprehension (rewrite using `{obj_type}()`)")
let UnnecessaryComprehension { kind } = self;
format!("Unnecessary {kind} comprehension (rewrite using `{kind}()`)")
}

fn fix_title(&self) -> String {
let UnnecessaryComprehension { obj_type } = self;
format!("Rewrite using `{obj_type}()`")
let UnnecessaryComprehension { kind } = self;
format!("Rewrite using `{kind}()`")
}
}

/// Add diagnostic for C416 based on the expression node id.
fn add_diagnostic(checker: &mut Checker, expr: &Expr) {
let id = match expr {
Expr::ListComp(_) => "list",
Expr::SetComp(_) => "set",
Expr::DictComp(_) => "dict",
_ => return,
let Some(comprehension_kind) = ComprehensionKind::try_from_expr(expr) else {
return;
};
if !checker.semantic().has_builtin_binding(id) {
if !checker
.semantic()
.has_builtin_binding(comprehension_kind.as_str())
{
return;
}
let mut diagnostic = Diagnostic::new(
UnnecessaryComprehension {
obj_type: id.to_string(),
kind: comprehension_kind,
},
expr.range(),
);
@@ -145,3 +145,35 @@ pub(crate) fn unnecessary_list_set_comprehension(
}
add_diagnostic(checker, expr);
}

#[derive(Debug, Copy, Clone, PartialEq, Eq)]
enum ComprehensionKind {
List,
Set,
Dict,
}

impl ComprehensionKind {
const fn as_str(self) -> &'static str {
match self {
Self::List => "list",
Self::Dict => "dict",
Self::Set => "set",
}
}

const fn try_from_expr(expr: &Expr) -> Option<Self> {
match expr {
Expr::ListComp(_) => Some(Self::List),
Expr::DictComp(_) => Some(Self::Dict),
Expr::SetComp(_) => Some(Self::Set),
_ => None,
}
}
}

impl std::fmt::Display for ComprehensionKind {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.write_str(self.as_str())
}
}
Original file line number Diff line number Diff line change
@@ -9,11 +9,11 @@ use ruff_text_size::{Ranged, TextRange};
use crate::checkers::ast::Checker;

/// ## What it does
/// Checks for unnecessary `dict` comprehension when creating a dictionary from
/// Checks for unnecessary dict comprehension when creating a dictionary from
/// an iterable.
///
/// ## Why is this bad?
/// It's unnecessary to use a `dict` comprehension to build a dictionary from
/// It's unnecessary to use a dict comprehension to build a dictionary from
/// an iterable when the value is static.
///
/// Prefer `dict.fromkeys(iterable)` over `{value: None for value in iterable}`,
@@ -155,7 +155,7 @@ fn is_constant_like(expr: &Expr) -> bool {
})
}

/// Generate a [`Fix`] to replace `dict` comprehension with `dict.fromkeys`.
/// Generate a [`Fix`] to replace a dict comprehension with `dict.fromkeys`.
///
/// For example:
/// - Given `{n: None for n in [1,2,3]}`, generate `dict.fromkeys([1,2,3])`.
Original file line number Diff line number Diff line change
@@ -9,13 +9,13 @@ use crate::checkers::ast::Checker;
use crate::rules::flake8_comprehensions::fixes;

/// ## What it does
/// Checks for unnecessary `list`, `reversed`, `set`, `sorted`, and `tuple`
/// call within `list`, `set`, `sorted`, and `tuple` calls.
/// Checks for unnecessary `list()`, `reversed()`, `set()`, `sorted()`, and
/// `tuple()` call within `list()`, `set()`, `sorted()`, and `tuple()` calls.
///
/// ## Why is this bad?
/// It's unnecessary to double-cast or double-process iterables by wrapping
/// the listed functions within an additional `list`, `set`, `sorted`, or
/// `tuple` call. Doing so is redundant and can be confusing for readers.
/// the listed functions within an additional `list()`, `set()`, `sorted()`, or
/// `tuple()` call. Doing so is redundant and can be confusing for readers.
///
/// ## Examples
/// ```python
@@ -27,8 +27,8 @@ use crate::rules::flake8_comprehensions::fixes;
/// list(iterable)
/// ```
///
/// This rule applies to a variety of functions, including `list`, `reversed`,
/// `set`, `sorted`, and `tuple`. For example:
/// This rule applies to a variety of functions, including `list()`, `reversed()`,
/// `set()`, `sorted()`, and `tuple()`. For example:
///
/// - Instead of `list(list(iterable))`, use `list(iterable)`.
/// - Instead of `list(tuple(iterable))`, use `list(iterable)`.
@@ -57,12 +57,12 @@ impl AlwaysFixableViolation for UnnecessaryDoubleCastOrProcess {
#[derive_message_formats]
fn message(&self) -> String {
let UnnecessaryDoubleCastOrProcess { inner, outer } = self;
format!("Unnecessary `{inner}` call within `{outer}()`")
format!("Unnecessary `{inner}()` call within `{outer}()`")
}

fn fix_title(&self) -> String {
let UnnecessaryDoubleCastOrProcess { inner, .. } = self;
format!("Remove the inner `{inner}` call")
format!("Remove the inner `{inner}()` call")
}
}

Loading

0 comments on commit a7f42c8

Please sign in to comment.