derive-msg-formats 1/5: Remove unnecessary usages of Rule::kind

This commit series removes the following associated function from the Violation trait: fn placeholder() -> Self; ruff previously used this placeholder approach for the messages it listed in the README and displayed when invoked with --explain <code>. This approach is suboptimal for three reasons: 1. The placeholder implementations are completely boring code since they just initialize the struct with some dummy values. 2. Displaying concrete error messages with arbitrary interpolated values can be confusing for the user since they might not recognize that the values are interpolated. 3. Some violations have varying format strings depending on the violation which could not be documented with the previous approach (while we could have changed the signature to return Vec<Self> this would still very much suffer from astral-sh#2 and astral-sh#3). We therefore drop Violation::placeholder in favor of a new macro-based approach, explaind in commit 4/5. Violation::placeholder is only invoked via Rule::kind, so we firstly have to get rid of all Rule::kind invocations ... this commit starts removing the trivial cases.
not-my-profile · Jan 19, 2023 · 8da817c · 8da817c
1 parent 6af31cd
commit 8da817c
Show file tree

Hide file tree

Showing 4 changed files with 90 additions and 100 deletions.
diff --git a/ruff_dev/src/generate_rules_table.rs b/ruff_dev/src/generate_rules_table.rs
@@ -25,13 +25,13 @@ fn generate_table(table_out: &mut String, prefix: &RuleCodePrefix) {
     table_out.push('\n');
     table_out.push_str("| ---- | ---- | ------- | --- |");
     table_out.push('\n');
-    for rule_code in prefix.codes() {
-        let kind = rule_code.kind();
+    for rule in prefix.codes() {
+        let kind = rule.kind();
         let fix_token = if kind.fixable() { "🛠" } else { "" };
         table_out.push_str(&format!(
             "| {} | {} | {} | {} |",
-            kind.rule().code(),
-            kind.as_ref(),
+            rule.code(),
+            rule.as_ref(),
             kind.summary().replace('|', r"\|"),
             fix_token
         ));

diff --git a/ruff_macros/src/define_rule_mapping.rs b/ruff_macros/src/define_rule_mapping.rs
@@ -70,6 +70,7 @@ pub fn define_rule_mapping(mapping: &Mapping) -> proc_macro2::TokenStream {
             Hash,
             PartialOrd,
             Ord,
+            AsRefStr,
         )]
         pub enum Rule { #rule_variants }
 

diff --git a/src/registry.rs b/src/registry.rs
@@ -725,17 +725,4 @@ mod tests {
             );
         }
     }
-
-    #[test]
-    fn fixable_codes() {
-        for rule in Rule::iter() {
-            let kind = rule.kind();
-            if kind.fixable() {
-                assert!(
-                    kind.commit().is_some(),
-                    "{rule:?} is fixable but has no commit message."
-                );
-            }
-        }
-    }
 }
diff --git a/src/rules/pandas_vet/rules.rs b/src/rules/pandas_vet/rules.rs
@@ -2,7 +2,7 @@ use rustpython_ast::{Constant, Expr, ExprKind, Keyword, Located};
 
 use crate::ast::types::{BindingKind, Range};
 use crate::checkers::ast::Checker;
-use crate::registry::{Diagnostic, Rule};
+use crate::registry::{Diagnostic, DiagnosticKind, Rule};
 use crate::violations;
 
 /// PD002
@@ -68,96 +68,98 @@ pub fn check_attr(
     value: &Located<ExprKind>,
     attr_expr: &Located<ExprKind>,
 ) {
-    for (code, name) in vec![
-        (Rule::UseOfDotIx, "ix"),
-        (Rule::UseOfDotAt, "at"),
-        (Rule::UseOfDotIat, "iat"),
-        (Rule::UseOfDotValues, "values"),
-    ] {
-        if checker.settings.rules.enabled(&code) {
-            if attr == name {
-                // Avoid flagging on function calls (e.g., `df.values()`).
-                if let Some(parent) = checker.current_expr_parent() {
-                    if matches!(parent.node, ExprKind::Call { .. }) {
-                        continue;
-                    }
-                }
-                // Avoid flagging on non-DataFrames (e.g., `{"a": 1}.values`).
-                if super::helpers::is_dataframe_candidate(value) {
-                    // If the target is a named variable, avoid triggering on
-                    // irrelevant bindings (like imports).
-                    if let ExprKind::Name { id, .. } = &value.node {
-                        if checker.find_binding(id).map_or(true, |binding| {
-                            matches!(
-                                binding.kind,
-                                BindingKind::Builtin
-                                    | BindingKind::ClassDefinition
-                                    | BindingKind::FunctionDefinition
-                                    | BindingKind::Export(..)
-                                    | BindingKind::FutureImportation
-                                    | BindingKind::StarImportation(..)
-                                    | BindingKind::Importation(..)
-                                    | BindingKind::FromImportation(..)
-                                    | BindingKind::SubmoduleImportation(..)
-                            )
-                        }) {
-                            continue;
-                        }
-                    }
+    let rules = &checker.settings.rules;
+    let violation: DiagnosticKind = match attr {
+        "ix" if rules.enabled(&Rule::UseOfDotIx) => violations::UseOfDotIx.into(),
+        "at" if rules.enabled(&Rule::UseOfDotAt) => violations::UseOfDotAt.into(),
+        "iat" if rules.enabled(&Rule::UseOfDotIat) => violations::UseOfDotIat.into(),
+        "values" if rules.enabled(&Rule::UseOfDotValues) => violations::UseOfDotValues.into(),
+        _ => return,
+    };
 
-                    checker
-                        .diagnostics
-                        .push(Diagnostic::new(code.kind(), Range::from_located(attr_expr)));
-                }
-            };
+    // Avoid flagging on function calls (e.g., `df.values()`).
+    if let Some(parent) = checker.current_expr_parent() {
+        if matches!(parent.node, ExprKind::Call { .. }) {
+            return;
         }
     }
+    // Avoid flagging on non-DataFrames (e.g., `{"a": 1}.values`).
+    if !super::helpers::is_dataframe_candidate(value) {
+        return;
+    }
+
+    // If the target is a named variable, avoid triggering on
+    // irrelevant bindings (like imports).
+    if let ExprKind::Name { id, .. } = &value.node {
+        if checker.find_binding(id).map_or(true, |binding| {
+            matches!(
+                binding.kind,
+                BindingKind::Builtin
+                    | BindingKind::ClassDefinition
+                    | BindingKind::FunctionDefinition
+                    | BindingKind::Export(..)
+                    | BindingKind::FutureImportation
+                    | BindingKind::StarImportation(..)
+                    | BindingKind::Importation(..)
+                    | BindingKind::FromImportation(..)
+                    | BindingKind::SubmoduleImportation(..)
+            )
+        }) {
+            return;
+        }
+    }
+
+    checker
+        .diagnostics
+        .push(Diagnostic::new(violation, Range::from_located(attr_expr)));
 }
 
 pub fn check_call(checker: &mut Checker, func: &Located<ExprKind>) {
-    for (code, name) in vec![
-        (Rule::UseOfDotIsNull, "isnull"),
-        (Rule::UseOfDotNotNull, "notnull"),
-        (Rule::UseOfDotPivotOrUnstack, "pivot"),
-        (Rule::UseOfDotPivotOrUnstack, "unstack"),
-        (Rule::UseOfDotReadTable, "read_table"),
-        (Rule::UseOfDotStack, "stack"),
-    ] {
-        if checker.settings.rules.enabled(&code) {
-            if let ExprKind::Attribute { value, attr, .. } = &func.node {
-                if attr == name {
-                    if super::helpers::is_dataframe_candidate(value) {
-                        // If the target is a named variable, avoid triggering on
-                        // irrelevant bindings (like non-Pandas imports).
-                        if let ExprKind::Name { id, .. } = &value.node {
-                            if checker.find_binding(id).map_or(true, |binding| {
-                                if let BindingKind::Importation(.., module) = &binding.kind {
-                                    module != &"pandas"
-                                } else {
-                                    matches!(
-                                        binding.kind,
-                                        BindingKind::Builtin
-                                            | BindingKind::ClassDefinition
-                                            | BindingKind::FunctionDefinition
-                                            | BindingKind::Export(..)
-                                            | BindingKind::FutureImportation
-                                            | BindingKind::StarImportation(..)
-                                            | BindingKind::Importation(..)
-                                            | BindingKind::FromImportation(..)
-                                            | BindingKind::SubmoduleImportation(..)
-                                    )
-                                }
-                            }) {
-                                continue;
-                            }
-                        }
+    let rules = &checker.settings.rules;
+    let ExprKind::Attribute { value, attr, .. } = &func.node else {return};
+    let violation: DiagnosticKind = match attr.as_str() {
+        "isnull" if rules.enabled(&Rule::UseOfDotIsNull) => violations::UseOfDotIsNull.into(),
+        "notnull" if rules.enabled(&Rule::UseOfDotNotNull) => violations::UseOfDotNotNull.into(),
+        "pivot" | "unstack" if rules.enabled(&Rule::UseOfDotPivotOrUnstack) => {
+            violations::UseOfDotPivotOrUnstack.into()
+        }
+        "read_table" if rules.enabled(&Rule::UseOfDotReadTable) => {
+            violations::UseOfDotReadTable.into()
+        }
+        "stack" if rules.enabled(&Rule::UseOfDotStack) => violations::UseOfDotStack.into(),
+        _ => return,
+    };
 
-                        checker
-                            .diagnostics
-                            .push(Diagnostic::new(code.kind(), Range::from_located(func)));
-                    }
-                };
+    if !super::helpers::is_dataframe_candidate(value) {
+        return;
+    }
+
+    // If the target is a named variable, avoid triggering on
+    // irrelevant bindings (like non-Pandas imports).
+    if let ExprKind::Name { id, .. } = &value.node {
+        if checker.find_binding(id).map_or(true, |binding| {
+            if let BindingKind::Importation(.., module) = &binding.kind {
+                module != &"pandas"
+            } else {
+                matches!(
+                    binding.kind,
+                    BindingKind::Builtin
+                        | BindingKind::ClassDefinition
+                        | BindingKind::FunctionDefinition
+                        | BindingKind::Export(..)
+                        | BindingKind::FutureImportation
+                        | BindingKind::StarImportation(..)
+                        | BindingKind::Importation(..)
+                        | BindingKind::FromImportation(..)
+                        | BindingKind::SubmoduleImportation(..)
+                )
             }
+        }) {
+            return;
         }
     }
+
+    checker
+        .diagnostics
+        .push(Diagnostic::new(violation, Range::from_located(func)));
 }
-Original file line number
+Diff line change
@@ Expand Up @@
                 Hash,
                 PartialOrd,
                 Ord,
+                AsRefStr,
             )]
             pub enum Rule { #rule_variants }
@@ Expand Down @@