[clang-tidy] modernize-deprecated-headers check should respect extern…

… "C" blocks

The check should not report includes wrapped by `extern "C" { ... }` blocks,
such as:

```lang=C++
#ifdef __cplusplus
extern "C" {
#endif

#include "assert.h"

#ifdef __cplusplus
}
#endif
```

This pattern comes up sometimes in header files designed to be consumed
by both C and C++ source files.
The check now reports false reports when the header file is consumed by
a C++ translation unit.

In this change, I'm not emitting the reports immediately from the
`PPCallback`, rather aggregating them for further processing.
After all preprocessing is done, the matcher will be called on the
`TranslationUnitDecl`, ensuring that the check callback is called only
once.

Within that callback, I'm recursively visiting each decls, looking for
`LinkageSpecDecls` which represent the `extern "C"` specifier.
After this, I'm dropping all the reports coming from inside of it.
After the visitation is done, I'm emitting the reports I'm left with.

For performance reasons, I'm sorting the `IncludeMarkers` by their
corresponding locations.
This makes the scan `O(log(N)` when looking up the `IncludeMarkers`
affected by the given `extern "C"` block. For this, I'm using
`lower_bound()` and `upper_bound()`.

Reviewed By: whisperity

Differential Revision: https://reviews.llvm.org/D125209

clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp

@@ -7,23 +7,37 @@
 //===----------------------------------------------------------------------===//
 
 #include "DeprecatedHeadersCheck.h"
+#include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Preprocessor.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringSet.h"
 
