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 @@ -4,6 +4,7 @@ DefinitionsInHeadersCheck.cpp MiscTidyModule.cpp MisplacedConstCheck.cpp + MissingHeaderFileDeclarationCheck.cpp NewDeleteOverloadsCheck.cpp NonCopyableObjects.cpp NonPrivateMemberVariablesInClassesCheck.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 "NonCopyableObjects.h" #include "NonPrivateMemberVariablesInClassesCheck.h" @@ -33,6 +34,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( 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,38 @@ +//===--- 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" + +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; + +private: + const bool CheckAnyHeader; +}; + +} // 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,155 @@ +//===--- 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 + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace misc { + +struct DecomposedFileName { + StringRef NamePart; + StringRef Extension; + + bool isSource() const { + return llvm::StringSwitch(Extension) + .CasesLower("c", "cc", "cxx", "cpp", true) + .Default(false); + } + bool isHeader() const { return !isSource(); } +}; + +static llvm::Optional +decomposeFileName(StringRef FileName) { + // Undecided about using llvm::sys::path + StringRef::size_type Pos = FileName.find_last_of("\\/"); + if (Pos != StringRef::npos) { + FileName = FileName.drop_front(Pos + 1); + } + Pos = FileName.find_last_of('.'); + if (Pos == StringRef::npos) + return None; + return DecomposedFileName{FileName.take_front(Pos), + FileName.drop_front(Pos + 1)}; +} + +template +static constexpr bool IsRedeclarableNamedDecl = + std::is_base_of, T>::value + &&std::is_base_of::value; + +template +static void checkDefinition(const Redecl &Decl, const SourceManager &SM, + bool AnyHeader, StringRef PrettyTypeName, + const DiagEmitter &Diags) { + static_assert(IsRedeclarableNamedDecl, + "Not a Named Redeclarable Decl"); + llvm::Optional File = + decomposeFileName(SM.getFilename(Decl.getBeginLoc())); + if (!File || File->isHeader()) + // File being detected as a header would occur if clang-tidy was being + // invoked on a header file. + // If the file is a header and this gets tripped, probably not a good thing + // either, definitions in headers is a receipe for disaster, but that't not + // what this specific check is looking for. + return; + for (const auto *RD : Decl.redecls()) { + SourceLocation BeginLoc = RD->getBeginLoc(); + if (SM.isInMainFile(BeginLoc)) + continue; + if (AnyHeader) + return; // Found a good candidate for matching decl + if (llvm::Optional ThisFile = + decomposeFileName(SM.getFilename(BeginLoc))) { + if (File->NamePart.startswith_lower(ThisFile->NamePart)) + return; // Found a good candidate for matching decl + } + } + Diags(Decl.getLocation(), + "%0 %1 is defined with external linkage but has no corresponding " + "declaration in %select{the corresponding|any}2 header file") + << PrettyTypeName << &Decl << static_cast(AnyHeader); +} + +template +void checkExternWrittenDecl(const Redecl &Decl, const SourceManager &SM, + StringRef PrettyTypeName, + const DiagEmitter &Diags) { + static_assert(IsRedeclarableNamedDecl, + "Not a Named Redeclarable Decl"); + if (llvm::Optional File = + decomposeFileName(SM.getFilename(Decl.getBeginLoc()))) { + if (File->isSource()) + Diags(Decl.getLocation(), "%0 %1 is declared as extern in a source file") + << PrettyTypeName << &Decl; + } +} + +template +void checkDecl(const Redecl &Decl, const SourceManager &SM, bool AnyHeader, + StringRef PrettyTypeName, const DiagEmitter &Diags) { + if (Decl.isThisDeclarationADefinition()) + checkDefinition(Decl, SM, AnyHeader, PrettyTypeName, Diags); + else if (Decl.getStorageClass() == StorageClass::SC_Extern) + checkExternWrittenDecl(Decl, SM, PrettyTypeName, Diags); +} + +MissingHeaderFileDeclarationCheck::MissingHeaderFileDeclarationCheck( + StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + CheckAnyHeader(Options.get("CheckAnyHeader", false)) {} + +void MissingHeaderFileDeclarationCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "CheckAnyHeader", CheckAnyHeader); +} + +void MissingHeaderFileDeclarationCheck::registerMatchers(MatchFinder *Finder) { + using clang::ast_matchers::isTemplateInstantiation; + if (getLangOpts().ObjC) + return; + Finder->addMatcher( + varDecl(hasExternalFormalLinkage(), isExpansionInMainFile(), + unless(anyOf(isConstexpr(), isTemplateInstantiation()))) + .bind("VarDecl"), + this); + Finder->addMatcher( + functionDecl(hasExternalFormalLinkage(), isExpansionInMainFile(), + unless(anyOf(isConstexpr(), isInline(), cxxMethodDecl(), + isMain(), isTemplateInstantiation()))) + .bind("FunctionDecl"), + this); +} + +void MissingHeaderFileDeclarationCheck::check( + const MatchFinder::MatchResult &Result) { + auto DiagEmitter = [&](SourceLocation Loc, StringRef Msg) { + return diag(Loc, Msg); + }; + + if (const auto *VD = Result.Nodes.getNodeAs("VarDecl")) + checkDecl(*VD, *Result.SourceManager, CheckAnyHeader, "Variable", + DiagEmitter); + if (const auto *FD = Result.Nodes.getNodeAs("FunctionDecl")) + checkDecl(*FD, *Result.SourceManager, CheckAnyHeader, "Function", + DiagEmitter); +} + +} // 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 @@ -88,6 +88,12 @@ Flags use of the `C` standard library functions ``memset``, ``memcpy`` and ``memcmp`` and similar derivatives on non-trivial types. +- 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 aliases ^^^^^^^^^^^ 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 @@ -188,6 +188,7 @@ `llvm-twine-local `_, "Yes" `misc-definitions-in-headers `_, "Yes" `misc-misplaced-const `_, + `misc-missing-header-file-declaration `_, "Yes" `misc-new-delete-overloads `_, `misc-non-copyable-objects `_, `misc-non-private-member-variables-in-classes `_, 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,27 @@ +.. 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 the corresponding header file. + +A corresponding header is a header file whose name appears at the start of +name for the source file. + +.. code-block:: c + + // In source file + #include "corresponding_header.h" + + +Options +------- + +.. option:: CheckAnyHeader + + If set to `0` the check will only look in a header file corresponding to the + source file for a matching declaration. + If set to `1` the check will look in any included header file for a matching + declaration. + Default value is `0`. 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,24 @@ +#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 + +#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,16 @@ +// RUN: %check_clang_tidy %s misc-missing-header-file-declaration %t -- \ +// RUN: -config='{CheckOptions: \ +// RUN: [{key: misc-missing-header-file-declaration.CheckAnyHeader, value: 1}]}' \ +// 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,87 @@ +// RUN: %check_clang_tidy %s misc-missing-header-file-declaration %t -- \ +// 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(); +}