diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt @@ -7,6 +7,7 @@ DefinitionsInHeadersCheck.cpp MiscTidyModule.cpp MisplacedConstCheck.cpp + MissingHeaderFileDeclarationCheck.cpp NewDeleteOverloadsCheck.cpp NoRecursionCheck.cpp NonCopyableObjects.cpp diff --git a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp --- a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp @@ -11,6 +11,7 @@ #include "../ClangTidyModuleRegistry.h" #include "DefinitionsInHeadersCheck.h" #include "MisplacedConstCheck.h" +#include "MissingHeaderFileDeclarationCheck.h" #include "NewDeleteOverloadsCheck.h" #include "NoRecursionCheck.h" #include "NonCopyableObjects.h" @@ -34,6 +35,8 @@ CheckFactories.registerCheck( "misc-definitions-in-headers"); CheckFactories.registerCheck("misc-misplaced-const"); + CheckFactories.registerCheck( + "misc-missing-header-file-declaration"); CheckFactories.registerCheck( "misc-new-delete-overloads"); CheckFactories.registerCheck("misc-no-recursion"); diff --git a/clang-tools-extra/clang-tidy/misc/MissingHeaderFileDeclarationCheck.h b/clang-tools-extra/clang-tidy/misc/MissingHeaderFileDeclarationCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/MissingHeaderFileDeclarationCheck.h @@ -0,0 +1,45 @@ +//===--- MissingHeaderFileDeclarationCheck.h - clang-tidy -------*- C++ -*-===// +// +// 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 +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MISSINGHEADERFILEDECLARATIONCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MISSINGHEADERFILEDECLARATIONCHECK_H + +#include "../ClangTidyCheck.h" +#include "../utils/FileExtensionsUtils.h" + +namespace clang { +namespace tidy { +namespace misc { + +/// Flags definitions with external linkage in source files that don't have a +/// declaration in the corresponding header file. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/misc-missing-header-file-declaration.html +class MissingHeaderFileDeclarationCheck : public ClangTidyCheck { +public: + MissingHeaderFileDeclarationCheck(StringRef Name, ClangTidyContext *Context); + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override; + + bool isImplementationFile(StringRef Filename) const; + bool checkCorrespongdingHeader() const { return CheckCorrespondingHeaders; } + +private: + utils::FileExtensionsSet ImplementationFileExtensions; + const std::string RawStringImplementationFileExtensions; + const bool CheckCorrespondingHeaders; +}; + +} // namespace misc +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MISSINGHEADERFILEDECLARATIONCHECK_H diff --git a/clang-tools-extra/clang-tidy/misc/MissingHeaderFileDeclarationCheck.cpp b/clang-tools-extra/clang-tidy/misc/MissingHeaderFileDeclarationCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/MissingHeaderFileDeclarationCheck.cpp @@ -0,0 +1,187 @@ +//===--- MissingHeaderFileDeclarationCheck.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 "MissingHeaderFileDeclarationCheck.h" + +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Basic/SourceLocation.h" +#include "clang/Basic/Specifiers.h" +#include "llvm/ADT/Optional.h" +#include "llvm/ADT/StringRef.h" +#include "llvm/ADT/StringSwitch.h" +#include "llvm/Support/Path.h" +#include + +using namespace clang::ast_matchers; +namespace path = llvm::sys::path; + +namespace clang { +namespace tidy { +namespace misc { + +template +static constexpr bool IsRedeclarableNamedDecl = + std::is_base_of, T>::value + &&std::is_base_of::value; + +template >> +static void checkDefinition(MissingHeaderFileDeclarationCheck &Check, + const Redecl &Decl, const SourceManager &SM, + StringRef PrettyTypeName) { + + SourceLocation Loc = SM.getExpansionLoc(Decl.getBeginLoc()); + + llvm::StringRef Filename = SM.getFilename(Loc); + auto FID = SM.getFileID(Loc); + if (FID.isInvalid()) + return; + + // This check should only find decls in the main file, and we only run the + // check if the main file is an implementation file. + if (!Check.isImplementationFile(Filename)) { + SmallString<128> Message{Filename, ": Name = '", Decl.getName(), "'"}; + llvm::errs() << "ERROR: " << Message << '\n'; + llvm_unreachable(Message.c_str()); + } + + StringRef Stem = path::stem(Filename); + assert(!Stem.empty()); + + for (const auto *RD : Decl.redecls()) { + SourceLocation BeginLoc = RD->getBeginLoc(); + if (SM.isInFileID(BeginLoc, FID)) + continue; + if (!Check.checkCorrespongdingHeader()) + return; // Found a good candidate for matching decl + StringRef ThisStem = path::stem(SM.getFilename(BeginLoc)); + if (!ThisStem.empty() && Stem.startswith_lower(ThisStem)) + return; // Found a good candidate for matching decl + } + Check.diag(Decl.getLocation(), + "%0 %1 is defined with external linkage but has no corresponding " + "declaration in %select{any|a corresponding}2 header file") + << PrettyTypeName << &Decl + << static_cast(Check.checkCorrespongdingHeader()); +} + +template >> +static void checkExternWrittenDecl(MissingHeaderFileDeclarationCheck &Check, + const Redecl &Decl, const SourceManager &SM, + StringRef PrettyTypeName) { + assert(Check.isImplementationFile(SM.getFilename(Decl.getBeginLoc()))); + Check.diag(Decl.getLocation(), "%0 %1 is declared as extern in a source file") + << PrettyTypeName << &Decl; +} + +template +static void checkDecl(MissingHeaderFileDeclarationCheck &Check, + const Redecl &Decl, const SourceManager &SM, + StringRef PrettyTypeName) { + if (Decl.isThisDeclarationADefinition()) + checkDefinition(Check, Decl, SM, PrettyTypeName); + else if (Decl.getStorageClass() == StorageClass::SC_Extern) + checkExternWrittenDecl(Check, Decl, SM, PrettyTypeName); +} + +static StringRef getPrettyTagName(TagTypeKind Kind) { + switch (Kind) { + case TTK_Struct: + return "Struct"; + case TTK_Class: + return "Class"; + case TTK_Enum: + return "Enum"; + case TTK_Union: + return "Union"; + case TTK_Interface: + return "Interface"; + } + llvm_unreachable("Unknown Tag Kind"); +} + +MissingHeaderFileDeclarationCheck::MissingHeaderFileDeclarationCheck( + StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + RawStringImplementationFileExtensions(Options.getLocalOrGlobal( + "ImplementationFileExtensions", + utils::defaultImplementationFileExtensions())), + CheckCorrespondingHeaders( + Options.get("CheckCorrespondingHeaders", false)) { + if (!utils::parseFileExtensions(RawStringImplementationFileExtensions, + ImplementationFileExtensions, + utils::defaultFileExtensionDelimiters())) { + llvm::errs() << "Invalid implementation file extension: " + << RawStringImplementationFileExtensions << "\n"; + } +} + +void MissingHeaderFileDeclarationCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "CheckCorrespondingHeaders", CheckCorrespondingHeaders); + Options.store(Opts, "RawStringImplementationFileExtensions", + RawStringImplementationFileExtensions); +} + +void MissingHeaderFileDeclarationCheck::registerMatchers(MatchFinder *Finder) { + using clang::ast_matchers::isTemplateInstantiation; + + Finder->addMatcher( + varDecl(hasExternalFormalLinkage(), isExpansionInMainFile(), + unless(anyOf(isConstexpr(), isTemplateInstantiation(), + isExplicitTemplateSpecialization()))) + .bind("VD"), + this); + Finder->addMatcher( + functionDecl(hasExternalFormalLinkage(), isExpansionInMainFile(), + unless(anyOf(isConstexpr(), isInline(), cxxMethodDecl(), + isMain(), isTemplateInstantiation(), + isExplicitTemplateSpecialization()))) + .bind("FD"), + this); + Finder->addMatcher( + tagDecl(hasExternalFormalLinkage(), isExpansionInMainFile(), + unless(cxxRecordDecl(anyOf(isTemplateInstantiation(), + isExplicitTemplateSpecialization())))) + .bind("TD"), + this); +} + +void MissingHeaderFileDeclarationCheck::check( + const MatchFinder::MatchResult &Result) { + if (const auto *VD = Result.Nodes.getNodeAs("VD")) + checkDecl(*this, *VD, *Result.SourceManager, "Variable"); + if (const auto *FD = Result.Nodes.getNodeAs("FD")) + checkDecl(*this, *FD, *Result.SourceManager, "Function"); + if (const auto *TD = Result.Nodes.getNodeAs("TD")) + if (TD->isThisDeclarationADefinition()) { + checkDefinition(*this, *TD, *Result.SourceManager, + getPrettyTagName(TD->getTagKind())); + } +} + +bool MissingHeaderFileDeclarationCheck::isImplementationFile( + StringRef Filename) const { + StringRef Ext = path::extension(Filename); + Ext.consume_front("."); + return ImplementationFileExtensions.contains(Ext); +} + +bool MissingHeaderFileDeclarationCheck::isLanguageVersionSupported( + const LangOptions &LangOpts) const { + // Disable running this check on a file that isn't classed as an + // implementation file. can occur when running in clangd. + if (!isImplementationFile(getCurrentMainFile())) + return false; + return !LangOpts.ObjC; +} +} // namespace misc +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -124,6 +124,12 @@ Alias to the :doc:`bugprone-signal-handler ` check. +- New :doc:`misc-missing-header-file-declaration + ` check. + + Flags definitions with external linkage in source files that don't have a + declaration in the corresponding header file. + - New :doc:`readability-function-cognitive-complexity ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -201,6 +201,7 @@ `llvmlibc-restrict-system-libc-headers `_, "Yes" `misc-definitions-in-headers `_, "Yes" `misc-misplaced-const `_, + `misc-missing-header-file-declaration `_, `misc-new-delete-overloads `_, `misc-no-recursion `_, `misc-non-copyable-objects `_, diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc-missing-header-file-declaration.rst b/clang-tools-extra/docs/clang-tidy/checks/misc-missing-header-file-declaration.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/misc-missing-header-file-declaration.rst @@ -0,0 +1,43 @@ +.. title:: clang-tidy - misc-missing-header-file-declaration + +misc-missing-header-file-declaration +==================================== + +Flags variable and function definitions with external linkage in source files +that don't have a declaration in a header file. + +Giving a symbol external linkage indicates that you intend that symbol to be +usable within other translation units, suggesting there should be a declaration +of the symbol in a header. No such symbol was found in this translation unit. + +If you want the symbol to be accessible from other translation units, add a +declaration of it to a suitable header file included as part of this translation +unit. Otherwise, give the definition internal linkage, using static or an +anonymous namespace. + +If you think the symbol is already declared in a file that's included as part of +this translation unit, check for any mismatches between the declaration and +definition, including the namespace, spelling and any arguments. + + +Options +------- + +.. option:: CheckCorrespondingHeaders + + If set to `true` the check will only look in a header file corresponding to the + source file for a matching declaration. + If set to `false` the check will look in any included header file for a matching + declaration. + Default value is `false`. + + A Corresponding header is one whose name appears at the start of the source + file name. `Example.h` is a corresponding header to `ExampleImpl.cpp` + +.. option:: ImplementationFileExtensions + + Default value: ``"c;cc;cpp;cxx"`` + A semicolon-separated list of filename extensions of implementation files + (the filename extensions should not contain a "." prefix). The check will + only run on files that match these extensions. + diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/misc-missing-header-file-declaration/misc-missing-header-file-declaration.h b/clang-tools-extra/test/clang-tidy/checkers/Inputs/misc-missing-header-file-declaration/misc-missing-header-file-declaration.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/misc-missing-header-file-declaration/misc-missing-header-file-declaration.h @@ -0,0 +1,26 @@ +#ifndef MISC_MISSING_HEADER_FILE_DECLARATON_H +#define MISC_MISSING_HEADER_FILE_DECLARATON_H + +extern bool DeclInHeader; +extern bool DeclInBoth; + +extern void declInHeader(); +extern void declInBoth(); + +struct Foo { + static int Bar; +}; + +namespace ns1 { +extern bool NS; +extern void nS(); +} // namespace ns1 + +namespace ns2 { +extern bool NS; +extern void nS(); +} // namespace ns2 + +struct SWithHeader; + +#endif diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/misc-missing-header-file-declaration/wrong_header.h b/clang-tools-extra/test/clang-tidy/checkers/Inputs/misc-missing-header-file-declaration/wrong_header.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/misc-missing-header-file-declaration/wrong_header.h @@ -0,0 +1,7 @@ +#ifndef WRONG_HEADER +#define WRONG_HEADER + +extern bool DeclInWrongHeader; +extern void declInWrongHeader(); + +#endif diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc-missing-header-file-declaration-any-header.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc-missing-header-file-declaration-any-header.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/misc-missing-header-file-declaration-any-header.cpp @@ -0,0 +1,14 @@ +// RUN: %check_clang_tidy %s misc-missing-header-file-declaration %t -- \ +// RUN: -- -I%S/Inputs/misc-missing-header-file-declaration + +#include "wrong_header.h" +// Checking any header, this is declared so no warning + +bool DeclInWrongHeader = false; +// CHECK-MESSAGES-NOT: :[[@LINE-1]]:6: warning: Variable 'DeclInWrongHeader' is defined with external linkage +void declInWrongHeader() {} +// CHECK-MESSAGES-NOT: :[[@LINE-1]]:6: warning: Function 'declInWrongHeader' is defined with external linkage + +// Test doesn't like receiving no messages so insert a dummy warning +bool Failure = false; +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: Variable 'Failure' is defined with external linkage diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc-missing-header-file-declaration.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc-missing-header-file-declaration.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/misc-missing-header-file-declaration.cpp @@ -0,0 +1,102 @@ +// RUN: %check_clang_tidy %s misc-missing-header-file-declaration %t -- \ +// RUN: -config='{CheckOptions: \ +// RUN: [{key: misc-missing-header-file-declaration.CheckCorrespondingHeaders, value: 1}]}' \ +// RUN: -- -I%S/Inputs/misc-missing-header-file-declaration + +#include "misc-missing-header-file-declaration.h" +#include "wrong_header.h" + +// These declarations should be ignored by the check as they are in the same +// file. +extern bool DeclInSource; +// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: Variable 'DeclInSource' is declared as extern in a source file +extern void declInSource(); +// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: Function 'declInSource' is declared as extern in a source file + +// These declarations should be ignored by the check as they are in the same +// file, however there is a corresponding decl in the header that will prevent +// a failing check. +extern bool DeclInBoth; +// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: Variable 'DeclInBoth' is declared as extern in a source file +extern void declInBoth(); +// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: Function 'declInBoth' is declared as extern in a source file + +// No external linkage so no warning +static bool StaticOK = false; +constexpr bool ConstexprOK = false; +inline void inlineOK() {} +static void staticOK() {} +constexpr bool constexprOK() { return true; } + +// External linkage but decl in header so no warning +bool DeclInHeader = false; +bool DeclInBoth = false; +void declInHeader() {} +void declInBoth() {} + +//Decls don't appear in corresponding header so issue a warning +bool DeclInSource = false; +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: Variable 'DeclInSource' is defined with external linkage +bool NoDecl = false; +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: Variable 'NoDecl' is defined with external linkage +void declInSource() {} +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: Function 'declInSource' is defined with external linkage +void noDecl() {} +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: Function 'noDecl' is defined with external linkage + +bool DeclInWrongHeader = false; +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: Variable 'DeclInWrongHeader' is defined with external linkage +void declInWrongHeader() {} +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: Function 'declInWrongHeader' is defined with external linkage + +// Decls in an anonymous namespace don't have external linkage, so no warning +// should be emitted +namespace { +bool AnonNS = false; +void anonNS() {} +} // namespace + +// Ensure in namespace definitions are correctly resolved +namespace ns1 { +bool NS = false; +void nS() {} +} // namespace ns1 + +// Ensure out of namespace definitions are correctly resolved +bool /*namespace*/ ns2::NS = false; +void /*namespace*/ ns2::nS() {} + +// Static class members declared in the header shouldn't be warned on. +int /*struct*/ Foo::Bar = 0; + +// main is special, don't warn for it. +int main() { +} + +template +void templateFuncNoHeader() {} +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: Function 'templateFuncNoHeader' is defined with external linkage + +// Warn on explicit instantiations +template <> +void templateFuncNoHeader() {} +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: Function 'templateFuncNoHeader' is defined with external linkage + +static void foo() { + // We don't want warnings for these implicit instantations + templateFuncNoHeader(); + templateFuncNoHeader(); +} + +struct SNoHeader {}; +// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: Struct 'SNoHeader' is defined with external linkage + +namespace { +struct SAnonNS {}; +} // namespace + +struct SWithHeader {}; + +struct SWithSourceDecl; +struct SWithSourceDecl {}; +// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: Struct 'SWithSourceDecl' is defined with external linkage \ No newline at end of file