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

[steakhal]

Summary

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

#ifdef __cplusplus
extern "C" {
#endif

#include "assert.h" // <-- actual include

#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.

---

The patch did not show measurable runtime difference on the following projects:
llvm-project,contour,codechecker,qtbase,protobuf,bitcoin,xerces,libwebm,tinyxml2,postgres,ffmpeg,sqlite,openssl,vim,twin,curl,tmux,memcached

So, even though the O(N * M): N includes, M extern decls might seem rough at first, it turns out to be negligible in practice.

[steakhal]

Fix the RUN line

[whisperity]

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

60–63

This unconditionally assumes that an encountered LSD will be extern "C". However, extern "C++" is also possible (and supported by Clang). (And things like extern "Java" are also possible according to the standard, albeit not supported by Clang.)

82–124

(🔮: Same here. Is this state that might keep a dangling reference if multiple TUs are consumed in sequence?)

87

(Perhaps it is worth an explicit comment here that you do this at first glance unconventional match because the entire check's logic is calculated only on the "preprocessor" level.)

136

POSIX defines a "global" include limits.h which gives you the definition of macros like PATH_MAX. Will such code keep on working if the include is turned into climits?

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

49

🔮: You are storing some TU-specific state here (SourceRange) that is not cleared. What happens if you run clang-tidy a.cpp b.cpp, i.e. two TUs in the same process execution?

[steakhal]

• Ignore extern "C++" blocks; also added a test for this.
• Also ignore LinkageSpecDecls without braces.
• Clear the IncludesToBeProcessed list at the end of each translation unit.

[steakhal]

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

60–63

O.O I did not know its a thing :D
It seems like clang only supports C or C++.

82–124

How can I acquire the right SourceManager if not like this?
I haven't found any alternative.
What do you suggest?

136

IDK.

[whisperity]

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

82–124

Is the PP a different variable when a new translation unit is consumed by the same check instance? If so, then there is no problem. A simple debug print llvm::errs() << PP << '\n';, recompile, re-run with clang-tidy a.cpp b.cpp should show how the infrastructure behaves. I do not know the exact behaviour of the preprocessor layer.

[whisperity]

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

136

Did an investigation on Ubuntu with G++ 7.5. So it turns out that the GNU G++ standard library implementation, on Linux, is implemented in a way that it will include the POSIX-specific limits.h into the C Standard limits.h and thus also climits.

And

Out of these projects llvm-project,contour,codechecker,qtbase,protobuf,bitcoin,xerces,libwebm,tinyxml2,postgres,ffmpeg,sqlite,openssl,vim,twin,curl,tmux,memcached, it suppressed 5 reports

Well, Contour, tmux and Qt (and perhaps even LLVM's OS-specific support lib) SHOULD be heavily reliant on interfacing with POSIX stuff when compiled against Linux, so I guess we can say this transformation is not dangerous.

[steakhal]

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

82–124

Okay, I've checked this. The lifetimes checks out. Thanks!

$ ./build/debug/bin/clang-tidy a.cpp b.cpp  -checks="-*,modernize-deprecated-headers"
DeprecatedHeadersCheck::registerPPCallbacks() acquires SM 0x27eb9e0
ctor IncludeModernizePPCallbacks acquires SM 0x27eb9e0
IncludeModernizePPCallbacks::InclusionDirective inserts to IncludesToBeProcessed using SM 0x27eb9e0
IncludeModernizePPCallbacks::InclusionDirective inserts to IncludesToBeProcessed using SM 0x27eb9e0
DeprecatedHeadersCheck::check() emits IncludesToBeProcessed using SM 0x27eb9e0
onEndOfTranslationUnit cleares IncludesToBeProcessed
1 warning generated.
dtor IncludeModernizePPCallbacks releases SM 0x27eb9e0
DeprecatedHeadersCheck::registerPPCallbacks() acquires SM 0x27eb9e0
ctor IncludeModernizePPCallbacks acquires SM 0x27eb9e0
IncludeModernizePPCallbacks::InclusionDirective inserts to IncludesToBeProcessed using SM 0x27eb9e0
IncludeModernizePPCallbacks::InclusionDirective inserts to IncludesToBeProcessed using SM 0x27eb9e0
DeprecatedHeadersCheck::check() emits IncludesToBeProcessed using SM 0x27eb9e0
onEndOfTranslationUnit cleares IncludesToBeProcessed
2 warnings generated.
dtor IncludeModernizePPCallbacks releases SM 0x27eb9e0
a.cpp:2:10: warning: inclusion of deprecated C++ header 'assert.h'; consider using 'cassert' instead [modernize-deprecated-headers]
#include <assert.h>
         ^~~~~~~~~~
         <cassert>
b.cpp:2:10: warning: inclusion of deprecated C++ header 'assert.h'; consider using 'cassert' instead [modernize-deprecated-headers]
#include <assert.h>
         ^~~~~~~~~~
         <cassert>

[whisperity]

LGTM. Some typos inline. Also I think this is a significant enough bug-fix that it warrants an entry in the Release Notes under Changes to existing checks.

Also, the tests are failing, but it fails on the formatting of the test file (???), and not the actual test itself. (Usually such issues would be sent back to Phab to appear an a machine-made inline comment, I am not sure what happened to that logic.)

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

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

69–72

(llvm/ADT/STLExtras.h define a range-based lower_bound and upper_bound)

89

[whisperity]

(Also: this is a fix to an issue of your own finding and not something that was reported on Bugzilla? Just to make sure we cross-reference and close tickets.)

[steakhal]

In D125209#3508573, @whisperity wrote:

LGTM. Some typos inline. Also I think this is a significant enough bug-fix that it warrants an entry in the Release Notes under Changes to existing checks.

Also, the tests are failing, but it fails on the formatting of the test file (???), and not the actual test itself. (Usually such issues would be sent back to Phab to appear an a machine-made inline comment, I am not sure what happened to that logic.)

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

Thanks.

In D125209#3508576, @whisperity wrote:

(Also: this is a fix to an issue of your own finding and not something that was reported on Bugzilla? Just to make sure we cross-reference and close tickets.)

It was an internal ticket.

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

69–72

Yup, but I'm using a different range.
The first call could be replaced by the llvm ranged version, but the second starts with Low instead of Begin.
At that point, I decided to keep the symmetry and use the std versions in both cases.

Well, I could llvm::make_range() and use the llvm::upper_bound but yea, no.

[steakhal]

• Fix typos.
• Mention this in the release notes.
• clang-format the test file as well.

[thakis]

Looks like this breaks tests: http://45.33.8.238/linux/76033/step_8.txt

Please take a look and revert for now if it takes a while to fix.

[steakhal]

It seems like we cannot use the sorting I did. The warning within the mylib.h header will have a different FileID than the original TU. Consequently, the upper_bound() lower_bound() range will be empty even if the extern "C" { ... } block.

I might need to fallback to the isBeforeInTranslationUnit or something like that. But I fear, if that's the case, the complexity will be O(n * m) for n extern "C" block and m includes. We will see.

[steakhal]

• Copy the mylib.h under the build directory to have deterministic relative order to the LIT test file.
• 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.

[LegalizeAdulthood]

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

93

In modernize-macro-to-enum I had to similarly discard macros defined inside a top-level decl. I did decl(hasParent(translationUnitDecl())) to match the top-level decls. Would that simplify your check as well?

125–126

IMO it's poor style to define entities that are declared within a namespace outside the namespace within which they were declared.

Is there some reason this isn't defined with the rest of the methods on the callback class?

[steakhal]

• Move all the details stuff into an anonymous namespace.
• Pass the std::vector<IncludeMarker> &IncludesToBeProcessed directly to the callbacks.
• Sink the IncludeMarker definition into the public part of DeprecatedHeadersCheck.

[steakhal]

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

93

The problem is that extern "C" {...} blocks might be within some namespace declarations.
https://godbolt.org/z/vr5jYx5az

125–126

I eradicated the use of detail namespace.
It indeed looks better.

[steakhal]

WDYT, are there any blocker issues with this patch stack?

[LegalizeAdulthood]

LGTM

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

 /// ~~~
 ///
 /// Example: ``<stdio.h> => <cstdio>``
 ///
 /// For the user-facing documentation see:
 /// 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
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_C_HEADERS_TO_CXX_H

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

 //===--- DeprecatedHeadersCheck.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 "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,
                           CharSourceRange FilenameRange,
                           Optional<FileEntryRef> File, StringRef SearchPath,
                           StringRef RelativePath, const Module *Imported,
                           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);
+}
+
+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(ClangTidyCheck &Check,
-                                                         LangOptions LangOpts)
-    : Check(Check), LangOpts(LangOpts) {
+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"},
             {"complex.h", "complex"},
             {"ctype.h", "cctype"},
             {"errno.h", "cerrno"},
             {"float.h", "cfloat"},
             {"limits.h", "climits"},
             {"locale.h", "clocale"},
             {"math.h", "cmath"},
             {"setjmp.h", "csetjmp"},
             {"signal.h", "csignal"},
             {"stdarg.h", "cstdarg"},
             {"stddef.h", "cstddef"},
             {"stdio.h", "cstdio"},
             {"stdlib.h", "cstdlib"},
void IncludeModernizePPCallbacks::InclusionDirective(
     SrcMgr::CharacteristicKind FileType) {
   // FIXME: Take care of library symbols from the global namespace.
   //
   // Reasonable options for the check:
   //
   // 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});
   }
 }
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

 - Fixed a false positive in :doc:`misc-redundant-expression
   <clang-tidy/checks/misc-redundant-expression>` involving overloaded
   comparison operators.
 
 - Fixed a false positive in :doc:`misc-redundant-expression
   <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.
 
 - Fixed a crash in :doc:`readability-const-return-type
   <clang-tidy/checks/readability-const-return-type>` when a pure virtual function
   overrided has a const return type. Removed the fix for a virtual function.
 

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

+#include <assert.h>

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

+#include <assert.h>

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

+
+// 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
+}