Repository navigation
Implement PartialEq/Eq for InvalidUri? #849
Description
Activity
Just curious, what about
assert_matches!(), or oldassert!(matches!()).Those both work fine, but the error itself is also nested. So you end up having to propagate the workaround everywhere rather than just assert_eq!
It's not a huge blocker in terms of this code can't be written. Just a thing that feels like it should be supported to avoid undifferentiated code across all places that embed InvalidUri in other errors. the exact code is:
impl PartialEq for ResourceError { fn eq(&self, other: &Self) -> bool { match (self, other) { (Self::RelativeReference, Self::RelativeReference) | (Self::InvalidCharacters, Self::InvalidCharacters) | (Self::InvalidPercentEncoding, Self::InvalidPercentEncoding) | (Self::MissingHttpAuthority, Self::MissingHttpAuthority) => true, (Self::InvalidHttpUri(left), Self::InvalidHttpUri(right)) => { left.to_string() == right.to_string() } _ => false, } } }
@seanmonstar is there any push back on this? Obv. the impl is extremely simple, and I've done as much due diligence as possible in understanding the underlying background here. This sort of feels like it's either LGTM-shipit (#850) or a message saying that I didn't think about this unknown unknown...
Even if "I'm not sure, please do some work and dig into x for me" would set a direction here.
Hm, I see what you're looking for. I did a little more thinking, and read up on where this has come up before. I still lean towards not merging.
I think in general, Rust
Errors shouldn't implementPartialEq. They might have internal details that don't make sense to compare. Like positional data, or a source chain. Even if we don't have those details yet, committing to a public API can restrict us from internal refactoring.(A public, exhaustive, flat
enumcould choose differently. But those look more like akind(). These are opaque error structs.)Similar to in that bincode issue, if testing is the main reason, I think
matches!,kind(), oris_*()patterns fit better.Thanks for considering this. I looked further into the concern that implementing equality could constrain future internal changes.
One relevant standard-library precedent is
TryReserveError. It originally derivedPartialEqandEq, and Rust later deliberately changed it into an opaque struct because the private error details were still unsettled (rust-lang/rust#87408). The stabilization proposal explicitly left open both adding support for future allocator details and reducing the error to a unit struct, while the public type was stabilized with its equality implementations intact (rust-lang/rust#87993).I did not find an explicit discussion of equality in those reviews, so I would not claim that this settles the question for
InvalidUri. It does show that std has stabilized equality for an opaque error whose private representation was deliberately left open to substantial change. Together withUtf8Errorimplementing equality despite carrying positional information, that makes me less concerned that these possible future details are necessarily blocking.My practical motivation remains that
InvalidUriis a leaf error from a foundational crate. Including it in a downstream error enum prevents that enum from derivingPartialEq, moving the implementation work into each downstream crate. My take is that this compositional benefit outweighs the limited compatibility constraint here, particularly if equality is defined without exposing the private kind or making future diagnostic details significant.That said, the downstream workaround is reasonable, and I understand the preference not to add the contract. I’m happy to leave this undone and close the PR.
I have a library with its own error type, and one variant wraps
http::uri::InvalidUri. I would like that error type to derivePartialEqandEqso unit tests can compare expected and actual errors directly, butInvalidUricurrently prevents that.For example:
Today I can work around this with a manual
PartialEqimplementation in my crate, but the only special case is this wrappedhttperror. The main thing I want is boring unit tests that compare errors normally.Would it be reasonable for
InvalidUriandInvalidUriPartsto implementPartialEqandEq?The small patch would be:
ErrorKindalready implementsPartialEqandEq, and this would keep it private. I am not trying to expose or match on the URI error kind.I also realize this is still a public contract. I saw the previous discussions around error trait impls, especially #128 and #204 around
Clone, and I understand the concern that trait impls can constrain future error internals.PartialEq/Eqseems like a smaller commitment thanClone, but it is still a commitment: users could compare twoInvalidUrivalues and observe whether they are considered the same broad error.My take is that this tradeoff is reasonable for these URI errors because the comparison can stay coarse,
ErrorKindstays private, and the concrete downstream benefit is allowing library error types to derive equality for tests. I may be underestimating the compatibility cost, though, so I would appreciate maintainer feedback.I am opening this as an issue even though the possible patch is small, because the useful part is the API decision. If the answer is "no, this is too much public contract for URI errors," that decision would be useful to have recorded for future searches.
One scope question: the same motivation may apply to other opaque conversion errors in this crate, such as
InvalidMethod,InvalidStatusCode,InvalidHeaderName,InvalidHeaderValue, andToStrError. I am starting withInvalidUribecause that is the case I ran into, andInvalidUriPartsis just a wrapper around it.