From 968635940638c3e34fe0564685c944db2cfce1a2 Mon Sep 17 00:00:00 2001 From: Brent Westbrook Date: Tue, 13 May 2025 15:15:06 -0400 Subject: [PATCH 1/2] Serialize `Rule` for `CacheMessage` and restore in Pascal-case Summary -- This PR is stacked on https://github.com/astral-sh/ruff/pull/18074 and uses the separation between `CacheMessage` and the other diagnostic types added there to store a `Rule` directly on `CacheMessage` and convert it back to a `&'static str` for use by `Diagnostic` and `DiagnosticMessage`. I think what we really want is to use only the kebab-case names everywhere, but I don't think we can accomplish that with `Diagnostic::new` living in the `ruff_diagnostics` crate, and the proc macros being called in `ruff_linter`. `Diagnostic::new` relies on `ViolationMetadata::rule_name`, which just calls `stringify!(#name)`. Test Plan -- Existing tests --- crates/ruff/src/cache.rs | 9 +++++---- crates/ruff_diagnostics/src/diagnostic.rs | 4 ++-- crates/ruff_linter/src/message/mod.rs | 16 ++++++++-------- crates/ruff_macros/src/map_codes.rs | 15 +++++++++++++-- crates/ruff_server/src/lint.rs | 2 +- 5 files changed, 29 insertions(+), 17 deletions(-) diff --git a/crates/ruff/src/cache.rs b/crates/ruff/src/cache.rs index c5716fac27523..7baff8ee98a98 100644 --- a/crates/ruff/src/cache.rs +++ b/crates/ruff/src/cache.rs @@ -13,6 +13,7 @@ use itertools::Itertools; use log::{debug, error}; use rayon::iter::ParallelIterator; use rayon::iter::{IntoParallelIterator, ParallelBridge}; +use ruff_linter::{codes::Rule, registry::AsRule}; use rustc_hash::FxHashMap; use serde::{Deserialize, Serialize}; use tempfile::NamedTempFile; @@ -348,7 +349,7 @@ impl FileCache { .iter() .map(|msg| { Message::Diagnostic(DiagnosticMessage { - name: msg.name.clone(), + name: msg.rule.pascal_name(), body: msg.body.clone(), suggestion: msg.suggestion.clone(), range: msg.range, @@ -446,7 +447,7 @@ impl LintCacheData { "message uses a different source file" ); CacheMessage { - name: msg.name.clone(), + rule: msg.rule(), body: msg.body.clone(), suggestion: msg.suggestion.clone(), range: msg.range, @@ -468,8 +469,8 @@ impl LintCacheData { /// On disk representation of a diagnostic message. #[derive(Deserialize, Debug, Serialize, PartialEq)] pub(super) struct CacheMessage { - /// The identifier of the diagnostic, used to align the diagnostic with a rule. - name: String, + /// The rule for the cached diagnostic. + rule: Rule, /// The message body to display to the user, to explain the diagnostic. body: String, /// The message to display to the user, to explain the suggested fix. diff --git a/crates/ruff_diagnostics/src/diagnostic.rs b/crates/ruff_diagnostics/src/diagnostic.rs index 81b7cbc3cdd01..f9a93c78f140a 100644 --- a/crates/ruff_diagnostics/src/diagnostic.rs +++ b/crates/ruff_diagnostics/src/diagnostic.rs @@ -8,7 +8,7 @@ use crate::{Fix, Violation}; #[derive(Debug, PartialEq, Eq, Clone)] pub struct Diagnostic { /// The identifier of the diagnostic, used to align the diagnostic with a rule. - pub name: String, + pub name: &'static str, /// The message body to display to the user, to explain the diagnostic. pub body: String, /// The message to display to the user, to explain the suggested fix. @@ -21,7 +21,7 @@ pub struct Diagnostic { impl Diagnostic { pub fn new(kind: T, range: TextRange) -> Self { Self { - name: T::rule_name().to_string(), + name: T::rule_name(), body: Violation::message(&kind), suggestion: Violation::fix_title(&kind), range, diff --git a/crates/ruff_linter/src/message/mod.rs b/crates/ruff_linter/src/message/mod.rs index 20c4c5d8cd7a3..fb7211cdef7c6 100644 --- a/crates/ruff_linter/src/message/mod.rs +++ b/crates/ruff_linter/src/message/mod.rs @@ -53,7 +53,7 @@ pub enum Message { /// A diagnostic message corresponding to a rule violation. #[derive(Clone, Debug, PartialEq, Eq)] pub struct DiagnosticMessage { - pub name: String, + pub name: &'static str, pub body: String, pub suggestion: Option, pub range: TextRange, @@ -195,7 +195,7 @@ impl Message { /// Returns the name used to represent the diagnostic. pub fn name(&self) -> &str { match self { - Message::Diagnostic(m) => &m.name, + Message::Diagnostic(m) => m.name, Message::SyntaxError(_) => "SyntaxError", } } @@ -429,7 +429,7 @@ def fibonacci(n): let unused_import_start = TextSize::from(7); let unused_import = DiagnosticMessage { - name: "UnusedImport".to_string(), + name: "UnusedImport", body: "`os` imported but unused".to_string(), suggestion: Some("Remove unused import: `os`".to_string()), range: TextRange::new(unused_import_start, TextSize::from(9)), @@ -444,7 +444,7 @@ def fibonacci(n): let unused_variable_start = TextSize::from(94); let unused_variable = DiagnosticMessage { - name: "UnusedVariable".to_string(), + name: "UnusedVariable", body: "Local variable `x` is assigned to but never used".to_string(), suggestion: Some("Remove assignment to unused variable `x`".to_string()), range: TextRange::new(unused_variable_start, TextSize::from(95)), @@ -461,7 +461,7 @@ def fibonacci(n): let undefined_name_start = TextSize::from(3); let undefined_name = DiagnosticMessage { - name: "UndefinedName".to_string(), + name: "UndefinedName", body: "Undefined name `a`".to_string(), suggestion: None, range: TextRange::new(undefined_name_start, TextSize::from(4)), @@ -495,7 +495,7 @@ def foo(): let unused_import_os_start = TextSize::from(16); let unused_import_os = DiagnosticMessage { - name: "UnusedImport".to_string(), + name: "UnusedImport", body: "`os` imported but unused".to_string(), suggestion: Some("Remove unused import: `os`".to_string()), range: TextRange::new(unused_import_os_start, TextSize::from(18)), @@ -510,7 +510,7 @@ def foo(): let unused_import_math_start = TextSize::from(35); let unused_import_math = DiagnosticMessage { - name: "UnusedImport".to_string(), + name: "UnusedImport", body: "`math` imported but unused".to_string(), suggestion: Some("Remove unused import: `math`".to_string()), range: TextRange::new(unused_import_math_start, TextSize::from(39)), @@ -525,7 +525,7 @@ def foo(): let unused_variable_start = TextSize::from(98); let unused_variable = DiagnosticMessage { - name: "UnusedVariable".to_string(), + name: "UnusedVariable", body: "Local variable `x` is assigned to but never used".to_string(), suggestion: Some("Remove assignment to unused variable `x`".to_string()), range: TextRange::new(unused_variable_start, TextSize::from(99)), diff --git a/crates/ruff_macros/src/map_codes.rs b/crates/ruff_macros/src/map_codes.rs index 1f7c1fe5ad756..39f7b60dbf3e4 100644 --- a/crates/ruff_macros/src/map_codes.rs +++ b/crates/ruff_macros/src/map_codes.rs @@ -403,6 +403,7 @@ fn register_rules<'a>(input: impl Iterator) -> TokenStream { let mut rule_message_formats_match_arms = quote!(); let mut rule_fixable_match_arms = quote!(); let mut rule_explanation_match_arms = quote!(); + let mut rule_pascal_match_arms = quote!(); let mut from_impls_for_diagnostic_kind = quote!(); @@ -425,6 +426,9 @@ fn register_rules<'a>(input: impl Iterator) -> TokenStream { // Enable conversion from `DiagnosticKind` to `Rule`. from_impls_for_diagnostic_kind .extend(quote! {#(#attrs)* stringify!(#name) => Rule::#name,}); + + // Enable conversion from a `Rule` to its Pascal-case variant name. + rule_pascal_match_arms.extend(quote! {#(#attrs)* Rule::#name => stringify!(#name),}); } quote! { @@ -443,6 +447,8 @@ fn register_rules<'a>(input: impl Iterator) -> TokenStream { ::ruff_macros::CacheKey, AsRefStr, ::strum_macros::IntoStaticStr, + ::serde::Serialize, + ::serde::Deserialize, )] #[repr(u16)] #[strum(serialize_all = "kebab-case")] @@ -464,11 +470,16 @@ fn register_rules<'a>(input: impl Iterator) -> TokenStream { pub const fn fixable(&self) -> ruff_diagnostics::FixAvailability { match self { #rule_fixable_match_arms } } + + /// Returns the Pascal-case variant name of this rule. + pub const fn pascal_name(&self) -> &'static str { + match self { #rule_pascal_match_arms } + } } impl AsRule for ruff_diagnostics::Diagnostic { fn rule(&self) -> Rule { - match self.name.as_str() { + match self.name { #from_impls_for_diagnostic_kind _ => unreachable!("invalid rule name: {}", self.name), } @@ -477,7 +488,7 @@ fn register_rules<'a>(input: impl Iterator) -> TokenStream { impl AsRule for crate::message::DiagnosticMessage { fn rule(&self) -> Rule { - match self.name.as_str() { + match self.name { #from_impls_for_diagnostic_kind _ => unreachable!("invalid rule name: {}", self.name), } diff --git a/crates/ruff_server/src/lint.rs b/crates/ruff_server/src/lint.rs index b46b782ad5647..586043a0249fd 100644 --- a/crates/ruff_server/src/lint.rs +++ b/crates/ruff_server/src/lint.rs @@ -273,7 +273,7 @@ fn to_lsp_diagnostic( new_text: noqa_edit.into_content().unwrap_or_default().into_string(), }); serde_json::to_value(AssociatedDiagnosticData { - title: suggestion.unwrap_or(name), + title: suggestion.unwrap_or_else(|| name.to_string()), noqa_edit, edits, code: rule.noqa_code().to_string(), From 9188fdf341eb847027f2cabc9d66f1b95601b764 Mon Sep 17 00:00:00 2001 From: Brent Westbrook Date: Tue, 13 May 2025 16:54:57 -0400 Subject: [PATCH 2/2] use kebab-case everywhere --- Cargo.lock | 1 + Cargo.toml | 1 + crates/ruff/src/cache.rs | 2 +- crates/ruff_linter/src/message/mod.rs | 12 ++++++------ crates/ruff_macros/Cargo.toml | 1 + crates/ruff_macros/src/kebab_case.rs | 15 +++------------ crates/ruff_macros/src/lib.rs | 2 +- crates/ruff_macros/src/map_codes.rs | 11 +---------- crates/ruff_macros/src/violation_metadata.rs | 2 +- 9 files changed, 16 insertions(+), 31 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a967621aae297..145dcd05d9801 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2876,6 +2876,7 @@ dependencies = [ name = "ruff_macros" version = "0.0.0" dependencies = [ + "heck", "itertools 0.14.0", "proc-macro2", "quote", diff --git a/Cargo.toml b/Cargo.toml index d28dd23848022..59fb0eeeea0c6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -85,6 +85,7 @@ hashbrown = { version = "0.15.0", default-features = false, features = [ "equivalent", "inline-more", ] } +heck = "0.5.0" ignore = { version = "0.4.22" } imara-diff = { version = "0.1.5" } imperative = { version = "1.0.4" } diff --git a/crates/ruff/src/cache.rs b/crates/ruff/src/cache.rs index 7baff8ee98a98..99eed7cbafca6 100644 --- a/crates/ruff/src/cache.rs +++ b/crates/ruff/src/cache.rs @@ -349,7 +349,7 @@ impl FileCache { .iter() .map(|msg| { Message::Diagnostic(DiagnosticMessage { - name: msg.rule.pascal_name(), + name: msg.rule.into(), body: msg.body.clone(), suggestion: msg.suggestion.clone(), range: msg.range, diff --git a/crates/ruff_linter/src/message/mod.rs b/crates/ruff_linter/src/message/mod.rs index fb7211cdef7c6..8e1961d025651 100644 --- a/crates/ruff_linter/src/message/mod.rs +++ b/crates/ruff_linter/src/message/mod.rs @@ -429,7 +429,7 @@ def fibonacci(n): let unused_import_start = TextSize::from(7); let unused_import = DiagnosticMessage { - name: "UnusedImport", + name: "unused-import", body: "`os` imported but unused".to_string(), suggestion: Some("Remove unused import: `os`".to_string()), range: TextRange::new(unused_import_start, TextSize::from(9)), @@ -444,7 +444,7 @@ def fibonacci(n): let unused_variable_start = TextSize::from(94); let unused_variable = DiagnosticMessage { - name: "UnusedVariable", + name: "unused-variable", body: "Local variable `x` is assigned to but never used".to_string(), suggestion: Some("Remove assignment to unused variable `x`".to_string()), range: TextRange::new(unused_variable_start, TextSize::from(95)), @@ -461,7 +461,7 @@ def fibonacci(n): let undefined_name_start = TextSize::from(3); let undefined_name = DiagnosticMessage { - name: "UndefinedName", + name: "undefined-name", body: "Undefined name `a`".to_string(), suggestion: None, range: TextRange::new(undefined_name_start, TextSize::from(4)), @@ -495,7 +495,7 @@ def foo(): let unused_import_os_start = TextSize::from(16); let unused_import_os = DiagnosticMessage { - name: "UnusedImport", + name: "unused-import", body: "`os` imported but unused".to_string(), suggestion: Some("Remove unused import: `os`".to_string()), range: TextRange::new(unused_import_os_start, TextSize::from(18)), @@ -510,7 +510,7 @@ def foo(): let unused_import_math_start = TextSize::from(35); let unused_import_math = DiagnosticMessage { - name: "UnusedImport", + name: "unused-import", body: "`math` imported but unused".to_string(), suggestion: Some("Remove unused import: `math`".to_string()), range: TextRange::new(unused_import_math_start, TextSize::from(39)), @@ -525,7 +525,7 @@ def foo(): let unused_variable_start = TextSize::from(98); let unused_variable = DiagnosticMessage { - name: "UnusedVariable", + name: "unused-variable", body: "Local variable `x` is assigned to but never used".to_string(), suggestion: Some("Remove assignment to unused variable `x`".to_string()), range: TextRange::new(unused_variable_start, TextSize::from(99)), diff --git a/crates/ruff_macros/Cargo.toml b/crates/ruff_macros/Cargo.toml index c0215053c5393..7e72d59855f72 100644 --- a/crates/ruff_macros/Cargo.toml +++ b/crates/ruff_macros/Cargo.toml @@ -17,6 +17,7 @@ doctest = false [dependencies] ruff_python_trivia = { workspace = true } +heck = { workspace = true } proc-macro2 = { workspace = true } quote = { workspace = true } syn = { workspace = true, features = ["derive", "parsing", "extra-traits", "full"] } diff --git a/crates/ruff_macros/src/kebab_case.rs b/crates/ruff_macros/src/kebab_case.rs index 1f6dcf42fd387..3b7ef3f8db0a0 100644 --- a/crates/ruff_macros/src/kebab_case.rs +++ b/crates/ruff_macros/src/kebab_case.rs @@ -1,19 +1,10 @@ +use heck::ToKebabCase; use proc_macro2::TokenStream; pub(crate) fn kebab_case(input: &syn::Ident) -> TokenStream { - let screaming_snake_case = input.to_string(); + let s = input.to_string(); - let mut kebab_case = String::with_capacity(screaming_snake_case.len()); - - for (i, word) in screaming_snake_case.split('_').enumerate() { - if i > 0 { - kebab_case.push('-'); - } - - kebab_case.push_str(&word.to_lowercase()); - } - - let kebab_case_lit = syn::LitStr::new(&kebab_case, input.span()); + let kebab_case_lit = syn::LitStr::new(&s.to_kebab_case(), input.span()); quote::quote!(#kebab_case_lit) } diff --git a/crates/ruff_macros/src/lib.rs b/crates/ruff_macros/src/lib.rs index 99bac807fe920..9275cd6adcb5b 100644 --- a/crates/ruff_macros/src/lib.rs +++ b/crates/ruff_macros/src/lib.rs @@ -49,7 +49,7 @@ pub fn derive_combine(input: TokenStream) -> TokenStream { .into() } -/// Converts a screaming snake case identifier to a kebab case string. +/// Converts an identifier to a kebab case string. #[proc_macro] pub fn kebab_case(input: TokenStream) -> TokenStream { let input = parse_macro_input!(input as syn::Ident); diff --git a/crates/ruff_macros/src/map_codes.rs b/crates/ruff_macros/src/map_codes.rs index 39f7b60dbf3e4..ed3ab8040f8a2 100644 --- a/crates/ruff_macros/src/map_codes.rs +++ b/crates/ruff_macros/src/map_codes.rs @@ -403,7 +403,6 @@ fn register_rules<'a>(input: impl Iterator) -> TokenStream { let mut rule_message_formats_match_arms = quote!(); let mut rule_fixable_match_arms = quote!(); let mut rule_explanation_match_arms = quote!(); - let mut rule_pascal_match_arms = quote!(); let mut from_impls_for_diagnostic_kind = quote!(); @@ -425,10 +424,7 @@ fn register_rules<'a>(input: impl Iterator) -> TokenStream { // Enable conversion from `DiagnosticKind` to `Rule`. from_impls_for_diagnostic_kind - .extend(quote! {#(#attrs)* stringify!(#name) => Rule::#name,}); - - // Enable conversion from a `Rule` to its Pascal-case variant name. - rule_pascal_match_arms.extend(quote! {#(#attrs)* Rule::#name => stringify!(#name),}); + .extend(quote! {#(#attrs)* ::ruff_macros::kebab_case!(#name) => Rule::#name,}); } quote! { @@ -470,11 +466,6 @@ fn register_rules<'a>(input: impl Iterator) -> TokenStream { pub const fn fixable(&self) -> ruff_diagnostics::FixAvailability { match self { #rule_fixable_match_arms } } - - /// Returns the Pascal-case variant name of this rule. - pub const fn pascal_name(&self) -> &'static str { - match self { #rule_pascal_match_arms } - } } impl AsRule for ruff_diagnostics::Diagnostic { diff --git a/crates/ruff_macros/src/violation_metadata.rs b/crates/ruff_macros/src/violation_metadata.rs index 5973c3db3e672..3cf60307abf40 100644 --- a/crates/ruff_macros/src/violation_metadata.rs +++ b/crates/ruff_macros/src/violation_metadata.rs @@ -12,7 +12,7 @@ pub(crate) fn violation_metadata(input: DeriveInput) -> syn::Result #[expect(deprecated)] impl ruff_diagnostics::ViolationMetadata for #name { fn rule_name() -> &'static str { - stringify!(#name) + ::ruff_macros::kebab_case!(#name) } fn explain() -> Option<&'static str> {