[clang-tidy] Ignore diagnostics due to macro expansion from not-interested headers

[DmitryPolukhin]

Summary

This diff is an attempt to workaround a common problem with clang-tidy deployment.
There are headers that might not be compatible with your project style guide but you
have to use macro from that headers so clang-tidy could report lots of style issues
that you cannot fix or is some cases even annotate with NOLINT. Solution proposed
in this diff is to avoid reporting diagnostics in macro expansion of the macro defined
in headers that doesn't match the desired --header-filter.

Just one real life example of this issue from a very popular gflags library.
This library requires using macro in use code like this:

DEFINE_bool(some_bool_flag,
            "the default value should go here, not the description",
            false);

But the macro has outdated version of compiler assert that uses C-style arrays.
Therefore use code becomes incompatible with clang-tidy check modernize-avoid-c-arrays.
Another example of problematic is googletest/googlemock with lots of macro that you cannot avoid.

New feature implemented behind configuration flag for backward compatibility.

[DmitryPolukhin]

Fix clang-tidy warning

[njames93]

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

249–251

This should also check if the Optional has a value

349–351

This looks suspicious, in clang-tidy land SystemHeaders is always set, however outside clang-tidy it may not be.
Perhaps getValueOr(false) should be used to prevent any asserts.

Also can a test be added to show this respecting the SystemHeaders setting.

358–360

Elide braces

[DmitryPolukhin]

• addressed comments in the diff
• stop ignoring system macro

[DmitryPolukhin]

@njames93 thank you for the feedback!

Still waiting for more feedback and especially high level one about this feature in general. Does it look useful? Should it be behind a flag i.e. changing default behaviour is too dangerous?

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

249–251

This code was moved from ClangTidyDiagnosticConsumer::getHeaderFilter but you are right, it makes sense to add support for missing value.

349–351

Running this code on bigger codebase I found undesired side-effect that it is better to report issues on system macro like NULL such issues usually from external code, not from macro itself.

[zigwei]

This is a helpful feature. We had the same problems with both GFLAG and GTEST, too.
But I am not sure if HeaderFilterRegex is the right option to control it. Is a new filter better for the ignored headers?

[antonl]

Let me second that this will be a very useful feature. Without enabling some clang-tidy checks will be very noisy.

[DmitryPolukhin]

I think there is no sense to invent another HeaderFilterRegex like option to control which headers should be excluded. HeaderFilterRegex should be good enough because if you would like to see warning from the header, most probably you can fix code in the header or at least add a suppression.

To be on a safe side I'm going to put this feature behind a configuration flag and send diff for review soon.

[DmitryPolukhin]

Put feature behind flag

[DmitryPolukhin]

Fix wrong character

[DmitryPolukhin]

Added test for system like object macro

[DmitryPolukhin]

@alexfh, @njames93 and @thakis please take a look! I added all tests cases and put new logic behind a flag to make it as safe as possible.
Issue with diagnostics from macro expansion from third-party headers is the one of the biggest problem with deployment that we have and it cannot be properly fixed with wrappers around clang-tidy.

[lebedev.ri]

This needs to explain why the existing functionality isn't sufficient - if the header is really not from this project,
then it should be included via -isystem, and that will naturally suppress all diagnostics from it.

[DmitryPolukhin]

In D90835#2808153, @lebedev.ri wrote:

This needs to explain why the existing functionality isn't sufficient - if the header is really not from this project,
then it should be included via -isystem, and that will naturally suppress all diagnostics from it.

The diagnostics are reported in the place of macro expansion i.e. user sources. It doesn't matter how they get included via -isystem or -I.

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

DiagLevelAndFormatString getDiagLevelAndFormatString(unsigned DiagnosticID,
                                                        SourceLocation Loc) {
     return DiagLevelAndFormatString(
         static_cast<DiagnosticIDs::Level>(
             DiagEngine->getDiagnosticLevel(DiagnosticID, Loc)),
         std::string(
             DiagEngine->getDiagnosticIDs()->getDescription(DiagnosticID)));
   }
 
+  /// Returns the \c HeaderFilter constructed for the options set in the
+  /// context.
+  llvm::Regex *getHeaderFilter() const;
+
 private:
   // Writes to Stats.
   friend class ClangTidyDiagnosticConsumer;
 
   DiagnosticsEngine *DiagEngine;
   std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider;
 
   std::string CurrentFile;
   ClangTidyOptions CurrentOptions;
   class CachedGlobList;
   std::unique_ptr<CachedGlobList> CheckFilter;
   std::unique_ptr<CachedGlobList> WarningAsErrorFilter;
+  // Cache for compiled regex with HeaderFilter.
+  mutable std::unique_ptr<llvm::Regex> HeaderFilter;
 
   LangOptions LangOpts;
 
   ClangTidyStats Stats;
 
   std::string CurrentBuildDirectory;
 
   llvm::DenseMap<unsigned, std::string> CheckNamesByDiagnosticID;
public:
   // Retrieve the diagnostics that were captured.
   std::vector<ClangTidyError> take();
 
 private:
   void finalizeLastError();
   void removeIncompatibleErrors();
   void removeDuplicatedDiagnosticsOfAliasCheckers();
 
-  /// Returns the \c HeaderFilter constructed for the options set in the
-  /// context.
-  llvm::Regex *getHeaderFilter();
-
   /// Updates \c LastErrorRelatesToUserCode and LastErrorPassesLineFilter
   /// according to the diagnostic \p Location.
   void checkFilters(SourceLocation Location, const SourceManager &Sources);
   bool passesLineFilter(StringRef FileName, unsigned LineNumber) const;
 
   void forwardDiagnostic(const Diagnostic &Info);
 
   ClangTidyContext &Context;
   DiagnosticsEngine *ExternalDiagEngine;
   bool RemoveIncompatibleErrors;
   std::vector<ClangTidyError> Errors;
-  std::unique_ptr<llvm::Regex> HeaderFilter;
   bool LastErrorRelatesToUserCode;
   bool LastErrorPassesLineFilter;
   bool LastErrorWasIgnored;
 };
 
 } // end namespace tidy
 } // end namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDYDIAGNOSTICCONSUMER_H

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

if (!ClangWarningOption.empty())
     return "clang-diagnostic-" + ClangWarningOption;
   llvm::DenseMap<unsigned, std::string>::const_iterator I =
       CheckNamesByDiagnosticID.find(DiagnosticID);
   if (I != CheckNamesByDiagnosticID.end())
     return I->second;
   return "";
 }
 
+llvm::Regex *ClangTidyContext::getHeaderFilter() const {
+  if (!HeaderFilter)
+    HeaderFilter = std::make_unique<llvm::Regex>(
+        getOptions().HeaderFilterRegex.getValueOr(""));
+  return HeaderFilter.get();
+}
+
 ClangTidyDiagnosticConsumer::ClangTidyDiagnosticConsumer(
     ClangTidyContext &Ctx, DiagnosticsEngine *ExternalDiagEngine,
     bool RemoveIncompatibleErrors)
     : Context(Ctx), ExternalDiagEngine(ExternalDiagEngine),
       RemoveIncompatibleErrors(RemoveIncompatibleErrors),
       LastErrorRelatesToUserCode(false), LastErrorPassesLineFilter(false),
       LastErrorWasIgnored(false) {}
 
if (IsNOLINTFound("NOLINT", RestOfLine, DiagID, Context))
     return true;
 
   // Check if there's a NOLINTNEXTLINE on the previous line.
   StringRef PrevLine =
       Buffer->substr(0, Offset).rsplit('\n').first.rsplit('\n').second;
   return IsNOLINTFound("NOLINTNEXTLINE", PrevLine, DiagID, Context);
 }
 
+static bool isErrorFromMacroInIgnoredHeader(const SourceManager &SM,
+                                            SourceLocation Loc,
+                                            const ClangTidyContext &Context) {
+  SourceLocation SpellingLoc = SM.getImmediateSpellingLoc(Loc);
+  // Don't skip macro from the main file.
+  if (SM.isInMainFile(SpellingLoc))
+    return false;
+
+  // Don't skip macro from system headers to report issues on `NULL`, ObjC
+  // `YES/NO` and similar system macro.
+  if (SM.isInSystemHeader(SpellingLoc))
+    return false;
+
+  FileID FID = SM.getDecomposedExpansionLoc(SpellingLoc).first;
+  const FileEntry *File = SM.getFileEntryForID(FID);
+
+  // -DMACRO definitions on the command line have locations in a virtual buffer
+  // that doesn't have a FileEntry. Don't skip these.
+  if (!File)
+    return false;
+
+  StringRef FileName(File->getName());
+  if (const FileEntry *MainFile = SM.getFileEntryForID(SM.getMainFileID())) {
+    if (MainFile->getName() == FileName) {
+      // This check should be equvalent to isInMainFile but when clangd loads
+      // preamble isInMainFile gives wrong result for macro spelling location.
+      // It looks like the location comes from file included into itself at
+      // position 1:1. This check fixes ClangdTests/DiagnosticsTest.ClangTidy
+      // TODO: find out what clangd does wrong with location
+      return false;
+    }
+  }
+  return !Context.getHeaderFilter()->match(FileName);
+}
+
 static bool LineIsMarkedWithNOLINTinMacro(const SourceManager &SM,
                                           SourceLocation Loc, unsigned DiagID,
                                           const ClangTidyContext &Context,
                                           bool AllowIO) {
   while (true) {
     if (LineIsMarkedWithNOLINT(SM, Loc, DiagID, Context, AllowIO))
       return true;
     if (!Loc.isMacroID())
       return false;
+    if (isErrorFromMacroInIgnoredHeader(SM, Loc, Context))
+      return true;
     Loc = SM.getImmediateExpansionRange(Loc).getBegin();
   }
   return false;
 }
 
 namespace clang {
 namespace tidy {
 
if (!File) {
     LastErrorRelatesToUserCode = true;
     LastErrorPassesLineFilter = true;
     return;
   }
 
   StringRef FileName(File->getName());
   LastErrorRelatesToUserCode = LastErrorRelatesToUserCode ||
                                Sources.isInMainFile(Location) ||
-                               getHeaderFilter()->match(FileName);
+                               Context.getHeaderFilter()->match(FileName);
 
   unsigned LineNumber = Sources.getExpansionLineNumber(Location);
   LastErrorPassesLineFilter =
       LastErrorPassesLineFilter || passesLineFilter(FileName, LineNumber);
 }
 
-llvm::Regex *ClangTidyDiagnosticConsumer::getHeaderFilter() {
-  if (!HeaderFilter)
-    HeaderFilter =
-        std::make_unique<llvm::Regex>(*Context.getOptions().HeaderFilterRegex);
-  return HeaderFilter.get();
-}
-
 void ClangTidyDiagnosticConsumer::removeIncompatibleErrors() {
   // Each error is modelled as the set of intervals in which it applies
   // replacements. To detect overlapping replacements, we use a sweep line
   // algorithm over these sets of intervals.
   // An event here consists of the opening or closing of an interval. During the
   // process, we maintain a counter with the amount of open intervals. If we
   // find an endpoint of an interval and this counter is different from 0, it
   // means that this interval overlaps with another one, so we set it as

clang-tools-extra/test/clang-tidy/infrastructure/macros.h

+#define HEADER_MACRO_DIAG_IN_ARG(a) a
+#define HEADER_MACRO_DIAG_IN_BODY int var_C1[10]
+#define HEADER_MACRO_DIAG_IN_BODY2 int var_C2[10]
+
+#define DEFINE_bool(name, val)                                                            \
+  namespace fLB {                                                                         \
+  typedef char FLAG_##name##_value_is_not_a_bool[(sizeof(val) != sizeof(bool)) ? -1 : 1]; \
+  }                                                                                       \
+  bool FLAG_##name = val

clang-tools-extra/test/clang-tidy/infrastructure/macros.cpp

-// RUN: clang-tidy -checks='-*,google-explicit-constructor' %s -- | FileCheck %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor,modernize-avoid-c-arrays' --header-filter='macros_filter.h' %s -- | FileCheck %s
+
+#include "macros.h"
+#include "macros_filter.h"
 
 #define Q(name) class name { name(int i); }
 
 Q(A);
 // CHECK: :[[@LINE-1]]:3: warning: single-argument constructors must be marked explicit
-// CHECK: :3:30: note: expanded from macro 'Q'
+// CHECK: :[[@LINE-4]]:30: note: expanded from macro 'Q'
+
+#define MAIN_MACRO_DIAG_IN_ARG(a) a
+MAIN_MACRO_DIAG_IN_ARG(int var_A[10]);
+// CHECK: :[[@LINE-1]]:24: warning: do not declare C-style arrays, use std::array<> instead
+
+#define MAIN_MACRO_DIAG_IN_BODY int var_A1[10]
+MAIN_MACRO_DIAG_IN_BODY;
+// CHECK: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use std::array<> instead
+// CHECK: :[[@LINE-3]]:33: note: expanded from macro 'MAIN_MACRO_DIAG_IN_BODY'
+
+HEADER_FILTER_MACRO_DIAG_IN_ARG(int var_B[10]);
+// CHECK: :[[@LINE-1]]:33: warning: do not declare C-style arrays, use std::array<> instead
+
+HEADER_FILTER_MACRO_DIAG_IN_BODY;
+// CHECK: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use std::array<> instead
+// CHECK: note: expanded from macro 'HEADER_FILTER_MACRO_DIAG_IN_BODY'
+
+#define MAIN_MACRO_WRAPPER HEADER_FILTER_MACRO_DIAG_IN_BODY2
+MAIN_MACRO_WRAPPER;
+// CHECK: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use std::array<> instead
+// CHECK: note: expanded from macro 'MAIN_MACRO_WRAPPER'
+// CHECK: note: expanded from macro 'HEADER_FILTER_MACRO_DIAG_IN_BODY2'
+
+HEADER_MACRO_DIAG_IN_ARG(int var_C[10]);
+HEADER_MACRO_DIAG_IN_BODY;
+
+#define MAIN_MACRO_WRAPPER2 HEADER_MACRO_DIAG_IN_BODY2
+MAIN_MACRO_WRAPPER2;
+
+DEFINE_bool(test, false);
+// CHECK-NOT: warning:

clang-tools-extra/test/clang-tidy/infrastructure/macros_filter.h

+#define HEADER_FILTER_MACRO_DIAG_IN_ARG(a) a
+#define HEADER_FILTER_MACRO_DIAG_IN_BODY int var_B1[10]
+#define HEADER_FILTER_MACRO_DIAG_IN_BODY2 int var_B2[10]