[clang][Diagnostics] Show line numbers when printing code snippets

[tbaeder]

Summary

Just putting this here because it's a pretty simple patch as it turns out.

Done:

1. Increase -fcaret-diagnostics-max-lines default to 16.
2. Add -fdiagnostics-show-line-numbers to disable the margin with the line numbers on the left
3. Actually show the line numbers in all diagnostics.
4. Fix up tests.

Sample output:

./array.cpp:98:1: error: static assertion failed due to requirement 'func()'
  98 | static_assert(
     | ^
  99 |     // I wonder if this works.
 100 |     // Also, what if it doesn't?
 101 |     func());
     |     ~~~~~~
1 error generated.

[tbaeder]

clang/lib/Frontend/TextDiagnostic.cpp

1141

Wrote this just to get things going but I hope there's something better than this...?

[aaron.ballman]

GCC emits line numbers by default these days, so it seems reasonable for Clang to do so as well. Having a way to disable the feature is important though as some folks have scripts that scrap diagnostic results and changing the format may break those scripts in challenging ways. GCC supports -fno-diagnostics-show-line-numbers and we should do the same. GCC also supports -fdiagnostics-minimum-margin-width=width which we might want to consider supporting up front as well.

As for -fcaret-diagnostics-max-lines, I think I agree that the current default of 1 makes this functionality a bit less than useful, but perhaps we want to change that default value now? Alternatively, do we want to tie this option to the number of max lines? e.g., if the max lines is one, then we disable line numbers by default, but if the max lines is > 1, we enable line numbers by default?

[cjdb]

I have no opinion on this addition. If folks find it useful in GCC, happy to see it added.

clang/lib/Frontend/TextDiagnostic.cpp

1139

I'd prefer this to be an assert rather than returning a value. We should never be able to return from here.

[tbaeder]

Wow, in FixIt/fixits-function-call, this output:

./array.cpp:123:3: error: no matching function for call to 'f1'
  f1(a + 1);
  ^~
./array.cpp:114:6: note: candidate function not viable: no known conversion from 'intTy2 *' (aka 'int *') to 'intTy &' (aka 'int &') for 1st argument; dereference the argument with *
void f1(intTy &a);
     ^
fix-it:"./array.cpp":{123:6-123:6}:"*("
fix-it:"./array.cpp":{123:11-123:11}:")"

now (with increased snippet line limit) prints:

./array.cpp:123:3: error: no matching function for call to 'f1'
  123 |   f1(a + 1);
      |   ^~
./array.cpp:114:6: note: candidate function not viable: no known conversion from 'intTy2 *' (aka 'int *') to 'intTy &' (aka 'int &') for 1st argument; dereference the argument with *
  114 | void f1(intTy &a);
      |      ^
  116 | void f2(intTy2 *a) {
  117 | // CHECK: error: no matching function for call to 'f1
  118 | // CHECK: dereference the argument with *
  119 | // CHECK: void f1(intTy &a);
  120 | // CHECK: fix-it{{.*}}*(
  121 | // CHECK-NEXT: fix-it{{.*}})
  122 | // CHECK: void f1(double *a);
  123 |   f1(a + 1);
      |      ~~~~~
      |      *(   )
fix-it:"./array.cpp":{123:6-123:6}:"*("
fix-it:"./array.cpp":{123:11-123:11}:")"

Is this actually what we want? It somehow makes sense but also doesn't. It's weird that the test ensures the parseable fixits but the output doesn't show the fixit at all (in the first case).

[tbaeder]

Since basically nobody has seen multiple snippet lines being printed, we probably need to fix up a bunch of diagnostics afterwards to be more useful, e.g.

./array.cpp:111:5: error: no matching function for call to 'func'
  111 |     func(1, 2, 3, 4), "hah, reason!");
      |     ^~~~
