[clang-tidy] Fix PR#34798 'readability-braces-around-statements breaks statements containing braces.'

[f00kat]

Summary

In method 'BracesAroundStatementsCheck::checkStmt' the call 'Lexer::makeFileCharRange(line 253)' return different EndSourceLocation for different statements.

CharSourceRange FileRange = Lexer::makeFileCharRange(
      CharSourceRange::getTokenRange(S->getSourceRange()), SM,
      Context->getLangOpts());

For example

class c {};
c test()
{
	if (true)
		return {};
	if (true)
		bool b[2] = { true, true };
}

In case

return {};

FileRange.getEnd() will be ';'
In case

bool b[2] = { true, true };

FileRange.getEnd() will be '\r'

Before call 'findEndLocation' we do this

const auto FREnd = FileRange.getEnd().getLocWithOffset(-1);

so 'findEndLocation' received '}' in the first case and ';' in the second what breaks the logic of 'findEndLocation'.

[aaron.ballman]

clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp

30

Elide the braces.

40

Elide braces

76–77

Is there evidence that this behavior is desired? I have a hunch that this is a bug in Clang -- not all InitListExprs will terminate with a semicolon, such as ones that appear as function arguments, like foo({1, 2, 3});, so I'm surprised to see it included here.

[ymandel]

clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp

21

Could you change this to llvm::Expected<Kind> tryGetTokenKind(SourceLocation Loc, const SourceManager &SM, const ASTContext *Context). From a readability and performance standpoint, returned parameters are generally better than out parameters.

76–77

On a related note, are you sure the cause of this issue is makeFileCharRange? AFAIK, that does not involve any examination of tokens. It purely (attempts) to map from potentially a mix of expanded locations and file locations to purely file locations (and in the same file for that matter).

[ymandel]

clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp

76–77

I believe the issue lies with DeclStmt, not InitListExpr. Specifically, the source range provided by DeclStmt. See https://godbolt.org/z/vVtQZ8. The non-decl statements have an end location on the token before the semi, whereas the decl statements given their end location as the semi.

[aaron.ballman]

clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp

76–77

Ah, yeah, that sounds plausible too (the end location should be before the semi in both places). Either way, I think we should be fixing this in the frontend rather than trying to hack around it in clang-tidy checks, if at all possible. This may require undoing hacks done elsewhere, though.

clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp

 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace readability {
 namespace {
 
-tok::TokenKind getTokenKind(SourceLocation Loc, const SourceManager &SM,
-                            const ASTContext *Context) {
-  Token Tok;
+bool tryGetTokenKind(SourceLocation Loc, const SourceManager &SM,
+                     const ASTContext *Context, tok::TokenKind &Kind) {
+  Kind = tok::NUM_TOKENS;
   SourceLocation Beginning =
       Lexer::GetBeginningOfToken(Loc, SM, Context->getLangOpts());
+
+  Token Tok;
   const bool Invalid =
       Lexer::getRawToken(Beginning, Tok, SM, Context->getLangOpts());
-  assert(!Invalid && "Expected a valid token.");
+  if (!Invalid) {
+    Kind = Tok.getKind();
+  }
 
-  if (Invalid)
-    return tok::NUM_TOKENS;
+  return !Invalid;
+}
 
-  return Tok.getKind();
+tok::TokenKind getTokenKind(SourceLocation Loc, const SourceManager &SM,
+                            const ASTContext *Context) {
+  tok::TokenKind Kind;
+  if (tryGetTokenKind(Loc, SM, Context, Kind)) {
+    return Kind;
+  }
+
+  assert(false && "Expected a valid token.");
+
+  return tok::NUM_TOKENS;
 }
 
 SourceLocation forwardSkipWhitespaceAndComments(SourceLocation Loc,
                                                 const SourceManager &SM,
                                                 const ASTContext *Context) {
   assert(Loc.isValid());
   for (;;) {
     while (isWhitespace(*SM.getCharacterData(Loc)))

 SourceLocation findEndLocation(SourceLocation LastTokenLoc,
                                const SourceManager &SM,
                                const ASTContext *Context) {
   SourceLocation Loc =
       Lexer::GetBeginningOfToken(LastTokenLoc, SM, Context->getLangOpts());
   // Loc points to the beginning of the last (non-comment non-ws) token
   // before end or ';'.
   assert(Loc.isValid());
-  bool SkipEndWhitespaceAndComments = true;
   tok::TokenKind TokKind = getTokenKind(Loc, SM, Context);
+
+  // We need to check that it is not 'InitListExpr' which ends with 
+  // the tokens '};' because it will break the following analysis
+  tok::TokenKind NextTokKind;
+  SourceLocation NextLoc =
+      Lexer::getLocForEndOfToken(Loc, 0, SM, Context->getLangOpts());
+  if (tryGetTokenKind(NextLoc, SM, Context, NextTokKind)) {
+    if (TokKind == tok::r_brace && NextTokKind == tok::semi) {
+      Loc = NextLoc;
+      TokKind = NextTokKind;
+    }
+  }
+
+  bool SkipEndWhitespaceAndComments = true;
   if (TokKind == tok::NUM_TOKENS || TokKind == tok::semi ||
       TokKind == tok::r_brace) {
     // If we are at ";" or "}", we found the last token. We could use as well
     // `if (isa<NullStmt>(S))`, but it wouldn't work for nested statements.
     SkipEndWhitespaceAndComments = false;
   }
 
   Loc = Lexer::getLocForEndOfToken(Loc, 0, SM, Context->getLangOpts());

clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp

M(
     for (;;)
       ;
   );
   // CHECK-MESSAGES: :[[@LINE-3]]:13: warning: statement should be inside braces
   // CHECK-FIXES: {{^}}    for (;;) {{{$}}
   // CHECK-FIXES-NEXT: {{^      ;$}}
   // CHECK-FIXES-NEXT: {{^}$}}
 }
+
+class X {};
+X test_initListExpr()
+{
+  if (cond("x")) return {};
+  else return {};
+  // CHECK-MESSAGES: :[[@LINE-2]]:24: warning: statement should be inside braces
+  // CHECK-FIXES: if (cond("x")) { return {};
+  // CHECK-FIXES: } else {
+  // CHECK-FIXES: return {};
+  // CHECK-FIXES: }
+
+  if (cond("y1"))
+    if (cond("y2") && cond("y3"))
+      return {};
+    else
+      return {};
+  // CHECK-MESSAGES: :[[@LINE-2]]:24: warning: statement should be inside braces
+  // CHECK-FIXES: if (cond("y1")) {
+  // CHECK-FIXES: if (cond("y2") && cond("y3")) {
+  // CHECK-FIXES: return {};
+  // CHECK-FIXES: } else {
+  // CHECK-FIXES: return {};
+  // CHECK-FIXES: }
+}