From a1ade35da1ef751191926f9b012d5317c9ee31da Mon Sep 17 00:00:00 2001 From: joeoc2001 Date: Fri, 22 Nov 2024 11:56:32 +0000 Subject: [PATCH] Fix PartialOrd misimplementation --- Cargo.toml | 2 +- src/impls.rs | 1 + src/perfect_macro.rs | 26 +-- src/perfect_parsing.rs | 4 +- tests/perfect_derive_base_macro.rs | 172 ++++++++++++++++++- tests/perfect_derive_simple_generic_macro.rs | 1 + 6 files changed, 188 insertions(+), 18 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 64285b0..184e275 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "perfect-derive" -version = "0.1.3" +version = "0.1.4" edition = "2021" license = "MIT" description = "Provides a prototype of the proposed perfect_derive macro" diff --git a/src/impls.rs b/src/impls.rs index c70cb82..d4632ab 100644 --- a/src/impls.rs +++ b/src/impls.rs @@ -14,4 +14,5 @@ macro_rules! impls { }; } +#[allow(clippy::single_component_path_imports)] pub(crate) use impls; diff --git a/src/perfect_macro.rs b/src/perfect_macro.rs index 66c10cc..b0f8405 100644 --- a/src/perfect_macro.rs +++ b/src/perfect_macro.rs @@ -22,7 +22,7 @@ fn is_attribute_default(a: &Attribute) -> bool { && a.path() .segments .iter() - .all(|i| i.ident.to_string() == "default" && i.arguments.is_empty()) + .all(|i| i.ident == "default" && i.arguments.is_empty()) } fn remove_debug_markers(obj: &mut StructOrEnum) { @@ -55,7 +55,7 @@ pub fn impl_traits(traits: DerivedList, mut obj: StructOrEnum) -> TokenStream { if already_derived.contains(&derived.name) { panic!("cannot derive {:?} twice", derived.name) } - already_derived.insert(derived.name.clone()); + already_derived.insert(derived.name); add_type_impl(&mut output, &derived, &obj); } @@ -71,7 +71,7 @@ pub fn impl_traits(traits: DerivedList, mut obj: StructOrEnum) -> TokenStream { #output }; - return output; + output } enum IdentOrLifetime { @@ -182,7 +182,7 @@ fn get_debug_enum_marker(enum_item: &ItemEnum) -> &Variant { .filter(|v| v.attrs.iter().any(is_attribute_default)) .collect::>(); assert!( - default_variants.len() > 0, + !default_variants.is_empty(), "one enum variant must be marked as default" ); assert_eq!( @@ -229,17 +229,17 @@ fn augment_where_clause( let mut predicates = clause .as_ref() .map(|c| c.predicates.clone()) - .unwrap_or(Punctuated::new()); + .unwrap_or_default(); for predicate in extra { predicates.push(predicate) } - return WhereClause { + WhereClause { where_token: clause.map(|c| c.where_token).unwrap_or(Where { span: trait_to_impl.span, }), predicates, - }; + } } fn get_named_idents(names: &FieldsNamed) -> Vec { @@ -251,7 +251,7 @@ fn get_named_idents(names: &FieldsNamed) -> Vec { } fn get_named_idents_suffix(names: &FieldsNamed, suffix: &str) -> Vec { - assert!(suffix != ""); + assert!(!suffix.is_empty()); names .named .iter() @@ -569,7 +569,7 @@ fn pord_struct(s: &ItemStruct) -> TokenStream { quote! { fn partial_cmp(&self, other: &Self) -> Option { Some(std::cmp::Ordering::Equal) #( - .and_then(|o| self.#idents.partial_cmp(&other.#idents).map(|v| v.then(o))) + .and_then(|o| self.#idents.partial_cmp(&other.#idents).map(|v| o.then(v))) )* } } @@ -584,7 +584,7 @@ fn pord_struct(s: &ItemStruct) -> TokenStream { let Self( #(#idents2),* ) = other; Some(std::cmp::Ordering::Equal) #( - .and_then(|o| #idents1.partial_cmp(#idents2).map(|v| v.then(o))) + .and_then(|o| #idents1.partial_cmp(#idents2).map(|v| o.then(v))) )* } } @@ -612,7 +612,7 @@ fn pord_enum(e: &ItemEnum) -> TokenStream { quote! { (Self::#ident{#(#idents: #idents1),*}, Self::#ident{#(#idents: #idents2),*}) => Some(std::cmp::Ordering::Equal) #( - .and_then(|o| #idents1.partial_cmp(#idents2).map(|v| v.then(o))) + .and_then(|o| #idents1.partial_cmp(#idents2).map(|v| o.then(v))) )* } } @@ -623,7 +623,7 @@ fn pord_enum(e: &ItemEnum) -> TokenStream { quote! { (Self::#ident(#(#idents1),*), Self::#ident(#(#idents2),*)) => Some(std::cmp::Ordering::Equal) #( - .and_then(|o| #idents1.partial_cmp(#idents2).map(|v| v.then(o))) + .and_then(|o| #idents1.partial_cmp(#idents2).map(|v| o.then(v))) )* } } @@ -714,6 +714,8 @@ fn hash_enum(e: &ItemEnum) -> TokenStream { quote! { fn hash(&self, state: &mut H) { + let dis = std::mem::discriminant(self); + dis.hash(state); match self { #( #variant_cases, diff --git a/src/perfect_parsing.rs b/src/perfect_parsing.rs index 2384b54..76b71a9 100644 --- a/src/perfect_parsing.rs +++ b/src/perfect_parsing.rs @@ -50,7 +50,7 @@ impl DerivedType { _ => {} // fall through to default unscoped path } - return type_enum_ident_as_path!(self); + type_enum_ident_as_path!(self) } pub fn get_trait(&self) -> TraitBound { @@ -91,7 +91,7 @@ impl Parse for DerivedType { parse_types_enum! { match name { ident..., - _ => Err(input.error(format!("type identifier {} is not supported - did you mean to use #[derive(...)]?", ident.to_string()))) + _ => Err(input.error(format!("type identifier {} is not supported - did you mean to use #[derive(...)]?", ident))) } } } diff --git a/tests/perfect_derive_base_macro.rs b/tests/perfect_derive_base_macro.rs index 0287125..bea4b76 100644 --- a/tests/perfect_derive_base_macro.rs +++ b/tests/perfect_derive_base_macro.rs @@ -1,6 +1,17 @@ +use std::hash::Hash; + use perfect_derive::perfect_derive; + +fn hash_to_int(v: &impl Hash) -> u64 { + use std::hash::Hasher; + let mut hasher = std::collections::hash_map::DefaultHasher::new(); + v.hash(&mut hasher); + hasher.finish() +} + macro_rules! make_test { ($trait_name:ident $(,$trait_name_tail:ident)*; $method_name:ident) => { + #[allow(unused)] mod $method_name { use perfect_derive::perfect_derive; @@ -65,25 +76,180 @@ make_test!(PartialOrd, PartialEq; pord); make_test!(Debug; debug); make_test!(Hash; hash); +#[derive(Copy, Clone, Ord, Eq, PartialOrd, PartialEq, Debug, Hash, Default)] +struct EverythingStructCore { + v1: usize, + pub v2: i32, +} + #[perfect_derive(Copy, Clone, Ord, Eq, PartialOrd, PartialEq, Debug, Hash, Default)] struct EverythingStruct { v1: usize, pub v2: i32, } +#[test] +fn struct_eq_matches() { + let c1 = EverythingStructCore { v1: 1, v2: 2 }; + let c2 = EverythingStructCore { v1: 2, v2: 1 }; + + let s1 = EverythingStruct { v1: 1, v2: 2 }; + let s2 = EverythingStruct { v1: 2, v2: 1 }; + + assert_eq!(s1.eq(&s1), c1.eq(&c1)); + assert_eq!(s1.eq(&s2), c1.eq(&c2)); + assert_eq!(s2.eq(&s1), c2.eq(&c1)); +} + +#[test] +fn struct_ord_matches() { + let c1 = EverythingStructCore { v1: 1, v2: 2 }; + let c2 = EverythingStructCore { v1: 2, v2: 1 }; + + let s1 = EverythingStruct { v1: 1, v2: 2 }; + let s2 = EverythingStruct { v1: 2, v2: 1 }; + + assert_eq!(s1.cmp(&s1), c1.cmp(&c1)); + assert_eq!(s1.cmp(&s2), c1.cmp(&c2)); + assert_eq!(s2.cmp(&s1), c2.cmp(&c1)); +} + +#[test] +fn struct_partial_ord_matches() { + let c1 = EverythingStructCore { v1: 1, v2: 2 }; + let c2 = EverythingStructCore { v1: 2, v2: 1 }; + + let s1 = EverythingStruct { v1: 1, v2: 2 }; + let s2 = EverythingStruct { v1: 2, v2: 1 }; + + assert_eq!(s1.partial_cmp(&s1), c1.partial_cmp(&c1)); + assert_eq!(s1.partial_cmp(&s2), c1.partial_cmp(&c2)); + assert_eq!(s2.partial_cmp(&s1), c2.partial_cmp(&c1)); +} + +#[test] +fn struct_hash_matches() { + let c1 = EverythingStructCore { v1: 1, v2: 2 }; + let c2 = EverythingStructCore { v1: 2, v2: 1 }; + + let s1 = EverythingStruct { v1: 1, v2: 2 }; + let s2 = EverythingStruct { v1: 2, v2: 1 }; + + assert_eq!(hash_to_int(&s1), hash_to_int(&c1)); + assert_eq!(hash_to_int(&s2), hash_to_int(&c2)); +} + +#[derive(Copy, Clone, Ord, Eq, PartialOrd, PartialEq, Debug, Hash, Default)] +#[allow(unused)] +enum EverythingEnumCore { + #[default] + E1, + E2(), + E3(usize, usize), + E4(u32, ()), + E5 { + name1: u32, + name2: (), + }, +} + #[perfect_derive(Copy, Clone, Ord, Eq, PartialOrd, PartialEq, Debug, Hash, Default)] enum EverythingEnum { + #[default] E1, E2(), - E3(usize), + E3(usize, usize), E4(u32, ()), - #[default] E5 { name1: u32, name2: (), }, } +#[test] +fn enum_eq_matches() { + let c1 = EverythingEnumCore::E1; + let c2 = EverythingEnumCore::E3(1, 2); + let c3 = EverythingEnumCore::E3(2, 1); + + let s1 = EverythingEnum::E1; + let s2 = EverythingEnum::E3(1, 2); + let s3 = EverythingEnum::E3(2, 1); + + assert_eq!(s1.eq(&s1), c1.eq(&c1)); + assert_eq!(s2.eq(&s2), c2.eq(&c2)); + assert_eq!(s3.eq(&s3), c3.eq(&c3)); + + assert_eq!(s1.eq(&s2), c1.eq(&c2)); + assert_eq!(s2.eq(&s1), c2.eq(&c1)); + + assert_eq!(s1.eq(&s3), c1.eq(&c3)); + assert_eq!(s3.eq(&s1), c3.eq(&c1)); + + assert_eq!(s3.eq(&s2), c3.eq(&c2)); + assert_eq!(s2.eq(&s3), c2.eq(&c3)); +} + +#[test] +fn enum_ord_matches() { + let c1 = EverythingEnumCore::E1; + let c2 = EverythingEnumCore::E3(1, 2); + let c3 = EverythingEnumCore::E3(2, 1); + + let s1 = EverythingEnum::E1; + let s2 = EverythingEnum::E3(1, 2); + let s3 = EverythingEnum::E3(2, 1); + + assert_eq!(s1.cmp(&s1), c1.cmp(&c1)); + assert_eq!(s2.cmp(&s2), c2.cmp(&c2)); + assert_eq!(s3.cmp(&s3), c3.cmp(&c3)); + + assert_eq!(s1.cmp(&s2), c1.cmp(&c2)); + assert_eq!(s2.cmp(&s1), c2.cmp(&c1)); + + assert_eq!(s1.cmp(&s3), c1.cmp(&c3)); + assert_eq!(s3.cmp(&s1), c3.cmp(&c1)); + + assert_eq!(s3.cmp(&s2), c3.cmp(&c2)); + assert_eq!(s2.cmp(&s3), c2.cmp(&c3)); +} + +#[test] +fn enum_partial_ord_matches() { + let c1 = EverythingEnumCore::E1; + let c2 = EverythingEnumCore::E3(1, 2); + let c3 = EverythingEnumCore::E3(2, 1); + + let s1 = EverythingEnum::E1; + let s2 = EverythingEnum::E3(1, 2); + let s3 = EverythingEnum::E3(2, 1); + + assert_eq!(s1.partial_cmp(&s1), c1.partial_cmp(&c1)); + assert_eq!(s2.partial_cmp(&s2), c2.partial_cmp(&c2)); + assert_eq!(s3.partial_cmp(&s3), c3.partial_cmp(&c3)); + + assert_eq!(s1.partial_cmp(&s2), c1.partial_cmp(&c2)); + assert_eq!(s2.partial_cmp(&s1), c2.partial_cmp(&c1)); + + assert_eq!(s1.partial_cmp(&s3), c1.partial_cmp(&c3)); + assert_eq!(s3.partial_cmp(&s1), c3.partial_cmp(&c1)); + + assert_eq!(s3.partial_cmp(&s2), c3.partial_cmp(&c2)); + assert_eq!(s2.partial_cmp(&s3), c2.partial_cmp(&c3)); +} + +#[test] +fn enum_hash_matches() { + let c1 = EverythingEnumCore::E1; + let c2 = EverythingEnumCore::E3(1, 2); + + let s1 = EverythingEnum::E1; + let s2 = EverythingEnum::E3(1, 2); + + assert_eq!(hash_to_int(&s1), hash_to_int(&c1)); + assert_eq!(hash_to_int(&s2), hash_to_int(&c2)); +} + #[test] pub fn copy_struct_eq() { let s1 = EverythingStruct { @@ -121,7 +287,7 @@ pub struct NonDefaultable {} pub enum DefaultableEnum { E1(NonDefaultable), #[default] - E2(usize), + E2, } #[test] diff --git a/tests/perfect_derive_simple_generic_macro.rs b/tests/perfect_derive_simple_generic_macro.rs index ba6a4af..3a0d3b3 100644 --- a/tests/perfect_derive_simple_generic_macro.rs +++ b/tests/perfect_derive_simple_generic_macro.rs @@ -1,5 +1,6 @@ macro_rules! make_test { ($trait_name:ident $(,$trait_name_tail:ident)*; $method_name:ident) => { + #[allow(unused)] mod $method_name { use perfect_derive::perfect_derive;