improve custom message format in assert_eq macro

[MakitaToki]

I just change the message format in assert_failed_inner function. There may be other better modifications, such as splitting custom message and error location, but I can't assert. More info, see #94005

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

[MakitaToki]

I checked the three panicked files. In fact, the test purpose of those files is to pass the test if an error is reported. Should I make some changes to these three files? @Mark-Simulacrum

[Mark-Simulacrum]

This seems ok to me, though I can't say that I'm particularly enthusiastic about "context" -- I'm wondering if there's maybe a better option. Maybe no word at all?

cc @rust-lang/libs in case others may have thoughts on a better design here, since we probably don't want to make changes here too frequently.

[Mark-Simulacrum]

It looks like the intent here was to align left: and right: with each other along the : - probably we want to do the same with context: if we do this?

[MakitaToki]

updated as suggested

[MakitaToki]

Do I continue to submit more commits according to yaahc's suggestions, or close this PR?

[yaahc]

Go ahead and update the format as suggested.

[MakitaToki]

Change panicking.rs in core package and that one in std remains the same as the latter makes more failed tests.
The current output format is like this:

thread 'main' panicked at 'assertion failed: `(upper_bounds == target)`
   Error: `1 + 1 definitely should be 3`,
   upper_bounds: `2`,
   target: `3`', /home/kougami/temp/rust/src/test/ui/macros/assert-eq-macro-msg.rs:8:5

[MakitaToki]

