Revert "[clang-tidy] Cache the locations of NOLINTBEGIN/END blocks"

Build warning here:
https://lab.llvm.org/buildbot/#/builders/57/builds/14322

Author: salman-javed-nz

clang-tools-extra/clang-tidy/CMakeLists.txt

@@ -17,7 +17,6 @@ add_clang_library(clangTidy
   ClangTidyProfiling.cpp
   ExpandModularHeadersPPCallbacks.cpp
   GlobList.cpp
-  NoLintDirectiveHandler.cpp
 
   DEPENDS
   ClangSACheckers

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

@@ -18,7 +18,6 @@
 #include "ClangTidyDiagnosticConsumer.h"
 #include "ClangTidyOptions.h"
 #include "GlobList.h"
-#include "NoLintDirectiveHandler.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTDiagnostic.h"
 #include "clang/AST/Attr.h"
@@ -189,7 +188,7 @@ DiagnosticBuilder ClangTidyContext::diag(
   return DiagEngine->Report(ID);
 }
 
-DiagnosticBuilder ClangTidyContext::diag(const tooling::Diagnostic &Error) {
+DiagnosticBuilder ClangTidyContext::diag(const ClangTidyError &Error) {
   SourceManager &SM = DiagEngine->getSourceManager();
   llvm::ErrorOr<const FileEntry *> File =
       SM.getFileManager().getFile(Error.Message.FilePath);
@@ -207,15 +206,6 @@ DiagnosticBuilder ClangTidyContext::configurationDiag(
   return diag("clang-tidy-config", Message, Level);
 }
 
-bool ClangTidyContext::shouldSuppressDiagnostic(
-    DiagnosticsEngine::Level DiagLevel, const Diagnostic &Info,
-    SmallVectorImpl<tooling::Diagnostic> &NoLintErrors, bool AllowIO,
-    bool EnableNoLintBlocks) {
-  std::string CheckName = getCheckName(Info.getID());
-  return NoLintHandler.shouldSuppress(DiagLevel, Info, CheckName, NoLintErrors,
-                                      AllowIO, EnableNoLintBlocks);
-}
-
 void ClangTidyContext::setSourceManager(SourceManager *SourceMgr) {
   DiagEngine->setSourceManager(SourceMgr);
 }
@@ -317,9 +307,218 @@ void ClangTidyDiagnosticConsumer::finalizeLastError() {
   LastErrorPassesLineFilter = false;
 }
 
+static bool isNOLINTFound(StringRef NolintDirectiveText, StringRef CheckName,
+                          StringRef Line, size_t *FoundNolintIndex = nullptr,
+                          StringRef *FoundNolintChecksStr = nullptr) {
+  if (FoundNolintIndex)
+    *FoundNolintIndex = StringRef::npos;
+  if (FoundNolintChecksStr)
+    *FoundNolintChecksStr = StringRef();
+
+  size_t NolintIndex = Line.find(NolintDirectiveText);
+  if (NolintIndex == StringRef::npos)
+    return false;
+
+  size_t BracketIndex = NolintIndex + NolintDirectiveText.size();
+  if (BracketIndex < Line.size() && isalnum(Line[BracketIndex])) {
+    // Reject this search result, otherwise it will cause false positives when
+    // NOLINT is found as a substring of NOLINT(NEXTLINE/BEGIN/END).
+    return false;
+  }
+
+  // Check if specific checks are specified in brackets.
+  if (BracketIndex < Line.size() && Line[BracketIndex] == '(') {
+    ++BracketIndex;
+    const size_t BracketEndIndex = Line.find(')', BracketIndex);
+    if (BracketEndIndex != StringRef::npos) {
+      StringRef ChecksStr =
+          Line.substr(BracketIndex, BracketEndIndex - BracketIndex);
+      if (FoundNolintChecksStr)
+        *FoundNolintChecksStr = ChecksStr;
+      // Allow specifying a few checks with a glob expression, ignoring
+      // negative globs (which would effectively disable the suppression).
+      GlobList Globs(ChecksStr, /*KeepNegativeGlobs=*/false);
+      if (!Globs.contains(CheckName))
+        return false;
+    }
+  }
+
+  if (FoundNolintIndex)
+    *FoundNolintIndex = NolintIndex;
+
+  return true;
+}
+
+static llvm::Optional<StringRef> getBuffer(const SourceManager &SM, FileID File,
+                                           bool AllowIO) {
+  return AllowIO ? SM.getBufferDataOrNone(File)
+                 : SM.getBufferDataIfLoaded(File);
+}
+
+static ClangTidyError createNolintError(const ClangTidyContext &Context,
+                                        const SourceManager &SM,
+                                        SourceLocation Loc,
+                                        bool IsNolintBegin) {
+  ClangTidyError Error("clang-tidy-nolint", ClangTidyError::Error,
+                       Context.getCurrentBuildDirectory(), false);
+  StringRef Message =
+      IsNolintBegin
+          ? ("unmatched 'NOLINTBEGIN' comment without a subsequent 'NOLINT"
+             "END' comment")
+          : ("unmatched 'NOLINTEND' comment without a previous 'NOLINT"
+             "BEGIN' comment");
+  Error.Message = tooling::DiagnosticMessage(Message, SM, Loc);
+  return Error;
+}
+
+static Optional<ClangTidyError> tallyNolintBegins(
+    const ClangTidyContext &Context, const SourceManager &SM,
+    StringRef CheckName, SmallVector<StringRef> Lines, SourceLocation LinesLoc,
+    SmallVector<std::pair<SourceLocation, StringRef>> &NolintBegins) {
+  // Keep a running total of how many NOLINT(BEGIN...END) blocks are active, as
+  // well as the bracket expression (if any) that was used in the NOLINT
+  // expression.
+  size_t NolintIndex;
+  StringRef NolintChecksStr;
+  for (const auto &Line : Lines) {
+    if (isNOLINTFound("NOLINTBEGIN", CheckName, Line, &NolintIndex,
+                      &NolintChecksStr)) {
+      // Check if a new block is being started.
+      NolintBegins.emplace_back(std::make_pair(
+          LinesLoc.getLocWithOffset(NolintIndex), NolintChecksStr));
+    } else if (isNOLINTFound("NOLINTEND", CheckName, Line, &NolintIndex,
+                             &NolintChecksStr)) {
+      // Check if the previous block is being closed.
+      if (!NolintBegins.empty() &&
+          NolintBegins.back().second == NolintChecksStr) {
+        NolintBegins.pop_back();
+      } else {
+        // Trying to close a nonexistent block. Return a diagnostic about this
+        // misuse that can be displayed along with the original clang-tidy check
+        // that the user was attempting to suppress.
+        return createNolintError(Context, SM,
+                                 LinesLoc.getLocWithOffset(NolintIndex), false);
+      }
+    }
+    // Advance source location to the next line.
+    LinesLoc = LinesLoc.getLocWithOffset(Line.size() + sizeof('\n'));
+  }
+  return None; // All NOLINT(BEGIN/END) use has been consistent so far.
+}
+
+static bool
+lineIsWithinNolintBegin(const ClangTidyContext &Context,
+                        SmallVectorImpl<ClangTidyError> &SuppressionErrors,
+                        const SourceManager &SM, SourceLocation Loc,
+                        StringRef CheckName, StringRef TextBeforeDiag,
+                        StringRef TextAfterDiag) {
+  Loc = SM.getExpansionRange(Loc).getBegin();
+  SourceLocation FileStartLoc = SM.getLocForStartOfFile(SM.getFileID(Loc));
+  SmallVector<std::pair<SourceLocation, StringRef>> NolintBegins;
+
+  // Check if there's an open NOLINT(BEGIN...END) block on the previous lines.
+  SmallVector<StringRef> PrevLines;
+  TextBeforeDiag.split(PrevLines, '\n');
+  auto Error = tallyNolintBegins(Context, SM, CheckName, PrevLines,
+                                 FileStartLoc, NolintBegins);
+  if (Error) {
+    SuppressionErrors.emplace_back(Error.getValue());
+  }
+  bool WithinNolintBegin = !NolintBegins.empty();
+
+  // Check that every block is terminated correctly on the following lines.
+  SmallVector<StringRef> FollowingLines;
+  TextAfterDiag.split(FollowingLines, '\n');
+  Error = tallyNolintBegins(Context, SM, CheckName, FollowingLines, Loc,
+                            NolintBegins);
+  if (Error) {
+    SuppressionErrors.emplace_back(Error.getValue());
+  }
+
+  // The following blocks were never closed. Return diagnostics for each
+  // instance that can be displayed along with the original clang-tidy check
+  // that the user was attempting to suppress.
+  for (const auto &NolintBegin : NolintBegins) {
+    SuppressionErrors.emplace_back(
+        createNolintError(Context, SM, NolintBegin.first, true));
+  }
+
+  return WithinNolintBegin && SuppressionErrors.empty();
+}
+
+static bool
+lineIsMarkedWithNOLINT(const ClangTidyContext &Context,
+                       SmallVectorImpl<ClangTidyError> &SuppressionErrors,
+                       bool AllowIO, const SourceManager &SM,
+                       SourceLocation Loc, StringRef CheckName,
+                       bool EnableNolintBlocks) {
+  // Get source code for this location.
+  FileID File;
+  unsigned Offset;
+  std::tie(File, Offset) = SM.getDecomposedSpellingLoc(Loc);
+  Optional<StringRef> Buffer = getBuffer(SM, File, AllowIO);
+  if (!Buffer)
+    return false;
+
+  // Check if there's a NOLINT on this line.
+  StringRef TextAfterDiag = Buffer->substr(Offset);
+  StringRef RestOfThisLine, FollowingLines;
+  std::tie(RestOfThisLine, FollowingLines) = TextAfterDiag.split('\n');
+  if (isNOLINTFound("NOLINT", CheckName, RestOfThisLine))
+    return true;
+
+  // Check if there's a NOLINTNEXTLINE on the previous line.
+  StringRef TextBeforeDiag = Buffer->substr(0, Offset);
+  size_t LastNewLinePos = TextBeforeDiag.rfind('\n');
+  StringRef PrevLines = (LastNewLinePos == StringRef::npos)
+                            ? StringRef()
+                            : TextBeforeDiag.slice(0, LastNewLinePos);
+  LastNewLinePos = PrevLines.rfind('\n');
+  StringRef PrevLine = (LastNewLinePos == StringRef::npos)
+                           ? PrevLines
+                           : PrevLines.substr(LastNewLinePos + 1);
+  if (isNOLINTFound("NOLINTNEXTLINE", CheckName, PrevLine))
+    return true;
+
+  // Check if this line is within a NOLINT(BEGIN...END) block.
+  return EnableNolintBlocks &&
+         lineIsWithinNolintBegin(Context, SuppressionErrors, SM, Loc, CheckName,
+                                 TextBeforeDiag, TextAfterDiag);
+}
+
+static bool lineIsMarkedWithNOLINTinMacro(
+    const Diagnostic &Info, const ClangTidyContext &Context,
+    SmallVectorImpl<ClangTidyError> &SuppressionErrors, bool AllowIO,
+    bool EnableNolintBlocks) {
+  const SourceManager &SM = Info.getSourceManager();
+  SourceLocation Loc = Info.getLocation();
+  std::string CheckName = Context.getCheckName(Info.getID());
+  while (true) {
+    if (lineIsMarkedWithNOLINT(Context, SuppressionErrors, AllowIO, SM, Loc,
+                               CheckName, EnableNolintBlocks))
+      return true;
+    if (!Loc.isMacroID())
+      return false;
+    Loc = SM.getImmediateExpansionRange(Loc).getBegin();
+  }
+  return false;
+}
+
 namespace clang {
 namespace tidy {
 
+bool shouldSuppressDiagnostic(
+    DiagnosticsEngine::Level DiagLevel, const Diagnostic &Info,
+    ClangTidyContext &Context,
+    SmallVectorImpl<ClangTidyError> &SuppressionErrors, bool AllowIO,
+    bool EnableNolintBlocks) {
+  return Info.getLocation().isValid() &&
+         DiagLevel != DiagnosticsEngine::Error &&
+         DiagLevel != DiagnosticsEngine::Fatal &&
+         lineIsMarkedWithNOLINTinMacro(Info, Context, SuppressionErrors,
+                                       AllowIO, EnableNolintBlocks);
+}
+
 const llvm::StringMap<tooling::Replacements> *
 getFixIt(const tooling::Diagnostic &Diagnostic, bool GetFixFromNotes) {
   if (!Diagnostic.Message.Fix.empty())
@@ -346,15 +545,12 @@ void ClangTidyDiagnosticConsumer::HandleDiagnostic(
   if (LastErrorWasIgnored && DiagLevel == DiagnosticsEngine::Note)
     return;
 
-  SmallVector<tooling::Diagnostic, 1> SuppressionErrors;
-  if (Context.shouldSuppressDiagnostic(DiagLevel, Info, SuppressionErrors,
-                                       EnableNolintBlocks)) {
+  SmallVector<ClangTidyError, 1> SuppressionErrors;
+  if (shouldSuppressDiagnostic(DiagLevel, Info, Context, SuppressionErrors,
+                               EnableNolintBlocks)) {
     ++Context.Stats.ErrorsIgnoredNOLINT;
     // Ignored a warning, should ignore related notes as well
     LastErrorWasIgnored = true;
-    Context.DiagEngine->Clear();
-    for (const auto &Error : SuppressionErrors)
-      Context.diag(Error);
     return;
   }
 

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h

@@ -11,7 +11,6 @@
 
 #include "ClangTidyOptions.h"
 #include "ClangTidyProfiling.h"
-#include "NoLintDirectiveHandler.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Tooling/Core/Diagnostic.h"
 #include "llvm/ADT/DenseMap.h"
@@ -96,33 +95,13 @@ class ClangTidyContext {
   DiagnosticBuilder diag(StringRef CheckName, StringRef Message,
                          DiagnosticIDs::Level Level = DiagnosticIDs::Warning);
 
-  DiagnosticBuilder diag(const tooling::Diagnostic &Error);
+  DiagnosticBuilder diag(const ClangTidyError &Error);
 
   /// Report any errors to do with reading the configuration using this method.
   DiagnosticBuilder
   configurationDiag(StringRef Message,
                     DiagnosticIDs::Level Level = DiagnosticIDs::Warning);
 
-  /// Check whether a given diagnostic should be suppressed due to the presence
-  /// of a "NOLINT" suppression comment.
-  /// This is exposed so that other tools that present clang-tidy diagnostics
-  /// (such as clangd) can respect the same suppression rules as clang-tidy.
-  /// This does not handle suppression of notes following a suppressed
-  /// diagnostic; that is left to the caller as it requires maintaining state in
-  /// between calls to this function.
-  /// If any NOLINT is malformed, e.g. a BEGIN without a subsequent END, an
-  /// error about it will be returned in output \param NoLintErrors.
-  /// If \param AllowIO is false, the function does not attempt to read source
-  /// files from disk which are not already mapped into memory; such files are
-  /// treated as not containing a suppression comment.
-  /// \param EnableNoLintBlocks controls whether to honor NOLINTBEGIN/NOLINTEND
-  /// blocks; if false, only considers line-level disabling.
-  bool
-  shouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel,
-                           const Diagnostic &Info,
-                           SmallVectorImpl<tooling::Diagnostic> &NoLintErrors,
-                           bool AllowIO = true, bool EnableNoLintBlocks = true);
-
   /// Sets the \c SourceManager of the used \c DiagnosticsEngine.
   ///
   /// This is called from the \c ClangTidyCheck base class.
@@ -229,10 +208,30 @@ class ClangTidyContext {
   std::string ProfilePrefix;
 
   bool AllowEnablingAnalyzerAlphaCheckers;
-
-  NoLintDirectiveHandler NoLintHandler;
 };
 
+/// Check whether a given diagnostic should be suppressed due to the presence
+/// of a "NOLINT" suppression comment.
+/// This is exposed so that other tools that present clang-tidy diagnostics
+/// (such as clangd) can respect the same suppression rules as clang-tidy.
+/// This does not handle suppression of notes following a suppressed diagnostic;
+/// that is left to the caller as it requires maintaining state in between calls
+/// to this function.
+/// If `AllowIO` is false, the function does not attempt to read source files
+/// from disk which are not already mapped into memory; such files are treated
+/// as not containing a suppression comment.
+/// \param EnableNolintBlocks controls whether to honor NOLINTBEGIN/NOLINTEND
+/// blocks; if false, only considers line-level disabling.
+/// If suppression is not possible due to improper use of "NOLINT" comments -
+/// for example, the use of a "NOLINTBEGIN" comment that is not followed by a
+/// "NOLINTEND" comment - a diagnostic regarding the improper use is returned
+/// via the output argument `SuppressionErrors`.
+bool shouldSuppressDiagnostic(
+    DiagnosticsEngine::Level DiagLevel, const Diagnostic &Info,
+    ClangTidyContext &Context,
+    SmallVectorImpl<ClangTidyError> &SuppressionErrors, bool AllowIO = true,
+    bool EnableNolintBlocks = true);
+
 /// Gets the Fix attached to \p Diagnostic.
 /// If there isn't a Fix attached to the diagnostic and \p AnyFix is true, Check
 /// to see if exactly one note has a Fix and return it. Otherwise return