diff --git a/CHANGELOG.md b/CHANGELOG.md index 7da5706..8b1dc1b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] - ReleaseDate +### Fixed +- Fixed `LicenseItem`'s `PartialEq` and `PartialOrd` implementations to agree with its total ordering, resolving [#95](https://github.com/EmbarkStudios/spdx/issues/95). + ## [0.13.4] - 2026-02-26 ### Changed - [PR#92](https://github.com/EmbarkStudios/spdx/pull/92) updated SPDX license list to [3.28.0](https://github.com/spdx/license-list-XML/releases/tag/v3.28.0). diff --git a/Cargo.lock b/Cargo.lock index f7c8be2..0a423d8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -58,9 +58,9 @@ dependencies = [ [[package]] name = "crossbeam-epoch" -version = "0.9.18" +version = "0.9.20" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5b82ac4a3c2ca9c3460964f020e1402edd5753411d7737aa39c3714ad1b5420e" +checksum = "2d6914041f254d6e9176c01941b21115dcfb7089e55135a35411081bd106ef3f" dependencies = [ "crossbeam-utils", ] diff --git a/src/detection/preproc.rs b/src/detection/preproc.rs index 1f5d27d..23fa91e 100644 --- a/src/detection/preproc.rs +++ b/src/detection/preproc.rs @@ -202,7 +202,7 @@ fn remove_common_tokens(input: Cow<'_, str>) -> Cow<'_, str> { // reconcile the count with other longer prefixes that may be stored let common_count = prefix_counts .iter() - .filter_map(|(s, count)| Some(count).filter(|_| s.starts_with(most_common))) + .filter_map(|(s, count)| s.starts_with(most_common).then_some(count)) .sum::(); let line_count = input.split('\n').count(); diff --git a/src/lib.rs b/src/lib.rs index d6b6a41..80ed4b0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -415,21 +415,15 @@ impl Ord for LicenseItem { } } -#[allow(clippy::non_canonical_partial_ord_impl)] impl PartialOrd for LicenseItem { fn partial_cmp(&self, o: &Self) -> Option { - match (self, o) { - (Self::Spdx { id: a, .. }, Self::Spdx { id: b, .. }) => a.partial_cmp(b), - (Self::Other(a), Self::Other(b)) => a.partial_cmp(b), - (Self::Spdx { .. }, Self::Other { .. }) => Some(cmp::Ordering::Less), - (Self::Other { .. }, Self::Spdx { .. }) => Some(cmp::Ordering::Greater), - } + Some(self.cmp(o)) } } impl PartialEq for LicenseItem { fn eq(&self, o: &Self) -> bool { - matches!(self.partial_cmp(o), Some(cmp::Ordering::Equal)) + self.cmp(o) == cmp::Ordering::Equal } } @@ -648,9 +642,10 @@ pub fn license_version() -> &'static str { #[cfg(test)] mod test { - use super::LicenseItem; + use super::{LicenseItem, LicenseRef, LicenseReq}; use crate::{Expression, license_id}; - use alloc::string::ToString; + use alloc::{borrow::ToOwned, boxed::Box, string::ToString}; + use core::cmp::Ordering; #[test] fn gnu_or_later_display() { @@ -676,4 +671,77 @@ mod test { assert_eq!(gpl_or_later_in_id.to_string(), "GPL-3.0-or-later"); assert_eq!(non_gnu_or_later.to_string(), "Apache-2.0+"); } + + #[test] + fn license_item_ordering_is_canonical() { + let mit = license_id("MIT").unwrap(); + let apache = license_id("Apache-2.0").unwrap(); + let items = [ + LicenseItem::Spdx { + id: apache, + or_later: false, + }, + LicenseItem::Spdx { + id: mit, + or_later: false, + }, + LicenseItem::Spdx { + id: mit, + or_later: true, + }, + LicenseItem::Other(Box::new(LicenseRef { + doc_ref: None, + lic_ref: "custom".to_owned(), + })), + LicenseItem::Other(Box::new(LicenseRef { + doc_ref: Some("document".to_owned()), + lic_ref: "custom".to_owned(), + })), + ]; + + assert_eq!(items[1].cmp(&items[2]), Ordering::Less); + assert_ne!(items[1], items[2]); + + for left in &items { + for right in &items { + assert_eq!(left.partial_cmp(right), Some(left.cmp(right))); + assert_eq!(*left == *right, left.cmp(right) == Ordering::Equal); + assert_eq!(left.cmp(right), right.cmp(left).reverse()); + + for last in &items { + if left <= right && right <= last { + assert!(left <= last); + } + } + } + } + } + + #[test] + fn license_req_ordering_is_canonical() { + let mit = license_id("MIT").unwrap(); + let requirements = [ + LicenseReq { + license: LicenseItem::Spdx { + id: mit, + or_later: false, + }, + addition: None, + }, + LicenseReq { + license: LicenseItem::Spdx { + id: mit, + or_later: true, + }, + addition: None, + }, + ]; + + for left in &requirements { + for right in &requirements { + assert_eq!(left.partial_cmp(right), Some(left.cmp(right))); + assert_eq!(*left == *right, left.cmp(right) == Ordering::Equal); + } + } + } } diff --git a/src/licensee.rs b/src/licensee.rs index a510969..467cf4f 100644 --- a/src/licensee.rs +++ b/src/licensee.rs @@ -245,14 +245,24 @@ impl Licensee { impl PartialOrd for Licensee { #[inline] fn partial_cmp(&self, o: &LicenseReq) -> Option { - self.inner.partial_cmp(o) + let license = match (&self.inner.license, &o.license) { + (LicenseItem::Spdx { id: a, .. }, LicenseItem::Spdx { id: b, .. }) => a.cmp(b), + (LicenseItem::Other(a), LicenseItem::Other(b)) => a.cmp(b), + (LicenseItem::Spdx { .. }, LicenseItem::Other { .. }) => core::cmp::Ordering::Less, + (LicenseItem::Other { .. }, LicenseItem::Spdx { .. }) => core::cmp::Ordering::Greater, + }; + + match license { + core::cmp::Ordering::Equal => self.inner.addition.partial_cmp(&o.addition), + ordering => Some(ordering), + } } } impl PartialEq for Licensee { #[inline] fn eq(&self, o: &LicenseReq) -> bool { - self.inner.eq(o) + self.partial_cmp(o) == Some(core::cmp::Ordering::Equal) } } @@ -309,12 +319,12 @@ mod test { // Licensees can't have the `or_later` assert!(licensees.binary_search_by(|l| l.inner.cmp(&req)).is_err()); - match &licensees[licensees + let licensee = &licensees[licensees .binary_search_by(|l| l.partial_cmp(&req).unwrap()) - .unwrap()] - .inner - .license - { + .unwrap()]; + assert_eq!(licensee, &req); + + match &licensee.inner.license { LicenseItem::Spdx { id, .. } => assert_eq!(*id, mpl_id), o @ LicenseItem::Other { .. } => panic!("unexpected {o:?}"), }