[Diagnostic] Add ftabstop to -Wmisleading-indentation

[Tyker]

Summary

this allow much better support of codebases like the linux kernel that mix tabs and spaces.

-ftabstop=Width allow specifying how large tabs are considered to be.

[Tyker]

clang/lib/Parse/ParseStmt.cpp

1261

this is more of a bug fix. although i was not able to produce a breaking test case. i am not convinced it isn't needed.

[Tyker]

i ran a build of the linux kernel using the stage1 compiler, about 30% of files crashed the compiler in the optimizer. everything was building fine 2 weeks ago with the same configuration but a previous compiler.
but aside from that from that file that complied fine, there was only one warning left. this warning is exactly the same case we had in llvm's source code.

[xbolva00]

One case in kernel can be easily fixed.

(This patch looks reasonable).

[aaron.ballman]

clang/include/clang/Basic/SourceManager.h

1394 ↗

(On Diff #232226)

otherwise -> Otherwise,

I'd probably drop the doxygen style /param as well (none of the other arguments are documented that way and this documentation applies to getSpellingColumnNumber() as well anyway.

1395–1400 ↗

(On Diff #232226)

Is there a reason we want these to default to false? Given that the option is intended to be used for calculating correct column numbers in the presence of tabs, I would expect that most places would want this to be true instead of false.

clang/lib/Parse/ParseStmt.cpp

1261

This seems unrelated to the tabstop patch, however. I think it should be commit separately.

[Tyker]

fixed aaron's comment

[Tyker]

clang/include/clang/Basic/SourceManager.h

1395–1400 ↗

(On Diff #232226)

i agree in theory but i am worried about the behavior change and potential performance impact of it.
haven't tested it tho.

[aaron.ballman]

clang/include/clang/Basic/SourceManager.h

1395–1400 ↗

(On Diff #232226)

I think it's worth experimenting to see if any tests break by switching the default to true.

I also think you should perform the timing tests -- this code is quite possibly on the hot path, and others have expressed concern about regressing performance.

clang/test/Parser/warn-misleading-indentation.cpp

307

Missing newline.

[Tyker]

clang/include/clang/Basic/SourceManager.h

1395–1400 ↗

(On Diff #232226)

i ran the testsuite with UseTabsStopOption = false as a default on clang + clangd + clang-tools. there is 13 test failures in total. including complier 3 crashes. so there is a non negligible behavior change.
the best solution is maybe to make a new getVisualColumnNumber which would be the current getSpellingColumnNumber with UseTabsStopOption = true. there is probably many places for which the 'visual' is more important the the actual number of char.

for the performance side:
the 'patched' clang is with all patches concerning the misleading indentation warning.
the 'master' clang was a commit before my first patch about the misleading indentation warning being committed.
clang was built with -02 -march=native but with dynamic linking and no lto nor pgo.
the test was compiling SemaDecl.cpp (one of the biggest files in the repo) with all normal built flags + -fsyntax-only on an otherwise idle pc.

and here are the results in number of seconds of user time on 100 iteration of building SemaDecl.cpp -fsyntax-only.
patched clang:
Min. : 9.612
1st Qu.: 9.838
Median : 9.943
Mean :10.001
3rd Qu.:10.061
Max. :11.573
master master:
Min. : 9.645
1st Qu.: 9.855
Median : 9.939
Mean : 9.974
3rd Qu.:10.062
Max. :10.736

this gives a p-value of 0.418 which is not statistically significant. so I believe it is safe to assume that the difference is not large.

[aaron.ballman]

Adding @rsmith to weigh in on the default behavior.

clang/include/clang/Basic/SourceManager.h

1395–1400 ↗

(On Diff #232226)

i ran the testsuite with UseTabsStopOption = false as a default on clang + clangd + clang-tools. there is 13 test failures in total. including complier 3 crashes. so there is a non negligible behavior change.

Oof, we should correct at least those crashes, that's pretty bad.

the best solution is maybe to make a new getVisualColumnNumber which would be the current getSpellingColumnNumber with UseTabsStopOption = true. there is probably many places for which the 'visual' is more important the the actual number of char.

I'm still not convinced we shouldn't correct the test failures and crashes, but this would be a viable option. @rsmith -- do you have a preference?

1395–1400 ↗

(On Diff #232226)

this gives a p-value of 0.418 which is not statistically significant. so I believe it is safe to assume that the difference is not large.

Thank you for running the timing tests -- the difference in speed seems negligible to me.

[rsmith]

In D71037#1784834, @aaron.ballman wrote:

Adding @rsmith to weigh in on the default behavior.

If we want correct visual columns, we have more work to do: we need to do UTF-8 decoding and use llvm::sys::locale::columnWidth to compute widths of individual characters (byteToColumn in TextDiagnostic.cpp already does this when printing caret diagnostics).

Maybe instead of changing the source manager, we should expose the byte <-> column map from the diagnostic printer and use it from the misleading indentation warning? Or perhaps we can provide an optimized form of it on the basis that we only care about the column number of the first non-whitespace character in the line (so we only need to deal with tabs and spaces) -- but then it's very clear that this is specific to misleading indentation and not a general computation that belongs in the source manager.

clang/include/clang/Basic/SourceManager.h

1393 ↗

(On Diff #233678)

Typo throughout: TabsStop -> TabStop.

clang/lib/Basic/SourceManager.cpp

1175 ↗

(On Diff #233678)

Is there a reason we can't still use the cache in this case? It seems like the process here should be:

1. Find the start of the line (possibly from the cache)
2. Find the number of columns between the start of the line and the query point (either a simple difference or by walking the line looking for tabs)

... and we should only need to alter the second step here.

1203 ↗

(On Diff #233678)

This can read one byte past the end of the buffer (FilePos is allowed to point just past the end of the buffer).

I think there's an off-by-one error in here: suppose we have FilePos == 0, LineStart == 0, and the buffer starts with a tab. The correct column number is 1, but we will return TabStop instead. I think we should be looping while LineStart < FilePos and returning VirtualCharCount + 1. (Column numbers in Clang are 1-based, which is why we need the +1.)

[Tyker]

my intent was not necessarily to create a generic solution but to get a solution that is sufficient to not have false positives on the Wmisleading-indentation warning for code base that have mixes of tabs and spaces.

after taking a look at SourceColumnMap and byteToColumn this seems like way overkill for what i am trying to solve.

Maybe instead of changing the source manager, we should expose the byte <-> column map from the diagnostic printer and use it from the misleading indentation warning?

i did not found such a mapping.

after fixing the bug rsmith spotted. there is no test failures in clang + clang-tool + clangd with UseTabStopOption = true (i found this quite surprising but i double checked). the previous test result were incorrect due to this bug sorry. however I am still not sure that UseTabStopOption = true is the right default for every uses of getSpellingColumnNumber.

one visible example of the impact is the indentation in diagnostics on code indented with tabs.
with ftabstop = 8 (the default)

test.cpp:5:3: warning: misleading indentation; statement is not part of the previous 'if'
                return 0;
                ^
test.cpp:3:2: note: previous statement is here
        if (0)
        ^
1 warning generated.

with ftabstop = 2

test.cpp:5:3: warning: misleading indentation; statement is not part of the previous 'if'
    return 0;
    ^
test.cpp:3:2: note: previous statement is here
  if (0)
  ^
1 warning generated.

i don't know if this is a good thing, i never coded with tabs so i don't know the habits.

clang/lib/Basic/SourceManager.cpp

1203 ↗

(On Diff #233678)

nice catch.

[jyknight]

In D71037#1789574, @Tyker wrote:

after fixing the bug rsmith spotted. there is no test failures in clang + clang-tool + clangd with UseTabStopOption = true (i found this quite surprising but i double checked). the previous test result were incorrect due to this bug sorry. however I am still not sure that UseTabStopOption = true is the right default for every uses of getSpellingColumnNumber.

I think that would be a bad idea. The column numbers are often typically used as offsets (e.g. in diagnostics/etc, that's the expectation) and shouldn't generally be changed to visual offsets.

one visible example of the impact is the indentation in diagnostics on code indented with tabs.

That was already the case -- that's basically the only part of Clang which used TabStop already.

I'm not sure this code really needs to be in SourceManager -- it seems maybe simple enough to just special case within MisleadingIndentationChecker, e.g. something like this function:

// Figure out the visually-apparent column number for a SourceLocation, using
// the TabStop value, from the "-ftabstop" option, to expand tabs.
unsigned GetVisualColumnNum(SourceLocation Loc) {
  SourceManager &SM = P.getPreprocessor().getSourceManager();
  unsigned TabStop = P.getActions().getDiagnostics().getDiagnosticOptions().TabStop;

  unsigned ColNo = SM.getSpellingColumnNumber(Loc);
  if (ColNo == 0 || TabStop == 1)
    return ColNo;

  auto FIDAndOffset = SM.getDecomposedLoc(Loc);

  bool Invalid;
  StringRef BufData = SM.getBufferData(FIDAndOffset.first, &Invalid);
  if (Invalid) return 0;

  const char *EndPos = BufData.data() + FIDAndOffset.second;
  assert(FIDAndOffset.second > ColNo && "Column number smaller than file offset?");

  unsigned VisualColumn = 0; // stored as 0-based column, here.
  // Loop from beginning of line up to Loc's file position, counting columns, expanding tabs
  for (const char *CurPos = EndPos - (ColNo - 1); CurPos != EndPos; ++CurPos) {
    if (*CurPos == '\t')
      // Advance visual column to next tabstop
      VisualColumn += (TabStop - VisualColumn % TabStop);
    else
      VisualColumn++;
  }
  return VisualColumn + 1;
}

[jyknight]

BTW, the commit here should mark llvm.org/PR44324 as fixed by it.

[Tyker]

i agree that it is the best solution.

[xbolva00]

clang/lib/Parse/ParseStmt.cpp

1217

Typo

[riccibruno]

clang/lib/Parse/ParseStmt.cpp

1217

Also, what about a little comment explaining what getAdjustedColumnNumber does ? It is not immediately clear what "adjusted" means here (adjusted for what ?)

[riccibruno]

clang/lib/Parse/ParseStmt.cpp

1217

Oh, and it should static.

[aaron.ballman]

clang/lib/Parse/ParseStmt.cpp

1217

And you can add a newline above it to give it some visual separation from the previous function.

1224

Please don't use auto as the type is not spelled out in the initialization.

1235

stored -> Stored

1237

Missing a full stop at the end of the sentence.

1241

Missing full stop.

[Tyker]

fixed comments.

[Tyker]

This comment was removed by Tyker.

[aaron.ballman]

LGTM, thank you!

[mstorsjo]

This broke my builds. A configure test that tries to compile int main (void) { for( int i = 0; i < 9; i++ ); return 0; } (a single line source file), with -Wall enabled, now triggers failed asserts:

clang-10: ../tools/clang/lib/Parse/ParseStmt.cpp:1236: static unsigned int {anonymous}::MisleadingIndentationChecker::getVisualIndentation(clang::SourceManager&, clang::SourceLocation): Assertion `FIDAndOffset.second > ColNo && "Column number smaller than file offset?"' failed.

<cut>

1.      oneline.c:1:49: current parser token 'return'
2.      oneline.c:1:17: parsing function body 'main'
3.      oneline.c:1:17: in compound statement ('{}')

[mstorsjo]

In D71037#1799792, @mstorsjo wrote:

This broke my builds.

I pushed a revert for this for now, to unbreak things.

[Tyker]

This update should fixes the issue.
the assert was incorrect. because file offsets are 0-based where as column numbers are 1-based.

however the issue can only occur for on the first line. so we can't have a run line. before it to add a test case.

[xbolva00]

Can you add a test case for that crash? Otherwise OK.

[Tyker]

In D71037#1803361, @xbolva00 wrote:

Can you add a test case for that crash? Otherwise OK.

as i said. the bug can only occur for one line files. so we can't have a run line and a the crashing line. so i wasn't able to make a test for it.

[xbolva00]

Ah, true.

[mstorsjo]

In D71037#1803574, @Tyker wrote:

In D71037#1803361, @xbolva00 wrote:

Can you add a test case for that crash? Otherwise OK.

as i said. the bug can only occur for one line files. so we can't have a run line and a the crashing line. so i wasn't able to make a test for it.

Is it possible to have the RUN bit at the end of the line in a comment, after the test code itself?

[Tyker]

In D71037#1803704, @mstorsjo wrote:

In D71037#1803574, @Tyker wrote:

In D71037#1803361, @xbolva00 wrote:

Can you add a test case for that crash? Otherwise OK.

as i said. the bug can only occur for one line files. so we can't have a run line and a the crashing line. so i wasn't able to make a test for it.

Is it possible to have the RUN bit at the end of the line in a comment, after the test code itself?

it is possible.

i committed the fix and the tests.

[efriedma]

I just ran into this warning, and I think there's a bit of a discoverability problem related to the width of tabs and -ftabstop. If you have mixed tabs and spaces, and you've correctly specified the tab stop width with -ftabstop, everything works fine. If you haven't specified -ftabstop, you get a warning, and the issue isn't obvious. Depending on your editor settings, the code might look fine at first glance, and the warning text doesn't mention -ftabstop at all. Maybe there's something that could be improved here, if we're printing a warning for two lines with mismatched indentation styles?

[Tyker]

In D71037#1871089, @efriedma wrote:

I just ran into this warning, and I think there's a bit of a discoverability problem related to the width of tabs and -ftabstop. If you have mixed tabs and spaces, and you've correctly specified the tab stop width with -ftabstop, everything works fine. If you haven't specified -ftabstop, you get a warning, and the issue isn't obvious. Depending on your editor settings, the code might look fine at first glance, and the warning text doesn't mention -ftabstop at all. Maybe there's something that could be improved here, if we're printing a warning for two lines with mismatched indentation styles?

i agree that this is not obvious. i added a patch to improve this https://reviews.llvm.org/D75009.

clang/lib/Parse/ParseStmt.cpp

MisleadingIndentationChecker(Parser &P, MisleadingStatementKind K,
         ShouldSkip(P.getCurToken().is(tok::l_brace)) {
     if (!P.MisleadingIndentationElseLoc.isInvalid()) {
       StmtLoc = P.MisleadingIndentationElseLoc;
       P.MisleadingIndentationElseLoc = SourceLocation();
     }
     if (Kind == MSK_else && !ShouldSkip)
       P.MisleadingIndentationElseLoc = SL;
   }
+  unsigned getAdujstedColumnNumber(SourceManager &SM, SourceLocation Loc) {
+    unsigned TabStop = SM.getDiagnostics().getDiagnosticOptions().TabStop;
+
+    unsigned ColNo = SM.getSpellingColumnNumber(Loc);
+    if (ColNo == 0 || TabStop == 1)
+      return ColNo;
+
+    auto FIDAndOffset = SM.getDecomposedLoc(Loc);
+
+    bool Invalid;
+    StringRef BufData = SM.getBufferData(FIDAndOffset.first, &Invalid);
+    if (Invalid)
+      return 0;
+
+    const char *EndPos = BufData.data() + FIDAndOffset.second;
+    assert(FIDAndOffset.second > ColNo &&
+           "Column number smaller than file offset?");
+
+    unsigned VisualColumn = 0; // stored as 0-based column, here.
+    // Loop from beginning of line up to Loc's file position, counting columns,
+    // expanding tabs
+    for (const char *CurPos = EndPos - (ColNo - 1); CurPos != EndPos;
+         ++CurPos) {
+      if (*CurPos == '\t')
+        // Advance visual column to next tabstop
+        VisualColumn += (TabStop - VisualColumn % TabStop);
+      else
+        VisualColumn++;
+    }
+    return VisualColumn + 1;
+  }
   void Check() {
     Token Tok = P.getCurToken();
     if (P.getActions().getDiagnostics().isIgnored(
             diag::warn_misleading_indentation, Tok.getLocation()) ||
         ShouldSkip || NumDirectives != P.getPreprocessor().getNumDirectives() ||
         Tok.isOneOf(tok::semi, tok::r_brace) || Tok.isAnnotation() ||
         Tok.getLocation().isMacroID() || PrevLoc.isMacroID() ||
         StmtLoc.isMacroID() ||
         (Kind == MSK_else && P.MisleadingIndentationElseLoc.isInvalid())) {
       P.MisleadingIndentationElseLoc = SourceLocation();
       return;
     }
     if (Kind == MSK_else)
       P.MisleadingIndentationElseLoc = SourceLocation();
 
     SourceManager &SM = P.getPreprocessor().getSourceManager();
-    unsigned PrevColNum = SM.getSpellingColumnNumber(PrevLoc);
-    unsigned CurColNum = SM.getSpellingColumnNumber(Tok.getLocation());
-    unsigned StmtColNum = SM.getSpellingColumnNumber(StmtLoc);
+    unsigned PrevColNum = getAdujstedColumnNumber(SM, PrevLoc);
+    unsigned CurColNum = getAdujstedColumnNumber(SM, Tok.getLocation());
+    unsigned StmtColNum = getAdujstedColumnNumber(SM, StmtLoc);
 
     if (PrevColNum != 0 && CurColNum != 0 && StmtColNum != 0 &&
         ((PrevColNum > StmtColNum && PrevColNum == CurColNum) ||
          !Tok.isAtStartOfLine()) && SM.getPresumedLineNumber(StmtLoc) !=
           SM.getPresumedLineNumber(Tok.getLocation())) {
       P.Diag(Tok.getLocation(), diag::warn_misleading_indentation)
           << Kind;
       P.Diag(StmtLoc, diag::note_previous_statement);

clang/test/Parser/warn-misleading-indentation.cpp

 // RUN: %clang_cc1 -x c -fsyntax-only -verify %s
-// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wmisleading-indentation -DWITH_WARN %s
-// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -DWITH_WARN -DCXX17 %s
 // RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -Wno-misleading-indentation -DCXX17 %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wmisleading-indentation -DWITH_WARN -ftabstop 8 -DTAB_SIZE=8 %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -DWITH_WARN  -ftabstop 4 -DTAB_SIZE=4 -DCXX17 %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall -Wno-unused -DWITH_WARN -ftabstop 1 -DTAB_SIZE=1 %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -Wmisleading-indentation -DCXX17 -DWITH_WARN -ftabstop 2 -DTAB_SIZE=2 %s
 
 #ifndef WITH_WARN
 // expected-no-diagnostics
 #endif
 
 void f0(int i) {
   if (i)
 #ifdef WITH_WARN

 // expected-note@-2 {{here}}
 #endif
         return;
         return;
 #ifdef WITH_WARN
 // expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
 #endif
 }
+int a4()
+{
+  if (0)
+    return 1;
+  return 0;
+#if (TAB_SIZE == 1)
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+// expected-note@-5 {{here}}
+#endif 
+}
+
+int a5()
+{
+  if (0)
+    return 1;
+    return 0;
+#if WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+// expected-note@-5 {{here}}
+#endif
+}
+
+int a6()
+{
+  if (0)
+    return 1;
+          return 0;
+#if (TAB_SIZE == 8)
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+// expected-note@-5 {{here}}
+#endif
+}
+
+#define FOO \
+ goto fail
+
+int main(int argc, char* argv[]) {
+  if (5 != 0)
+    goto fail;
+  else
+    goto fail;
+
+  if (1) {
+    if (1)
+      goto fail;
+    else if (1)
+      goto fail;
+    else if (1)
+      goto fail;
+    else
+      goto fail;
+  } else if (1) {
+    if (1)
+      goto fail;
+  }
+
+  if (1) {
+    if (1)
+      goto fail;
+  } else if (1)
+    goto fail;
+
+
+  if (1) goto fail; goto fail;
+
+    if (0)
+        goto fail;
+
+    goto fail;
+
+    if (0)
+        FOO;
+
+    goto fail;
+
+fail:;
+}