Add support for the NO_COLOR environment variable

[aaron.ballman]

Summary

Clang currently supports disabling color diagnostic output via -fno-color-diagnostics. However, there is a somewhat long-standing push to support use of an environment variable to override color output so that users can set up their terminal such that most color output is disabled (largely for accessibility reasons).

There are two competing de facto standards to accomplish this: NO_COLOR (https://no-color.org/) and CLICOLOR/CLICOLOR_FORCE (http://bixense.com/clicolors/).

This patch adds support for NO_COLOR as that appears to be the more commonly supported feature, at least when comparing issues and pull requests:
https://github.com/search?q=NO_COLOR&type=issues (2.2k issues, 35k pull requests)
https://github.com/search?q=CLICOLOR&type=issues (1k issues, 3k pull requests)

It's also the more straightforward and thoroughly-specified of the two options. If NO_COLOR is present as an environment variable (regardless of value), color output is suppressed unless the command line specifies use of color output (command line takes precedence over the environment variable).

[MaskRay]

Thanks for the detailed description. I am generally conservative when it comes to new use cases for environment variables. However, NO_COLOR seems to become a standard and a large number of tools respect it, I think it is the right call to support it.

If we don't intend to support both standards, we can close https://github.com/llvm/llvm-project/issues/23983 (CLICOLOR) as a wontfix :)

clang/lib/Frontend/CompilerInvocation.cpp

2346

We should inspect that NO_COLOR is not empty.

Command-line software which adds ANSI color to its output by default should check for a NO_COLOR environment variable that, when present and not an empty string (regardless of its value), prevents the addition of ANSI color.

clang/test/Driver/no-color.c

8

when present and not an empty string, the value does not matter.

[shafik]

CLICOLOR/CLICOLOR_FORCE seems a lot more complicated, NO-COLOR feels like a win for low cost and does not preclude supporting others later on if we feel there is a need. So LGTM but I would like to see more feedback.

[jhasse]

There's a valid usecase CLICOLOR_FORCE: Force color diagnostics. The "disable colors" part of https://bixense.com/clicolors/ is not that important to me, I could change it to point to NO_COLOR instead?

btw: I've tried to join the "standards" a few years ago: https://github.com/jcs/no_color/issues/28

Right now I think it would be best to drop CLICOLOR and have the following simple rules:

• if NO_COLOR set: disable colors
• if CLICOLOR_FORCE set: always set colors
• else: isatty

What do you think?

[aaron.ballman]

In D152285#4401348, @MaskRay wrote:

If we don't intend to support both standards, we can close https://github.com/llvm/llvm-project/issues/23983 (CLICOLOR) as a wontfix :)

That was my plan.

In D152285#4402603, @jhasse wrote:

There's a valid usecase CLICOLOR_FORCE: Force color diagnostics. The "disable colors" part of https://bixense.com/clicolors/ is not that important to me, I could change it to point to NO_COLOR instead?

btw: I've tried to join the "standards" a few years ago: https://github.com/jcs/no_color/issues/28

Right now I think it would be best to drop CLICOLOR and have the following simple rules:

• if NO_COLOR set: disable colors
• if CLICOLOR_FORCE set: always set colors
• else: isatty

What do you think?

I don't think we should implement all of one standard and part of another. My thinking is: users will have to do NO_COLOR=1 and CLICOLOR_FORCE=0 to disable colors due to there being competing standards, so we need to support only one of these. CLICOLOR_FORCE is the more powerful option, but I don't know what compelling use case there is for forcing colors *on*, but forcing them *off* makes a lot of sense to me from an accessibility standpoint. So I think supporting NO_COLOR alone is sufficient until we know why users need to force-enable colors. If we are compelled by that reasoning, we could implement the other standard at that point (and I'd suggest we implement it fully). But I'd like to avoid that right now because then we have to think about what happens in circumstances like NO_COLOR=1 CLICOLOR_FORCE=1

clang/lib/Frontend/CompilerInvocation.cpp

2346

Good catch!

[cor3ntin]

As I told Aaron, someone who disables color for accessibility reason is likely to have both variables(NO_COLOR/CLICOLOR) set. So supporting one is enough. Picking the one that is the simplest and most popular makes perfect sense.
This is great.

[aaron.ballman]

Update based on review feedback so that we check for an empty environment variable value.

This was a fun rabbit hole to fall into: lit is not a PTY, so our fallback of "automatically enable colors" means env NO_COLOR= still disables colors as far as lit tests are concerned. See https://github.com/llvm/llvm-project/blob/4418434c6de7a861e241ba2448ea4a12080cf08f/clang/test/Driver/color-diagnostics.c#L25 for another test running into this. What's more, Windows does not have a way to set an empty environment variable on the command line from what I can find, so testing this manually also doesn't work (at least for me). However, because this piggy-backs on the functionality for -fdiagnostics-color=auto, I think the test coverage I added is sufficient.

[MaskRay]

LGTM. Glad that my comment on https://github.com/llvm/llvm-project/issues/23983 might bring attention to users.

I'd like to make my stance softer: if we find CLICOLOR_FORCE useful, we can implement it later.
I don't mind how we handle NO_COLOR=1 CLICOLOR_FORCE=1. This seems invalid input from the user and making either option win should be fine (I'd prefer that NO_COLOR wins.)

[cor3ntin]

clang/lib/Frontend/CompilerInvocation.cpp

2348

NoColor->at(0) != '\0' seem very superfluous. do you have examples of scenario that would produce a null terminator?

[MaskRay]

clang/lib/Frontend/CompilerInvocation.cpp

2348

GetEnv returned std::string originates from a C string. Just !NoColor->empty() is sufficient.

2348

Also, basic_string::at may throw an exception, which may be undesired.

[jhasse]

In D152285#4403031, @aaron.ballman wrote:

In D152285#4401348, @MaskRay wrote:

If we don't intend to support both standards, we can close https://github.com/llvm/llvm-project/issues/23983 (CLICOLOR) as a wontfix :)

That was my plan.

As that issue was more about adding a way to force colors and not about disabling them, please leave it open.

[...] I don't know what compelling use case there is for forcing colors *on*, [...] until we know why users need to force-enable colors.

The reason is that adding -fcolor-diagnostics to the command line is not always feasible, e.g. most build systems would rebuild everything in that case. Relying on tty detection also doesn't work as many build tools buffer the output (and some CIs, too).

In D152285#4403318, @MaskRay wrote:

I don't mind how we handle NO_COLOR=1 CLICOLOR_FORCE=1. This seems invalid input from the user and making either option win should be fine (I'd prefer that NO_COLOR wins.)

