This is an automated email from the ASF dual-hosted git repository. mgrigorov pushed a commit to branch pedantic-clippy in repository https://gitbox.apache.org/repos/asf/avro-rs.git
commit fc6d0db8bb0f2244c4096b4991fbe70ee754c7ab Author: Martin Tzvetanov Grigorov <[email protected]> AuthorDate: Mon Mar 23 16:50:14 2026 +0200 chore: Apply some suggestions by -Dclippy::pedantic `-Dclippy::pedantic` is not added to test-lang-rust-clippy.yml because not all sugestions could be applied at the moment. Added `-Dclippy::cargo` to CI. `multiple_crate_versions` is needed because at the moment there are two versions of `heck` and `hashbrown` coming as transitive dependencies --- .github/workflows/test-lang-rust-clippy.yml | 2 +- avro/src/serde/ser_schema/mod.rs | 16 +++--- avro/tests/append_to_existing.rs | 4 +- avro_derive/build.rs | 2 +- avro_derive/src/attributes/avro.rs | 14 ++--- avro_derive/src/attributes/mod.rs | 6 +-- avro_derive/src/case.rs | 8 +-- avro_derive/src/enums/mod.rs | 2 +- avro_derive/src/enums/plain.rs | 4 +- avro_derive/src/lib.rs | 43 +++++++-------- avro_test_helper/src/data.rs | 4 +- avro_test_helper/src/lib.rs | 21 +++++--- avro_test_helper/src/logger.rs | 84 ++++++++++++++++++++++++++--- 13 files changed, 144 insertions(+), 66 deletions(-) diff --git a/.github/workflows/test-lang-rust-clippy.yml b/.github/workflows/test-lang-rust-clippy.yml index e5243f5..33b6dec 100644 --- a/.github/workflows/test-lang-rust-clippy.yml +++ b/.github/workflows/test-lang-rust-clippy.yml @@ -46,4 +46,4 @@ jobs: with: toolchain: ${{ matrix.rust }} components: clippy - - run: cargo clippy --all-features --all-targets + - run: cargo clippy --all-features --all-targets --workspace -- -Dclippy::cargo -Aclippy::multiple_crate_versions diff --git a/avro/src/serde/ser_schema/mod.rs b/avro/src/serde/ser_schema/mod.rs index fa9382f..6f29b47 100644 --- a/avro/src/serde/ser_schema/mod.rs +++ b/avro/src/serde/ser_schema/mod.rs @@ -1645,15 +1645,13 @@ impl<'s, W: Write> SchemaAwareWriteSerializer<'s, W> { Schema::Union(union_schema) => { for (i, variant_schema) in union_schema.schemas.iter().enumerate() { match variant_schema { - Schema::Record(inner) => { - if inner.fields.len() == len { - encode_int(i as i32, &mut *self.writer)?; - return self.serialize_tuple_struct_with_schema( - name, - len, - variant_schema, - ); - } + Schema::Record(inner) if inner.fields.len() == len => { + encode_int(i as i32, &mut *self.writer)?; + return self.serialize_tuple_struct_with_schema( + name, + len, + variant_schema, + ); } Schema::Array(_) | Schema::Ref { name: _ } => { encode_int(i as i32, &mut *self.writer)?; diff --git a/avro/tests/append_to_existing.rs b/avro/tests/append_to_existing.rs index ef5723e..571c05c 100644 --- a/avro/tests/append_to_existing.rs +++ b/avro/tests/append_to_existing.rs @@ -46,10 +46,8 @@ fn avro_3630_append_to_an_existing_file() -> TestResult { let new_bytes = writer.into_inner().expect("Cannot get the new bytes"); let reader = Reader::new(&*new_bytes).expect("Cannot read the new bytes"); - let mut i = 1; - for value in reader { + for (i, value) in (1..).zip(reader) { check(&value, i); - i += 1 } Ok(()) diff --git a/avro_derive/build.rs b/avro_derive/build.rs index 40e33af..e3b221c 100644 --- a/avro_derive/build.rs +++ b/avro_derive/build.rs @@ -18,7 +18,7 @@ //! Set the `nightly` cfg value on nightly toolchains. //! //! We would prefer to just do `#![rustversion::attr(nightly, feature(proc_macro_diagnostic)]` -//! but that's currently not possible, see <https://github.com/dtolnay/rustversion/issues/8> +//! but that's currently not possible, see <https://github.com/dtolnay/rustversion/issues/8>. #[rustversion::nightly] fn main() { diff --git a/avro_derive/src/attributes/avro.rs b/avro_derive/src/attributes/avro.rs index eb11b9e..e0a1433 100644 --- a/avro_derive/src/attributes/avro.rs +++ b/avro_derive/src/attributes/avro.rs @@ -66,14 +66,14 @@ impl ContainerAttributes { span, r#"`#[avro(name = "...")]` is deprecated."#, r#"Use `#[serde(rename = "...")]` instead."#, - ) + ); } if self.rename_all != RenameRule::None { super::warn( span, r#"`#[avro(rename_all = "..")]` is deprecated"#, r#"Use `#[serde(rename_all = "..")]` instead"#, - ) + ); } } } @@ -98,7 +98,7 @@ impl VariantAttributes { span, r#"`#[avro(rename = "..")]` is deprecated"#, r#"Use `#[serde(rename = "..")]` instead"#, - ) + ); } } } @@ -177,28 +177,28 @@ impl FieldAttributes { span, r#"`#[avro(alias = "..")]` is deprecated"#, r#"Use `#[serde(alias = "..")]` instead"#, - ) + ); } if self.rename.is_some() { super::warn( span, r#"`#[avro(rename = "..")]` is deprecated"#, r#"Use `#[serde(rename = "..")]` instead"#, - ) + ); } if self.skip { super::warn( span, "`#[avro(skip)]` is deprecated", "Use `#[serde(skip)]` instead", - ) + ); } if self.flatten { super::warn( span, "`#[avro(flatten)]` is deprecated", "Use `#[serde(flatten)]` instead", - ) + ); } } } diff --git a/avro_derive/src/attributes/mod.rs b/avro_derive/src/attributes/mod.rs index 27c1cbc..66911d9 100644 --- a/avro_derive/src/attributes/mod.rs +++ b/avro_derive/src/attributes/mod.rs @@ -202,7 +202,7 @@ pub enum With { impl With { fn from_avro_and_serde( avro: &avro::With, - serde: &Option<String>, + serde: Option<&String>, span: Span, ) -> Result<Self, syn::Error> { match &avro { @@ -327,7 +327,7 @@ impl FieldOptions { "`#[serde(skip_serializing)]` and `#[serde(skip_serializing_if)]` are incompatible with `#[avro(default = false)]`" )); } - let with = match With::from_avro_and_serde(&avro.with, &serde.with, span) { + let with = match With::from_avro_and_serde(&avro.with, serde.with.as_ref(), span) { Ok(with) => with, Err(error) => { errors.push(error); @@ -389,7 +389,7 @@ fn darling_to_syn(e: darling::Error) -> Vec<syn::Error> { fn warn(span: Span, message: &str, help: &str) { proc_macro::Diagnostic::spanned(span.unwrap(), proc_macro::Level::Warning, message) .help(help) - .emit() + .emit(); } #[cfg(not(nightly))] diff --git a/avro_derive/src/case.rs b/avro_derive/src/case.rs index c958562..9f8eff9 100644 --- a/avro_derive/src/case.rs +++ b/avro_derive/src/case.rs @@ -21,7 +21,10 @@ use darling::FromMeta; use syn::Lit; -use self::RenameRule::*; +use self::RenameRule::{ + CamelCase, KebabCase, LowerCase, None, PascalCase, ScreamingKebabCase, ScreamingSnakeCase, + SnakeCase, UpperCase, +}; use std::fmt::{self, Debug, Display}; /// The different possible ways to change case of fields in a struct, or variants in an enum. @@ -111,7 +114,7 @@ impl RenameRule { pub fn apply_to_field(self, field: &str) -> String { match self { None | LowerCase | SnakeCase => field.to_owned(), - UpperCase => field.to_ascii_uppercase(), + UpperCase | ScreamingSnakeCase => field.to_ascii_uppercase(), PascalCase => { let mut pascal = String::new(); let mut capitalize = true; @@ -131,7 +134,6 @@ impl RenameRule { let pascal = PascalCase.apply_to_field(field); pascal[..1].to_ascii_lowercase() + &pascal[1..] } - ScreamingSnakeCase => field.to_ascii_uppercase(), KebabCase => field.replace('_', "-"), ScreamingKebabCase => ScreamingSnakeCase.apply_to_field(field).replace('_', "-"), } diff --git a/avro_derive/src/enums/mod.rs b/avro_derive/src/enums/mod.rs index e44f4a1..1511f65 100644 --- a/avro_derive/src/enums/mod.rs +++ b/avro_derive/src/enums/mod.rs @@ -24,7 +24,7 @@ use syn::{Attribute, DataEnum, Fields, Meta}; /// Generate a schema definition for a enum. pub fn get_data_enum_schema_def( container_attrs: &NamedTypeOptions, - data_enum: DataEnum, + data_enum: &DataEnum, ident_span: Span, ) -> Result<TokenStream, Vec<syn::Error>> { if data_enum.variants.iter().all(|v| Fields::Unit == v.fields) { diff --git a/avro_derive/src/enums/plain.rs b/avro_derive/src/enums/plain.rs index 2832c08..4fb0adb 100644 --- a/avro_derive/src/enums/plain.rs +++ b/avro_derive/src/enums/plain.rs @@ -26,13 +26,13 @@ use syn::{DataEnum, Fields}; pub fn schema_def( container_attrs: &NamedTypeOptions, - data_enum: DataEnum, + data_enum: &DataEnum, ident_span: Span, ) -> Result<TokenStream, Vec<syn::Error>> { let doc = preserve_optional(container_attrs.doc.as_ref()); let enum_aliases = aliases(&container_attrs.aliases); if data_enum.variants.iter().all(|v| Fields::Unit == v.fields) { - let default_value = default_enum_variant(&data_enum, ident_span)?; + let default_value = default_enum_variant(data_enum, ident_span)?; let default = preserve_optional(default_value); let mut symbols = Vec::new(); for variant in &data_enum.variants { diff --git a/avro_derive/src/lib.rs b/avro_derive/src/lib.rs index 96f997e..2fafa38 100644 --- a/avro_derive/src/lib.rs +++ b/avro_derive/src/lib.rs @@ -51,7 +51,7 @@ use crate::{ pub fn proc_macro_derive_avro_schema(input: proc_macro::TokenStream) -> proc_macro::TokenStream { let input = parse_macro_input!(input as DeriveInput); derive_avro_schema(input) - .unwrap_or_else(to_compile_errors) + .unwrap_or_else(|errs| to_compile_errors(errs.as_slice())) .into() } @@ -68,16 +68,16 @@ fn derive_avro_schema(input: DeriveInput) -> Result<TokenStream, Vec<syn::Error> let (schema_def, record_fields) = get_struct_schema_def(&named_type_options, data_struct, input.ident.span())?; ( - handle_named_schemas(named_type_options.name, schema_def), + handle_named_schemas(&named_type_options.name, &schema_def), record_fields, ) }; Ok(create_trait_definition( - input.ident, + &input.ident, &input.generics, - get_schema_impl, - get_record_fields_impl, - named_type_options.default, + &get_schema_impl, + &get_record_fields_impl, + &named_type_options.default, )) } syn::Data::Enum(data_enum) => { @@ -89,14 +89,14 @@ fn derive_avro_schema(input: DeriveInput) -> Result<TokenStream, Vec<syn::Error> )]); } let schema_def = - get_data_enum_schema_def(&named_type_options, data_enum, input.ident.span())?; - let inner = handle_named_schemas(named_type_options.name, schema_def); + get_data_enum_schema_def(&named_type_options, &data_enum, input.ident.span())?; + let inner = handle_named_schemas(&named_type_options.name, &schema_def); Ok(create_trait_definition( - input.ident, + &input.ident, &input.generics, - inner, - quote! { ::std::option::Option::None }, - named_type_options.default, + &inner, + "e! { ::std::option::Option::None }, + &named_type_options.default, )) } syn::Data::Union(_) => Err(vec![syn::Error::new( @@ -108,11 +108,11 @@ fn derive_avro_schema(input: DeriveInput) -> Result<TokenStream, Vec<syn::Error> /// Generate the trait definition with the correct generics fn create_trait_definition( - ident: Ident, + ident: &Ident, generics: &Generics, - get_schema_impl: TokenStream, - get_record_fields_impl: TokenStream, - field_default_impl: TokenStream, + get_schema_impl: &TokenStream, + get_record_fields_impl: &TokenStream, + field_default_impl: &TokenStream, ) -> TokenStream { let (impl_generics, ty_generics, where_clause) = generics.split_for_impl(); quote! { @@ -134,7 +134,7 @@ fn create_trait_definition( } /// Generate the code to check `named_schemas` if this schema already exist -fn handle_named_schemas(full_schema_name: String, schema_def: TokenStream) -> TokenStream { +fn handle_named_schemas(full_schema_name: &str, schema_def: &TokenStream) -> TokenStream { quote! { let name = ::apache_avro::schema::Name::new_with_enclosing_namespace(#full_schema_name, enclosing_namespace).expect(concat!("Unable to parse schema name ", #full_schema_name)); if named_schemas.contains(&name) { @@ -448,15 +448,16 @@ fn type_to_field_default_expr(ty: &Type) -> Result<TokenStream, Vec<syn::Error>> } /// Stolen from serde -fn to_compile_errors(errors: Vec<syn::Error>) -> proc_macro2::TokenStream { +fn to_compile_errors(errors: &[syn::Error]) -> proc_macro2::TokenStream { let compile_errors = errors.iter().map(syn::Error::to_compile_error); quote!(#(#compile_errors)*) } fn preserve_optional(op: Option<impl quote::ToTokens>) -> TokenStream { - match op { - Some(tt) => quote! {::std::option::Option::Some(#tt.into())}, - None => quote! {::std::option::Option::None}, + if let Some(tt) = op { + quote! {::std::option::Option::Some(#tt.into())} + } else { + quote! {::std::option::Option::None} } } diff --git a/avro_test_helper/src/data.rs b/avro_test_helper/src/data.rs index 3713e83..b0999d7 100644 --- a/avro_test_helper/src/data.rs +++ b/avro_test_helper/src/data.rs @@ -37,7 +37,7 @@ pub const PRIMITIVE_EXAMPLES: &[(&str, bool)] = &[ (r#""double""#, true), (r#"{"type": "double"}"#, true), (r#""true""#, false), - (r#"true"#, false), + ("true", false), (r#"{"no_type": "test"}"#, false), (r#"{"type": "panther"}"#, false), ]; @@ -602,6 +602,7 @@ pub const LOCAL_TIMESTAMPMICROS_LOGICAL_TYPE: &[(&str, bool)] = &[ ), ]; +#[inline] pub fn examples() -> &'static Vec<(&'static str, bool)> { static EXAMPLES_ONCE: OnceLock<Vec<(&'static str, bool)>> = OnceLock::new(); EXAMPLES_ONCE.get_or_init(|| { @@ -629,6 +630,7 @@ pub fn examples() -> &'static Vec<(&'static str, bool)> { }) } +#[inline] pub fn valid_examples() -> &'static Vec<(&'static str, bool)> { static VALID_EXAMPLES_ONCE: OnceLock<Vec<(&'static str, bool)>> = OnceLock::new(); VALID_EXAMPLES_ONCE.get_or_init(|| examples().iter().copied().filter(|s| s.1).collect()) diff --git a/avro_test_helper/src/lib.rs b/avro_test_helper/src/lib.rs index 9b5248d..f036187 100644 --- a/avro_test_helper/src/lib.rs +++ b/avro_test_helper/src/lib.rs @@ -15,9 +15,14 @@ // specific language governing permissions and limitations // under the License. +pub mod data; +pub mod logger; + +use core::any::type_name; +use core::cell::RefCell; +use core::fmt::{Debug, Display}; #[cfg(not(target_arch = "wasm32"))] use ctor::{ctor, dtor}; -use std::cell::RefCell; thread_local! { // The unit tests run in parallel @@ -26,9 +31,6 @@ thread_local! { pub(crate) static LOG_MESSAGES: RefCell<Vec<String>> = const { RefCell::new(Vec::new()) }; } -pub mod data; -pub mod logger; - #[cfg(not(target_arch = "wasm32"))] #[ctor] fn before_all() { @@ -50,19 +52,21 @@ fn after_all() { } /// A custom error type for tests. +#[non_exhaustive] #[derive(Debug)] pub struct TestError; /// A converter of any error into [`TestError`]. /// /// It is used to print better error messages in the tests. -/// Borrowed from <https://bluxte.net/musings/2023/01/08/improving_failure_messages_rust_tests/> +/// Borrowed from <https://bluxte.net/musings/2023/01/08/improving_failure_messages_rust_tests/>. // The Display bound is needed so that the `From` implementation doesn't // apply to `TestError` itself. -impl<Err: std::fmt::Display + std::fmt::Debug> From<Err> for TestError { +impl<Err: Display + Debug> From<Err> for TestError { #[track_caller] + #[inline] fn from(err: Err) -> Self { - panic!("{}: {:?}", std::any::type_name::<Err>(), err); + panic!("{}: {:?}", type_name::<Err>(), err); } } @@ -71,4 +75,5 @@ pub type TestResult = Result<(), TestError>; /// Does nothing. Just loads the crate. /// Should be used in the integration tests, because they do not use [dev-dependencies] /// and do not auto-load this crate. -pub fn init() {} +#[inline] +pub const fn init() {} diff --git a/avro_test_helper/src/logger.rs b/avro_test_helper/src/logger.rs index 738cdee..f6c3b17 100644 --- a/avro_test_helper/src/logger.rs +++ b/avro_test_helper/src/logger.rs @@ -53,6 +53,14 @@ fn test_logger() -> &'static TestLogger { }) } +/// Clears all log messages stored in the thread-local storage. +/// +/// # Panics +/// This function will panic if: +/// - The thread-local `LOG_MESSAGES` is already borrowed and cannot +/// be mutably borrowed. +/// +/// The panic includes a debug representation of the error encountered. pub fn clear_log_messages() { LOG_MESSAGES.with(|msgs| match msgs.try_borrow_mut() { Ok(mut log_messages) => log_messages.clear(), @@ -60,6 +68,32 @@ pub fn clear_log_messages() { }); } +/// Asserts that a specific log message has not been logged. +/// +/// This function checks the most recently logged message from a thread-local +/// storage and ensures that it does not match the provided `unexpected_message`. +/// If the message does match, the function will panic with an appropriate error +/// message indicating the unexpected log entry. +/// +/// # Arguments +/// - `unexpected_message`: A string slice that represents the log message +/// that is not expected to be logged. If this message matches the last +/// logged message, the function will panic. +/// +/// # Panics +/// - This function will panic if the `unexpected_message` matches the most +/// recently logged message. +/// +/// # Example +/// ``` +/// // Assume LOG_MESSAGES is set up to capture log messages. +/// log_message("Unexpected Error"); +/// assert_not_logged("Unexpected Error"); +/// // This will panic with the message: +/// // "The following log message should not have been logged: 'Unexpected Error'" +/// +/// assert_not_logged("Non-existent Message"); +/// // This will pass without issue since the message was not logged. #[track_caller] pub fn assert_not_logged(unexpected_message: &str) { LOG_MESSAGES.with(|msgs| match msgs.borrow().last() { @@ -70,6 +104,43 @@ pub fn assert_not_logged(unexpected_message: &str) { }); } +/// Asserts that the specified log message has been logged and removes it from +/// the stored log messages. +/// +/// # Parameters +/// - `expected_message`: A string slice that holds the log message to be asserted. +/// +/// # Panics +/// This function will panic if the `expected_message` has not been logged. The +/// panic message will indicate the missing log message. +/// +/// # Behavior +/// - The function verifies if the `expected_message` exists in the log messages +/// stored using the thread-local storage (`LOG_MESSAGES`). +/// - If the message is found, it is removed from the log messages and the function +/// completes successfully. +/// - If the message is not found, a panic is triggered with an explanatory message. +/// +/// # Usage +/// This function is typically used in unit tests or scenarios where it is important +/// to ensure specific messages are logged during execution. +/// +/// # Example +/// ``` +/// // Assuming LOG_MESSAGES is set up and some log messages have been recorded: +/// assert_logged("Expected log entry"); +/// ``` +/// +/// # Notes +/// - The function uses the `#[track_caller]` attribute to improve debugging by +/// providing more accurate information about the location of the panic in the +/// source code. +/// - The `LOG_MESSAGES` must be a thread-local data structure that supports +/// borrowing and mutating of a collection of string messages. +/// +/// # Thread Safety +/// This function relies on thread-local variables to manage logs for each thread +/// independently. #[track_caller] pub fn assert_logged(expected_message: &str) { let mut deleted = false; @@ -81,19 +152,20 @@ pub fn assert_logged(expected_message: &str) { } else { true } - }) + }); }); - if !deleted { - panic!("Expected log message has not been logged: '{expected_message}'"); - } + assert!( + deleted, + "Expected log message has not been logged: '{expected_message}'" + ); } pub(crate) fn install() { log::set_logger(test_logger()) - .map(|_| log::set_max_level(LevelFilter::Trace)) + .map(|()| log::set_max_level(LevelFilter::Trace)) .map_err(|err| { eprintln!("Failed to set the custom logger: {err:?}"); }) - .unwrap(); + .expect("Failed to set the custom TestLogger"); }
