[clang-tidy] Fix a potential infinite loop in readability-isolate-declaration check.

[ilya-biryukov]

Do we have unit tests for clang-tidy? If yes, could we also add unit-tests for this function?

The modified function only needs a source manager and lang options, these are easy to mock.

clang-tools-extra/clang-tidy/utils/LexerUtils.h

38 ↗

(On Diff #220473)

Would findPrevious have the same problem on the first token of the file?
Can be hard to check without unit-tests, though.

66 ↗

(On Diff #220473)

We seem to be missing a corner case here: what if folks are searching for 'tok::eof'?

Currently we would return invalid location, but given the function's signature I'd expect it to return location of 'tok::eof' in that case.

I bet we don't run into this in practice, though.

[ilya-biryukov]

The fix itself LGTM, getting rid of an infinite loop is important.
All my comment can be addressed later...

[hokein]

Handle the eof corner case.

[hokein]

Do we have unit tests for clang-tidy? If yes, could we also add unit-tests for this function?
The modified function only needs a source manager and lang options, these are easy to mock.

Unfortunately, we don't have any unit tests for all these utility functions. The modified function is only used in the readability-isolate-declaration check.

I agree that we should add one, will file a bug.

clang-tools-extra/clang-tidy/utils/LexerUtils.h

38 ↗

(On Diff #220473)

it depends on the caller. Calling this function directly would probably get into infinite loop. As this function is only used in readability-isolate-declaration check, it seems that the check adds some additional guard code before calling this function, it is probably safe, I assume. (I could not figure out a case that causes the problem).

66 ↗

(On Diff #220473)

yeah, I thought that we shouldn't run into this corner case, I tweaked the code to fix this case.

[ilya-biryukov]

clang-tools-extra/clang-tidy/utils/LexerUtils.h

38 ↗

(On Diff #220473)

Fair point, but putting an assertion or fixing the function would be good too.
But we really need unit tests to properly ensure the invariants.

clang-tools-extra/trunk/clang-tidy/utils/LexerUtils.h

 //===--- LexerUtils.h - clang-tidy-------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_LEXER_UTILS_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_LEXER_UTILS_H
 
 #include "clang/AST/ASTContext.h"
+#include "clang/Basic/TokenKinds.h"
 #include "clang/Lex/Lexer.h"
 
 namespace clang {
 namespace tidy {
 namespace utils {
 namespace lexer {
 
 /// Returns previous token or ``tok::unknown`` if not found.
while (true) {
 
     if (!CurrentToken)
       return SourceLocation();
 
     Token PotentialMatch = *CurrentToken;
     if (PotentialMatch.isOneOf(TK, TKs...))
       return PotentialMatch.getLocation();
 
+    // If we reach the end of the file, and eof is not the target token, we stop
+    // the loop, otherwise we will get infinite loop (findNextToken will return
+    // eof on eof).
+    if (PotentialMatch.is(tok::eof))
+      return SourceLocation();
     Start = PotentialMatch.getLastLoc();
   }
 }
 
 /// Re-lex the provide \p Range and return \c false if either a macro spans
 /// multiple tokens, a pre-processor directive or failure to retrieve the
 /// next token is found, otherwise \c true.
 bool rangeContainsExpansionsOrDirectives(SourceRange Range,

clang-tools-extra/trunk/test/clang-tidy/readability-isolate-declaration-no-infinite-loop.cpp

+// RUN: %check_clang_tidy -expect-clang-tidy-error %s readability-isolate-declaration %t
+
+int main(){
+  int a, b
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: multiple declarations in a single statement reduces readability
+  // CHECK-MESSAGES: [[@LINE-2]]:11: error: expected ';' at end of declaration [clang-diagnostic-error]
+}