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

New feature panic color errors #125614

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1325,6 +1325,7 @@ symbols! {
panic_abort,
panic_bounds_check,
panic_cannot_unwind,
panic_color_errors,
panic_const_add_overflow,
panic_const_async_fn_resumed,
panic_const_async_fn_resumed_panic,
Expand Down
3 changes: 3 additions & 0 deletions library/std/src/panic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ pub use crate::panicking::{set_hook, take_hook};
#[unstable(feature = "panic_update_hook", issue = "92649")]
pub use crate::panicking::update_hook;

#[unstable(feature = "panic_color_errors", issue = "none")]
pub use crate::panicking::highlight_errors;

#[stable(feature = "panic_hooks", since = "1.10.0")]
pub use core::panic::{Location, PanicInfo};

Expand Down
43 changes: 42 additions & 1 deletion library/std/src/panicking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ use crate::sys::stdio::panic_output;
use crate::sys_common::backtrace;
use crate::thread;

use crate::io::{self, IsTerminal};

#[cfg(not(test))]
use crate::io::try_set_output_capture;
// make sure to use the stderr output configured
Expand Down Expand Up @@ -233,6 +235,42 @@ where
*hook = Hook::Custom(Box::new(move |info| hook_fn(&prev, info)));
}

/// Adjusts the visual representation of panic messages by enabling or
/// disabling color highlighting, or leaving it with a default behaviour. This
/// function sets, unsets or removes an environment variable (RUST_COLOR_ERRORS)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// function sets, unsets or removes an environment variable (RUST_COLOR_ERRORS)
/// function sets, unsets or removes an environment variable (`RUST_COLOR_ERRORS`)

/// accordingly, influencing downstream formatting functions to produce colorful
/// error messages.
#[unstable(feature = "panic_color_errors", issue = "none")]
pub fn highlight_errors(set_color: Option<bool>){
match set_color {
Some(true) => {crate::env::set_var("RUST_COLOR_ERRORS", "1")}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does CI not fail on this formatting?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's wrong here?

Some(false) => {crate::env::set_var("RUST_COLOR_ERRORS", "0")}
_ => {crate::env::remove_var("RUST_COLOR_ERRORS")}
Comment on lines +246 to +248
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anomalous formatting.

Suggested change
Some(true) => {crate::env::set_var("RUST_COLOR_ERRORS", "1")}
Some(false) => {crate::env::set_var("RUST_COLOR_ERRORS", "0")}
_ => {crate::env::remove_var("RUST_COLOR_ERRORS")}
Some(true) => crate::env::set_var("RUST_COLOR_ERRORS", "1"),
Some(false) => crate::env::set_var("RUST_COLOR_ERRORS", "0"),
_ => crate::env::remove_var("RUST_COLOR_ERRORS"),

}
}
Comment on lines +244 to +250
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both set_var and remove_var are unsafe fn because they update the environment in an unsynchronized way, but this function is marked as safe. We cannot accept this into the standard library.

This requires an API design that does not depend on racily setting the environment variable.


/// Alters the format of error messages by adding color codes to them.
/// It reads the RUST_COLOR_ERRORS environment variable to determine whether to
/// apply color formatting. If enabled, it formats the error message with ANSI
/// escape codes to display it in red, indicating an error state. The default
/// behavior is to apply highlighting when printing to a terminal, and no
/// highlighting otherwise.
#[unstable(feature = "panic_color_errors", issue = "none")]
fn format_error_message (msg: &str) -> String {
match crate::env::var_os("RUST_COLOR_ERRORS") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should cache this instead of checking it every time, as we do with most env vars.

Some(x) if x == "1" => format!("\x1b[31m{msg}\x1b[0m"),
None => {
if io::stderr().is_terminal() {
format!("\x1b[31m{msg}\x1b[0m")
}
else {
msg.to_string()
}
},
_ => msg.to_string()
}
Comment on lines +260 to +271
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not need to allocate to function. It is desirable if backtraces minimize the number of allocations they perform, even with std enabled. It should instead write to a formatter.

}

