rust-pretty-assertions icon indicating copy to clipboard operation
rust-pretty-assertions copied to clipboard

Unexpected regression with 1.2.0

Open mre opened this issue 3 years ago • 3 comments
trafficstars

We use rust-pretty-assertions for lychee. There is a PR for upgrading to 1.2.0 here, which fails because of the changes in #92. Previously, using assert_eq on two HashSets would work, but it fails now because of the missing AsRef<str> impl.

Before, the assertion was

assert_eq!(uris_html5gum, uris_html5ever);

and I could use a workaround like this

assert_eq!(format!("{uris_html5gum:?}"), format!("{uris_html5ever:?}"));

but I wonder if I'm missing something. The PR mentioned an assert_debug_eq that would behave like the old version. Is this planned?

mre avatar Mar 18 '22 23:03 mre

Hi, thanks for reporting this. This certainly looks like a bug that you have an upgrade failing with a compile error - I will take a look this weekend and ping you once I know more.

tommilligan avatar Mar 19 '22 07:03 tommilligan

Just to update you - this is definitely a bug, and I think I can see why it happens. I have a couple of avenues of investigation to persue, but I've been sick for a couple of days this week so probably won't make any progress until next weekend.

If you have the time, a minimally failing test case using standard library types would be very useful. I can see your specific test fails, but haven't been able add a minimally failing example to the library's test suite yet. If you're able to add one, it should live here: https://github.com/colin-kiegel/rust-pretty-assertions/blob/9654759b57a88cec10fd4ffac563a93a80006689/pretty_assertions/tests/macros.rs#L29


It looks like your case is trying to use the wrong impl block out of:

  • for &'a (T, U) https://github.com/colin-kiegel/rust-pretty-assertions/blob/9654759b57a88cec10fd4ffac563a93a80006689/pretty_assertions/src/lib.rs#L451
  • for (&'a T, &'a U) https://github.com/colin-kiegel/rust-pretty-assertions/blob/9654759b57a88cec10fd4ffac563a93a80006689/pretty_assertions/src/lib.rs#L458

and the compiler is correctly pointing out that your types do not implement CompareAsStrByDefault, which was introduced in #92.

This feels more generally like the problem of implementing mutually exclusive trait implementation for types, where trait implementations should always be additive :slightly_frowning_face: In general, it's not correct to say "and for all other types, use this other implementation", which is what #92 attempts by using a different tuple type, which can be automatically converted to (deref'd?) by the compiler.

tommilligan avatar Mar 23 '22 11:03 tommilligan

Thanks for looking into this and there is certainly no rush on our end to resolve this. The pervious version works fine still and we have a workaround for the the latest version. I will try to add a test case for it if I find the time, but if anyone else is faster, feel free to give it a shot. 😉

mre avatar Mar 23 '22 13:03 mre