-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
improve custom message format in assert_eq macro #94016
Conversation
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. |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
library/core/src/panicking.rs
Outdated
@@ -219,7 +219,8 @@ fn assert_failed_inner( | |||
Some(args) => panic!( | |||
r#"assertion failed: `(left {} right)` | |||
left: `{:?}`, | |||
right: `{:?}`: {}"#, | |||
right: `{:?}`, | |||
context: `{:?}`"#, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated as suggested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I continue to submit more commits according to yaahc's suggestions, or close this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go ahead and update the format as suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
),
// ....
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried my best to find some code snippets that needs to be updated. Is there anything else I'm missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated my commit
This comment has been minimized.
This comment has been minimized.
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 quick search of usages within
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 Here's an alternative output I think might be closer to what we'd want:
Also, should we consider stringifying the left and right arguments instead of simply writing
|
I agree with @yaahc's suggestion: let's put the string before the left and right values rather than after. |
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. |
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. |
This comment has been minimized.
This comment has been minimized.
r? @yaahc since I think your opinions here are stronger then mine |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
resolve conflicts and stringify original tokens accept local changes
This comment has been minimized.
This comment has been minimized.
@rustbot label: -S-waiting-on-review +S-waiting-on-author |
The job Click to see the possible cause of the failure (guessed by this bot)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
r#"assertion failed: `({left_name} {} {right_name})` | ||
left: `{:?}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
r#"assertion failed: `({left_name} {} {right_name})` | |
left: `{:?}`, | |
r#"assertion failed: `({left_name} {op} {right_name})` | |
Error: {args}, | |
left: `{left_val:?}`, |
right: `{:?}`: | ||
at: {}"#, | ||
op, left_val, right_val, args | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right: `{:?}`: | |
at: {}"#, | |
op, left_val, right_val, args | |
), | |
right: `{right_val:?}`, | |
at: "#), |
r#"assertion failed: `({left_name} {} {right_name})` | ||
left: `{:?}`, | ||
right: `{:?}`"#, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r#"assertion failed: `({left_name} {} {right_name})` | |
left: `{:?}`, | |
right: `{:?}`"#, | |
r#"assertion failed: `({left_name} {op} {right_name})` | |
left: `{left_val:?}`, | |
right: `{right_val:?}`, | |
at: "#), |
@Mizobrook-kan Ping from triage: |
@Mizobrook-kan @rustbot label: +S-inactive |
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? |
@nyurik just create a new PR |
Apply comments from the rust-lang#94016 review by @yaahc
I created a new #111030 PR taking over this PR |
Cleaner assert_eq! & assert_ne! panic messages This PR finishes refactoring of the assert messages per rust-lang#94005. The panic message format change rust-lang#112849 used to be part of this PR, but has been factored out and just merged. It might be better to keep both changes in the same release once FCP vote completes. Modify panic message for `assert_eq!`, `assert_ne!`, the currently unstable `assert_matches!`, as well as the corresponding `debug_assert_*` macros. ```rust assert_eq!(1 + 1, 3); assert_eq!(1 + 1, 3, "my custom message value={}!", 42); ``` #### Old messages ```plain thread 'main' panicked at $DIR/main.rs:6:5: assertion failed: `(left == right)` left: `2`, right: `3` ``` ```plain thread 'main' panicked at $DIR/main.rs:6:5: assertion failed: `(left == right)` left: `2`, right: `3`: my custom message value=42! ``` #### New messages ```plain thread 'main' panicked at $DIR/main.rs:6:5: assertion `left == right` failed left: 2 right: 3 ``` ```plain thread 'main' panicked at $DIR/main.rs:6:5: assertion `left == right` failed: my custom message value=42! left: 2 right: 3 ``` History of fixing rust-lang#94005 * rust-lang#94016 was a lengthy PR that was abandoned * rust-lang#111030 was similar, but it stringified left and right arguments, and thus caused compile time performance issues, thus closed * rust-lang#112849 factored out the two-line formatting of all panic messages Fixes rust-lang#94005 r? `@m-ou-se`
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