/// The default panic handler.
fn default_hook(info: &PanicInfo<'_>) {
// If this is a double panic, make sure that we print a backtrace
Expand All @@ -259,7 +297,10 @@ fn default_hook(info: &PanicInfo<'_>) {
let name = thread.as_ref().and_then(|t| t.name()).unwrap_or("<unnamed>");

let write = |err: &mut dyn crate::io::Write| {
let _ = writeln!(err, "thread '{name}' panicked at {location}:\n{msg}");
// We should check if the feature is active and only then procede to format the message, but we dont know how to do it for now.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// We should check if the feature is active and only then procede to format the message, but we dont know how to do it for now.
// We should check if the feature is active and only then proceed to format the message, but we dont know how to do it for now.

let msg_fmt = format_error_message(msg);

let _ = writeln!(err, "thread '{name}' panicked at {location}:\n{msg_fmt}");

static FIRST_PANIC: AtomicBool = AtomicBool::new(true);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# `panic_color_errors`

This feature has no tracking issue yet.

------------------------

With this feature enabled, error messages generated during panics are highlighted for improved visibility and quick identification. By employing color highlighting, developers can easily distinguish error messages from other output, making debugging more efficient and intuitive. This is optional and can be toggled on or off using a dedicated function. The default behaviour is to highlight the errors when there's a terminal.
6 changes: 6 additions & 0 deletions tests/ui/feature-gates/feature-gate-panic_color_errors.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
use std::panic;

fn main(){
panic::highlight_errors(Some(true));
//~^ ERROR E0658
}
12 changes: 12 additions & 0 deletions tests/ui/feature-gates/feature-gate-panic_color_errors.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
error[E0658]: use of unstable library feature 'panic_color_errors'
--> $DIR/feature-gate-panic_color_errors.rs:4:5
|
LL | panic::highlight_errors(Some(true));
| ^^^^^^^^^^^^^^^^^^^^^^^
|
= help: add `#![feature(panic_color_errors)]` to the crate attributes to enable
= note: this compiler was built on YYYY-MM-DD; consider upgrading it if it is out of date

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0658`.
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#![feature(panic_color_errors)]

//@ run-fail
//@ check-run-results
//@ exec-env:RUST_BACKTRACE=0

fn foo(x: u64) {
if x == 0 {
panic!("Oops sometging went wrong");
} else {
foo(x-1);
}
}

fn main() {
std::panic::highlight_errors(None);
foo(100);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
thread 'main' panicked at $DIR/explicit_panicking_with_backtrace_highlight_errors_default.rs:9:9:
Oops sometging went wrong
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#![feature(panic_color_errors)]

//@ run-fail
//@ check-run-results
//@ exec-env:RUST_BACKTRACE=0

fn foo(x: u64) {
if x == 0 {
panic!("Oops sometging went wrong");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
panic!("Oops sometging went wrong");
panic!("Oops something went wrong");

} else {
foo(x-1);
}
}

fn main() {
std::panic::highlight_errors(Some(false));
foo(100);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
thread 'main' panicked at $DIR/explicit_panicking_with_backtrace_highlight_errors_off.rs:9:9:
Oops sometging went wrong
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#![feature(panic_color_errors)]

//@ run-fail
//@ check-run-results
//@ exec-env:RUST_BACKTRACE=0

fn foo(x: u64) {
if x == 0 {
panic!("Oops sometging went wrong");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
panic!("Oops sometging went wrong");
panic!("Oops something went wrong");

} else {
foo(x-1);
}
}

fn main() {
std::panic::highlight_errors(Some(true));
foo(100);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
thread 'main' panicked at $DIR/explicit_panicking_with_backtrace_highlight_errors_on.rs:9:9:
Oops sometging went wrong
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#![feature(panic_color_errors)]

//@ run-fail
//@ check-run-results
//@ exec-env:RUST_BACKTRACE=0

static mut I: [u64; 2] = [0; 2];

fn foo(x: u64) {
if x == 0 {
unsafe{
let j = 12;
I[j] = 0;
}
} else {
foo(x-1);
}
}

fn main() {
std::panic::highlight_errors(None);
foo(100);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
thread 'main' panicked at $DIR/forced_error_panicking_highlight_errors_default.rs:13:13:
index out of bounds: the len is 2 but the index is 12
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#![feature(panic_color_errors)]

//@ run-fail
//@ check-run-results
//@ exec-env:RUST_BACKTRACE=0

static mut I: [u64; 2] = [0; 2];

fn foo(x: u64) {
if x == 0 {
unsafe{
let j = 12;
I[j] = 0;
}
} else {
foo(x-1);
}
}

fn main() {
std::panic::highlight_errors(Some(false));
foo(100);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
thread 'main' panicked at $DIR/forced_error_panicking_highlight_errors_off.rs:13:13:
index out of bounds: the len is 2 but the index is 12
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#![feature(panic_color_errors)]

//@ run-fail
//@ check-run-results
//@ exec-env:RUST_BACKTRACE=0

static mut I: [u64; 2] = [0; 2];

fn foo(x: u64) {
if x == 0 {
unsafe{
let j = 12;
I[j] = 0;
}
} else {
foo(x-1);
}
}

fn main() {
std::panic::highlight_errors(Some(true));
foo(100);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
thread 'main' panicked at $DIR/forced_error_panicking_highlight_errors_on.rs:13:13:
index out of bounds: the len is 2 but the index is 12
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Loading