Improve error messages and docs for flake8-comprehensions rules

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

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())
+    }
+}

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 {
@@ -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())
+    }
+}

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())
+    }
+}

crates/ruff_linter/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])`.

crates/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")
     }
 }