Skip to content
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

assert_eq! message format (take 2) #111030

Closed
wants to merge 16 commits into from
Closed

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented Apr 30, 2023

This takes over the stale #94016 by @MakitaToki (thx for amazing work!), and addresses comments by @yaahc. The code was rebased on the primary branch, preserving the history/contributions.

Fixes #94005

assert_eq!(1 + 1, 3, "this is clearly correct!");
thread 'main' panicked at $DIR/assert-eq-macro-msg.rs:6:5:
assertion failed: `1 + 1 == 3`
 error: this is clearly correct!
  left: `2`
 right: `3`

Note that there is a simpler alternative #111071 which does not improve the assertion failed message, only the formatting of the entire output.

@rustbot
Copy link
Collaborator

rustbot commented Apr 30, 2023

r? @m-ou-se

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 30, 2023
@rustbot

This comment was marked as resolved.

@rust-log-analyzer

This comment has been minimized.

@lukas-code
Copy link
Member

Is there a unit/integration test for asserts that validates the output of the assert_eq!?

These are in the UI test suite. You should be able to bless them with ./x test ui --bless. You might also need to adjust some of the // error-pattern:s in the test files (or just replace them with // check-run-results and bless away).


The ./x.py test --stage 1 library/std output did look suspicious though - it showed a lot of thread '<unnamed>' panicked at ... messages

I don't know what exactly is causing these, but you can safely ignore them.


Also, the clippy CI failure looks legit, you need to figure out why this program is failing now:

#![deny(clippy::missing_assert_message)]

fn main() {
    assert_eq!(1, 2, "oops");
}

library/core/src/panicking.rs Outdated Show resolved Hide resolved
library/core/src/panicking.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@nyurik
Copy link
Contributor Author

nyurik commented May 1, 2023

@lukas-code thank you so much for such a quick feedback!!!

Clippy Issue (CI failures)

It seems the clippy::missing_assert_message lint does some semi-magical (?) call tree examination, so I don't know what's the best way to proceed. I tried moving left_name and right_name params to the end, just in case the presence/absence of the panic message is detected by hardcoded position, but that didn't change anything. Any suggestions?

cc: @unexge @weihanglo @flip1995

Compiling

So the ./x test ui --bless doesn't seem to run, and neither did --stage 0. I had to run it as ./x test ui --stage 1 --bless - which seem to work ok. I assume this is what's needed.

❯ ./x test ui --stage 0 --bless
Building bootstrap
    Finished dev [unoptimized] target(s) in 0.03s
error: `--stage 0` runs compiletest on the beta compiler, not your local changes, and will almost always cause tests to fail
help: to test the compiler, use `--stage 1` instead
help: to test the standard library, use `--stage 0 library/std` instead
note: if you're sure you want to do this, please open an issue as to why. In the meantime, you can override this with `COMPILETEST_FORCE_STAGE0=1`.

@rustbot
Copy link
Collaborator

rustbot commented May 1, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented May 1, 2023

☀️ Try build successful - checks-actions
Build commit: 1c0c8b84d3fd5510f913b517c1a537ab0ab005e0 (1c0c8b84d3fd5510f913b517c1a537ab0ab005e0)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (1c0c8b84d3fd5510f913b517c1a537ab0ab005e0): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.8% [0.6%, 1.0%] 6
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.8% [0.6%, 1.0%] 6

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.1% [0.1%, 0.1%] 1
Regressions ❌
(secondary)
1.0% [1.0%, 1.0%] 1
Improvements ✅
(primary)
-2.5% [-3.5%, -1.9%] 4
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -1.9% [-3.5%, 0.1%] 5

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.3% [0.0%, 2.8%] 61
Regressions ❌
(secondary)
0.9% [0.2%, 1.1%] 7
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.3% [0.0%, 2.8%] 61

Bootstrap: 656.548s -> 656.35s (-0.03%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 1, 2023
@nyurik
Copy link
Contributor Author

nyurik commented May 1, 2023

@m-ou-se so it seems the new version is faster, but still slower than the primary branch. Any thoughts?

@nyurik
Copy link
Contributor Author

nyurik commented May 2, 2023

The simpler #111071 showed no performance impact, and I think it should be merged first, as it solves 90% of the issue. This PR should be rebased on top of it and we will discuss if adding a slightly better assert context is worth an extra 1% of compiler performance.

@m-ou-se m-ou-se assigned m-ou-se and unassigned m-ou-se Jun 5, 2023
@nyurik
Copy link
Contributor Author

nyurik commented Jun 30, 2023

Closing this in favor of #111071

@nyurik nyurik closed this Jun 30, 2023
@nyurik nyurik deleted the issue-94005-fix2 branch June 30, 2023 21:03
@nyurik nyurik restored the issue-94005-fix2 branch June 30, 2023 21:04
@nyurik nyurik deleted the issue-94005-fix2 branch August 3, 2023 20:46
@nyurik nyurik restored the issue-94005-fix2 branch August 3, 2023 20:46
@nyurik nyurik deleted the issue-94005-fix2 branch August 3, 2023 20:46
nyurik added a commit to nyurik/rust that referenced this pull request Aug 15, 2023
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);
```

```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!
```

```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
```

This PR is a simpler subset of the rust-lang#111030, but it does NOT stringify the original left and right source code assert expressions, thus should be faster to compile.
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 15, 2023
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`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf-regression Performance regression. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hard to read assert_eq!() custom message output
8 participants