From f9c811f2da064e9a45469adfb9b5181cd884938f Mon Sep 17 00:00:00 2001 From: Adrian Dapprich Date: Wed, 9 Jul 2025 23:10:12 +0200 Subject: [PATCH 1/6] Add tests for wrong behavior of Eq & Ord As described in #35 --- src/handler/core.rs | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/src/handler/core.rs b/src/handler/core.rs index faae09e..1ffd404 100644 --- a/src/handler/core.rs +++ b/src/handler/core.rs @@ -679,7 +679,7 @@ mod tests { handler::{endpoint, filter, filter_async}, }; - use std::{collections::HashSet, iter::FromIterator}; + use std::{any::Any, collections::HashSet, iter::FromIterator}; use maplit::{btreemap, btreeset, hashset}; @@ -1018,6 +1018,34 @@ Make sure all the required values are provided to the handler. For more informat ); } + #[test] + fn type_eq_ord_consistent() { + #[derive(Clone)] + struct A; + + let ta1 = Type { id: A.type_id(), name: "A1" }; + let ta2 = Type { id: A.type_id(), name: "A2" }; + + assert!(!(ta1 == ta2)); + assert!(ta1 < ta2); + assert!(!(ta1 > ta2)); + } + + #[test] + fn type_btreeset_not_contains_duplicate_name() { + #[derive(Clone)] + struct A; + #[derive(Clone)] + struct B; + + let ta = Type { id: A.type_id(), name: "DuplicateName" }; + let tb = Type { id: B.type_id(), name: "DuplicateName" }; + let set = btreeset! {ta}; + + assert!(ta != tb); + assert!(!set.contains(&tb)); + } + #[tokio::test] async fn type_infer_check_chained_combinators() { #[derive(Clone)] From 7c2eae1a33f47bbd262de754dd8327e87341f121 Mon Sep 17 00:00:00 2001 From: Adrian Dapprich Date: Wed, 9 Jul 2025 23:11:38 +0200 Subject: [PATCH 2/6] Use derived Eq & Ord for Type The previous handwritten implementations were not consistent with each other as they allowed values a & b where a == b but a < b. Fixes #35 --- src/handler/core.rs | 34 ++++++---------------------------- 1 file changed, 6 insertions(+), 28 deletions(-) diff --git a/src/handler/core.rs b/src/handler/core.rs index 1ffd404..d78a205 100644 --- a/src/handler/core.rs +++ b/src/handler/core.rs @@ -7,7 +7,6 @@ use crate::{description, prelude::DependencyMap, HandlerDescription}; use std::{ any::TypeId, - cmp::Ordering, collections::{BTreeMap, BTreeSet}, fmt::Write, future::Future, @@ -102,14 +101,16 @@ pub enum HandlerSignature { /// A run-time representation of a type. Used only for run-time type inference /// and checking of handler chains. /// +/// Type name defined before type identifier so that types are sorted alphabetically +/// using the derived Ord implementation. /// See [`crate::type_check`]. -#[derive(Clone, Copy, Debug)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)] pub struct Type { - /// The unique type identifier. - pub id: TypeId, - /// The type name used for printing. pub name: &'static str, + + /// The unique type identifier. + pub id: TypeId, } impl Hash for Type { @@ -119,29 +120,6 @@ impl Hash for Type { } } -impl PartialEq for Type { - /// Equality is done by type identifiers (type names are ignored). - fn eq(&self, other: &Self) -> bool { - self.id == other.id - } -} - -impl Eq for Type {} - -impl PartialOrd for Type { - /// The partial order is done by type names for better diagnostics. - fn partial_cmp(&self, other: &Self) -> Option { - Some(self.cmp(other)) - } -} - -impl Ord for Type { - /// The total order is done by type names for better diagnostics. - fn cmp(&self, other: &Self) -> Ordering { - self.name.cmp(other.name) - } -} - type DynFn<'a, Output> = dyn Fn(DependencyMap, Cont<'a, Output>) -> HandlerResult<'a, Output> + Send + Sync + 'a; From 286669d45eba8188c969ef6eec33ecf919969c13 Mon Sep 17 00:00:00 2001 From: Adrian Dapprich Date: Fri, 11 Jul 2025 00:54:34 +0200 Subject: [PATCH 3/6] Use rustfmt & reword comment --- src/handler/core.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/handler/core.rs b/src/handler/core.rs index d78a205..ec74358 100644 --- a/src/handler/core.rs +++ b/src/handler/core.rs @@ -101,8 +101,8 @@ pub enum HandlerSignature { /// A run-time representation of a type. Used only for run-time type inference /// and checking of handler chains. /// -/// Type name defined before type identifier so that types are sorted alphabetically -/// using the derived Ord implementation. +/// Type name field placed before type identifier field so that the derived Ord +/// implementation sorts types alphabetically. /// See [`crate::type_check`]. #[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)] pub struct Type { From 204ac85d1e05b483c8f1eb282df15c0c7f13d088 Mon Sep 17 00:00:00 2001 From: Adrian Dapprich Date: Fri, 11 Jul 2025 00:55:06 +0200 Subject: [PATCH 4/6] Add test for PartialEq for DependencyMap The current implementation only zips the two key iterators together. So if one of them is a subset of the other it only checks the keys in the subset for equality. In the simplest case, the empty dependency map is equal to every other dependency map. --- src/di.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/di.rs b/src/di.rs index 5e63de7..003444e 100644 --- a/src/di.rs +++ b/src/di.rs @@ -326,4 +326,18 @@ mod tests { assert_eq!(map.try_get(), Some(Arc::new(42i32))); assert_eq!(map.try_get::(), None); } + + #[test] + fn same_keys() { + let mut map_bool1 = DependencyMap::new(); + let mut map_bool2 = DependencyMap::new(); + let map_empty = DependencyMap::new(); + + map_bool1.insert(false); + map_bool2.insert(true); + + assert_eq!(map_bool1, map_bool2); + assert_ne!(map_bool1, map_empty); + assert_ne!(map_bool2, map_empty); + } } From ed719f0db065b6a1e75cf107be4d4f686561c7f9 Mon Sep 17 00:00:00 2001 From: Adrian Dapprich Date: Fri, 11 Jul 2025 01:00:40 +0200 Subject: [PATCH 5/6] Fix PartialEq implementation of DependencyMap Analogous to how equality is defined for BTreeMap, but only looking at keys. --- src/di.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/di.rs b/src/di.rs index 003444e..0eab331 100644 --- a/src/di.rs +++ b/src/di.rs @@ -93,7 +93,7 @@ impl PartialEq for DependencyMap { fn eq(&self, other: &Self) -> bool { let keys1 = self.map.keys(); let keys2 = other.map.keys(); - keys1.zip(keys2).map(|(k1, k2)| k1 == k2).all(|x| x) + keys1.len() == keys2.len() && keys1.zip(keys2).map(|(k1, k2)| k1 == k2).all(|x| x) } } From 042418b91fe3b44203466f1423eebd1aae79386a Mon Sep 17 00:00:00 2001 From: Adrian Dapprich Date: Fri, 11 Jul 2025 01:01:30 +0200 Subject: [PATCH 6/6] Add changelog entry --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f2ba60a..aec3b30 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## unreleased +### Fixed + + - Fix `PartialEq` and `Ord` implementations. ([PR #36](https://github.com/teloxide/dptree/pull/36)) + - For `Type` they were inconsistent and are replaced by the standard derived implementations. + - Adding a length check when checking equality of `DependencyMap`. + ## 0.5.0 - 2025-06-19 ### Added