+#include <algorithm>
 #include <vector>
 
 namespace clang {
 namespace tidy {
 namespace modernize {
+namespace detail {
+bool operator<(const IncludeMarker &LHS, const IncludeMarker &RHS) {
+  return LHS.DecomposedDiagLoc < RHS.DecomposedDiagLoc;
+}
+bool operator<(const IncludeMarker &LHS,
+               const std::pair<FileID, unsigned> &RHS) {
+  return LHS.DecomposedDiagLoc < RHS;
+}
+bool operator<(const std::pair<FileID, unsigned> &LHS,
+               const IncludeMarker &RHS) {
+  return LHS < RHS.DecomposedDiagLoc;
+}
 
-namespace {
 class IncludeModernizePPCallbacks : public PPCallbacks {
 public:
-  explicit IncludeModernizePPCallbacks(ClangTidyCheck &Check,
-                                       LangOptions LangOpts);
+  explicit IncludeModernizePPCallbacks(DeprecatedHeadersCheck &Check,
+                                       LangOptions LangOpts,
+                                       const SourceManager &SM);
 
   void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
                           StringRef FileName, bool IsAngled,
@@ -33,22 +47,98 @@ class IncludeModernizePPCallbacks : public PPCallbacks {
                           SrcMgr::CharacteristicKind FileType) override;
 
 private:
-  ClangTidyCheck &Check;
+  DeprecatedHeadersCheck &Check;
   LangOptions LangOpts;
   llvm::StringMap<std::string> CStyledHeaderToCxx;
   llvm::StringSet<> DeleteHeaders;
+  const SourceManager &SM;
+};
+
+class ExternCRefutationVisitor
+    : public RecursiveASTVisitor<ExternCRefutationVisitor> {
+  std::vector<IncludeMarker> &IncludesToBeProcessed;
+  const SourceManager &SM;
+
+public:
+  ExternCRefutationVisitor(std::vector<IncludeMarker> &IncludesToBeProcessed,
+                           SourceManager &SM)
+      : IncludesToBeProcessed(IncludesToBeProcessed), SM(SM) {}
+  bool shouldWalkTypesOfTypeLocs() const { return false; }
+  bool shouldVisitLambdaBody() const { return false; }
+
+  bool VisitLinkageSpecDecl(LinkageSpecDecl *LinkSpecDecl) const {
+    if (LinkSpecDecl->getLanguage() != LinkageSpecDecl::lang_c ||
+        !LinkSpecDecl->hasBraces())
+      return true;
+
+    auto ExternCBlockBegin =
+        SM.getDecomposedExpansionLoc(LinkSpecDecl->getBeginLoc());
+    auto ExternCBlockEnd =
+        SM.getDecomposedExpansionLoc(LinkSpecDecl->getEndLoc());
+
+    auto Begin = IncludesToBeProcessed.begin();
+    auto End = IncludesToBeProcessed.end();
+    auto Low = std::lower_bound(Begin, End, ExternCBlockBegin);
+    auto Up = std::upper_bound(Low, End, ExternCBlockEnd);
+    IncludesToBeProcessed.erase(Low, Up);
+    return true;
+  }
 };
-} // namespace
+} // namespace detail
 
 void DeprecatedHeadersCheck::registerPPCallbacks(
     const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
-    PP->addPPCallbacks(
-        ::std::make_unique<IncludeModernizePPCallbacks>(*this, getLangOpts()));
+  PP->addPPCallbacks(::std::make_unique<detail::IncludeModernizePPCallbacks>(
+      *this, getLangOpts(), PP->getSourceManager()));
+}
+void DeprecatedHeadersCheck::registerMatchers(
+    ast_matchers::MatchFinder *Finder) {
+  // Even though the checker operates on a "preprocessor" level, we still need
+  // to act on a "TranslationUnit" to acquire the AST where we can walk each
+  // Decl and look for `extern "C"` blocks where we will suppress the report we
+  // collected during the preprocessing phase.
+  // The `onStartOfTranslationUnit()` won't suffice, since we need some handle
+  // to the `ASTContext`.
+  Finder->addMatcher(ast_matchers::translationUnitDecl().bind("TU"), this);
+}
+
+void DeprecatedHeadersCheck::onEndOfTranslationUnit() {
+  IncludesToBeProcessed.clear();
+}
+
+void DeprecatedHeadersCheck::check(
+    const ast_matchers::MatchFinder::MatchResult &Result) {
+  SourceManager &SM = Result.Context->getSourceManager();
+  using detail::IncludeMarker;
+
+  llvm::sort(IncludesToBeProcessed);
+
+  // Suppress includes wrapped by `extern "C" { ... }` blocks.
+  detail::ExternCRefutationVisitor Visitor(IncludesToBeProcessed, SM);
+  Visitor.TraverseAST(*Result.Context);
+
+  // Emit all the remaining reports.
+  for (const IncludeMarker &Entry : IncludesToBeProcessed) {
+    SourceLocation DiagLoc = SM.getComposedLoc(Entry.DecomposedDiagLoc.first,
+                                               Entry.DecomposedDiagLoc.second);
+    if (Entry.Replacement.empty()) {
+      diag(DiagLoc, "including '%0' has no effect in C++; consider removing it")
+          << Entry.FileName << FixItHint::CreateRemoval(Entry.ReplacementRange);
+    } else {
+      diag(DiagLoc, "inclusion of deprecated C++ header "
+                    "'%0'; consider using '%1' instead")
+          << Entry.FileName << Entry.Replacement
+          << FixItHint::CreateReplacement(
+                 Entry.ReplacementRange,
+                 (llvm::Twine("<") + Entry.Replacement + ">").str());
+    }
+  }
 }
 
-IncludeModernizePPCallbacks::IncludeModernizePPCallbacks(ClangTidyCheck &Check,
-                                                         LangOptions LangOpts)
-    : Check(Check), LangOpts(LangOpts) {
+detail::IncludeModernizePPCallbacks::IncludeModernizePPCallbacks(
+    DeprecatedHeadersCheck &Check, LangOptions LangOpts,
+    const SourceManager &SM)
+    : Check(Check), LangOpts(LangOpts), SM(SM) {
   for (const auto &KeyValue :
        std::vector<std::pair<llvm::StringRef, std::string>>(
            {{"assert.h", "cassert"},
@@ -89,7 +179,7 @@ IncludeModernizePPCallbacks::IncludeModernizePPCallbacks(ClangTidyCheck &Check,
   }
 }
 
-void IncludeModernizePPCallbacks::InclusionDirective(
+void detail::IncludeModernizePPCallbacks::InclusionDirective(
     SourceLocation HashLoc, const Token &IncludeTok, StringRef FileName,
     bool IsAngled, CharSourceRange FilenameRange, Optional<FileEntryRef> File,
     StringRef SearchPath, StringRef RelativePath, const Module *Imported,
@@ -101,19 +191,16 @@ void IncludeModernizePPCallbacks::InclusionDirective(
   // 1. Insert std prefix for every such symbol occurrence.
   // 2. Insert `using namespace std;` to the beginning of TU.
   // 3. Do nothing and let the user deal with the migration himself.
+  std::pair<FileID, unsigned> DiagLoc =
+      SM.getDecomposedExpansionLoc(FilenameRange.getBegin());
   if (CStyledHeaderToCxx.count(FileName) != 0) {
-    std::string Replacement =
-        (llvm::Twine("<") + CStyledHeaderToCxx[FileName] + ">").str();
-    Check.diag(FilenameRange.getBegin(), "inclusion of deprecated C++ header "
-                                         "'%0'; consider using '%1' instead")
-        << FileName << CStyledHeaderToCxx[FileName]
-        << FixItHint::CreateReplacement(FilenameRange.getAsRange(),
-                                        Replacement);
+    Check.IncludesToBeProcessed.push_back(
+        IncludeMarker{CStyledHeaderToCxx[FileName], FileName,
+                      FilenameRange.getAsRange(), DiagLoc});
   } else if (DeleteHeaders.count(FileName) != 0) {
-    Check.diag(FilenameRange.getBegin(),
-               "including '%0' has no effect in C++; consider removing it")
-        << FileName << FixItHint::CreateRemoval(
-                           SourceRange(HashLoc, FilenameRange.getEnd()));
+    Check.IncludesToBeProcessed.push_back(
+        IncludeMarker{std::string{}, FileName,
+                      SourceRange{HashLoc, FilenameRange.getEnd()}, DiagLoc});
   }
 }
 

clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.h

@@ -15,6 +15,22 @@ namespace clang {
 namespace tidy {
 namespace modernize {
 
+namespace detail {
+class IncludeModernizePPCallbacks;
+class ExternCRefutationVisitor;
+struct IncludeMarker {
+  std::string Replacement;
+  StringRef FileName;
+  SourceRange ReplacementRange;
+  std::pair<FileID, unsigned> DecomposedDiagLoc;
+};
+bool operator<(const IncludeMarker &LHS, const IncludeMarker &RHS);
+bool operator<(const IncludeMarker &LHS,
+               const std::pair<FileID, unsigned> &RHS);
+bool operator<(const std::pair<FileID, unsigned> &LHS,
+               const IncludeMarker &RHS);
+} // namespace detail
+
 /// This check replaces deprecated C library headers with their C++ STL
 /// alternatives.
 ///
@@ -41,6 +57,14 @@ class DeprecatedHeadersCheck : public ClangTidyCheck {
   }
   void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
                            Preprocessor *ModuleExpanderPP) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void onEndOfTranslationUnit() override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  friend class detail::IncludeModernizePPCallbacks;
+  friend class detail::ExternCRefutationVisitor;
+  std::vector<detail::IncludeMarker> IncludesToBeProcessed;
 };
 
 } // namespace modernize

clang-tools-extra/docs/ReleaseNotes.rst

@@ -170,6 +170,11 @@ Changes in existing checks
   <clang-tidy/checks/misc-redundant-expression>` involving assignments in
   conditions. This fixes `Issue 35853 <https://github.com/llvm/llvm-project/issues/35853>`_.
 
+- Fixed a false positive in :doc:`modernize-deprecated-headers
+  <clang-tidy/checks/modernize-deprecated-headers>` involving including
+  C header files from C++ files wrapped by ``extern "C" { ... }`` blocks.
+  Such includes will be ignored by now.
+
 - Improved :doc:`performance-inefficient-vector-operation
   <clang-tidy/checks/performance-inefficient-vector-operation>` to work when
   the vector is a member of a structure.

clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-deprecated-headers/mylib.h

@@ -0,0 +1 @@
+#include "assert.h"

clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-extern-c.cpp

@@ -0,0 +1,53 @@
+// RUN: %check_clang_tidy -std=c++11-or-later %s modernize-deprecated-headers %t \
+// RUN:   -- -header-filter='.*' -system-headers \
+// RUN:      -extra-arg-before=-isystem%S/Inputs/modernize-deprecated-headers
+
+#define EXTERN_C extern "C"
+
+extern "C++" {
+// We should still have the warnings here.
+#include <stdbool.h>
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: including 'stdbool.h' has no effect in C++; consider removing it [modernize-deprecated-headers]
+}
+
+#include <assert.h>
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: inclusion of deprecated C++ header 'assert.h'; consider using 'cassert' instead [modernize-deprecated-headers]
+// CHECK-FIXES: {{^}}#include <cassert>{{$}}
+
+#include <stdbool.h>
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: including 'stdbool.h' has no effect in C++; consider removing it [modernize-deprecated-headers]
+
+#include <mylib.h>
+// CHECK-MESSAGES: mylib.h:1:10: warning: inclusion of deprecated C++ header 'assert.h'; consider using 'cassert' instead [modernize-deprecated-headers]
+
+namespace wrapping {
+extern "C" {
+#include <assert.h>  // no-warning
+#include <mylib.h>   // no-warning
+#include <stdbool.h> // no-warning
+}
+} // namespace wrapping
+
+extern "C" {
+namespace wrapped {
+#include <assert.h>  // no-warning
+#include <mylib.h>   // no-warning
+#include <stdbool.h> // no-warning
+} // namespace wrapped
+}
+
+namespace wrapping {
+extern "C" {
+namespace wrapped {
+#include <assert.h>  // no-warning
+#include <mylib.h>   // no-warning
+#include <stdbool.h> // no-warning
+} // namespace wrapped
+}
+} // namespace wrapping
+
+EXTERN_C {
+#include <assert.h>  // no-warning
+#include <mylib.h>   // no-warning
+#include <stdbool.h> // no-warning
+}