Skip to content

Commit

Permalink
[clang-tidy] Cache the locations of NOLINTBEGIN/END blocks
Browse files Browse the repository at this point in the history 
Support for NOLINT(BEGIN/END) blocks (implemented in D108560) is
currently costly. This patch aims to improve the performance with the
following changes:

- The use of tokenized NOLINTs instead of a series of repetitive ad-hoc
string operations (`find()`, `split()`, `slice()`, regex matching etc).
- The caching of NOLINT(BEGIN/END) block locations. Determining these
locations each time a new diagnostic is raised is wasteful as it
requires reading and parsing the entire source file.

Move NOLINT-specific code from `ClangTidyDiagnosticConsumer` to new
purpose-built class `NoLintDirectiveHandler`.

Differential Revision: https://reviews.llvm.org/D116085
  • Loading branch information
@salman-javed-nz
salman-javed-nz committed Jan 26, 2022
1 parent 9b6c2ea commit 19eaad9
Show file tree
Hide file tree
Showing 25 changed files with 752 additions and 334 deletions.
1 change: 1 addition & 0 deletions clang-tools-extra/clang-tidy/CMakeLists.txt
View file
Expand Up @@ -17,6 +17,7 @@ add_clang_library(clangTidy
ClangTidyProfiling.cpp
ExpandModularHeadersPPCallbacks.cpp
GlobList.cpp
NoLintDirectiveHandler.cpp

DEPENDS
ClangSACheckers
Expand Down
230 changes: 17 additions & 213 deletions clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
View file
Expand Up @@ -18,6 +18,7 @@
#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"
Expand Down Expand Up @@ -188,7 +189,7 @@ DiagnosticBuilder ClangTidyContext::diag(
return DiagEngine->Report(ID);
}

DiagnosticBuilder ClangTidyContext::diag(const ClangTidyError &Error) {
DiagnosticBuilder ClangTidyContext::diag(const tooling::Diagnostic &Error) {
SourceManager &SM = DiagEngine->getSourceManager();
llvm::ErrorOr<const FileEntry *> File =
SM.getFileManager().getFile(Error.Message.FilePath);
Expand All @@ -206,6 +207,15 @@ 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);
}
Expand Down Expand Up @@ -307,218 +317,9 @@ 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())
Expand All @@ -545,12 +346,15 @@ void ClangTidyDiagnosticConsumer::HandleDiagnostic(
if (LastErrorWasIgnored && DiagLevel == DiagnosticsEngine::Note)
return;

SmallVector<ClangTidyError, 1> SuppressionErrors;
if (shouldSuppressDiagnostic(DiagLevel, Info, Context, SuppressionErrors,
EnableNolintBlocks)) {
SmallVector<tooling::Diagnostic, 1> SuppressionErrors;
if (Context.shouldSuppressDiagnostic(DiagLevel, Info, 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;
}

Expand Down
47 changes: 24 additions & 23 deletions clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
View file
Expand Up @@ -11,6 +11,7 @@

#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"
Expand Down Expand Up @@ -95,13 +96,33 @@ class ClangTidyContext {
DiagnosticBuilder diag(StringRef CheckName, StringRef Message,
DiagnosticIDs::Level Level = DiagnosticIDs::Warning);

DiagnosticBuilder diag(const ClangTidyError &Error);
DiagnosticBuilder diag(const tooling::Diagnostic &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.
Expand Down Expand Up @@ -208,29 +229,9 @@ class ClangTidyContext {
std::string ProfilePrefix;

bool AllowEnablingAnalyzerAlphaCheckers;
};

/// 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);
NoLintDirectiveHandler NoLintHandler;
};

/// Gets the Fix attached to \p Diagnostic.
/// If there isn't a Fix attached to the diagnostic and \p AnyFix is true, Check
Expand Down

0 comments on commit 19eaad9

Please sign in to comment.