This is my mistake. I definitely misunderstood your original comment. Do you want to stringify parameter names? @yaahc
Is it like this?

        Some(args) => panic!(
            r#"assertion failed: `(left {} right)`
  left: {},
 right: {}: {}"#,
            stringify!(op), stringify!(left), stringify!((right), stringify!(args)
        ),

[yaahc]

not exactly, stringify! stringifies exactly whatever you pass into it, so calling stringify!(op) will just give you "op". Instead you'd need to call stringify! from inside of the assert_eq! macro_rules definition where you still have access to the original tokens, then you'd have to add arguments to assert_failed and assert_failed_inner to pass the strings all the way to the inner function that formats them, so it would look something like this:

// in `library/core/src/macros/mod.rs`
macro_rules! assert_eq {
    ($left:expr, $right:expr $(,)?) => ({
        match (&$left, &$right) {
            (left_val, right_val) => {
                if !(*left_val == *right_val) {
                    let kind = $crate::panicking::AssertKind::Eq;
                    let left_name = stringify!($left);
                    let right_name = stringify!($right);
                    // 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.
                    $crate::panicking::assert_failed(kind, &*left_val, &*right_val, left_name, right_name, $crate::option::Option::None);
                }
            }
        }
    });
   // the rest of the assert_eq definition ...
}

// in `library/core/src/panicking` assert_failed_inner
fn assert_failed_inner(
    kind: AssertKind,
    left: &dyn fmt::Debug,
    right: &dyn fmt::Debug,
    left_name: &'static str,
    right_name: &'static str,
    args: Option<fmt::Arguments<'_>>,
) -> ! {
    let op = match kind {
        AssertKind::Eq => "==",
        AssertKind::Ne => "!=",
        AssertKind::Match => "matches",
    };

    match args {
        Some(args) => panic!(
            r#"assertion failed: `({left_name} {} {right_name})`
  {left_name}: `{:?}`,
 {right_name}: `{:?}`: {}"#,
            op, left, right, args
        ),
       // ....
}

[MakitaToki]

I tried my best to find some code snippets that needs to be updated. Is there anything else I'm missing?

[yaahc]

no, this looks perfect! We just need to get the exact output sorted and update the test UI files so everything passes. Right now looking at the failed test in CI the output looks like this:

 thread 'main' panicked at 'assertion failed: `(1 + 1 == 5)`
  1 + 1: `2`,
 5: `5`', /checkout/src/test/ui/test-attrs/test-panic-abort.rs:34:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

So we probably want to clean this up a bit, and we may want to rethink how many times we repeat the stringified version of the expression. Here's my suggestion for what we should aim for:

thread 'main' panicked at 'assertion failed: `(1 + 1 == 5)`
  left: `2`,
 right: `5`'
    at: /checkout/src/test/ui/test-attrs/test-panic-abort.rs:34:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[MakitaToki]

updated my commit

[yaahc]

This seems ok to me, though I can't say that I'm particularly enthusiastic about "context" -- I'm wondering if there's maybe a better option. Maybe no word at all?

I agree that context is not the right word for this, that term is already way too overused / overloaded. I think the decision for what word we use there depends on how we conceptually expect people to use the formatted output from these assert macros. I can see a couple of options

• a message describing what pre-condition the assert was trying to check
• a description of runtime state that is relevant to the assertion other than the values being compared
• a description of the expected reason why the assert failed

A quick search of usages within rust-lang/rust:

library/alloc/tests/slice.rs
1012:                assert_eq!(lower_bounds, target, "lower bounds incorrect or not tight: {}", ctx);
1020:                assert_eq!(upper_bounds, target, "upper bounds incorrect or not tight: {}", ctx);

src/librustdoc/html/markdown/tests.rs
126:        assert_eq!(have, want, "Unexpected lang string split for `{}`", lang_string);

src/test/ui/intrinsics/const-eval-select-bad.rs
18:    assert_eq!(n, 0, "{} must be equal to {}", n, 0);

The example usages I could find all seem most consistent with the first or third options, rather than the second. For the second, context or some equivalent word would probably be the best header, but for the first and third I think Error or Message would be more appropriate.

Here's an alternative output I think might be closer to what we'd want:

thread 'main' panicked at 'assertion failed: `(left == right)`
Error: lower bounds incorrect or not tight: foobar
   left: `10`,
  right: `20`,
      at src/main.rs:4:5

Also, should we consider stringifying the left and right arguments instead of simply writing left or right?

thread 'main' panicked at 'assertion failed: `(upper_bounds == target)`
Error: lower bounds incorrect or not tight: foobar
   upper_bounds: `10`,
   target: `20`,
   at src/main.rs:4:5

[joshtriplett]

I agree with @yaahc's suggestion: let's put the string before the left and right values rather than after.

[Mark-Simulacrum]

FWIW, one downside of putting it before is that for cases where the left/right values are huge, that can make it much harder to find (at least IMO) -- the end of the assertion failure is likely towards the end of a log file, for example, but when wrapping text, scrolling up to find the line just before the left: might be pretty hard.

That said, modulo that concern, I agree the before is probably a better place -- this rationale is why I did not suggest it.

[cuviper]

I think huge left/right values are more of a reason to put the message before, so it is close to the "assertion failed" line, rather than letting lots of debug output separate that context. Ditto for src-loc, really.

[yaahc]

I think huge left/right values are more of a reason to put the message before, so it is close to the "assertion failed" line, rather than letting lots of debug output separate that context. Ditto for src-loc, really.

The source location is output by the panic handler, not the panic message itself, so we wouldn't be able to rearrange that as part of just this PR. We would have to do that for all panics from the default std handler.

[Mark-Simulacrum]

r? @yaahc since I think your opinions here are stronger then mine

[yaahc]

@rustbot label: -S-waiting-on-review +S-waiting-on-author

[rust-log-analyzer]

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

configure: rust.debug-assertions := True
configure: rust.overflow-checks := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 

[yaahc]

A couple more changes I noticed that I didn't communicate properly before. We want to add an Error: section to the output for the format args msg the user provides and just leave a blank spot for the location to get printed after we're done printing the assert output.

I added some suggestions to help explain what I mean, be careful applying them though because github restrictions prevented me from editing the whole snippet at once so I'm almost certain if you just apply these directly it will not compile. In the suggestions I went ahead and switched everything to used named arguments to make it a bit easier to read. Make sure to test these edits locally when you apply them with ./x.py test. If you're unfamiliar with the project's test framework you can check out the rustc-dev-guide for more info.

One thing you will need to do is update the expected output of existing tests that check our assert output. For a couple of them you should be able to fix it automatically by running ./x.py test src/test/ui --bless but for the rest you will need to manually edit the // error-pattern: lines at the beginning of the tests to expect the new input or update them to use // check-run-results similar to the other tests then --bless their output as well.

[yaahc]

The at: here shouldn't be followed by a formatting specifier that we print, this is meant to be where the location goes, which is printed by the caller of assert_failed_inner, so we should just finish the output here. The args that you're printing here is the error message and should come before left.

Suggested change

	r#"assertion failed: `({left_name} {} {right_name})`
	left: `{:?}`,
	r#"assertion failed: `({left_name} {op} {right_name})`
	Error: {args},
	left: `{left_val:?}`,

[yaahc]

Suggested change

	right: `{:?}`:
	at: {}"#,
	op, left_val, right_val, args
	),
	right: `{right_val:?}`,
	at: "#),

[yaahc]

Suggested change

	r#"assertion failed: `({left_name} {} {right_name})`
	left: `{:?}`,
	right: `{:?}`"#,
	r#"assertion failed: `({left_name} {op} {right_name})`
	left: `{left_val:?}`,
	right: `{right_val:?}`,
	at: "#),

[JohnCSimon]

@Mizobrook-kan

Ping from triage:
Can you please address the comments from the reviewer and fix the build failures? Thank you.

[JohnCSimon]

@Mizobrook-kan
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you do please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

[nyurik]

If @MakitaToki is unable to continue this PR, would it be possible to take the ownership (not certain if github allows that), or should I simply create a new PR with the same code + address feedback?

[JohnCSimon]

@nyurik just create a new PR

[nyurik]

I created a new #111030 PR taking over this PR