I'd prefer NO_COLOR winning, too. I can specify that case at https://bixense.com/clicolors/.

[aaron.ballman]

In D152285#4403357, @jhasse wrote:

In D152285#4403031, @aaron.ballman wrote:

In D152285#4401348, @MaskRay wrote:

If we don't intend to support both standards, we can close https://github.com/llvm/llvm-project/issues/23983 (CLICOLOR) as a wontfix :)

That was my plan.

As that issue was more about adding a way to force colors and not about disabling them, please leave it open.

The UX we follow for most options is "last option wins", and CLICOLOR_FORCE doesn't follow that model because it overrides what is specified on the command line explicitly. We can leave the issue open, but I think it's worth making it clear that we're probably unlikely to implement it.

[...] I don't know what compelling use case there is for forcing colors *on*, [...] until we know why users need to force-enable colors.

The reason is that adding -fcolor-diagnostics to the command line is not always feasible, e.g. most build systems would rebuild everything in that case. Relying on tty detection also doesn't work as many build tools buffer the output (and some CIs, too).

Ah, that's an interesting use case, thank you for mentioning it! I'm not certain that's super compelling to me; color diagnostics are on by default, so if someone forcibly disables them in the build system (which seems to not really happen too much in practice: https://sourcegraph.com/search?q=context:global+-fno-color-diagnostics+file:.*Makefile.*&patternType=standard&sm=0&groupBy=repo), that's likely done for a reason.

In D152285#4403318, @MaskRay wrote:

I don't mind how we handle NO_COLOR=1 CLICOLOR_FORCE=1. This seems invalid input from the user and making either option win should be fine (I'd prefer that NO_COLOR wins.)

I'd prefer NO_COLOR winning, too. I can specify that case at https://bixense.com/clicolors/.

Thanks!

clang/lib/Frontend/CompilerInvocation.cpp

2348

Ah this was leftover from my flailing around trying to figure out why env NO_COLOR= %clang ... was not emitting colors. I'll remove the superfluous bit when landing.

[cor3ntin]

LGTM!

[arichardson]

[...] I don't know what compelling use case there is for forcing colors *on*, [...] until we know why users need to force-enable colors.

The reason is that adding -fcolor-diagnostics to the command line is not always feasible, e.g. most build systems would rebuild everything in that case. Relying on tty detection also doesn't work as many build tools buffer the output (and some CIs, too).

Ah, that's an interesting use case, thank you for mentioning it! I'm not certain that's super compelling to me; color diagnostics are on by default, so if someone forcibly disables them in the build system (which seems to not really happen too much in practice: https://sourcegraph.com/search?q=context:global+-fno-color-diagnostics+file:.*Makefile.*&patternType=standard&sm=0&groupBy=repo), that's likely done for a reason.

I had the same use case quite a few years ago: I wanted to have Jenkins output coloured diagnostics in the logs to make them slightly easier to read. since the output goes to a file rather than a terminal it defaults to no colours and there were too many different build systems involved that adding compiler flags was not feasible. In the end I added a local hack to clang to default to colours being on if a given environment var is set (https://github.com/CTSRD-CHERI/llvm-project/commit/a427ffe13c19276ae94cc757632439fda08d9dc6). I'd be happy if we ended up supporting CLICOLOR_FORCE=1 as a way of ignoring the isatty check but colours in Jenkins without modifying build systems is just a nice to have feature rather than something super useful and there are better ways of making the warnings more visible (e.g. the plugin that scans the build logs and aggregates results).

clang/docs/ReleaseNotes.rst

 - A new builtin type trait ``__is_trivially_equality_comparable`` has been added,
   which checks whether comparing two instances of a type is equivalent to
   ``memcmp(&lhs, &rhs, sizeof(T)) == 0``.
 - Clang now ignores null directives outside of the include guard when deciding
   whether a file can be enabled for the multiple-include optimization.
 - Clang now support ``__builtin_FUNCSIG()`` which returns the same information
   as the ``__FUNCSIG__`` macro (available only with ``-fms-extensions`` flag).
   This fixes (`#58951 <https://github.com/llvm/llvm-project/issues/58951>`_).
+- Clang now supports the `NO_COLOR <https://no-color.org/>`_ environment
+  variable as a way to disable color diagnostics.
 
 New Compiler Flags
 ------------------
 - The flag ``-std=c++23`` has been added. This behaves the same as the existing
   flag ``-std=c++2b``.
 
 - ``-dumpdir`` has been implemented to specify auxiliary and dump output
   filenames for features like ``-gsplit-dwarf``.

clang/docs/UsersManual.rst

.. option:: -f[no-]color-diagnostics
 
    ::
 
          test.c:2:8: warning: extra tokens at end of #endif directive [-Wextra-tokens]
          #endif bad
                 ^
                 //
 
+   If the ``NO_COLOR`` environment variable is defined and not empty
+   (regardless of value), color diagnostics are disabled. If ``NO_COLOR`` is
+   defined and ``-fcolor-diagnostics`` is passed on the command line, Clang
+   will honor the command line argument.
+
 .. option:: -fansi-escape-codes
 
    Controls whether ANSI escape codes are used instead of the Windows Console
    API to output colored diagnostics. This option is only used on Windows and
    defaults to off.
 
 .. option:: -fdiagnostics-format=clang/msvc/vi
 

clang/lib/Frontend/CompilerInvocation.cpp

 }
 
 std::unique_ptr<DiagnosticOptions>
 clang::CreateAndPopulateDiagOpts(ArrayRef<const char *> Argv) {
   auto DiagOpts = std::make_unique<DiagnosticOptions>();
   unsigned MissingArgIndex, MissingArgCount;
   InputArgList Args = getDriverOptTable().ParseArgs(
       Argv.slice(1), MissingArgIndex, MissingArgCount);
+
+  bool ShowColors = true;
+  if (std::optional<std::string> NoColor =
+          llvm::sys::Process::GetEnv("NO_COLOR");
+      NoColor && !NoColor->empty()) {
+    // If the user set the NO_COLOR environment variable, we'll honor that
+    // unless the command line overrides it.
+    ShowColors = false;
+  }
+
   // We ignore MissingArgCount and the return value of ParseDiagnosticArgs.
   // Any errors that would be diagnosed here will also be diagnosed later,
   // when the DiagnosticsEngine actually exists.
-  (void)ParseDiagnosticArgs(*DiagOpts, Args);
+  (void)ParseDiagnosticArgs(*DiagOpts, Args, /*Diags=*/nullptr, ShowColors);
   return DiagOpts;
 }
 
 bool clang::ParseDiagnosticArgs(DiagnosticOptions &Opts, ArgList &Args,
                                 DiagnosticsEngine *Diags,
                                 bool DefaultDiagColor) {
   std::optional<DiagnosticsEngine> IgnoringDiags;
   if (!Diags) {

clang/test/Driver/no-color.c

+// RUN: env NO_COLOR=1 %clang -### %s 2>&1 | FileCheck --check-prefix=NO-COLOR %s
+// RUN: env NO_COLOR=1 %clang -fcolor-diagnostics -### %s 2>&1 | FileCheck --check-prefix=COLOR %s
+// RUN: env NO_COLOR=1 %clang -fdiagnostics-color=auto -### %s 2>&1 | FileCheck --check-prefix=NO-COLOR %s
+// RUN: env NO_COLOR=1 %clang -fdiagnostics-color=always -### %s 2>&1 | FileCheck --check-prefix=COLOR %s
+// RUN: env NO_COLOR=1 %clang -fdiagnostics-color=never -### %s 2>&1 | FileCheck --check-prefix=NO-COLOR %s
+
+// Note, the value of the environment variable does not matter, only that it is defined and not empty.
+// RUN: env NO_COLOR=0 %clang -### %s 2>&1 | FileCheck --check-prefix=NO-COLOR %s
+// Note, an empty value means we automatically decide whether to enable colors or not, and lit tests
+// are not run in a PTY, so colors are disabled by default. There is no easy way for us to test this
+// configuration where auto enables colors.
+// RUN: env NO_COLOR= %clang -### %s 2>&1 | FileCheck --check-prefix=NO-COLOR %s
+
+int main(void) {}
+
+// COLOR: -fcolor-diagnostics
+// NO-COLOR-NOT: -fcolor-diagnostics