Deranged 0.4.1 breaks build of other crates (PartialEq with empty slices)

[chifflier]

Hi,

After a cargo update from 0.4.0 to 0.4.1, building unit tests breaks in unrelated parts of my crate (asn1-rs) due to a conflicting implementation of PartialEq. Building with 0.4.0 works fine

I know this has already been reported, however I think this is not a duplicate:

• I have updated all dependencies using a global cargo update (including time and deranged), problem is still here
• cargo outdated shows to crate to upgrade
• this seems to happen only with empty slices
• this happens in code not importing time nor deranged

I tried to pin the problem to a minimal test case
The errors are related to comparisons of empty &[u8] (non-empty comparison work). For ex, the following fails:

fn build_failure() {
    let b1 = &[0_u8, 1];
    assert_eq!(b1, &[]);
}

Edit: the minimal crate I created to show the problem fails for another reason
Edit 2:
I have tried to isolate the problem in a minimal crate here
Adding use time::OffsetDateTime is enough to trigger the problem:
The first function build_ok builds, while the second fails with error:

error[E0283]: type annotations needed
  --> src/lib.rs:10:5
   |
10 |     assert_eq!(b1, &[]);
   |     ^^^^^^^^^^^^^^^^^^^ cannot infer type
   |
   = note: multiple `impl`s satisfying `u8: PartialEq<_>` found in the following crates: `core`, `deranged`:
           - impl PartialEq for u8;
           - impl<MIN, MAX> PartialEq<deranged::RangedU8<MIN, MAX>> for u8
             where the constant `MIN` has type `u8`, the constant `MAX` has type `u8`;
   = note: required for `[u8]` to implement `PartialEq<[_; 0]>`
   = note: 1 redundant requirement hidden
   = note: required for `&[u8]` to implement `PartialEq<&[_; 0]>`
   = note: this error originates in the macro `assert_eq` (in Nightly builds, run with -Z macro-backtrace for more info)

[ggwpez]

This breaks our code base for downstream users who do not use --locked to compile: paritytech/polkadot-sdk#8121

cargo update -p deranged --precise 0.4.0 for anybody wondering what to do.

[tdkt]

Please release 0.5.0 and then yank 0.4.1. It also messes with From<u16> for usize.

[jhpratt]

Unfortunately there isn't really anything that can be done on my end. The impl is considered acceptable breakage per RFC 1105, and even if it happened in a breaking release the same issue would arise due to it being an indirect (and not publicly exposed) dependency.

[RalfJung]

Something seems off if a crate introduces breakage to entirely unrelated code that isn't even close to any of the types involved in the crate -- maybe this is formally acceptable, but it's not something I encountered before with another crate, and it is not nice to have invasive changes on unrelated code paths in this way.

[chifflier]

Unfortunately there isn't really anything that can be done on my end.

I tend to disagree - there are other solutions than adding a trait that leaks on other crates. Implementing PartialEq for all T might be nice for you own use, but I'm sure this could be implemented another way. Besides, this wasn't present in the previous version.

The impl is considered acceptable breakage per RFC 1105, and even if it happened in a breaking release the same issue would arise due to it being an indirect (and not publicly exposed) dependency.

This RFC covers semver and version numbering. I do not think breaking a default trait implementation for all other crates can be described as an "API change"

I understand the changes you pushed might be nice, but I kindly ask you to reconsider the "wontfix". Maybe this trait can be used only internally (or when imported), or maybe using PartialEq is not possible without leaking.

[jhpratt]

Something seems off if a crate introduces breakage to entirely unrelated code that isn't even close to any of the types involved in the crate

imo this is on Rust — type inference errors are far too easy to cause.

impl<T, U> PartialEq<[U]> for [T]
where
    T: PartialEq<U>

If this were impl<T, U=T>, this should work. I don't know if that's currently possible in Rust or not.

Maybe this trait can be used only internally (or when imported)

That's simply not possible. The orphan rule prevents doing this in a downstream crate, which is precisely what the impl is intended for.

---

Not directed at anyone in particular, but I will restate this. Yanking and releasing as 0.5 accomplishes nothing, unless you are relying on deranged directly (which is rarely the case).

[tdkt]

Fair enough that a version bump doesn't help as the conflicting code is in downstreams. It does mean this is no longer possible in any crate that has deranged somewhere in the dependency graph:

// just so the crate actually gets included.
use deranged::OptionRangedI8 as _;

fn main() {
    let a = 3usize;
    let b = 3u16;

    if a == b.into() {
        println!("Hello, world!");
    }
}

Causes:

error[E0283]: type annotations needed
 --> src/main.rs:8:15
  |
8 |     if a == b.into() {
  |          --   ^^^^
  |          |
  |          type must be known at this point
  |
  = note: multiple `impl`s satisfying `usize: PartialEq<_>` found in the following crates: `core`, `deranged`:
          - impl PartialEq for usize;
          - impl<MIN, MAX> PartialEq<RangedUsize<MIN, MAX>> for usize
            where the constant `MIN` has type `usize`, the constant `MAX` has type `usize`;
help: try using a fully qualified path to specify the expected types
  |
8 |     if a == <u16 as Into<T>>::into(b) {
  |             +++++++++++++++++++++++ ~

For more information about this error, try `rustc --explain E0283`.

Should this be reported on rust issue tracker or is this working as intended?

[jhpratt]

While I still believe this can and should be handled better by the language, the reality is that it isn't and caused breakage in more ways than I thought would happen, including code that cannot easily be adjusted. For that reason, I am reconsidering whether to yank the release. If I do, there will not be a 0.5 release with the same. It would be removed entirely unless and until it is handled better on the language side (which I advocate for regardless).

Expect a final decision in the next day or two at the latest. I will announce it here regardless of outcome. The reason for the delay is that I'm currently busy and abroad, which is not a good combination for making OSS decisions.

[jhpratt]

Alright, it's yanked. Hopefully this improves things!

[RalfJung]

Thanks! I fully agree that this is an issue with the trait system. But that system is hard to improve an meanwhile we have to live with the system that we have.