diff --git a/.github/workflows/test-lang-rust-clippy.yml b/.github/workflows/test-lang-rust-clippy.yml index aae3834d..4992eb2a 100644 --- a/.github/workflows/test-lang-rust-clippy.yml +++ b/.github/workflows/test-lang-rust-clippy.yml @@ -19,9 +19,9 @@ name: Rust Clippy Check on: workflow_dispatch: push: - branches: [ main ] + branches: [main] pull_request: - branches: [ main ] + branches: [main] permissions: contents: read @@ -39,11 +39,11 @@ jobs: strategy: matrix: rust: - - 'stable' + - "stable" steps: - uses: actions/checkout@v6 - uses: dtolnay/rust-toolchain@29eef336d9b2848a0b548edc03f92a220660cdb8 # stable with: toolchain: ${{ matrix.rust }} components: clippy - - run: cargo clippy --all-features --all-targets + - run: cargo clippy --all-features --all-targets --workspace diff --git a/Cargo.toml b/Cargo.toml index 1d44064b..d57ce738 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -60,3 +60,12 @@ clippy.doc_markdown = "warn" #clippy.missing_errors_doc = "warn" #clippy.missing_panics_doc = "warn" rust.unexpected_cfgs = { level = "warn", check-cfg = ['cfg(nightly)'] } +clippy.cargo = { level = "warn", priority = -1 } +clippy.multiple_crate_versions = "allow" +# clippy.needless_raw_string_hashes = "warn" +# clippy.missing_panics_doc = "warn" +# clippy.semicolon_if_nothing_returned = "warn" +# clippy.manual_assert = "warn" +# clippy.enum_glob_use = "warn" +# clippy.needless_pass_by_value = "warn" +# clippy.single_match_else = "warn" diff --git a/avro/src/serde/ser_schema/mod.rs b/avro/src/serde/ser_schema/mod.rs index fa9382f4..6f29b47d 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_derive/build.rs b/avro_derive/build.rs index 40e33af3..e3b221c6 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 +//! but that's currently not possible, see . #[rustversion::nightly] fn main() { diff --git a/avro_derive/src/attributes/avro.rs b/avro_derive/src/attributes/avro.rs index eb11b9e5..e0a1433c 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 27c1cbc3..66911d9e 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, + serde: Option<&String>, span: Span, ) -> Result { 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 { 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 c9585621..9f8eff97 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 e44f4a1a..1511f65e 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> { 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 2832c088..4fb0adbe 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> { 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 96f997ea..d63ed8f4 100644 --- a/avro_derive/src/lib.rs +++ b/avro_derive/src/lib.rs @@ -68,16 +68,16 @@ fn derive_avro_schema(input: DeriveInput) -> Result 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 )]); } 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 /// 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> } /// Stolen from serde -fn to_compile_errors(errors: Vec) -> proc_macro2::TokenStream { - let compile_errors = errors.iter().map(syn::Error::to_compile_error); +fn to_compile_errors(errors: impl AsRef<[syn::Error]>) -> proc_macro2::TokenStream { + let compile_errors = errors.as_ref().iter().map(syn::Error::to_compile_error); quote!(#(#compile_errors)*) } fn preserve_optional(op: Option) -> 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 3713e83e..18a4310e 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), ]; diff --git a/avro_test_helper/src/lib.rs b/avro_test_helper/src/lib.rs index 9b5248db..686d96f1 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> = const { RefCell::new(Vec::new()) }; } -pub mod data; -pub mod logger; - #[cfg(not(target_arch = "wasm32"))] #[ctor] fn before_all() { @@ -56,13 +58,13 @@ pub struct TestError; /// A converter of any error into [`TestError`]. /// /// It is used to print better error messages in the tests. -/// Borrowed from +/// Borrowed from . // The Display bound is needed so that the `From` implementation doesn't // apply to `TestError` itself. -impl From for TestError { +impl From for TestError { #[track_caller] fn from(err: Err) -> Self { - panic!("{}: {:?}", std::any::type_name::(), err); + panic!("{}: {:?}", type_name::(), err); } } @@ -71,4 +73,4 @@ 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() {} +pub const fn init() {} diff --git a/avro_test_helper/src/logger.rs b/avro_test_helper/src/logger.rs index 738cdeed..e3d13abf 100644 --- a/avro_test_helper/src/logger.rs +++ b/avro_test_helper/src/logger.rs @@ -26,7 +26,6 @@ struct TestLogger { } impl Log for TestLogger { - #[inline] fn enabled(&self, _metadata: &Metadata) -> bool { true } @@ -53,47 +52,85 @@ fn test_logger() -> &'static TestLogger { }) } +/// Clears all log messages of this thread. pub fn clear_log_messages() { - LOG_MESSAGES.with(|msgs| match msgs.try_borrow_mut() { - Ok(mut log_messages) => log_messages.clear(), - Err(err) => panic!("Failed to clear log messages: {err:?}"), - }); + LOG_MESSAGES.with_borrow_mut(Vec::clear); } +/// Asserts that the message is not in the log for this thread. +/// +/// # Panics +/// Will panic if the provided message is an exact match for any message in the log. +/// +/// # Example +/// ```should_panic +/// # use apache_avro_test_helper::logger::assert_not_logged; +/// # +/// log::error!("Unexpected Error"); +/// +/// // This will not panic as it is not an exact match +/// assert_not_logged("No Unexpected Error"); +/// +/// // This will panic +/// assert_not_logged("Unexpected Error"); +/// ``` #[track_caller] pub fn assert_not_logged(unexpected_message: &str) { - LOG_MESSAGES.with(|msgs| match msgs.borrow().last() { - Some(last_log) if last_log == unexpected_message => { - panic!("The following log message should not have been logged: '{unexpected_message}'") - } - _ => (), + LOG_MESSAGES.with_borrow(|msgs| { + let is_logged = msgs.iter().any(|msg| msg == unexpected_message); + assert!( + !is_logged, + "The following log message should not have been logged: '{unexpected_message}'" + ); }); } +/// Asserts that the message has been logged and removes it from the log of this thread. +/// +/// # Panics +/// Will panic if the message does not appear in the log. +/// +/// # Example +/// ```should_panic +/// # use apache_avro_test_helper::logger::assert_logged; +/// # +/// log::error!("Something went wrong"); +/// log::info!("Something happened"); +/// log::error!("Something went wrong"); +/// +/// // This will not panic as the message was logged +/// assert_logged("Something went wrong"); +/// +/// // This will not panic as the message was logged +/// assert_logged("Something happened"); +/// +/// // This will not panic as the first call removed only the first matching log message +/// assert_logged("Something went wrong"); +/// +/// // This will panic as all matching log messages have been removed +/// assert_logged("Something went wrong"); +/// ``` #[track_caller] pub fn assert_logged(expected_message: &str) { let mut deleted = false; - LOG_MESSAGES.with(|msgs| { - msgs.borrow_mut().retain(|msg| { - if msg == expected_message { - deleted = true; - false - } else { - true - } - }) + LOG_MESSAGES.with_borrow_mut(|msgs| { + if let Some(pos) = msgs.iter().position(|msg| msg == expected_message) { + msgs.remove(pos); + deleted = 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"); }