[clang-tidy] Added diagnostics about incorrect usage of NOLINT comment

[xgsa]

Summary

As discussed in the previous review [1], diagnostics about incorrect usage of NOLINT comment was added, i.e..:

• usage of NOLINT with unknown check name;
• usage of NOLINT for line, where there is no diagnostics;
• usage of NOLINT without closing parenthesis.

I have covered the implementation with tests, but I haven't updated the documentation yet, because I'd like to approve the implemented approach in general. If nobody insists, I'd prefer updating the documentation in follow-up patches, because this will make the patch even bigger and the review longer.

[1] - https://reviews.llvm.org/D40671

[xgsa]

A few minor coding style fixes.

[Eugene.Zelenko]

clang-tidy/ClangTidyDiagnosticConsumer.cpp

172

Please separate with empty line.

173

Please include header for SmallVector.h.

328–329

Please use default members initialization for DiagEngine and Profile.

[xgsa]

clang-tidy/ClangTidyDiagnosticConsumer.cpp

328–329

Actually, it is not necessary, as unique_ptr is initialized with nullptr by default. I have just removed initializing them here.

[xgsa]

Review comments were applied.

[xgsa]

clang-tidy/ClangTidyDiagnosticConsumer.cpp

328–329

Sorry, you are right, just ignore my previous comment.

[xgsa]

Review comments applied.

[JVApen]

I'm missing some documentation to understand the corner cases. How does this check behave with suppressed warnings for checks which ain't currently checked. (Using -no-... on a code base or suppressing the warnings via the pragmas)

[xgsa]

In D41326#957749, @JVApen wrote:

I'm missing some documentation to understand the corner cases. How does this check behave with suppressed warnings for checks which ain't currently checked. (Using -no-... on a code base or suppressing the warnings via the pragmas)

The check just gathers NOLINT comments and compares them with a list of reported diagnostics for the current configuration. Thus, it will report diagnostics on the NOLINT comments, which are added for the checks, which are disabled in current configuration (e.g. with option -check=-some_check).

Note also that in spite NOLINT for clang warnings now works for clang-tidy, it doesn't work for clang itself, so I wouldn't recommended using something like "// NOLINT(clang-diagnostic-unused-variable)". As you mentioned, pragmas or -no-... options should be used instead. The check doesn't perform any processing of those things.

[xgsa]

Ping.

[aaron.ballman]

clang-tidy/ClangTidyDiagnosticConsumer.cpp

23–24

Please keep the includes in sorted order.

134

We don't typically mark local, non-pointer types as const.

163

The & should bind to the right. However, instead of returning the concrete container, why not expose a range interface instead?

265

Spurious space after operator(). i does not match our coding conventions (and could use a better name).

270–271

The parameter names don't match our coding conventions.

277

Mark the constructor explicit?

280

Is there a better LLVM ADT to use here?

320

I'd reword this slightly to: "unknown check name '%0' specified in %1 directive"

329–330

How about: "unbalanced parentheses in %0 comment; all diagnostics silenced for this line" ?

369

Name does not match coding standard.

412

Making the cache volatile will have no impact on this.

Any reason not to make the cache mutable, however? That's quite common for implementation details.

485

Formatting.

556

s/change/chance?

Also: if diagnostics is filtered -> if diagnostics are filtered

829–830

I don't think the user is going to care about the distinction between no diagnostics being triggered and the expected diagnostic not being triggered. Also, it's dangerous to claim the lint comment is redundant because it's possible the user has NOLINT(foo, bar) and while foo is not triggered, bar still is. The NOLINT comment itself isn't redundant, it's that the check specified doesn't occur.

I would consolidate those scenarios into a single diagnostic: "expected diagnostic '%0' not generated" and "expected diagnostic '%0' not generated for the following line".

One concern I have with this functionality is: how should users silence a lint diagnostic that's target sensitive? e.g., a diagnostic that triggers based on the underlying type of size_t or the signedness of plain char. In that case, the diagnostic may trigger for some targets but not others, but on the targets where the diagnostic is not triggered, they now get a diagnostic they cannot silence. There should be a way to silence the "bad NOLINT" diagnostics.

clang-tidy/ClangTidyDiagnosticConsumer.h

29

Please add a newline after this.

146

This function can be marked const.

206

Comma after storeError()

299

Perhaps this would make more sense as an llvm::IndexedMap or other non-STL datatype?

[xgsa]

Aaron, thank you for your review and sorry for the coding convention mistakes -- I still cannot get used to the llvm coding convention, because it quite differs from the one I have been using in my projects.

clang-tidy/ClangTidyDiagnosticConsumer.cpp

280

This data structure provides the fast lookup by check name+line number and it's exactly what is necessary. What are the concerns about this data structure?

320

I'd used the word "comment" instead of "directive" for consistency with documentation and the other messages. The rest is applied.

412

Sorry, certainly, instead of "volatile" I meant "mutable".

Actually, using of "mutable" violates a constancy contract, as the field is get modified in a const method. Thus I'd tend to avoid using mutable, if possible, because e.g. in multi-threaded applications these fields require additional protection/synchronization. Moreover, I see that using of mutable is not very spread in clang-tidy. Thus as, currently, hasCheck is called from the non-constant context, I'd prefer leaving it non-const instead of making cache mutable. Please, let me know if you insist on the mutable option.

829–830

I don't think the user is going to care about the distinction between no diagnostics being triggered and the expected diagnostic not being triggered. Also, it's dangerous to claim the lint comment is redundant because it's possible the user has NOLINT(foo, bar) and while foo is not triggered, bar still is. The NOLINT comment itself isn't redundant, it's that the check specified doesn't occur.

I would consolidate those scenarios into a single diagnostic: "expected diagnostic '%0' not generated" and "expected diagnostic '%0' not generated for the following line".

This branch of if (NolintEntry.first.CheckName == NolintCommentsCollector::AnyCheck) reports only about NOLINT/NOLINTNEXTLINE comments without check list, so I suppose it's fair to claim that this comment is redundant (we have already checked that no single check reported diagnostics on the line). The else-branch reports the diagnostics for the definite check in a list in case of NOLINT(foo, bar) (actually, if neither foo nor bar checks reported diagnostics for the line, there will be a few diagnostics from nolint-usage - not sure if it's good, but it seems acceptable). That is why, I suppose, it is necessary to distinct these cases.

One concern I have with this functionality is: how should users silence a lint diagnostic that's target sensitive? e.g., a diagnostic that triggers based on the underlying type of size_t or the signedness of plain char. In that case, the diagnostic may trigger for some targets but not others, but on the targets where the diagnostic is not triggered, they now get a diagnostic they cannot silence. There should be a way to silence the "bad NOLINT" diagnostics.

There is such mechanism: it is possible to specify // NOLINT(nolint-usage) or //NOLINT(check1, check2, nolint-usage) to silence the nolint-usage`-mechanism. Please, see tests for details and more examples.

clang-tidy/ClangTidyDiagnosticConsumer.h

299

I think, the map of vectors fits well: it provides efficient adding, storing and iterating through elements. In spite items in vector can be duplicated, it's not a usual case (when there are a lot of diagnostics for the same check on the same line), so I don't think it worth using set. I don't see how llvm::IndexedMap could help here. What are the concerns about the current data structure?

[xgsa]

Review comments applied.

[xgsa]

The full diff (but not only the incremental one) was uploaded. Please, skip previous revision. Sorry.

[aaron.ballman]

clang-tidy/ClangTidyDiagnosticConsumer.cpp

163

This could use a typedef to make the return type more readable. Also, the function should be checkName(), dropping the "get".

280

Same as above.

369

Please do not name the parameter the same as a type name.

412

Use of mutable does not violate constancy; the cache is not exposed via any interface; it is purely an implementation detail. Very little of our code base is concerned with multithreaded scenarios (we use bit-fields *everywhere*, for instance).

I won't insist on using mutable if you are set against it, but this is the exact scenario in which it is the correct solution.

