[clang-tidy] Introduce the CheckHeaderFile option to modernize-deprecated-headers

[steakhal]

Summary

Unfortunately, we must restrict the checker to warn for deprecated headers
only if the header is included directly from a c++ source file.

For header files, we cannot know if the project has a C source file
that also directly/indirectly includes the offending header file
otherwise. Thus, it's better to be on the safe side and suppress those
reports.

One can opt-in the old behavior, emitting diagnostics into header files,
if one explicitly sets the CheckHeaderFile=true, in which case nothing
will be changed.

[LegalizeAdulthood]

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

57

1. How is this different from the clang-tidy option that specifies whether or not fixits are applied to header files?
   
   As an owner of a code base, I would know which header files are included from C source files and I would set my header-file regex (honestly, not a fan of a regex for that option; I'd prefer white/black lists, but that's another discussion) to exclude header files that are known to be included in C source files.

2. Assuming that the header-file regex is somehow insufficient to cover this scenario, I like the functionality but the name of this option feels "off". (Naming things is hard.) Elsewhere we have options that say HeaderFile not Headers and Into just doesn't sound like the way normal conversation would state the situation. Something like CheckHeaderFile would be more consistent with existing options.

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

2–4

IMO, all of this hackery is simply ducking the issue, which is that check_clang_tidy.py doesn't have proper support for validating fixits applied to header files.

See https://reviews.llvm.org/D17482

[steakhal]

rebase

[whisperity]

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

57

I do not know the answer to question 1, but as an owner of a code-base, I would not want to know or deal with keeping additional lists (be it file-by-file, glob expressions, or regex...) specific to what headers are includable from C and what aren't. Especially considering that every C header could be included from C++. (Note how LLVM uses .h instead of .hpp for its headers, even though >90% of our headers would never compile in C mode...) If a check is misbehaving for my codebase, I'd just simply disable that check (1 line of code) and go on with my life, instead of creating a curated list, that is at least 2 LoC in a config file, and has to be maintained down the line...

Moreover, the -header-filter regex and -system-headers are Tidy-level global flags. This means that I would either have to let go of EVERY potential diagnostic that might be placed in headers, or tinker with my environment to run multiple instances of Clang-Tidy with different configurations.

[steakhal]

• Renamed WarnIntoHeaders -> CheckHeaderFile
• I forgot to extend the check's docs about this option. I've addressed that one too.

[steakhal]

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

2–4

I agree that this is nasty, but this was the only way I found to still test the feature I'm about to introduce.

[LegalizeAdulthood]

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

57

Those are all good points. One of the weakest aspects of clang-tidy is the selection of files that should be analyzed and/or fixed. Otherwise, we wouldn't need clang-tidy/tool/run-clang-tidy.py at all.

[LegalizeAdulthood]

clang-tools-extra/docs/ReleaseNotes.rst

180

Capitalize to C++

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

2–4

Agreed

[LegalizeAdulthood]

LGTM
Address one nit and ship.

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

 /// 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);
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
     return LangOpts.CPlusPlus;
   }
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   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;
+  bool CheckHeaderFile;
 };
 
 } // 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

 namespace clang {
 namespace tidy {
 namespace modernize {
 namespace {
 
 class IncludeModernizePPCallbacks : public PPCallbacks {
 public:
   explicit IncludeModernizePPCallbacks(
-      std::vector<IncludeMarker> &IncludesToBeProcessed, LangOptions LangOpts);
+      std::vector<IncludeMarker> &IncludesToBeProcessed, LangOptions LangOpts,
+      const SourceManager &SM, bool CheckHeaderFile);
 
   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:
   std::vector<IncludeMarker> &IncludesToBeProcessed;
   LangOptions LangOpts;
   llvm::StringMap<std::string> CStyledHeaderToCxx;
   llvm::StringSet<> DeleteHeaders;
+  const SourceManager &SM;
+  bool CheckHeaderFile;
 };
 
 class ExternCRefutationVisitor
     : public RecursiveASTVisitor<ExternCRefutationVisitor> {
   std::vector<IncludeMarker> &IncludesToBeProcessed;
   const SourceManager &SM;
 
 public:
bool VisitLinkageSpecDecl(LinkageSpecDecl *LinkSpecDecl) const {
     llvm::erase_if(IncludesToBeProcessed, IsWrapped);
     return true;
   }
 };
 } // namespace
 
 DeprecatedHeadersCheck::DeprecatedHeadersCheck(StringRef Name,
                                                ClangTidyContext *Context)
-    : ClangTidyCheck(Name, Context) {}
+    : ClangTidyCheck(Name, Context),
+      CheckHeaderFile(Options.get("CheckHeaderFile", false)) {}
+
+void DeprecatedHeadersCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "CheckHeaderFile", CheckHeaderFile);
+}
 
 void DeprecatedHeadersCheck::registerPPCallbacks(
     const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
   PP->addPPCallbacks(std::make_unique<IncludeModernizePPCallbacks>(
-      IncludesToBeProcessed, getLangOpts()));
+      IncludesToBeProcessed, getLangOpts(), PP->getSourceManager(),
+      CheckHeaderFile));
 }
 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
