[clang-tidy] Fix readability-braces-around-statements assert failure

[mattsta]

Summary

What: clang-tidy fails to complete on some source files with always-braces fixup enabled.

Why: Failure check is after findRParenLoc() already asserted on error and crashed.

Fix: Check for invalid location before findRParenLoc() instead of after.

Removes error:

Assertion failed: (CondEndLoc.isValid()), function findRParenLoc, file ../llvm/tools/clang/tools/extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp, line 183.

[alexfh]

Please add a test that crashes without the patch and passes with it.

[mattsta]

Updated to include fix for:

Assertion failed: (InitialLoc.isValid()), function checkStmt, file ../llvm/tools/clang/tools/extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp, line 226.

[mattsta]

It's difficult to track down *why* the invalid locations are happening because by the time we get to an invalid location, all source location information is lost. The best I've been able to come up with is fixing the early return conditions (which were previously impossible to reach due to asserts catching the invalid conditions first).

It's easy to have these happen on any large codebase (e.g. try to recursively clang-tidy the erlang source tree with readability-braces-around-statements).

[alexfh]

In D15797#317793, @mattsta wrote:

It's difficult to track down *why* the invalid locations are happening because by the time we get to an invalid location, all source location information is lost. The best I've been able to come up with is fixing the early return conditions (which were previously impossible to reach due to asserts catching the invalid conditions first).

It's easy to have these happen on any large codebase (e.g. try to recursively clang-tidy the erlang source tree with readability-braces-around-statements).

I'd still like to add a test for this (and I have a different idea on how to fix this, namely return an invalid location from findRParenLoc instead of asserting). If you can't come up with a test case, I'll try to find one myself by running on some files, but we absolutely have to add a test to avoid regressions when this code is changed again.

[mattsta]

Yeah, I fully understand the need to make sure it doesn't break again (or that this actually fixes it properly), but these changes are about the limit of my LLVM+Tidy internals understanding. (What's missing from this diff is the 20 hours across 4-6 months where I tried to fix it "properly" but could never figure out how things were getting to an Invalid state to begin with.)

I'll leave this in more capable hands than my own at this point. It's easy to reproduce the assert failures on large source dirs (a freebsd checkout, an erlang checkout, etc), but I couldn't narrow down what triggers the failures.

[alexfh]

An alternative fix has recently been committed. Can you run clang-tidy built after r260505 on your code?

BracesAroundStatementsCheck.cpp

BracesAroundStatementsCheck::check(const MatchFinder::MatchResult &Result) {
   // Get location of closing parenthesis or 'do' to insert opening brace.
   if (auto S = Result.Nodes.getNodeAs<ForStmt>("for")) {
     checkStmt(Result, S->getBody(), S->getRParenLoc());
   } else if (auto S = Result.Nodes.getNodeAs<CXXForRangeStmt>("for-range")) {
     checkStmt(Result, S->getBody(), S->getRParenLoc());
   } else if (auto S = Result.Nodes.getNodeAs<DoStmt>("do")) {
     checkStmt(Result, S->getBody(), S->getDoLoc(), S->getWhileLoc());
   } else if (auto S = Result.Nodes.getNodeAs<WhileStmt>("while")) {
-    SourceLocation StartLoc = findRParenLoc(S, SM, Context);
-    if (StartLoc.isInvalid())
+    if (S->getCond()->getLocStart().isInvalid())
       return;
+    SourceLocation StartLoc = findRParenLoc(S, SM, Context);
     checkStmt(Result, S->getBody(), StartLoc);
   } else if (auto S = Result.Nodes.getNodeAs<IfStmt>("if")) {
-    SourceLocation StartLoc = findRParenLoc(S, SM, Context);
-    if (StartLoc.isInvalid())
+    if (S->getCond()->getLocStart().isInvalid())
       return;
+    SourceLocation StartLoc = findRParenLoc(S, SM, Context);
     if (ForceBracesStmts.erase(S))
       ForceBracesStmts.insert(S->getThen());
     bool BracedIf = checkStmt(Result, S->getThen(), StartLoc, S->getElseLoc());
     const Stmt *Else = S->getElse();
     if (Else && BracedIf)
       ForceBracesStmts.insert(Else);
     if (Else && !isa<IfStmt>(Else)) {
       // Omit 'else if' statements here, they will be handled directly.
bool BracesAroundStatementsCheck::checkStmt(
 
   const SourceManager &SM = *Result.SourceManager;
   const ASTContext *Context = Result.Context;
 
   // Treat macros.
   CharSourceRange FileRange = Lexer::makeFileCharRange(
       CharSourceRange::getTokenRange(S->getSourceRange()), SM,
       Context->getLangOpts());
-  if (FileRange.isInvalid())
+  if (FileRange.isInvalid() || InitialLoc.isInvalid())
     return false;
 
   // InitialLoc points at the last token before opening brace to be inserted.
   assert(InitialLoc.isValid());
   // Convert InitialLoc to file location, if it's on the same macro expansion
   // level as the start of the statement. We also need file locations for
   // Lexer::getLocForEndOfToken working properly.
   InitialLoc = Lexer::makeFileCharRange(