./array.cpp:96:16: note: candidate function not viable: requires 7 arguments, but 4 were provided
   96 | consteval bool func(int why,
      |                ^    ~~~~~~~~
   97 |                     int would_my,
      |                     ~~~~~~~~~~~~~
   98 |                     int compiler_complain,
      |                     ~~~~~~~~~~~~~~~~~~~~~~
   99 |                     int about_the_arguments,
      |                     ~~~~~~~~~~~~~~~~~~~~~~~~
  100 |                     int but_not_show_them_to_me,
      |                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  101 |                     int if_the_declaration_covers_multiple_lines,
      |                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  102 |                     int big_question_mark) {
      |                     ~~~~~~~~~~~~~~~~~~~~~
1 error generated.

has annoyed me in the past (using one argument per line is common in some coding styles).

[tbaeder]

Ping

[tbaeder]

Fix all existing tests

[tbaeder]

CCing @njames93 since I have no idea how to properly fix the clang-tidy test failures.

[PiotrZSL]

In D147875#4283135, @tbaeder wrote:

CCing @njames93 since I have no idea how to properly fix the clang-tidy test failures.

I look into some of those tests, there are 1 way to fix them. Tried to -fno-diagnostics-show-line-numbers but clang-tidy got history of ignoring those diagnostic flags, I thing there are some issues open for it.

In those tests you got thing like this:

// CHECK-MESSAGES-NEXT: ^~~
// CHECK-MESSAGES-NEXT: {{^ *}}LU{{$}}

this could be changed into something like this:

// CHECK-MESSAGES-NEXT: ^~~
// CHECK-MESSAGES-NEXT: {{^ *\| *}}LU{{$}}

Simply this matching beginning of a line. by using ^ *\| * we match: beginning of line, any number of white spaces, character | any number of white spaces.

[PiotrZSL]

clang-tools-extra/test/clang-tidy/checkers/cert/uppercase-literal-suffix-integer.cpp

34 ↗

(On Diff #517450)

In theory you should escape this character (|) by using \, otherwise it could be interpreted as an regexp or. Still looks like this test file passes, so 7 to go.

[tbaeder]

clang-tools-extra/test/clang-tidy/checkers/cert/uppercase-literal-suffix-integer.cpp

34 ↗

(On Diff #517450)

Yeah I noticed the missing backslash but the test passed.

For the other tests, I've just removed the {{^ *}} before the uppercase suffix. It seems pointless to check that the line contains nothing else(?). I can revert that of course, but it seemed like the easier fix.

[PiotrZSL]

Clang-tidy related changes looks fine.

[aaron.ballman]

This should definitely have explicit documentation added to https://github.com/llvm/llvm-project/blob/main/clang/docs/UsersManual.rst?plain=1#L156 and a release note.

Adding clang-vendors because of the potential that this is a breaking change for some folks doing diagnostic scraping; perhaps we should also mention this under potentially breaking changes in the release notes?

clang/include/clang/Driver/Options.td

2544 ↗

(On Diff #517463)

I'm hopeful that defaulting this to true (which matches the GCC behavior) is reasonable, but I'm a bit worried about how downstreams will react to this. Specifically, I'm wondering about things like clang-cl integration in MSVC or clang use in Xcode, where diagnostics are being displayed other than on the command line.

clang/test/FixIt/fixit-function-call.cpp

1 ↗

(On Diff #517463)

Just to double-check, parseable diagnostic output still works even if line numbers are shown, yes?

[kwk]

@tbaeder I have a small suggestion.

clang/lib/Frontend/TextDiagnostic.cpp

1124–1141

This function screamed at me to be generalized so I gave it a try: https://gist.github.com/kwk/7e408065ea291e49fea4a83cf90a9cdf

[jrtc27]

clang/lib/Frontend/TextDiagnostic.cpp

1124–1141

I don't think I want floating point arithmetic in my compiler... Something like:

unsigned L, M;
for (L = 1U, M = 10U; N >= M && M != ~0U; ++L)
    M = (M > ((~0U) / 10U)) ? (~0U) : (M * 10U);

return (L);

is the generalised form (without all the redundant parentheses I added during debugging).

[tbaeder]

clang/test/FixIt/fixit-function-call.cpp

1 ↗

(On Diff #517463)

Yes, at leat we don't prepend the line numbers to the parseable fixits:

   21 | // CHECK-NEXT: fix-it{{.*}})
   22 | // CHECK: void f1(double *a);
   23 |   f1(a + 1);
      |      ~~~~~
      |      *(   )
fix-it:"../clang/test/FixIt/fixit-function-call.cpp":{23:6-23:6}:"*("
fix-it:"../clang/test/FixIt/fixit-function-call.cpp":{23:11-23:11}:")"

[tbaeder]

Add docs and a release note.

[aaron.ballman]

clang/lib/Frontend/TextDiagnostic.cpp

1124–1141

Cleaned up a bit:

constexpr unsigned getNumDisplayWidth(unsigned N) {
  unsigned L = 1U, M = 10U;
  constexpr unsigned Upper = ~0U / 10U;
  for (; N >= M && M != ~0U; ++L)
    M = M > Upper ? ~0U : M * 10U;
  return L;
}

https://godbolt.org/z/szTYsEM4v

clang/test/FixIt/fixit-function-call.cpp

1 ↗

(On Diff #517463)

Excellent, thank you!

[efriedma]

clang/lib/Frontend/TextDiagnostic.cpp

1124–1141

Slightly less efficient, but easier to read:

unsigned getNumDisplayWidth(unsigned N) {
  unsigned Width = 1;
  for (; N >= 10; ++Width)
    N /= 10;
  return Width;
}

[aaronpuchert]

clang/include/clang/Driver/Options.td

2544 ↗

(On Diff #517463)

I don't like line number margins that much, but I think it should be Ok. What I've seen of compiler output parsers, they mainly concentrate on the <file>:<line>[:<column>]: <kind>: <message> lines and ignore code snippets. After all, once they have the source location, they can just look at the file.

clang/lib/Frontend/TextDiagnostic.cpp

1124–1141

I'd suggest to replace ~0U by std::numeric_limits<unsigned>::max(). And if we're looking at <limits>, why not ask it directly how far we need to go?

static unsigned getNumDisplayWidth(unsigned N) {
  unsigned L = 1u, M = 10u;
  constexpr unsigned Max = std::numeric_limits<unsigned>::digits10 + 1;
  for (; M <= N && L != Max; ++L)
    M *= 10u;
  return L;
}

This will let the overflow happen, but that should be fine for unsigned arithmetic. Without overflow:

static unsigned getNumDisplayWidth(unsigned N) {
  unsigned L = 1u, M = 10u;
  while (M <= N && L++ != std::numeric_limits<unsigned>::digits10)
    M *= 10u;
  return L;
}

The trick is that the increment is done even if the comparison fails and we exit the loop.

[aaronpuchert]

clang/lib/Frontend/TextDiagnostic.cpp

1124–1141

Though pre-increment also does it if we change the right-hand side:

while (M <= N && ++L != std::numeric_limits<unsigned>::digits10 + 1)
  M *= 10u;

[tbaeder]

Ping

[aaron.ballman]

clang/lib/Frontend/TextDiagnostic.cpp

1124–1141

+1 to the final suggestion that's fully generalized.

[aaron.ballman]

LGTM!

[hans]

clang/docs/UsersManual.rst

578 ↗

(On Diff #522634)

I think this is still a cc1-only option. Should it be made available as a driver option now?

[tbaeder]

clang/docs/UsersManual.rst

578 ↗

(On Diff #522634)

No strong opinion from my side, but would certainly make sense to me.

[dyung]

clang/docs/ReleaseNotes.rst

290 ↗

(On Diff #522634)

Is this correct? I notice that all of the tests you update used the form of the option "-fcaret-diagnostics-max-lines n" rather than "-fcaret-diagnostics-max-lines=n". When I tried to update some private tests to use "-fcaret-diagnostics-max-lines=n", the compiler did not accept it, but when I changed it to use "-fcaret-diagnostics-max-lines n" it worked. I think we either need to update the option to accept both forms or to correct the documentation.

[tbaeder]

clang/docs/ReleaseNotes.rst

290 ↗

(On Diff #522634)

Yes, you're right, that should be changed.

[tbaeder]

@hans @dyung I've opened https://reviews.llvm.org/D152090 which should fix both of your concerns.

[hans]

I noticed that at least for some cases of -Wformat, the line numbers on the left seem to be off: https://github.com/llvm/llvm-project/issues/63524

[jrtc27]

In D147875#4448545, @hans wrote:

I noticed that at least for some cases of -Wformat, the line numbers on the left seem to be off: https://github.com/llvm/llvm-project/issues/63524

That's because DisplayLineNo refers to the line of the diagnostic, not the first source line, and then counts up. What's the reason for DisplayLineNo in the first place? Given the printed line comes from LineNo, surely we should be using LineNo instead?

clang/include/clang/Frontend/TextDiagnostic.h

 
 private:
   void emitFilename(StringRef Filename, const SourceManager &SM);
 
   void emitSnippetAndCaret(FullSourceLoc Loc, DiagnosticsEngine::Level Level,
                            SmallVectorImpl<CharSourceRange> &Ranges,
                            ArrayRef<FixItHint> Hints);
 
-  void emitSnippet(StringRef SourceLine);
+  void emitSnippet(StringRef SourceLine, unsigned MaxLineNoDisplayWidth,
+                   unsigned LineNo);
 
   void emitParseableFixits(ArrayRef<FixItHint> Hints, const SourceManager &SM);
 };
 
 } // end namespace clang
 
 #endif

clang/lib/Frontend/TextDiagnostic.cpp

for (ArrayRef<FixItHint>::iterator I = Hints.begin(), E = Hints.end();
     }
   }
 
   expandTabs(FixItInsertionLine, DiagOpts->TabStop);
 
   return FixItInsertionLine;
 }
 
+static unsigned getNumDisplayWidth(unsigned N) {
+  if (N < 10)
+    return 1;
+  if (N < 100)
+    return 2;
+  if (N < 1'000)
+    return 3;
+  if (N < 10'000)
+    return 4;
+  if (N < 100'000)
+    return 5;
+  if (N < 1'000'000)
+    return 6;
+  if (N < 10'000'000)
+    return 7;
+  return 0;
+}
+
 /// Emit a code snippet and caret line.
 ///
 /// This routine emits a single line's code snippet and caret line..
 ///
 /// \param Loc The location for the caret.
 /// \param Ranges The underlined ranges for this code snippet.
 /// \param Hints The FixIt hints active for this diagnostic.
 void TextDiagnostic::emitSnippetAndCaret(
void TextDiagnostic::emitSnippetAndCaret(
   const unsigned MaxLines = DiagOpts->SnippetLineLimit;
   std::pair<unsigned, unsigned> Lines = {CaretLineNo, CaretLineNo};
   for (SmallVectorImpl<CharSourceRange>::iterator I = Ranges.begin(),
                                                   E = Ranges.end();
        I != E; ++I)
     if (auto OptionalRange = findLinesForRange(*I, FID, SM))
       Lines = maybeAddRange(Lines, *OptionalRange, MaxLines);
 
+  unsigned MaxLineNoDisplayWidth = std::max(getNumDisplayWidth(Lines.first),
+                                            getNumDisplayWidth(Lines.second));
+  unsigned HighlightIndent = MaxLineNoDisplayWidth + 1;
+
   for (unsigned LineNo = Lines.first; LineNo != Lines.second + 1; ++LineNo) {
     const char *BufStart = BufData.data();
     const char *BufEnd = BufStart + BufData.size();
 
     // Rewind from the current position to the start of the line.
     const char *LineStart =
         BufStart +
         SM.getDecomposedLoc(SM.translateLineCol(FID, LineNo, 1)).second;
if (DiagOpts->ShowSourceRanges) {
       CaretLine = ' ' + CaretLine;
     }
 
     // Finally, remove any blank spaces from the end of CaretLine.
     while (!CaretLine.empty() && CaretLine[CaretLine.size() - 1] == ' ')
       CaretLine.erase(CaretLine.end() - 1);
 
     // Emit what we have computed.
-    emitSnippet(SourceLine);
+    emitSnippet(SourceLine, MaxLineNoDisplayWidth, LineNo);
 
     if (!CaretLine.empty()) {
+      // Indent for line numbers
+      for (unsigned I = 0; I != HighlightIndent; ++I)
+        OS << ' ';
+      OS << " | ";
       if (DiagOpts->ShowColors)
         OS.changeColor(caretColor, true);
       OS << CaretLine << '\n';
       if (DiagOpts->ShowColors)
         OS.resetColor();
     }
 
     if (!FixItInsertionLine.empty()) {
+      for (unsigned I = 0; I != HighlightIndent; ++I)
+        OS << ' ';
+      OS << " | ";
       if (DiagOpts->ShowColors)
         // Print fixit line in color
         OS.changeColor(fixitColor, false);
       if (DiagOpts->ShowSourceRanges)
         OS << ' ';
       OS << FixItInsertionLine << '\n';
       if (DiagOpts->ShowColors)
         OS.resetColor();
     }
   }
 
   // Print out any parseable fixit information requested by the options.
   emitParseableFixits(Hints, SM);
 }
 
-void TextDiagnostic::emitSnippet(StringRef line) {
+void TextDiagnostic::emitSnippet(StringRef line, unsigned MaxLineNoDisplayWidth,
+                                 unsigned LineNo) {
   if (line.empty())
     return;
 
   size_t i = 0;
 
   std::string to_print;
   bool print_reversed = false;
 
+  // Emit line number.
+  {
+    unsigned LineNoDisplayWidth = getNumDisplayWidth(LineNo);
+    OS << ' ';
+    for (unsigned I = LineNoDisplayWidth; I < MaxLineNoDisplayWidth; ++I)
+      OS << ' ';
+    OS << LineNo;
+    OS << " | ";
+  }
+
   while (i<line.size()) {
     std::pair<SmallString<16>,bool> res
         = printableTextForNextCharacter(line, &i, DiagOpts->TabStop);
     bool was_printable = res.second;
 
     if (DiagOpts->ShowColors && was_printable == print_reversed) {
       if (print_reversed)
         OS.reverseColor();