From 4ef4947364031c25b1a60fc913ed290842df4263 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?dj8yf0=CE=BCl?= Date: Tue, 22 Aug 2023 17:03:39 +0300 Subject: [PATCH] feat!: raise bound on keys in hashcollections `PartialOrd` -> `Ord` --- borsh-derive/src/lib.rs | 8 ++++---- borsh/src/de/mod.rs | 10 ++++------ borsh/src/lib.rs | 1 + borsh/src/ser/mod.rs | 8 ++++---- borsh/tests/test_generic_struct.rs | 14 ++++++-------- borsh/tests/test_recursive_structs.rs | 2 +- 6 files changed, 20 insertions(+), 23 deletions(-) diff --git a/borsh-derive/src/lib.rs b/borsh-derive/src/lib.rs index 838d26e17..4c64df1a0 100644 --- a/borsh-derive/src/lib.rs +++ b/borsh-derive/src/lib.rs @@ -83,13 +83,13 @@ Attribute adds possibility to override bounds for `BorshSerialize` in order to e 2. fixing complex cases, when derive hasn't figured out the right bounds on type parameters automatically. ```ignore -/// additional bound `T: PartialOrd` (required by `HashMap`) is injected into +/// additional bound `T: Ord` (required by `HashMap`) is injected into /// derived trait implementation via attribute to avoid adding the bounds on the struct itself #[derive(BorshSerialize)] struct A { a: String, #[borsh(bound(serialize = - "T: borsh::ser::BorshSerialize + PartialOrd, + "T: borsh::ser::BorshSerialize + Ord, U: borsh::ser::BorshSerialize"))] b: HashMap, } @@ -360,14 +360,14 @@ Attribute adds possibility to override bounds for `BorshDeserialize` in order to 2. fixing complex cases, when derive hasn't figured out the right bounds on type parameters automatically. ```ignore -/// additional bounds `T: PartialOrd + Hash + Eq` (required by `HashMap`) are injected into +/// additional bounds `T: Ord + Hash + Eq` (required by `HashMap`) are injected into /// derived trait implementation via attribute to avoid adding the bounds on the struct itself #[derive(BorshDeserialize)] struct A { a: String, #[borsh(bound( deserialize = - "T: PartialOrd + Hash + Eq + borsh::de::BorshDeserialize, + "T: Ord + Hash + Eq + borsh::de::BorshDeserialize, U: borsh::de::BorshDeserialize" ))] b: HashMap, diff --git a/borsh/src/de/mod.rs b/borsh/src/de/mod.rs index 01e42185b..489d471da 100644 --- a/borsh/src/de/mod.rs +++ b/borsh/src/de/mod.rs @@ -490,12 +490,10 @@ pub mod hashes { #[cfg(feature = "de_strict_order")] use crate::__private::maybestd::io::{Error, ErrorKind}; use crate::error::check_zst; - #[cfg(feature = "de_strict_order")] - use core::cmp::Ordering; impl BorshDeserialize for HashSet where - T: BorshDeserialize + Eq + Hash + PartialOrd, + T: BorshDeserialize + Eq + Hash + Ord, H: BuildHasher + Default, { #[inline] @@ -513,7 +511,7 @@ pub mod hashes { let [a, b] = pair else { unreachable!("`windows` always return a slice of length 2 or nothing"); }; - let cmp_result = a.partial_cmp(b).map_or(false, Ordering::is_lt); + let cmp_result = a.cmp(b).is_lt(); if !cmp_result { return Err(Error::new( ErrorKind::InvalidData, @@ -528,7 +526,7 @@ pub mod hashes { impl BorshDeserialize for HashMap where - K: BorshDeserialize + Eq + Hash + PartialOrd, + K: BorshDeserialize + Eq + Hash + Ord, V: BorshDeserialize, H: BuildHasher + Default, { @@ -548,7 +546,7 @@ pub mod hashes { let [(a_k, _a_v), (b_k, _b_v)] = pair else { unreachable!("`windows` always return a slice of length 2 or nothing"); }; - let cmp_result = a_k.partial_cmp(b_k).map_or(false, Ordering::is_lt); + let cmp_result = a_k.cmp(b_k).is_lt(); if !cmp_result { return Err(Error::new( ErrorKind::InvalidData, diff --git a/borsh/src/lib.rs b/borsh/src/lib.rs index 70aedaa76..2663b7b10 100644 --- a/borsh/src/lib.rs +++ b/borsh/src/lib.rs @@ -43,6 +43,7 @@ are encountered in ascending order with respect to [PartialOrd](core::cmp::PartialOrd) for hash collections, and [Ord](core::cmp::Ord) for btree ones. Deserialization emits error otherwise. + If this feature is not enabled, it is possible that two different byte slices could deserialize into the same `HashMap`/`HashSet` object. ### Config aliases diff --git a/borsh/src/ser/mod.rs b/borsh/src/ser/mod.rs index 1b8c1ca75..2ab3ade2e 100644 --- a/borsh/src/ser/mod.rs +++ b/borsh/src/ser/mod.rs @@ -364,7 +364,7 @@ pub mod hashes { impl BorshSerialize for HashMap where - K: BorshSerialize + PartialOrd, + K: BorshSerialize + Ord, V: BorshSerialize, H: BuildHasher, { @@ -373,7 +373,7 @@ pub mod hashes { check_zst::()?; let mut vec = self.iter().collect::>(); - vec.sort_by(|(a, _), (b, _)| a.partial_cmp(b).unwrap()); + vec.sort_by(|(a, _), (b, _)| a.cmp(b)); u32::try_from(vec.len()) .map_err(|_| ErrorKind::InvalidData)? .serialize(writer)?; @@ -387,7 +387,7 @@ pub mod hashes { impl BorshSerialize for HashSet where - T: BorshSerialize + PartialOrd, + T: BorshSerialize + Ord, H: BuildHasher, { #[inline] @@ -395,7 +395,7 @@ pub mod hashes { check_zst::()?; let mut vec = self.iter().collect::>(); - vec.sort_by(|a, b| a.partial_cmp(b).unwrap()); + vec.sort(); u32::try_from(vec.len()) .map_err(|_| ErrorKind::InvalidData)? .serialize(writer)?; diff --git a/borsh/tests/test_generic_struct.rs b/borsh/tests/test_generic_struct.rs index a60324324..c0944d899 100644 --- a/borsh/tests/test_generic_struct.rs +++ b/borsh/tests/test_generic_struct.rs @@ -54,7 +54,7 @@ struct NamedA { /// `T: Hash + Eq` bound is required for `BorshDeserialize` derive to be successful #[cfg(hash_collections)] #[derive(BorshSerialize, BorshDeserialize)] -struct C { +struct C { a: String, b: HashMap, } @@ -65,7 +65,7 @@ struct C { #[derive(BorshSerialize)] struct C1 { a: String, - #[borsh(bound(serialize = "T: borsh::ser::BorshSerialize + PartialOrd, + #[borsh(bound(serialize = "T: borsh::ser::BorshSerialize + Ord, U: borsh::ser::BorshSerialize"))] b: HashMap, } @@ -77,10 +77,8 @@ struct C1 { #[derive(BorshDeserialize)] struct C2 { a: String, - #[borsh(bound( - deserialize = "T: PartialOrd + Hash + Eq + borsh::de::BorshDeserialize, - U: borsh::de::BorshDeserialize" - ))] + #[borsh(bound(deserialize = "T: Ord + Hash + Eq + borsh::de::BorshDeserialize, + U: borsh::de::BorshDeserialize"))] b: HashMap, } @@ -101,7 +99,7 @@ struct G1(#[borsh(skip)] HashMap, U); #[cfg(hash_collections)] #[derive(BorshDeserialize)] -struct G2(HashMap, #[borsh(skip)] U); +struct G2(HashMap, #[borsh(skip)] U); /// implicit derived `core::default::Default` bounds on `K` and `V` are dropped by empty bound /// specified, as `HashMap` hash its own `Default` implementation @@ -139,7 +137,7 @@ enum I1 { #[cfg(hash_collections)] #[derive(BorshSerialize, BorshDeserialize, Debug)] -enum I2 { +enum I2 { B { x: HashMap, y: String }, C(K, #[borsh(skip)] U), } diff --git a/borsh/tests/test_recursive_structs.rs b/borsh/tests/test_recursive_structs.rs index a580b86c7..f281ddb57 100644 --- a/borsh/tests/test_recursive_structs.rs +++ b/borsh/tests/test_recursive_structs.rs @@ -17,7 +17,7 @@ use alloc::{boxed::Box, string::String, vec::Vec}; #[cfg(hash_collections)] #[derive(BorshSerialize, BorshDeserialize)] -struct CRec { +struct CRec { a: String, b: HashMap>, }