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

Strange \n in output of assert #108341

Closed
safinaskar opened this issue Feb 22, 2023 · 7 comments
Closed

Strange \n in output of assert #108341

safinaskar opened this issue Feb 22, 2023 · 7 comments
Labels
A-panic Area: Panicking machinery C-bug Category: This is a bug. D-inconsistent Diagnostics: Inconsistency in formatting, grammar or style between diagnostic messages. D-papercut Diagnostics: An error or lint that needs small tweaks. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@safinaskar
Copy link
Contributor

safinaskar commented Feb 22, 2023

I tried this code:

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=101cc1b94e74b6bcdec96994d034dbab

I see strange \n in the output. I don't like the output

Compiling playground v0.0.1 (/playground)
    Finished dev [unoptimized + debuginfo] target(s) in 0.79s
     Running `target/debug/playground`
thread 'main' panicked at 'assertion failed: 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0\n                                + 0 + 0 + 0 + 0 + 0 + 0 + 0 == 1', src/main.rs:2:5
note: [run with `RUST_BACKTRACE=1` environment variable to display a backtrace](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=101cc1b94e74b6bcdec96994d034dbab#)

1.69.0-nightly (2023-02-22 fdbc4329cb781c7768ff)

@safinaskar safinaskar added the C-bug Category: This is a bug. label Feb 22, 2023
@safinaskar
Copy link
Contributor Author

@rustbot label +D-papercut +D-inconsistent

@rustbot rustbot added D-inconsistent Diagnostics: Inconsistency in formatting, grammar or style between diagnostic messages. D-papercut Diagnostics: An error or lint that needs small tweaks. labels Feb 22, 2023
@bvanjoi
Copy link
Contributor

bvanjoi commented Feb 22, 2023

@rustbot claim

@bvanjoi
Copy link
Contributor

bvanjoi commented Feb 23, 2023

I did not find a good solution. @rustbot release-assignment

@Noratrieb Noratrieb added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 24, 2023
nnethercote added a commit to nnethercote/rust that referenced this issue Oct 9, 2023
The assertion in `assert-long-condition.rs` used to be fail like this, all on
one line:
```
thread 'main' panicked at 'assertion failed: 1 + 2 + 3 + 4 + 5 + 6 + 7 + 8 + 9 + 10 + 11 + 12 + 13 + 14 + 15 + 16 + 17 + 18\n                                + 19 + 20 + 21 + 22 + 23 + 24 + 25 == 0', tests/ui/macros/assert-long-condition.rs:7:5
```
The `\n` and subsequent indent is because the condition is pretty-printed, and
the pretty-printer inserts a newline. Printing the newline in this way is
arguably reasonable given that the message appears within single quotes, which
is very similar to a string literal.

However, after the assertion printing improvements that were released in 1.73,
the assertion now fails like this:
```
thread 'main' panicked at tests/ui/macros/assert-long-condition.rs:7:5:
assertion failed: 1 + 2 + 3 + 4 + 5 + 6 + 7 + 8 + 9 + 10 + 11 + 12 + 13 + 14 + 15 + 16 + 17 + 18\n                                + 19 + 20 + 21 + 22 + 23 + 24 + 25 == 0
```
Now that there are no single quotes around the pretty-printed condition, the
`\n` is quite strange.

This commit gets rid of the `\n`, by removing the `escape_debug` done on the
pretty-printed message. This results in the following:
```
thread 'main' panicked at tests/ui/macros/assert-long-condition.rs:7:5:
assertion failed: 1 + 2 + 3 + 4 + 5 + 6 + 7 + 8 + 9 + 10 + 11 + 12 + 13 + 14 + 15 + 16 + 17 + 18
                                + 19 + 20 + 21 + 22 + 23 + 24 + 25 == 0
```
The overly-large indent is still strange, but that's a separate pretty-printing issue.

This change helps with rust-lang#108341.
@nnethercote
Copy link
Contributor

This happens because the compiler pretty-prints the expression when putting it into the assertion failure message, and it inserts a newline in there because the line is so long. Which is reasonable. But printing that newline as \n rather than an actual newline is a bit weird, IMO. I have filed #116548 to change this newline behaviour.

There is still an overly-large indent after the newline, which is a separate pretty-printing issue, and one I haven't addressed.

@safinaskar
Copy link
Contributor Author

@rustbot label: +A-panic

@rustbot rustbot added the A-panic Area: Panicking machinery label Oct 9, 2023
nnethercote added a commit to nnethercote/rust that referenced this issue Oct 9, 2023
The assertion in `assert-long-condition.rs` used to be fail like this, all on
one line:
```
thread 'main' panicked at 'assertion failed: 1 + 2 + 3 + 4 + 5 + 6 + 7 + 8 + 9 + 10 + 11 + 12 + 13 + 14 + 15 + 16 + 17 + 18\n                                + 19 + 20 + 21 + 22 + 23 + 24 + 25 == 0', tests/ui/macros/assert-long-condition.rs:7:5
```
The `\n` and subsequent indent is because the condition is pretty-printed, and
the pretty-printer inserts a newline. Printing the newline in this way is
arguably reasonable given that the message appears within single quotes, which
is very similar to a string literal.

However, after the assertion printing improvements that were released in 1.73,
the assertion now fails like this:
```
thread 'main' panicked at tests/ui/macros/assert-long-condition.rs:7:5:
assertion failed: 1 + 2 + 3 + 4 + 5 + 6 + 7 + 8 + 9 + 10 + 11 + 12 + 13 + 14 + 15 + 16 + 17 + 18\n                                + 19 + 20 + 21 + 22 + 23 + 24 + 25 == 0
```
Now that there are no single quotes around the pretty-printed condition, the
`\n` is quite strange.

This commit gets rid of the `\n`, by removing the `escape_debug` done on the
pretty-printed message. This results in the following:
```
thread 'main' panicked at tests/ui/macros/assert-long-condition.rs:7:5:
assertion failed: 1 + 2 + 3 + 4 + 5 + 6 + 7 + 8 + 9 + 10 + 11 + 12 + 13 + 14 + 15 + 16 + 17 + 18
                                + 19 + 20 + 21 + 22 + 23 + 24 + 25 == 0
```
The overly-large indent is still strange, but that's a separate pretty-printing issue.

This change helps with rust-lang#108341.
@nnethercote
Copy link
Contributor

#116548 has now been merged. Here's another example:

fn very_long_function_name(x: u32) -> u32 { x }

fn main() {
    assert!(
        {
            let x = 1;
            let y = 2;
            very_long_function_name(x) + very_long_function_name(y) 
        } == 
        {
            let x = 0;
            let y = 1;
            very_long_function_name(x) + very_long_function_name(y)
        }
    );
}

Previously it would produce a single line of output on failure:

assertion failed: {\n        let x = 1;\n        let y = 2;\n        very_long_function_name(x) + very_long_function_name(y)\n    } ==\n    {\n        let x = 0;\n        let y = 1;\n        very_long_function_name(x) + very_long_function_name(y)\n    }

Now it produces something much better:

thread 'main' panicked at s3.rs:4:5:
assertion failed: {
        let x = 1;
        let y = 2;
        very_long_function_name(x) + very_long_function_name(y)
    } ==
    {
        let x = 0;
        let y = 1;
        very_long_function_name(x) + very_long_function_name(y)
    }

@safinaskar: The original example still has the weird over-indent, which is a separate pretty-printing imperfection, but the \n chars are gone. Are you happy to close this issue now?

@safinaskar
Copy link
Contributor Author

Yes, I'm closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-panic Area: Panicking machinery C-bug Category: This is a bug. D-inconsistent Diagnostics: Inconsistency in formatting, grammar or style between diagnostic messages. D-papercut Diagnostics: An error or lint that needs small tweaks. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants