Reland "[clang-tidy] modernize-deprecated-headers check should respect extern "C" blocks""

This partially reverts commit e8cae48702.
Changes since that commit:
 - Use `SourceManager::isBeforeInTranslationUnit` instead of the fancy
   decomposed decl logarithmic search.
 - Add a test for including a system header containing a deprecated
   include.
 - Add `REQUIRES: system-linux` clause to the test.

Reviewed By: LegalizeAdulthood, whisperity

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

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

@@ -7,23 +7,27 @@
 //===----------------------------------------------------------------------===//
 
 #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>
 
+using IncludeMarker =
+    clang::tidy::modernize::DeprecatedHeadersCheck::IncludeMarker;
 namespace clang {
 namespace tidy {
 namespace modernize {
-
 namespace {
+
 class IncludeModernizePPCallbacks : public PPCallbacks {
 public:
-  explicit IncludeModernizePPCallbacks(ClangTidyCheck &Check,
-                                       LangOptions LangOpts);
+  explicit IncludeModernizePPCallbacks(
+      std::vector<IncludeMarker> &IncludesToBeProcessed, LangOptions LangOpts);
 
   void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
                           StringRef FileName, bool IsAngled,
@@ -33,22 +37,95 @@ public:
                           SrcMgr::CharacteristicKind FileType) override;
 
 private:
-  ClangTidyCheck &Check;
+  std::vector<IncludeMarker> &IncludesToBeProcessed;
   LangOptions LangOpts;
   llvm::StringMap<std::string> CStyledHeaderToCxx;
   llvm::StringSet<> DeleteHeaders;
 };
+
+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 = LinkSpecDecl->getBeginLoc();
+    auto ExternCBlockEnd = LinkSpecDecl->getEndLoc();
+    auto IsWrapped = [=, &SM = SM](const IncludeMarker &Marker) -> bool {
+      return SM.isBeforeInTranslationUnit(ExternCBlockBegin, Marker.DiagLoc) &&
+             SM.isBeforeInTranslationUnit(Marker.DiagLoc, ExternCBlockEnd);
+    };
+
+    llvm::erase_if(IncludesToBeProcessed, IsWrapped);
+    return true;
+  }
+};
 } // namespace
 
+DeprecatedHeadersCheck::DeprecatedHeadersCheck(StringRef Name,
+                                               ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context) {}
+
 void DeprecatedHeadersCheck::registerPPCallbacks(
     const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
-    PP->addPPCallbacks(
-        ::std::make_unique<IncludeModernizePPCallbacks>(*this, getLangOpts()));
+  PP->addPPCallbacks(std::make_unique<IncludeModernizePPCallbacks>(
+      IncludesToBeProcessed, getLangOpts()));
+}
+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);
 }
 
-IncludeModernizePPCallbacks::IncludeModernizePPCallbacks(ClangTidyCheck &Check,
-                                                         LangOptions LangOpts)
-    : Check(Check), LangOpts(LangOpts) {
+void DeprecatedHeadersCheck::onEndOfTranslationUnit() {
+  IncludesToBeProcessed.clear();
+}
+
+void DeprecatedHeadersCheck::check(
+    const ast_matchers::MatchFinder::MatchResult &Result) {
+  SourceManager &SM = Result.Context->getSourceManager();
+
+  // Suppress includes wrapped by `extern "C" { ... }` blocks.
+  ExternCRefutationVisitor Visitor(IncludesToBeProcessed, SM);
+  Visitor.TraverseAST(*Result.Context);
+
+  // Emit all the remaining reports.
+  for (const IncludeMarker &Marker : IncludesToBeProcessed) {
+    if (Marker.Replacement.empty()) {
+      diag(Marker.DiagLoc,
+           "including '%0' has no effect in C++; consider removing it")
+          << Marker.FileName
+          << FixItHint::CreateRemoval(Marker.ReplacementRange);
+    } else {
+      diag(Marker.DiagLoc, "inclusion of deprecated C++ header "
+                           "'%0'; consider using '%1' instead")
+          << Marker.FileName << Marker.Replacement
+          << FixItHint::CreateReplacement(
+                 Marker.ReplacementRange,
+                 (llvm::Twine("<") + Marker.Replacement + ">").str());
+    }
+  }
+}
+
+IncludeModernizePPCallbacks::IncludeModernizePPCallbacks(
+    std::vector<IncludeMarker> &IncludesToBeProcessed, LangOptions LangOpts)
+    : IncludesToBeProcessed(IncludesToBeProcessed), LangOpts(LangOpts) {
   for (const auto &KeyValue :
        std::vector<std::pair<llvm::StringRef, std::string>>(
            {{"assert.h", "cassert"},
@@ -101,19 +178,15 @@ 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.
+  SourceLocation DiagLoc = 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);
+    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()));
+    IncludesToBeProcessed.push_back(
+        IncludeMarker{std::string{}, FileName,
+                      SourceRange{HashLoc, FilenameRange.getEnd()}, DiagLoc});
   }
 }
 

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

@@ -34,13 +34,25 @@ namespace modernize {
 /// http://clang.llvm.org/extra/clang-tidy/checks/modernize-deprecated-headers.html
 class DeprecatedHeadersCheck : public ClangTidyCheck {
 public:
-  DeprecatedHeadersCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+  DeprecatedHeadersCheck(StringRef Name, ClangTidyContext *Context);
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
     return LangOpts.CPlusPlus;
   }
   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;
+
+  struct IncludeMarker {
+    std::string Replacement;
+    StringRef FileName;
+    SourceRange ReplacementRange;
+    SourceLocation DiagLoc;
+  };
+
+private:
+  std::vector<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/Inputs/modernize-deprecated-headers/mysystemlib.h

@@ -0,0 +1 @@
+#include <assert.h>

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

@@ -0,0 +1,66 @@
+
+// Copy the 'mylib.h' to a directory under the build directory. This is
+// required, since the relative order of the emitted diagnostics depends on the
+// absolute file paths which is sorted by clang-tidy prior emitting.
+//
+// RUN: mkdir -p %t/sys && mkdir -p %t/usr \
+// RUN:   && cp %S/Inputs/modernize-deprecated-headers/mysystemlib.h %t/sys/mysystemlib.h \
+// RUN:   && cp %S/Inputs/modernize-deprecated-headers/mylib.h       %t/usr/mylib.h
+
+// RUN: %check_clang_tidy -std=c++11 %s modernize-deprecated-headers %t \
+// RUN:   --header-filter='.*' --system-headers \
+// RUN:   -- -I %t/usr -isystem %t/sys -isystem %S/Inputs/modernize-deprecated-headers
+
+// REQUIRES: system-linux
+
+#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]
+
+#include <stdbool.h>
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: including 'stdbool.h' has no effect in C++; consider removing it [modernize-deprecated-headers]
+
+#include <mysystemlib.h> // FIXME: We should have no warning into system headers.
+// CHECK-MESSAGES: mysystemlib.h:1:10: warning: inclusion of deprecated C++ header 'assert.h'; consider using 'cassert' instead [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
+}