New feature panic color errors #125614

Vudvud · 2024-05-27T13:14:39Z

We have introduced a new feature in Rust that enhances the visibility of errors in backtraces by highlighting them in red. This improvement aims to make debugging easier by drawing immediate attention to errors. The color highlighting is enabled by default when the output is directed to a terminal. However, we also provide flexibility through an environment variable and a function to control this behavior programmatically.

Co-authored-by: Henrique Carrão <henrique.carrao@tecnico.ulisboa.pt>

rustbot · 2024-05-27T13:14:46Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @BoxyUwU (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

Noratrieb · 2024-05-27T16:05:12Z

Hi, thank you for your PR! There are a few improvements that need to be done to the implementation, but before addressing that I think it makes more sense to first figure out whether we want this in the first place.
While having colors for panic messages can be useful, there are some complexities with this (for example, this doesn't handle Windows very well) and it can also cause issues when in places where stderr is a TTY but doesn't really support or expect colors.
I recommend opening a thread in https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs and discussing there whether this is something that is desired or not.

BoxyUwU · 2024-05-27T16:06:40Z

r? libs

bjorn3 · 2024-05-27T19:32:39Z

compiler/rustc_feature/src/unstable.rs

@@ -213,6 +213,8 @@ declare_features! (
    (internal, negative_bounds, "1.71.0", None),
    /// Allows using `#[omit_gdb_pretty_printer_section]`.
    (internal, omit_gdb_pretty_printer_section, "1.5.0", None),
+    /// Highlights the error message of pannic calls.
+    (unstable, panic_color_errors,"CURRENT_RUSTC_VERSION", None),


This is only necessary for compiler features. Library features like this PR should not get an entry in the compiler.

Okay, so its only that part or there's more we should delete?

Actually, we found this guide somewhat confusing regarding library features. The link it provides for the library features guide leads only to stability attributes. As a result, we followed the guide for compiler features instead.

It should really link to https://std-dev-guide.rust-lang.org/development/feature-lifecycle.html

bjorn3 · 2024-05-27T19:33:27Z

library/std/src/panicking.rs

+#[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")}


Why does CI not fail on this formatting?

What's wrong here?

juntyr · 2024-05-28T06:48:11Z

compiler/rustc_feature/src/unstable.rs

@@ -213,6 +213,8 @@ declare_features! (
    (internal, negative_bounds, "1.71.0", None),
    /// Allows using `#[omit_gdb_pretty_printer_section]`.
    (internal, omit_gdb_pretty_printer_section, "1.5.0", None),
+    /// Highlights the error message of pannic calls.


Suggested change

/// Highlights the error message of pannic calls.

/// Highlights the error message of panic calls.

juntyr · 2024-05-28T06:48:37Z

library/std/src/panicking.rs

@@ -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 remove an environment variable (RUST_COLOR_ERRORS)


Suggested change

/// function sets, unsets or remove an environment variable (RUST_COLOR_ERRORS)

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

juntyr · 2024-05-28T06:50:20Z

src/doc/unstable-book/src/library-features/panic-color-errors.md

+
+------------------------
+
+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 highlight the errors when there's a terminal


Suggested change

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 highlight the errors when there's a terminal

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

workingjubilee · 2024-06-02T12:00:08Z

Another environment variable?

Regardless, waiting on like, an ACP or something.

@rustbot author

c4rrao · 2024-06-03T22:58:56Z

Hi, thank you for your PR! There are a few improvements that need to be done to the implementation, but before addressing that I think it makes more sense to first figure out whether we want this in the first place. While having colors for panic messages can be useful, there are some complexities with this (for example, this doesn't handle Windows very well) and it can also cause issues when in places where stderr is a TTY but doesn't really support or expect colors. I recommend opening a thread in https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs and discussing there whether this is something that is desired or not.

We forgot to address it but there was an issue regarding this feature.

rust-log-analyzer · 2024-06-03T23:23:37Z

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

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

Download action repository 'msys2/setup-msys2@v2.22.0' (SHA:cc11e9188b693c2b100158c3322424c4cc1dadea)
Download action repository 'actions/checkout@v4' (SHA:a5ac7e51b41094c92402da3b24376905380afc29)
Download action repository 'actions/setup-python@v5' (SHA:82c7e631bb3cdc910f68e0081d67478d79c6982d)
Download action repository 'actions/upload-artifact@v4' (SHA:65462800fd760344b1a7b4382951275a0abb4808)
Complete job name: PR - mingw-check-tidy
git config --global core.autocrlf false
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
---
COPY scripts/sccache.sh /scripts/
RUN sh /scripts/sccache.sh

COPY host-x86_64/mingw-check/reuse-requirements.txt /tmp/
RUN pip3 install --no-deps --no-cache-dir --require-hashes -r /tmp/reuse-requirements.txt \
    && pip3 install virtualenv
COPY host-x86_64/mingw-check/validate-toolstate.sh /scripts/
COPY host-x86_64/mingw-check/validate-error-codes.sh /scripts/

# NOTE: intentionally uses python2 for x.py so we can test it still works.
# NOTE: intentionally uses python2 for x.py so we can test it still works.
# validate-toolstate only runs in our CI, so it's ok for it to only support python3.
ENV SCRIPT TIDY_PRINT_DIFF=1 python2.7 ../x.py test \
           --stage 0 src/tools/tidy tidyselftest --extra-checks=py:lint
# This file is autogenerated by pip-compile with Python 3.10
# by the following command:
#
#    pip-compile --allow-unsafe --generate-hashes reuse-requirements.in
---

#10 [4/8] RUN sh /scripts/sccache.sh
#10 CACHED

#11 [6/8] RUN pip3 install --no-deps --no-cache-dir --require-hashes -r /tmp/reuse-requirements.txt     && pip3 install virtualenv

#12 [2/8] RUN apt-get update && apt-get install -y --no-install-recommends   g++   make   ninja-build   file   curl   ca-certificates   python2.7   python3   python3-pip   python3-pkg-resources   git   cmake   sudo   gdb   xz-utils   libssl-dev   pkg-config   mingw-w64   && rm -rf /var/lib/apt/lists/*
#12 CACHED

---
DirectMap4k:      184256 kB
DirectMap2M:     6107136 kB
DirectMap1G:    12582912 kB
##[endgroup]
Executing TIDY_PRINT_DIFF=1 python2.7 ../x.py test            --stage 0 src/tools/tidy tidyselftest --extra-checks=py:lint
+ TIDY_PRINT_DIFF=1 python2.7 ../x.py test --stage 0 src/tools/tidy tidyselftest --extra-checks=py:lint
    Finished `dev` profile [unoptimized] target(s) in 0.04s
##[endgroup]
downloading https://ci-artifacts.rust-lang.org/rustc-builds-alt/7c52d2db6348b038276198e88a835125849f322e/rust-dev-nightly-x86_64-unknown-linux-gnu.tar.xz
extracting /checkout/obj/build/cache/llvm-7c52d2db6348b038276198e88a835125849f322e-true/rust-dev-nightly-x86_64-unknown-linux-gnu.tar.xz to /checkout/obj/build/x86_64-unknown-linux-gnu/ci-llvm
---
   Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
    Finished `release` profile [optimized] target(s) in 25.48s
##[endgroup]
fmt check
##[error]Diff in /checkout/library/std/src/panicking.rs at line 241:
 /// accordingly, influencing downstream formatting functions to produce colorful
 #[unstable(feature = "panic_color_errors", issue = "none")]
 #[unstable(feature = "panic_color_errors", issue = "none")]
-pub fn highlight_errors(set_color: Option<bool>){
+pub fn highlight_errors(set_color: Option<bool>) {
     match set_color {

workingjubilee

I have reviewed this more closely and am recommending that T-libs-api reject this PR in its current state unless its internals are considerably redesigned.

workingjubilee · 2024-06-04T21:18:08Z

library/std/src/panicking.rs

+        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")}


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"),

workingjubilee · 2024-06-04T21:19:31Z

library/std/src/panicking.rs

@@ -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)


Suggested change

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

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

workingjubilee · 2024-06-04T21:20:52Z

tests/ui/panics/panic_color_errors/explicit_panicking_with_backtrace_highlight_errors_off.rs

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


Suggested change

panic!("Oops sometging went wrong");

panic!("Oops something went wrong");

workingjubilee · 2024-06-04T21:20:59Z

tests/ui/panics/panic_color_errors/explicit_panicking_with_backtrace_highlight_errors_on.rs

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


Suggested change

panic!("Oops sometging went wrong");

panic!("Oops something went wrong");

workingjubilee · 2024-06-04T21:22:47Z

library/std/src/panicking.rs

+pub fn highlight_errors(set_color: Option<bool>){
+    match set_color {
+        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")}
+    }
+}


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.

workingjubilee · 2024-06-04T21:26:38Z

library/std/src/panicking.rs

@@ -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.


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.

workingjubilee · 2024-06-04T21:28:56Z

library/std/src/panicking.rs

+    match crate::env::var_os("RUST_COLOR_ERRORS") {
+        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()
+    }


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.

workingjubilee · 2024-06-04T21:29:42Z

library/std/src/panicking.rs

+/// highlighting otherwise.
+#[unstable(feature = "panic_color_errors", issue = "none")]
+fn format_error_message (msg: &str) -> String {
+    match crate::env::var_os("RUST_COLOR_ERRORS") {


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

bors · 2024-07-13T06:28:13Z

☔ The latest upstream changes (presumably #127397) made this pull request unmergeable. Please resolve the merge conflicts.

Dylan-DPC · 2024-08-17T11:09:31Z

@Vudvud any updates on creating the ACP? thanks

JohnCSimon · 2024-10-06T05:54:05Z

@Vudvud

I'm closing this due to inactivity because the author hasn't touched the PR in a few months.
If want to continue on this PR, please reopen before committing to the branch. Thank you.

@rustbot label: +S-inactive

Vudvud and others added 2 commits May 27, 2024 12:32

feat: added feature coloring error (uncomplete)

c7a1ba3

Co-authored-by: Henrique Carrão <henrique.carrao@tecnico.ulisboa.pt>

fix:implemented feature

ba68d49

Co-authored-by: Henrique Carrão <henrique.carrao@tecnico.ulisboa.pt>

rustbot assigned BoxyUwU May 27, 2024

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

rustbot assigned Noratrieb and unassigned BoxyUwU May 27, 2024

bjorn3 reviewed May 27, 2024

View reviewed changes

juntyr reviewed May 28, 2024

View reviewed changes

workingjubilee added the S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. label Jun 2, 2024

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 2, 2024

workingjubilee added S-blocked Status: Blocked on something else such as an RFC or other implementation work. needs-acp This change is blocked on the author creating an ACP. and removed S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. labels Jun 2, 2024

fix: some typos and useless code for library features

d7b6b1f

workingjubilee reviewed Jun 4, 2024

View reviewed changes

Dylan-DPC removed the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Aug 17, 2024

JohnCSimon closed this Oct 6, 2024

rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Oct 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New feature panic color errors #125614

New feature panic color errors #125614

Vudvud commented May 27, 2024

rustbot commented May 27, 2024

Noratrieb commented May 27, 2024

BoxyUwU commented May 27, 2024

bjorn3 May 27, 2024

c4rrao Jun 3, 2024

workingjubilee Jun 4, 2024

bjorn3 May 27, 2024

fmease May 28, 2024

c4rrao Jun 3, 2024

juntyr May 28, 2024

juntyr May 28, 2024

juntyr May 28, 2024

workingjubilee commented Jun 2, 2024

c4rrao commented Jun 3, 2024

rust-log-analyzer commented Jun 3, 2024

workingjubilee left a comment •

edited

Loading

workingjubilee Jun 4, 2024

workingjubilee Jun 4, 2024

workingjubilee Jun 4, 2024

workingjubilee Jun 4, 2024

workingjubilee Jun 4, 2024

workingjubilee Jun 4, 2024

workingjubilee Jun 4, 2024

workingjubilee Jun 4, 2024

bors commented Jul 13, 2024

Dylan-DPC commented Aug 17, 2024

JohnCSimon commented Oct 6, 2024

	/// Highlights the error message of pannic calls.
	/// Highlights the error message of panic calls.

	/// function sets, unsets or remove an environment variable (RUST_COLOR_ERRORS)
	/// function sets, unsets or removes an environment variable (RUST_COLOR_ERRORS)


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

		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 highlight the errors when there's a terminal

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

	// 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.

New feature panic color errors #125614

New feature panic color errors #125614

Conversation

Vudvud commented May 27, 2024

rustbot commented May 27, 2024

Noratrieb commented May 27, 2024

BoxyUwU commented May 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee commented Jun 2, 2024

c4rrao commented Jun 3, 2024

rust-log-analyzer commented Jun 3, 2024

workingjubilee left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jul 13, 2024

Dylan-DPC commented Aug 17, 2024

JohnCSimon commented Oct 6, 2024

workingjubilee left a comment •

edited

Loading