829–830

Can you provide an example where this distinction will make a difference to the user and help clarify a confusing situation? I cannot think of one, and it would be nice to simplify this code.

Thank you for the explanation about nolint-usage. This is not terminology I've seen before -- is this your invention, or is it a documented feature for NOLINT comments?

clang-tidy/ClangTidyDiagnosticConsumer.h

299

STL data structures are not particularly well-optimized; they're meant for general usage cases. This map can get very large in real projects, so I worry about performance concerns. That's why we have specialized ADTs -- you should generally prefer them to STL containers.

[xgsa]

clang-tidy/ClangTidyDiagnosticConsumer.cpp

280

I have reviewed llvm guide [1] and found that it recommends using ordered vector in such cases. I have implemented this approach, however I'd like to notice that lookup complexity in reportRedundantNolintComments() increased from average O(1) for std::unordered_map to O(log(N)) for binary search. However, memory usage become less. As this collection is gathered for each translation unit, I don't expect millions of NOLINT comments in it, thus this approach looks suitable.

[1] - http://llvm.org/docs/ProgrammersManual.html#picking-the-right-data-structure-for-a-task

412

I've just tried the mutalbe-approach and discovered one more issue: ClangTidyASTConsumerFactory requires non constant ClangTidyContext. Thus, if current approach is not suitable, either ClangTidyASTConsumerFactory should be refactored on const and nonconst parts or caching should be done in constructor (which makes fetching the names not lazy). Because of this, currently I am suggesting to leave hasCheck nonconstant.

829–830

Can you provide an example where this distinction will make a difference to the user and help clarify a confusing situation? I cannot think of one, and it would be nice to simplify this code.

Example for the diagnostics emitted in the if-branch:

class A2 { explicit A2(int i); }; // NOLINT
=> warning: there is no diagnostics on this line, the NOLINT comment is redundant

Thus, the whole NOLINT comment should be removed.

Example for the diagnostics emitted in the else-branch:

// Case: NO_LINT for the specific check on line with an error on another check.
class A4 { A4(int i); }; // NOLINT(misc-unused-parameters, google-explicit-constructor)
=> warning: there is no diagnostics for the 'misc-unused-parameters' check on this line, the NOLINT comment is redundant

In this case, only misc-unused-parameters check should be removed from the list, but not the whole NOLINT.

Note also that if in the last example there is only misc-unused-parameters in the NOLINT checks list, the diagnostics will be the same. However, I suppose it's acceptable, because even if a user removed only a check name from the check list without removing NOLINT itself, clang-tidy will suggest removing NOLINT itself on the next run.

Thank you for the explanation about nolint-usage. This is not terminology I've seen before -- is this your invention, or is it a documented feature for NOLINT comments?

It's my invention, I suppose, from the user point of view it's logical to consider this mechanism as a check.

clang-tidy/ClangTidyDiagnosticConsumer.h

299

I have reviewed llvm guide [1] and haven't found more suitable ADT, except std::map, which provides O(log(N)) complexity for lookup and insert against average O(1) for std::unordered_map. llvm::IndexedMap doesn't look suitable, as there could be a lot of lines in the file, whereas only a few will contain NOLINT comment, so llvm::IndexedMap won't store data efficiently.

Moreover, as far as I understand, this collection stores data for the translation unit and is cleaned up after it is processed, so there shouldn't be millions of diagnostics line numbers stored here.

Thus, I'd prefer leaving the container type as is to optimize for performance, but I would be also OK with std::map to optimize for memory usage.

[1] - http://llvm.org/docs/ProgrammersManual.html#picking-the-right-data-structure-for-a-task

[xgsa]

Review comments applied.

[aaron.ballman]

clang-tidy/ClangTidyDiagnosticConsumer.cpp

412

Ah, okay, that makes sense to me -- thank you for looking into it, I think how you have it is reasonable enough then.

829–830

Thank you for the examples, but I don't think they would add any clarification for the user, either. That said, I think we could collapse all four diagnostics into: "there are no diagnostics %select{|for '%2'}0 on %select{this|the next}1 line" where %0 distinguishes between NOLINT with and without an explicit check name, %1 distinguishes between NOLINT and NOLINTNEXTLINE, and %2 is the name of the check in question (if any).

Thank you for the explanation about nolint-usage. This is not terminology I've seen before -- is this your invention, or is it a documented feature for NOLINT comments?

It's my invention, I suppose, from the user point of view it's logical to consider this mechanism as a check.

I think we should devise a better way of expressing this unless there's industry practice suggesting a specific term or syntax. nolint-usage isn't very discoverable. Whatever we wind up with, it should work naturally for constructs like your first example. In the first example, diagnosing that NOLINT is redundant only makes sense in some circumstances, such as when the construct is in a source file that is not compiled with google-explicit-constructor enabled. However, if the code is in a shared header file where some (but not all) source code is checked with google-explicit-constructor enabled, it serves a purpose and removing it would be incorrect. The same holds for NOLINT(check).

Ultimately, I think that diagnosing NOLINT on a line that issues no diagnostics should not be enabled by default. It's highly likely that the NOLINT directive is due to reasons unknowable to the implementation, and suggesting the user remove the comment seems unhelpful. My example above is one plausible case where NOLINT may serve a purpose. Another example is code checked by clang-tidy as well as other tools; the NOLINT may be silencing the other tools. I think it should take extra work by the user to enable this functionality in a catch-all manner -- either via different syntax or enabled only through an explicit flag.

I think that diagnosing NOLINT(check-name) when that diagnostic is not triggered is useful by default because the user is explicitly expecting a diagnostic by name. However, I think the user needs an almost-trivial way to silence the "not triggered" diagnostic on a case-by-case basis. Target specific diagnostics as well as shared code are both examples of situations where the NOLINT comment may be accurate but for situations unknown to clang-tidy. Telling the user to remove the NOLINT comment would be the wrong thing to do.

clang-tidy/ClangTidyDiagnosticConsumer.h

299

Being single-TU doesn't really matter much for number of lines processed -- generated code is quite common. That said, we can leave it until performance concerns become a problem in practice.

[xgsa]

clang-tidy/ClangTidyDiagnosticConsumer.cpp

829–830

If to look at cpplint, which I suppose, NOLINT syntax was taken from, it also reports diagnostics for incorrect usage of NOLINT as messages in category 'readability/nolint' [1].

In spite of the fact, that nolint-usage is technically not a check, it mimics the behavior of a simple check: it is only enabled if nolint-usage is enabled via -check option (explicitly or via *) or configuration; it reports diagnostics for this category and allows this diagnostics to be suppressed via NOLINT. The only missing thing for now is documentation page for the nolint-usage check and I am going to work on this after this patch is approved. Thus, I don't think that this approach is not discoverable and it seems more natural than adding a separate command line option and a separate way of suppressing NOLINT diagnostics.

I agree that this diagnostics will only be useful if NOLINT comments are carefully maintained exactly for clang-tidy. I don't think that if they are also added for another tool, the check names or even diagnostics lines will match. Thus, for these cases the check just shouldn't be enabled in configuration. The same thing is for headers. I suppose, that typically the list of checks is the same for the whole project, thus diagnostics for headers won't be an issue, but if it is not the case, the nolint-usage check could just be disabled for some modules. Anyway, I don't see the way to determine if a header contains the diagnostics been included from the other source files.

If it is necessary, some configuration options could be added for this check (e.g. an option to check only NOLINT comments in source files, but no headers; or to check only NOLINT comments with/without explicit list of checks specification).

[1] - https://github.com/google/styleguide/blob/9663cabfeeea8f1307b1acde59471f74953b8fa9/cpplint/cpplint.py#L577

[xgsa]

Rename the check nolint-usage => readability-nolint-usage for consistency.
Update diagnostics message according to the review comments.

[aaron.ballman]

clang-tidy/ClangTidyDiagnosticConsumer.cpp

118

Why "readability"? That module is generally for code readability issues like redundant code or naming conventions. I think this should probably be in "misc" or perhaps "bugprone".

829–830

In spite of the fact, that nolint-usage is technically not a check

That's part of what concerns me about this in terms of discoverability and usability -- it's not a real check but it sort of behaves like one. Does this get listed with -list-checks? If the user passes -nolint-* on the command line does it still get disabled?

Also, all of this is predicated by the fact that these nolint warnings are going to trigger in such a way that sometimes needs to be silenced. What I think isn't very intuitive is that the recommended way to silence a nolint-comment-related diagnostic is to use another nolint comment to disable reporting bugs with nolint comments. However, documentation can help with that, so perhaps it's not bad.

[xgsa]

clang-tidy/ClangTidyDiagnosticConsumer.cpp

118

I consider this check as a detector of redundant comments (i.e. redundant code). Moreover, it seems, these comments influence only on code readability and don't bring bugs, so "bugprone" doesn't look good for me. One more minor argument for "readability" is that it is in such category in cpplint, so it will be more natural for its users. Summing up, I am for leaving the check in "readability", but I don't mind also putting it in "misc".

829–830

Does this get listed with -list-checks?

Not yet, I missed that, but according to my understanding, the "nolint-usage" should be in the list. I'll fix it.

If the user passes -nolint-* on the command line does it still get disabled?

It should work (there is the isCheckEnabled call in ClangTidyContext::setPreprocessor, so if no comments are collected, no checks will be done and no diagnostics reported).

What I think isn't very intuitive is that the recommended way to silence a nolint-comment-related diagnostic is to use another nolint comment to disable reporting bugs with nolint comments.

Could you please suggest a more intuitive way as an example?

However, documentation can help with that, so perhaps it's not bad.

So is documentation the only concern about this patch or don't you like the overall idea?

[xgsa]

Fixed showing the check in -list-checks.

[alexfh]

A high-level comment: NOLINT category parsing and warning on incorrect NOLINT categories makes it more difficult for code to comply both with clang-tidy and cpplint (https://github.com/cpplint/cpplint), since:

1. the category names in these tools are different,
2. cpplint currently doesn't support more than one category with the // NOLINT(category) syntax,
3. cpplint complains on // NOLINT directives with an unknown category.

So when suppressing a warning that is present in both clang-tidy and cpplint (e.g. google-runtime-int) one has to use // NOLINT without categories, which kind of undermines the effort of allowing category names explicitly specified in suppression directives.

There are multiple solutions possible. One would be to add a clang-tidy-specific suppression directive and only support specifying check names in it (// NOCLANGTIDY seems verbose, but I don't have a better name. Suggestions are welcome.). Leaving support for // NOLINT without category names would be nice for suppressing warnings common with cpplint.

Another option would be changing cpplint to somehow recognize and ignore clang-tidy check names in the suppressed categories list.

clang-tidy/ClangTidy.cpp

402

clang-format, please

clang-tidy/ClangTidy.cpp

for (const auto &Opt : Opts.CheckOptions) {
     if (!OptName.startswith(AnalyzerPrefix))
       continue;
     AnalyzerOptions->Config[OptName.substr(AnalyzerPrefix.size())] = Opt.second;
   }
 }
 
 typedef std::vector<std::pair<std::string, bool>> CheckersList;
 
-static CheckersList getCheckersControlList(ClangTidyContext &Context) {
+static CheckersList getCheckersControlList(const ClangTidyContext &Context) {
   CheckersList List;
 
   const auto &RegisteredCheckers =
       AnalyzerOptions::getRegisteredCheckers(/*IncludeExperimental=*/false);
   bool AnalyzerChecksEnabled = false;
   for (StringRef CheckName : RegisteredCheckers) {
     std::string ClangTidyCheckName((AnalyzerCheckNamePrefix + CheckName).str());
     AnalyzerChecksEnabled |= Context.isCheckEnabled(ClangTidyCheckName);

 std::unique_ptr<clang::ASTConsumer>
 ClangTidyASTConsumerFactory::CreateASTConsumer(
     clang::CompilerInstance &Compiler, StringRef File) {
   // FIXME: Move this to a separate method, so that CreateASTConsumer doesn't
   // modify Compiler.
   Context.setSourceManager(&Compiler.getSourceManager());
   Context.setCurrentFile(File);
   Context.setASTContext(&Compiler.getASTContext());
+  Context.setPreprocessor(&Compiler.getPreprocessor());
 
   auto WorkingDir = Compiler.getSourceManager()
                         .getFileManager()
                         .getVirtualFileSystem()
                         ->getCurrentWorkingDirectory();
   if (WorkingDir)
     Context.setCurrentBuildDirectory(WorkingDir.get());
 

 
 std::vector<std::string> ClangTidyASTConsumerFactory::getCheckNames() {
   std::vector<std::string> CheckNames;
   for (const auto &CheckFactory : *CheckFactories) {
     if (Context.isCheckEnabled(CheckFactory.first))
       CheckNames.push_back(CheckFactory.first);
   }
 
+  if (Context.isCheckEnabled(NolintCheckName))
+        CheckNames.push_back(NolintCheckName);
+
   for (const auto &AnalyzerCheck : getCheckersControlList(Context))
     CheckNames.push_back(AnalyzerCheckNamePrefix + AnalyzerCheck.first);
 
   std::sort(CheckNames.begin(), CheckNames.end());
   return CheckNames;
 }
 
 ClangTidyOptions::OptionMap ClangTidyASTConsumerFactory::getCheckOptions() {

clang-tidy/ClangTidyDiagnosticConsumer.h

 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Tooling/Core/Diagnostic.h"
 #include "clang/Tooling/Refactoring.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/Regex.h"
 #include "llvm/Support/Timer.h"
+#include <unordered_map>
+#include <vector>
 
 namespace clang {
 
 class ASTContext;
 class CompilerInstance;
+class Preprocessor;
+
 namespace ast_matchers {
 class MatchFinder;
 }
 namespace tooling {
 class CompilationDatabase;
 }
 
 namespace tidy {
+class NolintCommentsCollector;
 
 /// \brief A detected error complete with information to display diagnostic and
 /// automatic fix.
 ///
 /// This is used as an intermediate format to transport Diagnostics without a
 /// dependency on a SourceManager.
 ///
 /// FIXME: Make Diagnostics flexible enough to support this directly.
public:
   void setCurrentFile(StringRef File);
 
   /// \brief Returns the main file name of the current translation unit.
   StringRef getCurrentFile() const { return CurrentFile; }
 
   /// \brief Sets ASTContext for the current translation unit.
   void setASTContext(ASTContext *Context);
 
+  /// \brief Subscribes the context to the preprocessor events if necessary.
+  void setPreprocessor(Preprocessor *preprocessor);
+
   /// \brief Gets the language options from the AST context.
   const LangOptions &getLangOpts() const { return LangOpts; }
 
+  /// \brief Checks if a check with the specified name exists.
+  bool hasCheck(StringRef CheckName);
+
   /// \brief Returns the name of the clang-tidy check which produced this
   /// diagnostic ID.
   StringRef getCheckName(unsigned DiagnosticID) const;
 
+  /// \brief Returns the name of diagnostics as it will be output in report
+  /// for the specified diagnostic ID. It can be either a check name or
+  /// clang diagnostics with "clang-diagnostic-" prefix.
+  std::string getDiagnosticsName(unsigned DiagnosticID) const;
+
   /// \brief Returns \c true if the check is enabled for the \c CurrentFile.
   ///
   /// The \c CurrentFile can be changed using \c setCurrentFile.
   bool isCheckEnabled(StringRef CheckName) const;
 
   /// \brief Returns \c true if the check should be upgraded to error for the
   /// \c CurrentFile.
   bool treatAsError(StringRef CheckName) const;
public:
   }
 
   /// \brief Returns build directory of the current translation unit.
   const std::string &getCurrentBuildDirectory() {
     return CurrentBuildDirectory;
   }
 
 private:
-  // Calls setDiagnosticsEngine() and storeError().
+  // Calls setDiagnosticsEngine(), storeError(), and getNolintCollector().
   friend class ClangTidyDiagnosticConsumer;
   friend class ClangTidyPluginAction;
 
   /// \brief Sets the \c DiagnosticsEngine so that Diagnostics can be generated
   /// correctly.
   void setDiagnosticsEngine(DiagnosticsEngine *Engine);
 
   /// \brief Store an \p Error.
   void storeError(const ClangTidyError &Error);
 
+  // \brief Provides access to the \c NOLINT/NOLINTNEXTLINE info collector.
+  NolintCommentsCollector &getNolintCollector();
+
   std::vector<ClangTidyError> Errors;
-  DiagnosticsEngine *DiagEngine;
+  DiagnosticsEngine *DiagEngine = nullptr;
   std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider;
 
   std::string CurrentFile;
   ClangTidyOptions CurrentOptions;
   class CachedGlobList;
   std::unique_ptr<CachedGlobList> CheckFilter;
   std::unique_ptr<CachedGlobList> WarningAsErrorFilter;
 
   LangOptions LangOpts;
 
   ClangTidyStats Stats;
 
   std::string CurrentBuildDirectory;
 
   llvm::DenseMap<unsigned, std::string> CheckNamesByDiagnosticID;
 
-  ProfileData *Profile;
+  ProfileData *Profile = nullptr;
+
+  std::unique_ptr<NolintCommentsCollector> NolintCollector;
+  std::vector<std::string> AllCheckNamesCache;
 };
 
 /// \brief A diagnostic consumer that turns each \c Diagnostic into a
 /// \c SourceManager-independent \c ClangTidyError.
 //
 // FIXME: If we move away from unit-tests, this can be moved to a private
 // implementation file.
 class ClangTidyDiagnosticConsumer : public DiagnosticConsumer {
 public:
   ClangTidyDiagnosticConsumer(ClangTidyContext &Ctx,
                               bool RemoveIncompatibleErrors = true);
 
   // FIXME: The concept of converting between FixItHints and Replacements is
   // more generic and should be pulled out into a more useful Diagnostics
   // library.
   void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
                         const Diagnostic &Info) override;
 
   /// \brief Flushes the internal diagnostics buffer to the ClangTidyContext.
   void finish() override;
 
 private:
   void finalizeLastError();
 
+  bool isLineMarkedWithNolintInMacro(SourceManager &SM, SourceLocation Loc,
+                                     StringRef CheckName);
+  bool lineIsMarkedWithNolint(SourceManager &SM, SourceLocation Loc,
+                              StringRef CheckName);
+  bool isNolintFound(StringRef NolintDirectiveText, StringRef Line,
+                     StringRef CheckName);
+
   void removeIncompatibleErrors(SmallVectorImpl<ClangTidyError> &Errors) const;
 
+  void reportRedundantNolintComments();
+
   /// \brief Returns the \c HeaderFilter constructed for the options set in the
   /// context.
   llvm::Regex *getHeaderFilter();
 
   /// \brief Updates \c LastErrorRelatesToUserCode and LastErrorPassesLineFilter
   /// according to the diagnostic \p Location.
   void checkFilters(SourceLocation Location);
   bool passesLineFilter(StringRef FileName, unsigned LineNumber) const;
 
   ClangTidyContext &Context;
   bool RemoveIncompatibleErrors;
   std::unique_ptr<DiagnosticsEngine> Diags;
   SmallVector<ClangTidyError, 8> Errors;
+  // Unfortunately, line numbers are not stored in Errors, so gather them
+  // separately to process NOLINT misuse.
+  std::unordered_map<unsigned, std::vector<unsigned>> DiagIDsToLineNumbers;
   std::unique_ptr<llvm::Regex> HeaderFilter;
-  bool LastErrorRelatesToUserCode;
-  bool LastErrorPassesLineFilter;
-  bool LastErrorWasIgnored;
+  bool LastErrorRelatesToUserCode = false;
+  bool LastErrorPassesLineFilter = false;
+  bool LastErrorWasIgnored = false;
 };
 
+extern const StringRef NolintCheckName;
 } // end namespace tidy
 } // end namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDYDIAGNOSTICCONSUMER_H

clang-tidy/ClangTidyDiagnosticConsumer.cpp

 ///  and ClangTidyError classes.
 ///
 ///  This tool uses the Clang Tooling infrastructure, see
 ///    http://clang.llvm.org/docs/HowToSetupToolingForLLVM.html
 ///  for details on setting it up with LLVM source tree.
 ///
 //===----------------------------------------------------------------------===//
 
+#include "ClangTidy.h"
 #include "ClangTidyDiagnosticConsumer.h"
+#include "ClangTidyModule.h"
 #include "ClangTidyOptions.h"
 #include "clang/AST/ASTDiagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Frontend/DiagnosticRenderer.h"
+#include "clang/Lex/Preprocessor.h"
+#include "llvm/ADT/Hashing.h"
+#include "llvm/ADT/iterator_range.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/SmallVector.h"
+#include <algorithm>
 #include <tuple>
 #include <vector>
 using namespace clang;
 using namespace tidy;
 
+const StringRef clang::tidy::NolintCheckName = "readability-nolint-usage";
 namespace {
 class ClangTidyDiagnosticRenderer : public DiagnosticRenderer {
 public:
   ClangTidyDiagnosticRenderer(const LangOptions &LangOpts,
                               DiagnosticOptions *DiagOpts,
                               ClangTidyError &Error)
       : DiagnosticRenderer(LangOpts, DiagOpts), Error(Error) {}
 
protected:
   void endDiagnostic(DiagOrStoredDiag D,
                      DiagnosticsEngine::Level Level) override {
     assert(!Error.Message.Message.empty() && "Message has not been set");
   }
 
 private:
   ClangTidyError &Error;
 };
+
+const StringRef ClangDiagnosticsPrefix = "clang-diagnostic-";
+enum class NolintCommentType : uint8_t {
+  Nolint,
+  NolintNextLine,
+};
+const StringRef NolintComment = "NOLINT";
+const StringRef NolintNextLineComment = "NOLINTNEXTLINE";
+
+class NolintCommentParser {
+public:
+  using CheckNamesRange =
+      llvm::iterator_range<SmallVectorImpl<StringRef>::const_iterator>;
+
+  NolintCommentParser(StringRef NolintDirectiveText, StringRef Line) {
+    if ((NolintIndex = Line.find(NolintDirectiveText)) != StringRef::npos) {
+      size_t BracketIndex = NolintIndex + NolintDirectiveText.size();
+      // Check if the specific checks are specified in brackets.
+      if (BracketIndex < Line.size() && Line[BracketIndex] == '(') {
+        ++BracketIndex;
+        size_t BracketEndIndex = Line.find(')', BracketIndex);
+        if ((ClosingBracketFound = BracketEndIndex != StringRef::npos)) {
+          StringRef ChecksStr =
+              Line.substr(BracketIndex, BracketEndIndex - BracketIndex);
+          // Allow disabling all the checks with "*".
+          if (ChecksStr != "*") {
+            // Allow specifying a few check names, delimited with comma.
+            ChecksStr.split(CheckNames, ',', -1, false);
+            llvm::transform(CheckNames, CheckNames.begin(),
+                            [](StringRef S) { return S.trim(); });
+          }
+        }
+      }
+    }
+  }
+
+  bool isNolintFound() const {
+    return NolintIndex != StringRef::npos;
+  }
+
+  size_t getNolintIndex() const {
+    assert(isNolintFound());
+    return NolintIndex;
+  }
+
+  bool isAppliedToAllChecks() const {
+    return CheckNames.empty();
+  }
+
+  CheckNamesRange сheckNames() const {
+    assert(isNolintFound());
+    assert(!isAppliedToAllChecks());
+    return llvm::make_range(CheckNames.begin(), CheckNames.end());
+  }
+
+  bool isClosingBracketFound() const {
+    return ClosingBracketFound;
+  }
+
+private:
+  SmallVector<StringRef, 1> CheckNames;
+  size_t NolintIndex = 0;
+  bool ClosingBracketFound = true;
+};
 } // end anonymous namespace
 
 ClangTidyError::ClangTidyError(StringRef CheckName,
                                ClangTidyError::Level DiagLevel,
                                StringRef BuildDirectory, bool IsWarningAsError)
     : tooling::Diagnostic(CheckName, DiagLevel, BuildDirectory),
       IsWarningAsError(IsWarningAsError) {}
 
public:
   }
 
 private:
   GlobList Globs;
   enum Tristate { None, Yes, No };
   llvm::StringMap<Tristate> Cache;
 };
 
+class clang::tidy::NolintCommentsCollector : public CommentHandler {
+public:
+  struct NolintInfo {
+    unsigned LineNumber;
+    StringRef CheckName;
+    SourceLocation Location;
+    NolintCommentType Type;
+    bool isActual;
+  };
+  using NolintInfos = std::vector<NolintInfo>;
+
+  static const StringRef AnyCheck;
+
+  explicit NolintCommentsCollector(ClangTidyContext *Context)
+    : Context(Context) {}
+
+  NolintInfos takeNolintInfos() {
+    NolintInfos result;
+    std::swap(result, NolintComments);
+    return result;
+  }
+
+private:
+  bool HandleComment(Preprocessor &PP, SourceRange Range) override {
+    SourceManager &SrcMgr = PP.getSourceManager();
+    StringRef Text = Lexer::getSourceText(CharSourceRange::getCharRange(Range),
+                                          SrcMgr, PP.getLangOpts());
+    // Search for NOLINTNEXTLINE first (as NOLINT could be a part of it).
+    StringRef CommentName = NolintNextLineComment;
+    NolintCommentType CommentType = NolintCommentType::NolintNextLine;
+    unsigned LineLocationOffset = 1;
+    NolintCommentParser NolintParser(CommentName, Text);
+    if (!NolintParser.isNolintFound()) {
+      // Search for NOLINT otherwise.
+      CommentName = NolintComment;
+      CommentType = NolintCommentType::Nolint;
+      LineLocationOffset = 0;
+      NolintParser = NolintCommentParser(CommentName, Text);
+    }
+    if (NolintParser.isNolintFound()) {
+      SourceLocation NolintLoc =
+          Range.getBegin().getLocWithOffset(NolintParser.getNolintIndex());
+      unsigned LineNo =
+          SrcMgr.getExpansionLineNumber(Range.getBegin()) + LineLocationOffset;
+      static const SmallVector<StringRef, 1> AllChecks = { AnyCheck };
+      for (StringRef CheckName : NolintParser.isAppliedToAllChecks() ?
+            AllChecks : NolintParser.сheckNames()) {
+        // Don't report diagnostics to itself.
+        if (CheckName == NolintCheckName)
+          continue;
+        if (!CheckName.empty() && !Context->hasCheck(CheckName) &&
+            !CheckName.startswith(ClangDiagnosticsPrefix)) {
+          Context->diag(NolintCheckName, NolintLoc,
+              "unknown check name '%0' specified in %1 comment")
+                  << CheckName << CommentName;
+        } else {
+          NolintComments.emplace_back(
+              NolintInfo{LineNo, CheckName, NolintLoc, CommentType, true});
+        }
+      }
+      if (!NolintParser.isClosingBracketFound()) {
+        Context->diag(NolintCheckName, NolintLoc,
+            "unbalanced parentheses in %0 comment; all diagnostics silenced "
+            "for this line")
+                << CommentName;
+      }
+    }
+    return false;
+  }
+
+private:
+  NolintInfos NolintComments;
+  ClangTidyContext *Context;
+};
+const StringRef NolintCommentsCollector::AnyCheck;
+
 ClangTidyContext::ClangTidyContext(
     std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider)
-    : DiagEngine(nullptr), OptionsProvider(std::move(OptionsProvider)),
-      Profile(nullptr) {
+    : OptionsProvider(std::move(OptionsProvider)),
+      NolintCollector(llvm::make_unique<NolintCommentsCollector>(this)) {
   // Before the first translation unit we can get errors related to command-line
   // parsing, use empty string for the file name in this case.
   setCurrentFile("");
 }
 
 ClangTidyContext::~ClangTidyContext() = default;
 
 DiagnosticBuilder ClangTidyContext::diag(
WarningAsErrorFilter =
       llvm::make_unique<CachedGlobList>(*getOptions().WarningsAsErrors);
 }
 
 void ClangTidyContext::setASTContext(ASTContext *Context) {
   DiagEngine->SetArgToStringFn(&FormatASTNodeDiagnosticArgument, Context);
   LangOpts = Context->getLangOpts();
 }
 
+void ClangTidyContext::setPreprocessor(Preprocessor *ActualPreprocessor) {
+  if (ActualPreprocessor && isCheckEnabled(NolintCheckName)) {
+    ActualPreprocessor->addCommentHandler(NolintCollector.get());
+  }
+}
+
 const ClangTidyGlobalOptions &ClangTidyContext::getGlobalOptions() const {
   return OptionsProvider->getGlobalOptions();
 }
 
 const ClangTidyOptions &ClangTidyContext::getOptions() const {
   return CurrentOptions;
 }
 
bool ClangTidyContext::treatAsError(StringRef CheckName) const {
   return WarningAsErrorFilter->contains(CheckName);
 }
 
 /// \brief Store a \c ClangTidyError.
 void ClangTidyContext::storeError(const ClangTidyError &Error) {
   Errors.push_back(Error);
 }
 
+NolintCommentsCollector &ClangTidyContext::getNolintCollector() {
+  return *NolintCollector;
+}
+
+// This method is not const, because it fills cache on first demand.
+// It is possible to fill the cache in constructor, but as it is not always
+// used, so it is preferred to do it lazily.
+// Making cache mutable is not enough, because creation of
+// ClangTidyASTConsumerFactory also requires nonconstant ClangTidyContext.
+bool ClangTidyContext::hasCheck(StringRef CheckName) {
+  if (AllCheckNamesCache.empty()) {
+    ClangTidyASTConsumerFactory Factory(*this);
+    AllCheckNamesCache = Factory.getCheckNames();
+    assert(std::is_sorted(AllCheckNamesCache.begin(), AllCheckNamesCache.end()));
+  }
+  return std::binary_search(AllCheckNamesCache.begin(),
+                            AllCheckNamesCache.end(), CheckName);
+}
+
 StringRef ClangTidyContext::getCheckName(unsigned DiagnosticID) const {
   llvm::DenseMap<unsigned, std::string>::const_iterator I =
       CheckNamesByDiagnosticID.find(DiagnosticID);
   if (I != CheckNamesByDiagnosticID.end())
     return I->second;
   return "";
 }
 
+std::string ClangTidyContext::getDiagnosticsName(unsigned DiagnosticID) const {
+  StringRef WarningOption =
+      DiagEngine->getDiagnosticIDs()->getWarningOptionForDiag(DiagnosticID);
+  return !WarningOption.empty()
+      ? (ClangDiagnosticsPrefix + WarningOption).str()
+      : getCheckName(DiagnosticID).str();
+}
+
 ClangTidyDiagnosticConsumer::ClangTidyDiagnosticConsumer(
     ClangTidyContext &Ctx, bool RemoveIncompatibleErrors)
-    : Context(Ctx), RemoveIncompatibleErrors(RemoveIncompatibleErrors),
-      LastErrorRelatesToUserCode(false), LastErrorPassesLineFilter(false),
-      LastErrorWasIgnored(false) {
+    : Context(Ctx), RemoveIncompatibleErrors(RemoveIncompatibleErrors) {
   IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
   Diags = llvm::make_unique<DiagnosticsEngine>(
       IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs), &*DiagOpts, this,
       /*ShouldOwnClient=*/false);
   Context.setDiagnosticsEngine(Diags.get());
 }
 
 void ClangTidyDiagnosticConsumer::finalizeLastError() {
if (!Errors.empty()) {
     } else {
       ++Context.Stats.ErrorsDisplayed;
     }
   }
   LastErrorRelatesToUserCode = false;
   LastErrorPassesLineFilter = false;
 }
 
-static bool IsNOLINTFound(StringRef NolintDirectiveText, StringRef Line,
-                          unsigned DiagID, const ClangTidyContext &Context) {
-  const size_t NolintIndex = Line.find(NolintDirectiveText);
-  if (NolintIndex == StringRef::npos)
+bool ClangTidyDiagnosticConsumer::isNolintFound(StringRef NolintDirectiveText,
+                                                StringRef Line,
+                                                StringRef CheckName) {
+  NolintCommentParser NolintParser(NolintDirectiveText, Line);
+  if (!NolintParser.isNolintFound()) {
     return false;
-
-  size_t BracketIndex = NolintIndex + NolintDirectiveText.size();
-  // Check if the 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);
-      // Allow disabling all the checks with "*".
-      if (ChecksStr != "*") {
-        StringRef CheckName = Context.getCheckName(DiagID);
-        // Allow specifying a few check names, delimited with comma.
-        SmallVector<StringRef, 1> Checks;
-        ChecksStr.split(Checks, ',', -1, false);
-        llvm::transform(Checks, Checks.begin(),
-                        [](StringRef S) { return S.trim(); });
-        return llvm::find(Checks, CheckName) != Checks.end();
-      }
-    }
-  }
-  return true;
+  if (NolintParser.isAppliedToAllChecks()) {
+    // NOLINT without check name shouldn't block NolintCheck itself, because
+    // otherwise there will never be diagnostics on "// NOLINT".
+    return CheckName != NolintCheckName;
+  }
+  const auto &CheckNames = NolintParser.сheckNames();
+  return llvm::find(CheckNames, CheckName) != CheckNames.end();
 }
 
-static bool LineIsMarkedWithNOLINT(SourceManager &SM, SourceLocation Loc,
-                                   unsigned DiagID,
-                                   const ClangTidyContext &Context) {
+bool ClangTidyDiagnosticConsumer::lineIsMarkedWithNolint(SourceManager &SM,
+                                                         SourceLocation Loc,
+                                                         StringRef CheckName) {
   bool Invalid;
   const char *CharacterData = SM.getCharacterData(Loc, &Invalid);
   if (Invalid)
     return false;
 
   // Check if there's a NOLINT on this line.
   const char *P = CharacterData;
   while (*P != '\0' && *P != '\r' && *P != '\n')
     ++P;
   StringRef RestOfLine(CharacterData, P - CharacterData + 1);
-  if (IsNOLINTFound("NOLINT", RestOfLine, DiagID, Context))
+  if (isNolintFound(NolintComment, RestOfLine, CheckName))
     return true;
 
   // Check if there's a NOLINTNEXTLINE on the previous line.
   const char *BufBegin =
       SM.getCharacterData(SM.getLocForStartOfFile(SM.getFileID(Loc)), &Invalid);
   if (Invalid || P == BufBegin)
     return false;
 
bool ClangTidyDiagnosticConsumer::lineIsMarkedWithNolint(SourceManager &SM,
   --P;
   const char *LineEnd = P;
 
   // Now we're on the previous line. Skip to the beginning of it.
   while (P != BufBegin && *P != '\n')
     --P;
 
   RestOfLine = StringRef(P, LineEnd - P + 1);
-  if (IsNOLINTFound("NOLINTNEXTLINE", RestOfLine, DiagID, Context))
+  if (isNolintFound(NolintNextLineComment, RestOfLine, CheckName))
     return true;
 
   return false;
 }
 
-static bool LineIsMarkedWithNOLINTinMacro(SourceManager &SM, SourceLocation Loc,
-                                          unsigned DiagID,
-                                          const ClangTidyContext &Context) {
+bool ClangTidyDiagnosticConsumer::isLineMarkedWithNolintInMacro(
+    SourceManager &SM, SourceLocation Loc, StringRef CheckName) {
   while (true) {
-    if (LineIsMarkedWithNOLINT(SM, Loc, DiagID, Context))
+    if (lineIsMarkedWithNolint(SM, Loc, CheckName))
       return true;
     if (!Loc.isMacroID())
       return false;
     Loc = SM.getImmediateExpansionRange(Loc).first;
   }
   return false;
 }
 
 void ClangTidyDiagnosticConsumer::HandleDiagnostic(
     DiagnosticsEngine::Level DiagLevel, const Diagnostic &Info) {
   if (LastErrorWasIgnored && DiagLevel == DiagnosticsEngine::Note)
     return;
 
-  if (Info.getLocation().isValid() && DiagLevel != DiagnosticsEngine::Error &&
+  std::string CheckName = Context.getDiagnosticsName(Info.getID());
+  SourceLocation DiagLocation = Info.getLocation();
+  if (DiagLocation.isValid()) {
+    // This is necessary for finding redundant NOLINT comments. Fill collection
+    // here, as there won't be another chance if diagnostics are filtered.
+    DiagIDsToLineNumbers[Info.getID()].push_back(
+        Info.getSourceManager().getExpansionLineNumber(DiagLocation));
+
+    if (DiagLevel != DiagnosticsEngine::Error &&
         DiagLevel != DiagnosticsEngine::Fatal &&
-      LineIsMarkedWithNOLINTinMacro(Diags->getSourceManager(),
-                                    Info.getLocation(), Info.getID(),
-                                    Context)) {
+        isLineMarkedWithNolintInMacro(Diags->getSourceManager(), DiagLocation,
+                                      CheckName)) {
       ++Context.Stats.ErrorsIgnoredNOLINT;
-    // Ignored a warning, should ignore related notes as well
+      // Ignored a warning, should ignore related notes as well.
       LastErrorWasIgnored = true;
       return;
     }
+  }
 
   LastErrorWasIgnored = false;
   // Count warnings/errors.
   DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info);
 
-  if (DiagLevel == DiagnosticsEngine::Note) {
-    assert(!Errors.empty() &&
-           "A diagnostic note can only be appended to a message.");
-  } else {
+  if (DiagLevel != DiagnosticsEngine::Note) {
     finalizeLastError();
-    StringRef WarningOption =
-        Context.DiagEngine->getDiagnosticIDs()->getWarningOptionForDiag(
-            Info.getID());
-    std::string CheckName = !WarningOption.empty()
-                                ? ("clang-diagnostic-" + WarningOption).str()
-                                : Context.getCheckName(Info.getID()).str();
-
     if (CheckName.empty()) {
       // This is a compiler diagnostic without a warning option. Assign check
       // name based on its level.
       switch (DiagLevel) {
       case DiagnosticsEngine::Error:
       case DiagnosticsEngine::Fatal:
         CheckName = "clang-diagnostic-error";
         break;
if (DiagLevel == DiagnosticsEngine::Error ||
       Level = ClangTidyError::Error;
       LastErrorRelatesToUserCode = true;
       LastErrorPassesLineFilter = true;
     }
     bool IsWarningAsError = DiagLevel == DiagnosticsEngine::Warning &&
                             Context.treatAsError(CheckName);
     Errors.emplace_back(CheckName, Level, Context.getCurrentBuildDirectory(),
                         IsWarningAsError);
+  } else {
+    assert(!Errors.empty() &&
+           "A diagnostic note can only be appended to a message.");
   }
 
   ClangTidyDiagnosticRenderer Converter(
       Context.getLangOpts(), &Context.DiagEngine->getDiagnosticOptions(),
       Errors.back());
   SmallString<100> Message;
   Info.FormatDiagnostic(Message);
   FullSourceLoc Loc =
for (unsigned I = 0; I < Errors.size(); ++I) {
     if (!Apply[I]) {
       Errors[I].Fix.clear();
       Errors[I].Notes.emplace_back(
           "this fix will not be applied because it overlaps with another fix");
     }
   }
 }
 
+void ClangTidyDiagnosticConsumer::reportRedundantNolintComments() {
+  NolintCommentsCollector::NolintInfos NolintInfos =
+      Context.getNolintCollector().takeNolintInfos();
+  if (NolintInfos.empty())
+    return;
+
+  // For fast lookups the collection should be ordered.
+  auto NolintInfoComparator =
+      [] (const NolintCommentsCollector::NolintInfo &Info1,
+          const NolintCommentsCollector::NolintInfo &Info2) {
+    return std::tie(Info1.LineNumber, Info1.CheckName) <
+           std::tie(Info2.LineNumber, Info2.CheckName);
+  };
+  std::sort(NolintInfos.begin(), NolintInfos.end(), NolintInfoComparator);
+
+  // Mark the comments on lines, which there were diagnostics on.
+  auto MarkNotActualNolint =
+      [&NolintInfos, &NolintInfoComparator](unsigned LineNumber,
+                                            StringRef CheckName) {
+        auto resultIt = std::lower_bound(NolintInfos.begin(), NolintInfos.end(),
+            NolintCommentsCollector::NolintInfo{LineNumber, CheckName,
+                // These values are not taken into account by comparator,
+                // so they can be anything.
+                SourceLocation(), NolintCommentType(), true},
+            NolintInfoComparator);
+        // lower_bound doesn't guarantee existence, so check it manually.
+        if (resultIt != NolintInfos.end() &&
+            resultIt->LineNumber == LineNumber &&
+            resultIt->CheckName == CheckName) {
+          resultIt->isActual = false;
+        }
+      };
+  for (const auto &DiagIDToLineNumbers : DiagIDsToLineNumbers) {
+    std::string CheckName = Context.getDiagnosticsName(DiagIDToLineNumbers.first);
+    if (CheckName.empty())
+      continue;
+    for (unsigned LineNumber : DiagIDToLineNumbers.second) {
+      MarkNotActualNolint(LineNumber, CheckName);
+      MarkNotActualNolint(LineNumber, NolintCommentsCollector::AnyCheck);
+    }
+  }
+
+  // Reports diagnostics on the rest of comments.
+  for (const auto &NolintEntry : NolintInfos) {
+    if (NolintEntry.isActual)
+      Context.diag(NolintCheckName, NolintEntry.Location, "there are no "
+          "diagnostics %select{|for '%2'}0 on %select{this|the next}1 line")
+            << (NolintEntry.CheckName == NolintCommentsCollector::AnyCheck ? 0 : 1)
+            << (uint8_t)NolintEntry.Type
+            << NolintEntry.CheckName;
+  }
+}
+
 namespace {
 struct LessClangTidyError {
   bool operator()(const ClangTidyError &LHS, const ClangTidyError &RHS) const {
     const tooling::DiagnosticMessage &M1 = LHS.Message;
     const tooling::DiagnosticMessage &M2 = RHS.Message;
 
     return std::tie(M1.FilePath, M1.FileOffset, M1.Message) <
            std::tie(M2.FilePath, M2.FileOffset, M2.Message);
   }
 };
 struct EqualClangTidyError {
   bool operator()(const ClangTidyError &LHS, const ClangTidyError &RHS) const {
     LessClangTidyError Less;
     return !Less(LHS, RHS) && !Less(RHS, LHS);
   }
 };
 } // end anonymous namespace
 
 // Flushes the internal diagnostics buffer to the ClangTidyContext.
 void ClangTidyDiagnosticConsumer::finish() {
+  reportRedundantNolintComments();
   finalizeLastError();
 
   std::sort(Errors.begin(), Errors.end(), LessClangTidyError());
   Errors.erase(std::unique(Errors.begin(), Errors.end(), EqualClangTidyError()),
                Errors.end());
 
   if (RemoveIncompatibleErrors)
     removeIncompatibleErrors(Errors);
 
   for (const ClangTidyError &Error : Errors)
     Context.storeError(Error);
   Errors.clear();
+  DiagIDsToLineNumbers.clear();
 }

test/clang-tidy/nolint.cpp

 class C4 { C4(int i); }; // NOLINT(some-check, google-explicit-constructor)
 
 class C5 { C5(int i); }; // NOLINT without-brackets-skip-all, another-check
 
 void f() {
   int i;
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'i' [clang-diagnostic-unused-variable]
   int j; // NOLINT
+  int k; // NOLINT(clang-diagnostic-unused-variable)
+  int l; // NOLINT(clang-diagnostic-literal-conversion)
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'l' [clang-diagnostic-unused-variable]
 }
 
 #define MACRO(X) class X { X(int i); };
 MACRO(D)
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: single-argument constructors must be marked explicit
 MACRO(E) // NOLINT
 
 #define MACRO_NOARG class F { F(int i); };
 MACRO_NOARG // NOLINT
 
 #define MACRO_NOLINT class G { G(int i); }; // NOLINT
 MACRO_NOLINT
 
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
-// CHECK-MESSAGES: Suppressed 12 warnings (12 NOLINT)
+// CHECK-MESSAGES: Suppressed 13 warnings (13 NOLINT)

test/clang-tidy/nolintnextline.cpp

 
 // NOLINTNEXTLINE(some-check, google-explicit-constructor)
 class C4 { C4(int i); };
 
 // NOLINTNEXTLINE without-brackets-skip-all, another-check
 class C5 { C5(int i); };
 
 
+void f() {
+  int i;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'i' [clang-diagnostic-unused-variable]
+  // NOLINTNEXTLINE
+  int j;
+  // NOLINTNEXTLINE(clang-diagnostic-unused-variable)
+  int k;
+  // NOLINTNEXTLINE(clang-diagnostic-literal-conversion)
+  int l;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'l' [clang-diagnostic-unused-variable]
+}
+
 // NOLINTNEXTLINE
 
 class D { D(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 // NOLINTNEXTLINE
 //
 class E { E(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 #define MACRO(X) class X { X(int i); };
 MACRO(F)
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: single-argument constructors must be marked explicit
 // NOLINTNEXTLINE
 MACRO(G)
 
 #define MACRO_NOARG class H { H(int i); };
 // NOLINTNEXTLINE
 MACRO_NOARG
 
-// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
+// CHECK-MESSAGES: Suppressed 10 warnings (10 NOLINT)
 
-// RUN: %check_clang_tidy %s google-explicit-constructor %t --
+// RUN: %check_clang_tidy %s google-explicit-constructor,clang-diagnostic-unused-variable %t -- -extra-arg=-Wunused-variable --

test/clang-tidy/readability-nolint-usage.cpp

+// RUN: %check_clang_tidy %s readability-nolint-usage,google-explicit-constructor,misc-unused-parameters %t --
+
+// Case: NO_LINT on line with an error.
+class A1 { A1(int i); }; // NOLINT
+
+// Case: NO_LINT on line without errors.
+class A2 { explicit A2(int i); }; // NOLINT
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: there are no diagnostics on this line
+
+// Case: NO_LINT for the specific check on line with an error on it.
+class A3 { A3(int i); }; // NOLINT(google-explicit-constructor)
+
+// Case: NO_LINT for the specific check on line with an error on another check.
+class A4 { A4(int i); }; // NOLINT(misc-unused-parameters)
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// CHECK-MESSAGES: :[[@LINE-2]]:29: warning: there are no diagnostics for 'misc-unused-parameters' on this line
+
+// Case: NO_LINT for the specific check on line without errors.
+class A5 { explicit A5(int i); }; // NOLINT(google-explicit-constructor)
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: there are no diagnostics for 'google-explicit-constructor' on this line
+
+// Case: NO_LINT for unknown check on line with an error on some check.
+class A6 { A6(int i); }; // NOLINT(unknown-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// CHECK-MESSAGES: :[[@LINE-2]]:29: warning: unknown check name 'unknown-check' specified in NOLINT comment
+
+// Case: NO_LINT for unknown check on line without errors.
+class A7 { explicit A7(int i); }; // NOLINT(unknown-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: unknown check name 'unknown-check' specified in NOLINT comment
+
+// Case: NO_LINT with not closed parenthesis without check names.
+class A8 { A8(int i); }; // NOLINT(
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: unbalanced parentheses in NOLINT comment; all diagnostics silenced for this line
+
+// Case: NO_LINT with not closed parenthesis with check names.
+class A9 { A9(int i); }; // NOLINT(unknown-check
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: unbalanced parentheses in NOLINT comment; all diagnostics silenced for this line
+
+// Case: NO_LINT with clang diagnostics
+int f() {
+  int i = 0; // NOLINT
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: there are no diagnostics on this line
+  int j = 0; // NOLINT(clang-diagnostic-unused-variable)
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: there are no diagnostics for 'clang-diagnostic-unused-variable' on this line
+  return i + j;
+}
+
+#define MACRO(X) class X { explicit X(int i); };
+
+// Case: NO_LINT on macro instantiation line
+MACRO(M1) // NOLINT
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: there are no diagnostics on this line
+
+// Case: NO_LINT with the specific check on macro instantiation line
+MACRO(M2) // NOLINT(google-explicit-constructor)
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: there are no diagnostics for 'google-explicit-constructor' on this line
+
+// Case: NO_LINT on macro line
+#define MACRO2(X) class X { explicit X(int i); }; // NOLINT
+MACRO2(M3)
+// CHECK-MESSAGES: :[[@LINE-2]]:54: warning: there are no diagnostics on this line
+
+// Case: NO_LINT with the specific check on macro line
+#define MACRO3(X) class X { explicit X(int i); }; // NOLINT(google-explicit-constructor)
+MACRO(M4)
+// CHECK-MESSAGES: :[[@LINE-2]]:54: warning: there are no diagnostics for 'google-explicit-constructor' on this line
+
+// Case: NO_LINT with readability-nolint-usage on line without errors.
+class B1 { explicit B1(int i); }; // NOLINT(readability-nolint-usage)
+
+// Case: NO_LINT with the specific check and readability-nolint-usage on line without errors.
+class B2 { explicit B2(int i); }; // NOLINT(google-explicit-constructor, readability-nolint-usage)
+
+// Case: NO_LINT with the unknown check and readability-nolint-usage on line without errors.
+class B3 { explicit B3(int i); }; // NOLINT(unknown-check, readability-nolint-usage)
+
+
+// Case: NO_LINT_NEXT_LINE before line with an error.
+// NOLINTNEXTLINE
+class C1 { C1(int i); };
+
+// Case: NO_LINT_NEXT_LINE before line without errors.
+// NOLINTNEXTLINE
+class C2 { explicit C2(int i); };
+// CHECK-MESSAGES: :[[@LINE-2]]:4: warning: there are no diagnostics on the next line
+
+// Case: NO_LINT_NEXT_LINE for the specific check before line with an error on it.
+// NOLINTNEXTLINE(google-explicit-constructor)
+class C3 { C3(int i); };
+
+// Case: NO_LINT_NEXT_LINE for the specific check before line with an error on another check.
+// NOLINTNEXTLINE(misc-unused-parameters)
+class C4 { C4(int i); };
+// CHECK-MESSAGES: :[[@LINE-2]]:4: warning: there are no diagnostics for 'misc-unused-parameters' on the next line
+// CHECK-MESSAGES: :[[@LINE-2]]:12: warning: single-argument constructors must be marked explicit
+
+// Case: NO_LINT_NEXT_LINE for the specific check before line without errors.
+// NOLINTNEXTLINE(google-explicit-constructor)
+class C5 { explicit C5(int i); };
+// CHECK-MESSAGES: :[[@LINE-2]]:4: warning: there are no diagnostics for 'google-explicit-constructor' on the next line
+
+// Case: NO_LINT_NEXT_LINE for unknown check before line with an error on some check.
+// NOLINTNEXTLINE(unknown-check)
+class C6 { C6(int i); };
+// CHECK-MESSAGES: :[[@LINE-2]]:4: warning: unknown check name 'unknown-check' specified in NOLINTNEXTLINE comment
+// CHECK-MESSAGES: :[[@LINE-2]]:12: warning: single-argument constructors must be marked explicit
+
+// Case: NO_LINT_NEXT_LINE for unknown check before line without errors.
+// NOLINTNEXTLINE(unknown-check)
+class C7 { explicit C7(int i); };
+// CHECK-MESSAGES: :[[@LINE-2]]:4: warning: unknown check name 'unknown-check' specified in NOLINTNEXTLINE comment
+
+// Case: NO_LINT_NEXT_LINE with not closed parenthesis without check names.
+// NOLINTNEXTLINE(
+class C8 { C8(int i); };
+// CHECK-MESSAGES: :[[@LINE-2]]:4: warning: unbalanced parentheses in NOLINTNEXTLINE comment; all diagnostics silenced for this line
+
+// Case: NO_LINT_NEXT_LINE with not closed parenthesis with check names.
+// NOLINTNEXTLINE(unknown-check
+class C9 { C9(int i); };
+// CHECK-MESSAGES: :[[@LINE-2]]:4: warning: unbalanced parentheses in NOLINTNEXTLINE comment; all diagnostics silenced for this line
+
+// Case: NO_LINT_NEXT_LINE with clang diagnostics
+int g() {
+  // NOLINTNEXTLINE
+  int i = 0;
+// CHECK-MESSAGES: :[[@LINE-2]]:6: warning: there are no diagnostics on the next line
+  // NOLINTNEXTLINE(clang-diagnostic-unused-variable)
+  int j = 0;
+// CHECK-MESSAGES: :[[@LINE-2]]:6: warning: there are no diagnostics for 'clang-diagnostic-unused-variable' on the next line
+  return i + j;
+}
+
+#define MACRO(X) class X { explicit X(int i); };
+
+// Case: NO_LINT_NEXT_LINE before macro instantiation line
+// NOLINTNEXTLINE
+MACRO(MM1)
+// CHECK-MESSAGES: :[[@LINE-2]]:4: warning: there are no diagnostics on the next line
+
+// Case: NO_LINT_NEXT_LINE with the specific check before macro instantiation line
+// NOLINTNEXTLINE(google-explicit-constructor)
+MACRO(MM2)
+// CHECK-MESSAGES: :[[@LINE-2]]:4: warning: there are no diagnostics for 'google-explicit-constructor' on the next line
+
+// Case: NO_LINT_NEXT_LINE before macro line
+// NOLINTNEXTLINE
+#define MACRO2(X) class X { explicit X(int i); };
+MACRO2(MM3)
+// CHECK-MESSAGES: :[[@LINE-3]]:4: warning: there are no diagnostics on the next line
+
+// Case: NO_LINT_NEXT_LINE with the specific check before macro line
+// NOLINTNEXTLINE(google-explicit-constructor)
+#define MACRO3(X) class X { explicit X(int i); };
+MACRO(MM4)
+// CHECK-MESSAGES: :[[@LINE-3]]:4: warning: there are no diagnostics for 'google-explicit-constructor' on the next line
+
+// Case: NO_LINT_NEXT_LINE with readability-nolint-usage before line without errors.
+// NOLINTNEXTLINE(readability-nolint-usage)
+// NOLINTNEXTLINE
+class D1 { explicit D1(int i); };
+
+// Case: NO_LINT_NEXT_LINE with the specific check and readability-nolint-usage before line without errors.
+// NOLINTNEXTLINE(readability-nolint-usage)
+// NOLINTNEXTLINE(google-explicit-constructor)
+class D2 { explicit D2(int i); };
+
+// Case: NO_LINT_NEXT_LINE with the unknown check and readability-nolint-usage before line without errors.
+// NOLINTNEXTLINE(readability-nolint-usage)
+// NOLINTNEXTLINE(unknown-check)
+class D3 { explicit D3(int i); };