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

…n ruff

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.

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
  ));

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 }
 

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."
- );
- }
- }
- }
 }

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)));
 }