Make semantics of asset comparisons consistent#1263
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1263 +/- ##
==========================================
+ Coverage 89.74% 89.76% +0.01%
==========================================
Files 57 57
Lines 8195 8189 -6
Branches 8195 8189 -6
==========================================
- Hits 7355 7351 -4
+ Misses 544 542 -2
Partials 296 296 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors AssetRef comparison behavior to make Eq/Ord/Hash consistent and non-panicking by introducing an internal AssetCmp key type that derives the comparison traits, addressing the inconsistency described in #1215.
Changes:
- Remove derived
PartialEqfromAsset. - Implement
AssetRef’sPartialEq/Eq/Ord/PartialOrd/Hashvia a new derivedAssetCmpenum. - Update/replace
AssetRefdocumentation and comparison logic to avoid panics and align semantics.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[derive(Clone)] | ||
| pub struct Asset { |
There was a problem hiding this comment.
Removing PartialEq from the public Asset type is an API-breaking change for downstream crates (they can no longer write asset1 == asset2 or use Asset in APIs requiring PartialEq). If the intent is to avoid deep comparisons for performance, consider implementing a manual PartialEq with the desired semantics (e.g., ID-based when present / a shallow key otherwise), or gate the removal behind a major version bump / explicit deprecation plan.
| /// Get a representation of this [`AssetRef`] that can be used for comparisons | ||
| fn get_asset_cmp(&self) -> AssetCmp<'_> { | ||
| if let Some(id) = self.id() { | ||
| AssetCmp::WithID(id) | ||
| } else { | ||
| AssetCmp::WithoutID(( | ||
| self.process_id(), | ||
| self.region_id(), | ||
| self.commission_year, | ||
| self.agent_id(), | ||
| self.group_id(), | ||
| )) | ||
| } | ||
| } |
There was a problem hiding this comment.
This PR changes the fundamental Eq/Ord/Hash semantics for AssetRef (and removes prior panics). There are existing tests in this module, but none that assert key invariants like: (1) a == b implies equal hashes, (2) hashing/ordering does not panic for all AssetState variants, and (3) commissioned (WithID) assets compare/hash purely by ID. Adding focused unit tests around get_asset_cmp/AssetCmp would help prevent regressions.
alexdewar
requested review from
AdrianDAlessandro,
Aurashk,
Copilot,
mjademitchell and
tsmbland
alexdewar
force-pushed
the
fix-comparing-assets
branch
from
790b463 to
4148f8c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1212,12 +1200,31 @@ impl PartialOrd for AssetRef { | |||
| } | |||
| } | |||
|
|
|||
| impl Ord for AssetRef { | |||
| fn cmp(&self, other: &Self) -> Ordering { | |||
| self.id().unwrap().cmp(&other.id().unwrap()) | |||
| impl Hash for AssetRef { | |||
| fn hash<H: Hasher>(&self, state: &mut H) { | |||
| self.get_asset_cmp().hash(state); | |||
| } | |||
There was a problem hiding this comment.
This change is specifically about making Eq/Ord/Hash for AssetRef consistent and non-panicking, but there don’t appear to be any unit tests covering the new comparison/hashing semantics (e.g., hashing/ordering Future/Selected/Decommissioned assets and asserting Eq/Ord/Hash consistency). Adding targeted tests here would help prevent regressions in key semantics used by HashMap/sorting.
| /// [`AssetRef`] implements equality, ordering, and hashing using the comparison key represented by | ||
| /// `AssetCmp`. When the underlying asset has an ID, that ID is used for `Eq`/`Ord`/`Hash`. |
There was a problem hiding this comment.
The doc comment references the comparison key as AssetCmp, but AssetCmp is an internal/private type and won’t be visible in generated docs. Consider rewording to describe it as an internal comparison key (or link it as [AssetCmp] and make it pub(crate) if you want it referenced in docs), to avoid confusing readers of the public API docs.
| /// [`AssetRef`] implements equality, ordering, and hashing using the comparison key represented by | |
| /// `AssetCmp`. When the underlying asset has an ID, that ID is used for `Eq`/`Ord`/`Hash`. | |
| /// [`AssetRef`] implements equality, ordering, and hashing using an internal comparison key. | |
| /// When the underlying asset has an ID, that ID is used for `Eq`/`Ord`/`Hash`. |
alexdewar
force-pushed
the
fix-comparing-assets
branch
from
4148f8c to
2950ee2
Compare
Description
AssetRefneeds to implementOrd,EqandHashand we have to do this manually as opposed to deriving the traits. There are a couple of problems with the current implementations, however:AssetStateis one that we don't need to hash)While this might not cause problems with the code as it is, it is a fragile and bugprone setup and we may end up violating some invariant in the standard library somewhere at some point (e.g. see documentation for
HashMap).Let's make these implementations all consistent. The way I've done this is to define a separate
AssetCmpenum, which contains just the properties we use for comparison and which derivesOrd,Eq,Hashetc. The implementations of these traits forAssetRefcan then just createAssetCmpobjects as needed and then provide a thin wrapper around the derived implementation forAssetCmp. Another upside of this approach is it makes it easy to change the properties that are compared by just changingAssetCmp, rather than having to change the code in a bunch of different places.One difference from the old code is that we are now comparing agent IDs and parent IDs (if present) for
OrdandEq, whereas before we didn't bother, but in practice, this won't change anything. Similarly, there were places where we compared assets' processes withRc::ptr_eqbut now use the process ID, but it shouldn't be a functional change in our code base.Closes #1215.
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks