[clang-tidy] Use ANSI escape codes for --use-color on Windows

[dsanders11]

Summary

On Windows the --use-color option cannot be used for its originally intended purpose of forcing color when piping stdout, since Windows does not use ANSI escape codes by default. This change turns on ANSI escape codes on Windows when forcing color to a non-displayed stdout (e.g. piped).

[dsanders11]

Added a few inline comments for clarification.

Note: I was not able to get a debug build and unit tests working on my Windows set up, so this has only been tested manually. I'm not sure if the unit test added in D79477 runs on Windows, but in theory it should be broken on Windows since it expects ANSI escape codes in the output.

clang-tools-extra/clang-tidy/ClangTidy.cpp

113

To prevent any unintended changes this has been confined to only Windows, but the code should be safe to run on any platform. I believe it would be a no-op on Unix, which always uses ANSI escape codes and UseANSIEscapeCodes is a no-op.

114

Limited to only non-displayed standard out so that using '--use-color' from a console will be a no-op, rather than turning on ANSI escape codes and making it act differently than a default invocation which has color on by default.

llvm/lib/Support/Windows/Process.inc

330–333

Fixes a crash when calling UseANSIEscapeCodes if standard out isn't a console.

[aaron.ballman]

In D90109#2352180, @dsanders11 wrote:

Added a few inline comments for clarification.

Note: I was not able to get a debug build and unit tests working on my Windows set up, so this has only been tested manually. I'm not sure if the unit test added in D79477 runs on Windows, but in theory it should be broken on Windows since it expects ANSI escape codes in the output.

The functionality test specifies REQUIRES: ansi-escape-sequences so I'm guessing that test is not run on Windows. The unit test is parsing configuration options, not exercising the options themselves.

What issues did you run into regarding testing, because I feel like the patch should have test coverage (esp given that color vs ANSI escape codes are a bit of an oddity to reason about)?

clang-tools-extra/clang-tidy/ClangTidy.cpp

113

I agree that the code should work on all supported platforms, I'd say you can remove the #if.

llvm/lib/Support/Windows/Process.inc

330–333

If this fails, do we still want to set UseANSI to enable below? Similar if setting the mode fails?

[dblaikie]

In D90109#2356317, @aaron.ballman wrote:

In D90109#2352180, @dsanders11 wrote:

Added a few inline comments for clarification.

Note: I was not able to get a debug build and unit tests working on my Windows set up, so this has only been tested manually. I'm not sure if the unit test added in D79477 runs on Windows, but in theory it should be broken on Windows since it expects ANSI escape codes in the output.

The functionality test specifies REQUIRES: ansi-escape-sequences so I'm guessing that test is not run on Windows. The unit test is parsing configuration options, not exercising the options themselves.

What issues did you run into regarding testing, because I feel like the patch should have test coverage (esp given that color vs ANSI escape codes are a bit of an oddity to reason about)?

+1, I'd expect such a test could unconditionally FileCheck the output to test for the escape sequences being present there.

[dsanders11]

What issues did you run into regarding testing, because I feel like the patch should have test coverage (esp given that color vs ANSI escape codes are a bit of an oddity to reason about)?

I'm unable to create a debug build, due to issues with LLVM and Visual Studio 16.7. There's Google results on the issue, but I don't see any solution and I'm using master and the latest version of Visual Studio. It seems you can (and perhaps should?) run the tests with a Release build, so I've gone ahead and done that.

I also wasn't seeing any of the test targets being made by CMake until I included -DLLVM_INCLUDE_TESTS=On, I think maybe I set that to off when trying to determine why the debug build was failing and didn't realize that would stay in CMakeCache.txt until I set it back on.

Removing REQUIRES: ansi-escape-sequences from the clang-tidy/infrastructure/use-color.cpp test does run it on Windows and does pass as expected with this change. Is that all you're looking for with test coverage?

llvm/lib/Support/Windows/Process.inc

330–333

If we don't set UseANSI below then this patch won't do what it's intended to do, since piped stdoutwouldn't have ANSI escape codes. This code block is assuming that standard output is a console, so GetConsoleMode won't work on redirected stdout.

You could use llvm::sys::Process::StandardOutIsDisplayed() to check, but for Windows that's actually just doing the same thing, doing a GetConsoleMode call and seeing if it succeeds or not.

For this patch to do what it's intended to do, UseANSIEscapeCodes needs to turn on ANSI escape codes even if there isn't a console.

[aaron.ballman]

In D90109#2360538, @dsanders11 wrote:

What issues did you run into regarding testing, because I feel like the patch should have test coverage (esp given that color vs ANSI escape codes are a bit of an oddity to reason about)?

I'm unable to create a debug build, due to issues with LLVM and Visual Studio 16.7. There's Google results on the issue, but I don't see any solution and I'm using master and the latest version of Visual Studio. It seems you can (and perhaps should?) run the tests with a Release build, so I've gone ahead and done that.

Oh, how neat. The bug you linked to was closed as a dup of another bug. The other bug was then closed as something that's not in scope with the general product direction, whatever that means.

I also wasn't seeing any of the test targets being made by CMake until I included -DLLVM_INCLUDE_TESTS=On, I think maybe I set that to off when trying to determine why the debug build was failing and didn't realize that would stay in CMakeCache.txt until I set it back on.

Removing REQUIRES: ansi-escape-sequences from the clang-tidy/infrastructure/use-color.cpp test does run it on Windows and does pass as expected with this change. Is that all you're looking for with test coverage?

Yeah, that would be reasonable test coverage.

llvm/lib/Support/Windows/Process.inc

330–333

Ah, thank you for the explanation -- my complaint is more that UseANSIEscapeCodes can fail but doesn't alert the caller to the failure. However, that's not a new issue, so I think it's fine as-is.

[dsanders11]

code review changes

[dsanders11]

Oh, how neat. The bug you linked to was closed as a dup of another bug. The other bug was then closed as something that's not in scope with the general product direction, whatever that means.

Yea, typical Microsoft/Windows fun there.

Yeah, that would be reasonable test coverage.

Sounds good. I've updated the revision to remove the #if guard and removed REQUIRES: ansi-escape-sequences from the test.

[aaron.ballman]

LGTM! Do you need someone to commit on your behalf?

[dsanders11]

In D90109#2366415, @aaron.ballman wrote:

LGTM! Do you need someone to commit on your behalf?

Yea, I do.

[aaron.ballman]

In D90109#2366420, @dsanders11 wrote:

In D90109#2366415, @aaron.ballman wrote:

LGTM! Do you need someone to commit on your behalf?

Yea, I do.

I've commit on your behalf in d915d403d7410b17e8ac52f1f435859713a7b354, thank you for the patch!

clang-tools-extra/clang-tidy/ClangTidy.cpp

ErrorReporter(ClangTidyContext &Context, bool ApplyFixes,
         DiagPrinter(new TextDiagnosticPrinter(llvm::outs(), &*DiagOpts)),
         Diags(IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs), &*DiagOpts,
               DiagPrinter),
         SourceMgr(Diags, Files), Context(Context), ApplyFixes(ApplyFixes),
         TotalFixes(0), AppliedFixes(0), WarningsAsErrors(0) {
     DiagOpts->ShowColors = Context.getOptions().UseColor.getValueOr(
         llvm::sys::Process::StandardOutHasColors());
     DiagPrinter->BeginSourceFile(LangOpts);
+    if (DiagOpts->ShowColors && !llvm::sys::Process::StandardOutIsDisplayed()) {
+      llvm::sys::Process::UseANSIEscapeCodes(true);
+    }
   }
 
   SourceManager &getSourceManager() { return SourceMgr; }
 
   void reportDiagnostic(const ClangTidyError &Error) {
     const tooling::DiagnosticMessage &Message = Error.Message;
     SourceLocation Loc = getLocation(Message.FilePath, Message.FileOffset);
     // Contains a pair for each attempted fix: location and whether the fix was

clang-tools-extra/test/clang-tidy/infrastructure/use-color.cpp

 // RUN: clang-tidy -dump-config | FileCheck %s
 // RUN: clang-tidy -dump-config -use-color | FileCheck -check-prefix=CHECK-CONFIG-COLOR %s
 // RUN: clang-tidy -dump-config -use-color=false | FileCheck -check-prefix=CHECK-CONFIG-NO-COLOR %s
 // RUN: clang-tidy -config='UseColor: true' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-COLOR %s
 // RUN: clang-tidy -config='UseColor: false' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-NO-COLOR %s
 // RUN: clang-tidy -help | FileCheck -check-prefix=CHECK-OPT-PRESENT %s
 
-// REQUIRES: ansi-escape-sequences
 // RUN: clang-tidy -checks='-*, modernize-use-override' -extra-arg=-std=c++11 -use-color=false %s | FileCheck -check-prefix=CHECK-NO-COLOR %s
 // RUN: clang-tidy -checks='-*, modernize-use-override' -extra-arg=-std=c++11 %s | FileCheck -check-prefix=CHECK-NO-COLOR %s
 // RUN: clang-tidy -checks='-*, modernize-use-override' -extra-arg=-std=c++11 -use-color %s | FileCheck -check-prefix=CHECK-COLOR %s
 
 // CHECK-NOT: UseColor
 // CHECK-CONFIG-NO-COLOR: UseColor: false
 // CHECK-CONFIG-COLOR: UseColor: true
 // CHECK-OPT-PRESENT: --use-color

llvm/lib/Support/Windows/Process.inc

 }
 
 static bool UseANSI = false;
 void Process::UseANSIEscapeCodes(bool enable) {
 #if defined(ENABLE_VIRTUAL_TERMINAL_PROCESSING)
   if (enable) {
     HANDLE Console = GetStdHandle(STD_OUTPUT_HANDLE);
     DWORD Mode;
-    GetConsoleMode(Console, &Mode);
+    if (GetConsoleMode(Console, &Mode) != 0) {
       Mode |= ENABLE_VIRTUAL_TERMINAL_PROCESSING;
       SetConsoleMode(Console, Mode);
     }
+  }
 #endif
   UseANSI = enable;
 }
 
 namespace {
 class DefaultColors
 {
   private: