[clang-tidy] Disable clang-tidy warnings from system macros

[carlosgalvezp]

Summary

Currently, it's inconsistent that warnings are disabled if they
come from system headers, unless they come from macros.
Typically a user cannot act upon these warnings coming from
system macros, so clang-tidy should ignore them unless the
user specifically requests warnings from system headers
via the corresponding configuration.

This change broke the ProTypeVarargCheck check, because it
was checking for the usage of va_arg indirectly, expanding it
(it's a system macro) to detect the usage of __builtin_va_arg.
The check has been fixed by checking directly what the rule
is about: "do not use va_arg", by adding a PP callback that
checks if any macro with name "va_arg" is expanded. The old
AST matcher is still kept for compatibility with Windows.

Add unit test that ensures warnings from macros are disabled
when not using the -system-headers flag. Document the change
in the Release Notes.

[salman-javed-nz]

clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp

23–40

Should this be in another patch?

Line 722 in ClangTidyDiagnosticConsumer.cpp makes it so that clang-tidy filters warnings from system macros. This would benefit all checks.

This VaArgPPCallBack change here only benefits cppcoreguidelines-pro-type-vararg.

29–60

Formatting changed for code not directly involved in this patch. Should be moved to a separate NFC commit that you don't have to run by us.

[carlosgalvezp]

clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp

23–40

The change to ClangTidyDiagnosticConsumer.cpp broke this check, because it was checking for the use of vararg indirectly, checking for the use of __builtin_vararg, which was expanded from the system macro vararg. This patch updates the code so it checks directly what the rule is about "do not use vararg". I should perhaps have been more clear about that in the commit message :)

29–60

Yes, it's a bit unfortunate, this was an automatic change on save from my editor. Is it really worth it to revert this change though? I will need to disable this feature on my IDE, revert each line manually by adding a space, push, then re-enable the feature. Sounds like a lot of time spent on reverting a change that anyway improves the state of the code.

[carlosgalvezp]

Revert formatting, clarify necessary changes to existing check in the commit message.

[carlosgalvezp]

clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp

23–40

Messed up names above, I meant va_arg and __builtin_va_arg.

29–60

Fixed.

[aaron.ballman]

LGTM aside from a minor nit, thanks! This was a good catch.

clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp

32–33

Rather than duplicate the diagnostic text, I think we should hoist the text into a constant string that's used in both places.

That makes it easier to fix the diagnostic (in a follow up) to properly quote va_arg and convert the grammar to singular form instead of plural (e.g, do not use 'va_arg' to define a C-style vararg function; use a variadic template instead).

[carlosgalvezp]

• Refactor duplicated warning message into a common variable.
• Remove double backticks change, TBH I get conflicting review comments about this and can't find any docs supporting either, so I'll just keep it consistent.
• Move extra class to the existing anonymous namespace instead of creating a new one.

[carlosgalvezp]

Thanks for the review! I've fixed the nit, reverted unrelated changes to backticks and moved the class to the existing anonymous namespace. Since those are NFC changes I'll push now, if you don't agree let me know and I'll revert right away :)

clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp

32–33

Of course!

[salman-javed-nz]

This resolves the long-standing gripe I have with clang-tidy raising warnings about GoogleTest macros expanded in my code. Good work.
Do you think we can close issues #44873, #43325, and #31587 now that we have this fix?

[carlosgalvezp]

In D116378#3226242, @salman-javed-nz wrote:

This resolves the long-standing gripe I have with clang-tidy raising warnings about GoogleTest macros expanded in my code. Good work.
Do you think we can close issues #44873, #43325, and #31587 now that we have this fix?

Thanks! It has been bugging me for long time as well :)

It seems to me those issues could be closed, yes.

By the way, the similar problem exists in Clang compiler. I have written in cfe-dev, Discourse and submitted an issue without any feedback so far (I understand it's been vacation). Do you think it would be beneficial to implement a similar fix there? I don't know if the fix is as straightforward as in clang-tidy or if it will have to be done on a per-check basis.

[salman-javed-nz]

In D116378#3226780, @carlosgalvezp wrote:

By the way, the similar problem exists in Clang compiler. I have written in cfe-dev, Discourse and submitted an issue without any feedback so far (I understand it's been vacation). Do you think it would be beneficial to implement a similar fix there? I don't know if the fix is as straightforward as in clang-tidy or if it will have to be done on a per-check basis.

Speaking as just a user of the compiler and not a contributor (I tend to stick in Clang-Tidy land), yes, a fix would be great. Especially if you're getting unactionable warnings coming from STM32 libraries - it's a platform I have to develop for too. Not sure where in Clang you would make the change. At a cursory glance, the commits in Clang/ I saw to do with stopping system header warnings from bubbling up were done in the individual warnings, not in common code. Best to check with someone who works in that area.

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

void ClangTidyDiagnosticConsumer::checkFilters(SourceLocation Location,
   // Invalid location may mean a diagnostic in a command line, don't skip these.
   if (!Location.isValid()) {
     LastErrorRelatesToUserCode = true;
     LastErrorPassesLineFilter = true;
     return;
   }
 
   if (!*Context.getOptions().SystemHeaders &&
-      Sources.isInSystemHeader(Location))
+      (Sources.isInSystemHeader(Location) || Sources.isInSystemMacro(Location)))
     return;
 
   // FIXME: We start with a conservative approach here, but the actual type of
   // location needed depends on the check (in particular, where this check wants
   // to apply fixes).
   FileID FID = Sources.getDecomposedExpansionLoc(Location).first;
   const FileEntry *File = Sources.getFileEntryForID(FID);
 

clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.h

 class ProTypeVarargCheck : public ClangTidyCheck {
 public:
   ProTypeVarargCheck(StringRef Name, ClangTidyContext *Context)
       : ClangTidyCheck(Name, Context) {}
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
     return LangOpts.CPlusPlus;
   }
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+                           Preprocessor *ModuleExpanderPP) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 };
 
 } // namespace cppcoreguidelines
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_PRO_TYPE_VARARG_H

clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp

 //===--- ProTypeVarargCheck.cpp - clang-tidy-------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "ProTypeVarargCheck.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Basic/TargetInfo.h"
+#include "clang/Lex/PPCallbacks.h"
+#include "clang/Lex/Preprocessor.h"
+#include "clang/Lex/Token.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace cppcoreguidelines {
 
 const internal::VariadicDynCastAllOfMatcher<Stmt, VAArgExpr> VAArgExpr;
 
 static constexpr StringRef AllowedVariadics[] = {
     // clang-format off
     "__builtin_isgreater", 
     "__builtin_isgreaterequal", 
     "__builtin_isless",
     "__builtin_islessequal", 
     "__builtin_islessgreater", 
     "__builtin_isunordered",
     "__builtin_fpclassify", 
     "__builtin_isfinite", 
     "__builtin_isinf",
     "__builtin_isinf_sign", 
     "__builtin_isnan", 
     "__builtin_isnormal",
     "__builtin_signbit", 
     "__builtin_constant_p", 
     "__builtin_classify_type",
     "__builtin_va_start",
     "__builtin_assume_aligned", // Documented as variadic to support default 
                                 // parameters.
     "__builtin_prefetch",       // Documented as variadic to support default
                                 // parameters.
     "__builtin_shufflevector",  // Documented as variadic but with a defined
                                 // number of args based on vector size.
     "__builtin_convertvector", 
     "__builtin_call_with_static_chain",
     "__builtin_annotation", 
     "__builtin_add_overflow", 
     "__builtin_sub_overflow",
     "__builtin_mul_overflow", 
     "__builtin_preserve_access_index",
     "__builtin_nontemporal_store", 
     "__builtin_nontemporal_load",
     "__builtin_ms_va_start",
     // clang-format on
 };
 
+static constexpr StringRef VaArgWarningMessage =
+    "do not use va_arg to define c-style vararg functions; "
+    "use variadic templates instead";
+
 namespace {
 AST_MATCHER(QualType, isVAList) {
   ASTContext &Context = Finder->getASTContext();
   QualType Desugar = Node.getDesugaredType(Context);
   QualType NodeTy = Node.getUnqualifiedType();
 
   auto CheckVaList = [](QualType NodeTy, QualType Expected,
                         const ASTContext &Context) {
AST_MATCHER(QualType, isVAList) {
 
   return false;
 }
 
 AST_MATCHER_P(AdjustedType, hasOriginalType,
               ast_matchers::internal::Matcher<QualType>, InnerType) {
   return InnerType.matches(Node.getOriginalType(), Finder, Builder);
 }
+
+class VaArgPPCallbacks : public PPCallbacks {
+public:
+  VaArgPPCallbacks(ProTypeVarargCheck *Check) : Check(Check) {}
+
+  void MacroExpands(const Token &MacroNameTok, const MacroDefinition &MD,
+                    SourceRange Range, const MacroArgs *Args) override {
+    if (MacroNameTok.getIdentifierInfo()->getName() == "va_arg") {
+      Check->diag(MacroNameTok.getLocation(), VaArgWarningMessage);
+    }
+  }
+
+private:
+  ProTypeVarargCheck *Check;
+};
 } // namespace
 
 void ProTypeVarargCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(VAArgExpr().bind("va_use"), this);
 
   Finder->addMatcher(
       callExpr(callee(functionDecl(isVariadic(),
                                    unless(hasAnyName(AllowedVariadics)))))
           .bind("callvararg"),
       this);
 
   Finder->addMatcher(
       varDecl(unless(parmVarDecl()),
               hasType(qualType(
                   anyOf(isVAList(), decayedType(hasOriginalType(isVAList()))))))
           .bind("va_list"),
       this);
 }
 
+void ProTypeVarargCheck::registerPPCallbacks(const SourceManager &SM,
+                                             Preprocessor *PP,
+                                             Preprocessor *ModuleExpanderPP) {
+  PP->addPPCallbacks(std::make_unique<VaArgPPCallbacks>(this));
+}
+
 static bool hasSingleVariadicArgumentWithValue(const CallExpr *C, uint64_t I) {
   const auto *FDecl = dyn_cast<FunctionDecl>(C->getCalleeDecl());
   if (!FDecl)
     return false;
 
   auto N = FDecl->getNumParams(); // Number of parameters without '...'
   if (C->getNumArgs() != N + 1)
     return false; // more/less than one argument passed to '...'

 void ProTypeVarargCheck::check(const MatchFinder::MatchResult &Result) {
   if (const auto *Matched = Result.Nodes.getNodeAs<CallExpr>("callvararg")) {
     if (hasSingleVariadicArgumentWithValue(Matched, 0))
       return;
     diag(Matched->getExprLoc(), "do not call c-style vararg functions");
   }
 
   if (const auto *Matched = Result.Nodes.getNodeAs<Expr>("va_use")) {
-    diag(Matched->getExprLoc(),
-         "do not use va_arg to define c-style vararg functions; "
-         "use variadic templates instead");
+    diag(Matched->getExprLoc(), VaArgWarningMessage);
   }
 
   if (const auto *Matched = Result.Nodes.getNodeAs<VarDecl>("va_list")) {
     auto SR = Matched->getSourceRange();
     if (SR.isInvalid())
       return; // some implicitly generated builtins take va_list
     diag(SR.getBegin(), "do not declare variables of type va_list; "
                         "use variadic templates instead");
   }
 }
 
 } // namespace cppcoreguidelines
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
+- Ignore warnings from macros defined in system headers, if not using the
+  `-system-headers` flag.
+
 - Added support for globbing in `NOLINT*` expressions, to simplify suppressing
   multiple warnings in the same line.
 
 - Added support for `NOLINTBEGIN` ... `NOLINTEND` comments to suppress
   Clang-Tidy warnings over multiple lines.
 
 - Generalized the `modernize-use-default-member-init` check to handle non-default
   constructors.
- Removed default setting ``cppcoreguidelines-explicit-virtual-functions.IgnoreDestructors = "true"``,
   to match the current state of the C++ Core Guidelines.
 
 - Updated :doc:`google-readability-casting
   <clang-tidy/checks/google-readability-casting>` to diagnose and fix functional
   casts, to achieve feature parity with the corresponding ``cpplint.py`` check.
 
 - Fixed a false positive in :doc:`fuchsia-trailing-return
   <clang-tidy/checks/fuchsia-trailing-return>` for C++17 deduction guides.
-  
 - Fixed a false positive in :doc:`bugprone-throw-keyword-missing
   <clang-tidy/checks/bugprone-throw-keyword-missing>` when creating an exception object
   using placement new
 
 - :doc:`cppcoreguidelines-narrowing-conversions <clang-tidy/checks/cppcoreguidelines-narrowing-conversions>`
   check now supports a `WarnOnIntegerToFloatingPointNarrowingConversion`
   option to control whether to warn on narrowing integer to floating-point
   conversions.

clang-tools-extra/test/clang-tidy/infrastructure/Inputs/file-filter/system/system-header.h

 class A0 { A0(int); };
+
+#define TO_FLOAT_PTR(x) ((float *)(x))

clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp

 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='' %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck %s
 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='' -quiet %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK-QUIET %s
 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK2 %s
 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -quiet %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK2-QUIET %s
 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='header2\.h' %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK3 %s
 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='header2\.h' -quiet %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK3-QUIET %s
 // FIXME: "-I %S/Inputs/file-filter/system/.." must be redundant.
 //       On Win32, file-filter/system\system-header1.h precedes
 //       file-filter\header*.h due to code order between '/' and '\\'.
 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers %s -- -I %S/Inputs/file-filter/system/.. -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK4 %s
 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers -quiet %s -- -I %S/Inputs/file-filter/system/.. -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK4-QUIET %s
+// RUN: clang-tidy -checks='-*,cppcoreguidelines-pro-type-cstyle-cast' -header-filter='.*' -system-headers %s -- -I %S/Inputs/file-filter/system/.. -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK5 %s
+// RUN: clang-tidy -checks='-*,cppcoreguidelines-pro-type-cstyle-cast' -header-filter='.*' %s -- -I %S/Inputs/file-filter/system/.. -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK5-NO-SYSTEM-HEADERS %s
 
 #include "header1.h"
 // CHECK-NOT: warning:
 // CHECK-QUIET-NOT: warning:
 // CHECK2: header1.h:1:12: warning: single-argument constructors must be marked explicit
 // CHECK2-QUIET: header1.h:1:12: warning: single-argument constructors must be marked explicit
 // CHECK3-NOT: warning:
 // CHECK3-QUIET-NOT: warning:

 // CHECK2: Use -header-filter=.* {{.*}}
 // CHECK2-QUIET-NOT: Suppressed
 // CHECK3: Suppressed 2 warnings (2 in non-user code)
 // CHECK3: Use -header-filter=.* {{.*}}
 // CHECK3-QUIET-NOT: Suppressed
 // CHECK4-NOT: Suppressed {{.*}} warnings
 // CHECK4-NOT: Use -header-filter=.* {{.*}}
 // CHECK4-QUIET-NOT: Suppressed
+
+int x = 123;
+auto x_ptr = TO_FLOAT_PTR(&x);
+// CHECK5: :[[@LINE-1]]:14: warning: do not use C-style cast to convert between unrelated types
+// CHECK5-NO-SYSTEM-HEADERS-NOT: :[[@LINE-2]]:14: warning: do not use C-style cast to convert between unrelated types