if (Marker.Replacement.empty()) {
           << FixItHint::CreateReplacement(
                  Marker.ReplacementRange,
                  (llvm::Twine("<") + Marker.Replacement + ">").str());
     }
   }
 }
 
 IncludeModernizePPCallbacks::IncludeModernizePPCallbacks(
-    std::vector<IncludeMarker> &IncludesToBeProcessed, LangOptions LangOpts)
-    : IncludesToBeProcessed(IncludesToBeProcessed), LangOpts(LangOpts) {
+    std::vector<IncludeMarker> &IncludesToBeProcessed, LangOptions LangOpts,
+    const SourceManager &SM, bool CheckHeaderFile)
+    : IncludesToBeProcessed(IncludesToBeProcessed), LangOpts(LangOpts), SM(SM),
+      CheckHeaderFile(CheckHeaderFile) {
   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"},
IncludeModernizePPCallbacks::IncludeModernizePPCallbacks(
   }
 }
 
 void IncludeModernizePPCallbacks::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) {
+
+  // If we don't want to warn for non-main file reports and this is one, skip
+  // it.
+  if (!CheckHeaderFile && !SM.isInMainFile(HashLoc))
+    return;
+
   // 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();

clang-tools-extra/docs/ReleaseNotes.rst

 - 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.
+  By default now it doesn't warn for including deprecated headers from header
+  files, since that header file might be used from C source files. By passing
+  the ``CheckHeaderFile=true`` option if header files of the project only
+  included by C++ source files.
 
 - 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/docs/clang-tidy/checks/modernize-deprecated-headers.rst

 .. title:: clang-tidy - modernize-deprecated-headers
 
 modernize-deprecated-headers
 ============================
 
 Some headers from C library were deprecated in C++ and are no longer welcome in
 C++ codebases. Some have no effect in C++. For more details refer to the C++ 14
 Standard [depr.c.headers] section.
 
 This check replaces C standard library headers with their C++ alternatives and
 removes redundant ones.
 
+.. code-block:: c++
+
+  // C++ source file...
+  #include <assert.h>
+  #include <stdbool.h>
+
+  // becomes
+
+  #include <cassert>
+  // No 'stdbool.h' here.
+
 Important note: the Standard doesn't guarantee that the C++ headers declare all
 the same functions in the global namespace. The check in its current form can
 break the code that uses library symbols from the global namespace.
 
 * `<assert.h>`
 * `<complex.h>`
 * `<ctype.h>`
 * `<errno.h>`

 headers deprecated before C++11, otherwise -- every header that appeared in
 the previous list.
 
 These headers don't have effect in C++:
 
 * `<iso646.h>`
 * `<stdalign.h>`
 * `<stdbool.h>`
+
+The checker ignores `include` directives within `extern "C" { ... }` blocks,
+since a library might want to expose some API for C and C++ libraries.
+
+.. code-block:: c++
+
+  // C++ source file...
+  extern "C" {
+  #include <assert.h>  // Left intact.
+  #include <stdbool.h> // Left intact.
+  }
+
+Options
+-------
+
+.. option:: CheckHeaderFile
+
+   `clang-tidy` cannot know if the header file included by the currently
+   analyzed C++ source file is not included by any other C source files.
+   Hence, to omit false-positives and wrong fixit-hints, we ignore emitting
+   reports into header files. One can set this option to `true` if they know
+   that the header files in the project are only used by C++ source file.
+   Default is `false`.

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:   -check-suffixes=DEFAULT \
+// RUN:   --header-filter='.*' --system-headers \
+// RUN:   -- -I %t/usr -isystem %t/sys -isystem %S/Inputs/modernize-deprecated-headers
+
+// RUN: %check_clang_tidy -std=c++11 %s modernize-deprecated-headers %t \
+// RUN:   -check-suffixes=DEFAULT,CHECK-HEADER-FILE \
+// RUN:   -config="{CheckOptions: [{key: modernize-deprecated-headers.CheckHeaderFile, value: 'true'}]}" \
 // 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]
+// CHECK-MESSAGES-DEFAULT: :[[@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-MESSAGES-DEFAULT: :[[@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]
+// CHECK-MESSAGES-DEFAULT: :[[@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]
+// CHECK-MESSAGES-CHECK-HEADER-FILE: 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]
+// CHECK-MESSAGES-CHECK-HEADER-FILE: 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