From 8da817c4ff7634f988b209ad19228fd6381236af Mon Sep 17 00:00:00 2001 From: Martin Fischer Date: Thu, 19 Jan 2023 13:16:48 +0100 Subject: [PATCH] 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 . 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 this would still very much suffer from #2 and #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 | 8 +- ruff_macros/src/define_rule_mapping.rs | 1 + src/registry.rs | 13 -- src/rules/pandas_vet/rules.rs | 168 +++++++++++++------------ 4 files changed, 90 insertions(+), 100 deletions(-) diff --git a/ruff_dev/src/generate_rules_table.rs b/ruff_dev/src/generate_rules_table.rs index e24d4f3273373f..a1cfa001787ad8 100644 --- 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 index b6537e6f4fe2cd..006360b103611c 100644 --- 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 index 8ca76132ecc83a..10aecb6512f8d1 100644 --- 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 index 8244bf676f9131..9fab1cdf86f4a0 100644 --- 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, attr_expr: &Located, ) { - 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) { - 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))); }