From 16e79c8db6356055bd045790ddb7795ad1efa6e0 Mon Sep 17 00:00:00 2001 From: Martin Fischer Date: Thu, 19 Jan 2023 08:36:48 +0100 Subject: [PATCH] derive-msg-formats 4/5: Implement #[derive_message_formats] The idea is nice and simple we replace: fn placeholder() -> Self; with fn message_formats() -> &'static [&'static str]; So e.g. if a Violation implementation defines: fn message(&self) -> String { format!("Local variable `{name}` is assigned to but never used") } it would also have to define: fn message_formats() -> &'static [&'static str] { &["Local variable `{name}` is assigned to but never used"] } Since we however obviously do not want to duplicate all of our format strings we simply introduce a new procedural macro attribute #[derive_message_formats] that can be added to the message method declaration in order to automatically derive the message_formats implementation. This commit implements the macro. The following and final commit updates violations.rs to use the macro. (The changes have been separated because the next commit is autogenerated via a Python script.) --- ruff_cli/src/commands.rs | 4 +- ruff_dev/src/generate_rules_table.rs | 3 +- ruff_macros/src/define_rule_mapping.rs | 13 ++--- ruff_macros/src/derive_message_formats.rs | 55 +++++++++++++++++++ ruff_macros/src/lib.rs | 10 +++- src/rules/flake8_tidy_imports/banned_api.rs | 1 + .../flake8_tidy_imports/relative_imports.rs | 1 + src/rules/mod.rs | 1 + src/violation.rs | 13 +++-- src/violations.rs | 6 +- 10 files changed, 87 insertions(+), 20 deletions(-) create mode 100644 ruff_macros/src/derive_message_formats.rs diff --git a/ruff_cli/src/commands.rs b/ruff_cli/src/commands.rs index f35177e7a6..d4b1af8603 100644 --- a/ruff_cli/src/commands.rs +++ b/ruff_cli/src/commands.rs @@ -297,7 +297,7 @@ pub fn explain(rule: &Rule, format: SerializationFormat) -> Result<()> { "{} ({}): {}", rule.code(), rule.origin().name(), - rule.kind().body() + rule.message_formats()[0] ); } SerializationFormat::Json => { @@ -306,7 +306,7 @@ pub fn explain(rule: &Rule, format: SerializationFormat) -> Result<()> { serde_json::to_string_pretty(&Explanation { code: rule.code(), origin: rule.origin().name(), - summary: &rule.kind().body(), + summary: rule.message_formats()[0], })? ); } diff --git a/ruff_dev/src/generate_rules_table.rs b/ruff_dev/src/generate_rules_table.rs index 65ddcbae25..2c41c1177c 100644 --- a/ruff_dev/src/generate_rules_table.rs +++ b/ruff_dev/src/generate_rules_table.rs @@ -26,7 +26,6 @@ fn generate_table(table_out: &mut String, prefix: &RuleCodePrefix) { table_out.push_str("| ---- | ---- | ------- | --- |"); table_out.push('\n'); for rule in prefix.codes() { - let kind = rule.kind(); let fix_token = match rule.autofixable() { None => "", Some(_) => "🛠", @@ -36,7 +35,7 @@ fn generate_table(table_out: &mut String, prefix: &RuleCodePrefix) { "| {} | {} | {} | {} |", rule.code(), rule.as_ref(), - kind.body().replace('|', r"\|"), + rule.message_formats()[0].replace('|', r"\|"), fix_token )); table_out.push('\n'); diff --git a/ruff_macros/src/define_rule_mapping.rs b/ruff_macros/src/define_rule_mapping.rs index 2e3486894f..f38dda64b2 100644 --- a/ruff_macros/src/define_rule_mapping.rs +++ b/ruff_macros/src/define_rule_mapping.rs @@ -8,7 +8,7 @@ use syn::{Ident, LitStr, Path, Token}; pub fn define_rule_mapping(mapping: &Mapping) -> proc_macro2::TokenStream { let mut rule_variants = quote!(); let mut diagkind_variants = quote!(); - let mut rule_kind_match_arms = quote!(); + let mut rule_message_formats_match_arms = quote!(); let mut rule_autofixable_match_arms = quote!(); let mut rule_origin_match_arms = quote!(); let mut rule_code_match_arms = quote!(); @@ -26,9 +26,8 @@ pub fn define_rule_mapping(mapping: &Mapping) -> proc_macro2::TokenStream { #name, }); diagkind_variants.extend(quote! {#name(#path),}); - rule_kind_match_arms.extend( - quote! {Self::#name => DiagnosticKind::#name(<#path as Violation>::placeholder()),}, - ); + rule_message_formats_match_arms + .extend(quote! {Self::#name => <#path as Violation>::message_formats(),}); rule_autofixable_match_arms.extend(quote! {Self::#name => <#path as Violation>::AUTOFIX,}); let origin = get_origin(code); rule_origin_match_arms.extend(quote! {Self::#name => RuleOrigin::#origin,}); @@ -86,9 +85,9 @@ pub fn define_rule_mapping(mapping: &Mapping) -> proc_macro2::TokenStream { } impl Rule { - /// A placeholder representation of the `DiagnosticKind` for the diagnostic. - pub fn kind(&self) -> DiagnosticKind { - match self { #rule_kind_match_arms } + /// Returns the format strings used to report violations of this rule. + pub fn message_formats(&self) -> &'static [&'static str] { + match self { #rule_message_formats_match_arms } } pub fn autofixable(&self) -> Option { diff --git a/ruff_macros/src/derive_message_formats.rs b/ruff_macros/src/derive_message_formats.rs new file mode 100644 index 0000000000..5de71fda95 --- /dev/null +++ b/ruff_macros/src/derive_message_formats.rs @@ -0,0 +1,55 @@ +use proc_macro2::TokenStream; +use quote::{quote, quote_spanned, ToTokens}; +use syn::spanned::Spanned; +use syn::{Block, Expr, ItemFn, Stmt}; + +pub fn derive_message_formats(func: &ItemFn) -> proc_macro2::TokenStream { + let mut strings = quote!(); + + if let Err(err) = parse_block(&func.block, &mut strings) { + return err; + } + + quote! { + #func + fn message_formats() -> &'static [&'static str] { + &[#strings] + } + } +} + +fn parse_block(block: &Block, strings: &mut TokenStream) -> Result<(), TokenStream> { + let Some(Stmt::Expr(last)) = block.stmts.last() else {panic!("expected last statement in block to be an expression")}; + parse_expr(last, strings)?; + Ok(()) +} + +fn parse_expr(expr: &Expr, strings: &mut TokenStream) -> Result<(), TokenStream> { + match expr { + Expr::Macro(mac) if mac.mac.path.is_ident("format") => { + let Some(first_token) = mac.mac.tokens.to_token_stream().into_iter().next() else { + return Err(quote_spanned!(expr.span() => compile_error!("expected format! to have an argument"))) + }; + strings.extend(quote! {#first_token,}); + Ok(()) + } + Expr::Block(block) => parse_block(&block.block, strings), + Expr::If(expr) => { + parse_block(&expr.then_branch, strings)?; + if let Some((_, then)) = &expr.else_branch { + parse_expr(then, strings)?; + } + Ok(()) + } + Expr::Match(block) => { + for arm in &block.arms { + parse_expr(&arm.body, strings)?; + } + Ok(()) + } + _ => Err(quote_spanned!( + expr.span() => + compile_error!("expected last expression to be a format! macro or a match block") + )), + } +} diff --git a/ruff_macros/src/lib.rs b/ruff_macros/src/lib.rs index 3584a6361d..d4b19dc521 100644 --- a/ruff_macros/src/lib.rs +++ b/ruff_macros/src/lib.rs @@ -13,10 +13,12 @@ )] #![forbid(unsafe_code)] -use syn::{parse_macro_input, DeriveInput}; +use proc_macro::TokenStream; +use syn::{parse_macro_input, DeriveInput, ItemFn}; mod config; mod define_rule_mapping; +mod derive_message_formats; mod prefixes; mod rule_code_prefix; @@ -34,3 +36,9 @@ pub fn define_rule_mapping(item: proc_macro::TokenStream) -> proc_macro::TokenSt let mapping = parse_macro_input!(item as define_rule_mapping::Mapping); define_rule_mapping::define_rule_mapping(&mapping).into() } + +#[proc_macro_attribute] +pub fn derive_message_formats(_attr: TokenStream, item: TokenStream) -> TokenStream { + let func = parse_macro_input!(item as ItemFn); + derive_message_formats::derive_message_formats(&func).into() +} diff --git a/src/rules/flake8_tidy_imports/banned_api.rs b/src/rules/flake8_tidy_imports/banned_api.rs index 53cde15265..a20d366815 100644 --- a/src/rules/flake8_tidy_imports/banned_api.rs +++ b/src/rules/flake8_tidy_imports/banned_api.rs @@ -1,3 +1,4 @@ +use ruff_macros::derive_message_formats; use rustc_hash::FxHashMap; use rustpython_ast::{Alias, Expr, Located}; use schemars::JsonSchema; diff --git a/src/rules/flake8_tidy_imports/relative_imports.rs b/src/rules/flake8_tidy_imports/relative_imports.rs index 07764ce161..a3d679e5df 100644 --- a/src/rules/flake8_tidy_imports/relative_imports.rs +++ b/src/rules/flake8_tidy_imports/relative_imports.rs @@ -1,3 +1,4 @@ +use ruff_macros::derive_message_formats; use rustpython_ast::Stmt; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; diff --git a/src/rules/mod.rs b/src/rules/mod.rs index d3c424f21d..4a76d917a8 100644 --- a/src/rules/mod.rs +++ b/src/rules/mod.rs @@ -1,3 +1,4 @@ +#![allow(clippy::useless_format)] pub mod eradicate; pub mod flake8_2020; pub mod flake8_annotations; diff --git a/src/violation.rs b/src/violation.rs index 48036c60e1..ab0d938ffe 100644 --- a/src/violation.rs +++ b/src/violation.rs @@ -18,8 +18,8 @@ pub trait Violation: Debug + PartialEq + Eq + Serialize + DeserializeOwned { None } - /// A placeholder instance of the violation. - fn placeholder() -> Self; + /// Returns the format strings used by [`message`](Violation::message). + fn message_formats() -> &'static [&'static str]; } pub struct AutofixKind { @@ -48,8 +48,9 @@ pub trait AlwaysAutofixableViolation: /// The title displayed for the available autofix. fn autofix_title(&self) -> String; - /// A placeholder instance of the violation. - fn placeholder() -> Self; + /// Returns the format strings used by + /// [`message`](AlwaysAutofixableViolation::message). + fn message_formats() -> &'static [&'static str]; } /// A blanket implementation. @@ -64,8 +65,8 @@ impl Violation for VA { Some(Self::autofix_title) } - fn placeholder() -> Self { - ::placeholder() + fn message_formats() -> &'static [&'static str] { + ::message_formats() } } diff --git a/src/violations.rs b/src/violations.rs index 3056fbc156..c8a7a2b9aa 100644 --- a/src/violations.rs +++ b/src/violations.rs @@ -1,6 +1,8 @@ +#![allow(clippy::useless_format)] use std::fmt; use itertools::Itertools; +use ruff_macros::derive_message_formats; use rustpython_ast::Cmpop; use serde::{Deserialize, Serialize}; @@ -255,7 +257,7 @@ define_violation!( impl Violation for IOError { fn message(&self) -> String { let IOError(message) = self; - message.clone() + format!("{message}") } fn placeholder() -> Self { @@ -4280,7 +4282,7 @@ define_violation!( ); impl Violation for NoThisPrefix { fn message(&self) -> String { - "First word of the docstring should not be \"This\"".to_string() + r#"First word of the docstring should not be "This""#.to_string() } fn placeholder() -> Self {