Improve assert_eq message

[a1phyr]

Improve error message for assert_eq and assert_ne macros, by printing the expressions instead of left and right.

Example:

assert_eq!(1 + 1, 5);

now gives

thread 'main' panicked at 'assertion failed: `(1 + 1) == (5)`
1 + 1: `2`,
    5: `5`

instead of

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `2`,
 right: `5`,

This may greatly improve debugging experience.

[rust-highfive]

r? @shepmaster

(rust_highfive has picked a reviewer for you, use r? to override)

[lcnr]

I am afraid that the current impl may cause a pretty noticable slowdown + binary bloat.

While I agree that the better error messages are nice, we really should do a perf run before landing this.

[lcnr]

If you want to check out the difference (it is not that huge, but you have to consider how often assert_eq is used)

https://rust.godbolt.org/z/zUc6wc

edit: Something like this may still give most of the benefit without a big perf impact, will still slightly increase binary sizes though...

 ($left:expr, $right:expr) => ({
        match (&$left, &$right) {
            (left_val, right_val) => {
                if !(*left_val == *right_val) {
                    // The reborrows below are intentional. Without them, the stack slot for the
                    // borrow is initialized even before the values are compared, leading to a
                    // noticeable slow down.
                    panic!(concat!(r#"assertion failed: `(", stringify!($left), ") == (", stringify!($right), ")`",
  left: `{:?}`,
 right: `{:?}`"#), &*left_val, &*right_val)
                }
            }
        }
    });

[pickfire]

What happens if the line is very long? Let's say

assert_eq!(
    "the quick brown fox jumps over the lazy dog.",
    "The wizard quickly jinxed the gnomes before they vaporized."
);

Won't the error message be too long? Do we need some kind of truncation here?

Also, I find it quite redundant to have 1: 1 on the left and 2: 2 on the right, I believe it should only be done when it is an expression or a variable. But that probably involves heuristics.

[a1phyr]

If you want to check out the difference (it is not that huge, but you have to consider how often assert_eq is used)

https://rust.godbolt.org/z/zUc6wc

They are several things here that can be noticed:

• The fast path is exactly the same.
• The panic path is a little longer. Speed is not an issue here, but there is more codegen work to do. Also, because there are more instructions it may be less cache-friendly for the fast path.
• New strings have to fit in the binary for each assert_eq. I don't think it has a significant impact, but for embedded as WASM target it might count.

Maybe we should do some benchmarks ?

edit: Something like this may still give most of the benefit without a big perf impact, will still slightly increase binary sizes though...

I tested that too, but there where issues because $left and $right can contain {, which causes issues with formatting.

Won't the error message be too long? Do we need some kind of truncation here?

IMHO truncating the message would be more confusing than helpful.

Also, I find it quite redundant to have 1: 1 on the left and 2: 2 on the right, I believe it should only be done when it is an expression or a variable. But that probably involves heuristics.

We could match on literals to print them differently. It would complicate the macro, though.

[lcnr]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit 4105abb with merge f28fc8b1abbb9968879d04458b6fa25b74a7ba89...

[pickfire]

@a1phyr In any case, can we keep the original left and right? Why not?

thread 'main' panicked at 'assertion failed: `left == right`
left (1 + 1): `2`,
   right (5): `5`

The original reason is to improve debugging experience, it would just be as weird for developer (me personally) to suddenly see 1 + 1 and 5, I didn't know they meant the left and right expression the first time I see it.

[bors]

☀️ Try build successful - checks-actions, checks-azure
Build commit: f28fc8b1abbb9968879d04458b6fa25b74a7ba89 (f28fc8b1abbb9968879d04458b6fa25b74a7ba89)

[rust-timer]

Queued f28fc8b1abbb9968879d04458b6fa25b74a7ba89 with parent f844ea1, future comparison URL.

[mark-i-m]

I wonder if we could have the nicer messages if debug assertions are on and the existing assertions otherwise?

[workingjubilee]

The original reason is to improve debugging experience, it would just be as weird for developer (me personally) to suddenly see 1 + 1 and 5, I didn't know they meant the left and right expression the first time I see it.

Does not seem clearer to me either. left/right are a fine convention for the current version. I would appreciate better assertion failure messages but I would rather have the ability to provide my own context for my assertion failure rather than this, because this is very much in the field of "a tool trying to guess what the user wants rather than letting the user say exactly what".

[rust-timer]

Finished benchmarking try commit (f28fc8b1abbb9968879d04458b6fa25b74a7ba89): comparison url.

[a1phyr]

Given the terrible benchmark results, I think we can close this anyway.

[mark-i-m]

Thanks anyway @a1phyr! I run into this sort of problem every once in a while...