Improve error messages and docs for flake8-comprehensions rules

astral-sh · Dec 2, 2024 · b73ddb7 · b73ddb7
1 parent 76d2e56
commit b73ddb7
Show file tree

Hide file tree

Showing 36 changed files with 686 additions and 561 deletions.
diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs
@@ -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

diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_call_around_sorted.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_call_around_sorted.rs
@@ -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())
     }
 }
diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_collection_call.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_collection_call.rs
@@ -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 {
@@ -90,7  90,7 @@ pub(crate) fn unnecessary_collection_call(
 
     let mut diagnostic = Diagnostic::new(
         UnnecessaryCollectionCall {
-            obj_type: builtin.to_string(),
             kind: collection,
         },
         call.range(),
     );
@@ -136,8  136,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())
     }
 }
diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension.rs
@@ -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())
     }
 }
diff --git a/...nter/src/rules/flake8_comprehensions/rules/unnecessary_dict_comprehension_for_iterable.rs b/...nter/src/rules/flake8_comprehensions/rules/unnecessary_dict_comprehension_for_iterable.rs
@@ -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])`.

diff --git a/...s/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_double_cast_or_process.rs b/...s/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_double_cast_or_process.rs
@@ -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")
     }
 }