diff --git a/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp b/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp --- a/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp +++ b/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp @@ -9,37 +9,126 @@ #include "DontModifyStdNamespaceCheck.h" #include "clang/AST/ASTContext.h" #include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchersInternal.h" +using namespace clang; using namespace clang::ast_matchers; +namespace { + +AST_POLYMORPHIC_MATCHER_P( + hasAnyTemplateArgumentIncludingPack, + AST_POLYMORPHIC_SUPPORTED_TYPES(ClassTemplateSpecializationDecl, + TemplateSpecializationType, FunctionDecl), + clang::ast_matchers::internal::Matcher, InnerMatcher) { + ArrayRef Args = + clang::ast_matchers::internal::getTemplateSpecializationArgs(Node); + for (const auto &Arg : Args) { + if (Arg.getKind() != TemplateArgument::Pack) + continue; + ArrayRef PackArgs = Arg.getPackAsArray(); + if (matchesFirstInRange(InnerMatcher, PackArgs.begin(), PackArgs.end(), + Finder, Builder) != PackArgs.end()) + return true; + } + return matchesFirstInRange(InnerMatcher, Args.begin(), Args.end(), Finder, + Builder) != Args.end(); +} + +} // namespace + namespace clang { namespace tidy { namespace cert { void DontModifyStdNamespaceCheck::registerMatchers(MatchFinder *Finder) { - Finder->addMatcher( - namespaceDecl(unless(isExpansionInSystemHeader()), - hasAnyName("std", "posix"), - has(decl(unless(anyOf( - functionDecl(isExplicitTemplateSpecialization()), - cxxRecordDecl(isExplicitTemplateSpecialization())))))) - .bind("nmspc"), - this); -} + auto HasStdParent = + hasDeclContext(namespaceDecl(hasAnyName("std", "posix"), + unless(hasParent(namespaceDecl()))) + .bind("nmspc")); + auto UserDefinedType = qualType( + hasUnqualifiedDesugaredType(tagType(unless(hasDeclaration(tagDecl( + hasAncestor(namespaceDecl(hasAnyName("std", "posix"), + unless(hasParent(namespaceDecl())))))))))); + auto HasNoProgramDefinedTemplateArgument = unless( + hasAnyTemplateArgumentIncludingPack(refersToType(UserDefinedType))); + auto InsideStdClassOrClassTemplateSpecialization = hasDeclContext( + anyOf(cxxRecordDecl(HasStdParent), + classTemplateSpecializationDecl( + HasStdParent, HasNoProgramDefinedTemplateArgument))); -void DontModifyStdNamespaceCheck::check( - const MatchFinder::MatchResult &Result) { - const auto *N = Result.Nodes.getNodeAs("nmspc"); + // Try to follow exactly CERT rule DCL58-CPP (this text is taken from C++ + // standard into the CERT rule): + // " + // 1 The behavior of a C++ program is undefined if it adds declarations or + // definitions to namespace std or to a namespace within namespace std unless + // otherwise specified. A program may add a template specialization for any + // standard library template to namespace std only if the declaration depends + // on a user-defined type and the specialization meets the standard library + // requirements for the original template and is not explicitly prohibited. 2 + // The behavior of a C++ program is undefined if it declares — an explicit + // specialization of any member function of a standard library class template, + // or — an explicit specialization of any member function template of a + // standard library class or class template, or — an explicit or partial + // specialization of any member class template of a standard library class or + // class template. + // " + // The "standard library requirements" and explicit prohibition are not + // checked. - // Only consider top level namespaces. - if (N->getParent() != Result.Context->getTranslationUnitDecl()) - return; + auto BadNonTemplateSpecializationDecl = + decl(unless(anyOf(functionDecl(isExplicitTemplateSpecialization()), + varDecl(isExplicitTemplateSpecialization()), + cxxRecordDecl(isExplicitTemplateSpecialization()))), + HasStdParent); + auto BadClassTemplateSpec = classTemplateSpecializationDecl( + HasNoProgramDefinedTemplateArgument, HasStdParent); + auto BadInnerClassTemplateSpec = classTemplateSpecializationDecl( + InsideStdClassOrClassTemplateSpecialization); + auto BadFunctionTemplateSpec = + functionDecl(unless(cxxMethodDecl()), isExplicitTemplateSpecialization(), + HasNoProgramDefinedTemplateArgument, HasStdParent); + auto BadMemberFunctionSpec = + cxxMethodDecl(isExplicitTemplateSpecialization(), + InsideStdClassOrClassTemplateSpecialization); - diag(N->getLocation(), - "modification of %0 namespace can result in undefined behavior") - << N; + Finder->addMatcher(decl(anyOf(BadNonTemplateSpecializationDecl, + BadClassTemplateSpec, BadInnerClassTemplateSpec, + BadFunctionTemplateSpec, BadMemberFunctionSpec), + unless(isExpansionInSystemHeader())) + .bind("decl"), + this); } - } // namespace cert } // namespace tidy } // namespace clang + +static const NamespaceDecl *getTopLevelLexicalNamespaceDecl(const Decl *D) { + const NamespaceDecl *LastNS = nullptr; + while (D) { + if (const auto *NS = dyn_cast(D)) + LastNS = NS; + D = dyn_cast_or_null(D->getLexicalDeclContext()); + } + return LastNS; +} + +void clang::tidy::cert::DontModifyStdNamespaceCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *D = Result.Nodes.getNodeAs("decl"); + const auto *NS = Result.Nodes.getNodeAs("nmspc"); + if (!D || !NS) + return; + + diag(D->getLocation(), + "modification of %0 namespace can result in undefined behavior") + << NS; + // 'NS' is not always the namespace declaration that lexically contains 'D', + // try to find such a namespace. + if (const NamespaceDecl *LexNS = getTopLevelLexicalNamespaceDecl(D)) { + assert(NS->getCanonicalDecl() == LexNS->getCanonicalDecl() && + "Mismatch in found namespace"); + diag(LexNS->getLocation(), "%0 namespace opened here", DiagnosticIDs::Note) + << LexNS; + } +} 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 @@ -183,6 +183,11 @@ - Don't emit an erroneous warning on self-moves. +- Improved :doc:`cert-dcl58-cpp + ` check. + + The check now detects explicit template specializations that are handled specially. + - Made :doc:`cert-oop57-cpp ` more sensitive by checking for an arbitrary expression in the second argument of ``memset``. diff --git a/clang-tools-extra/docs/clang-tidy/checks/cert/dcl58-cpp.rst b/clang-tools-extra/docs/clang-tidy/checks/cert/dcl58-cpp.rst --- a/clang-tools-extra/docs/clang-tidy/checks/cert/dcl58-cpp.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/cert/dcl58-cpp.rst @@ -6,15 +6,54 @@ Modification of the ``std`` or ``posix`` namespace can result in undefined behavior. This check warns for such modifications. +The ``std`` (or ``posix``) namespace is allowed to be extended with (class or +function) template specializations that depend on an user-defined type (a type +that is not defined in the standard system headers). + +The check detects the following (user provided) declarations in namespace ``std`` or ``posix``: + +- Anything that is not a template specialization. +- Explicit specializations of any standard library function template or class template, if it does not have any user-defined type as template argument. +- Explicit specializations of any member function of a standard library class template. +- Explicit specializations of any member function template of a standard library class or class template. +- Explicit or partial specialization of any member class template of a standard library class or class template. Examples: .. code-block:: c++ namespace std { - int x; // May cause undefined behavior. + int x; // warning: modification of 'std' namespace can result in undefined behavior [cert-dcl58-cpp] } + namespace posix::a { // warning: modification of 'posix' namespace can result in undefined behavior + } + + template <> + struct ::std::hash { // warning: modification of 'std' namespace can result in undefined behavior + unsigned long operator()(const long &K) const { + return K; + } + }; + + struct MyData { long data; }; + + template <> + struct ::std::hash { // no warning: specialization with user-defined type + unsigned long operator()(const MyData &K) const { + return K.data; + } + }; + + namespace std { + template <> + void swap(bool &a, bool &b); // warning: modification of 'std' namespace can result in undefined behavior + + template <> + bool less::operator()(MyData &&, MyData &&) const { // warning: modification of 'std' namespace can result in undefined behavior + return true; + } + } This check corresponds to the CERT C++ Coding Standard rule `DCL58-CPP. Do not modify the standard namespaces diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/system-header-simulation.h b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/system-header-simulation.h --- a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/system-header-simulation.h +++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/system-header-simulation.h @@ -18,7 +18,60 @@ template struct is_error_code_enum : false_type {}; -template +template void swap(T &a, T &b); + +enum class io_errc { + stream = 1, +}; + +template +class tuple; + +template +class less; + +template <> +class less { +public: + template + bool operator()(T &&Lhs, U &&Rhs) const { + return static_cast(Lhs) < static_cast(Rhs); + } + template + struct X {}; +}; + +template +struct hash; + +template +class numeric_limits; + +struct Outer { + struct Inner {}; +}; + +namespace detail { +struct X {}; +} // namespace detail + +} // namespace std + +// Template specializations that are in a system-header file. +// The purpose is to test cert-dcl58-cpp (no warnings here). +namespace std { +template <> +void swap(short &, short &){}; + +template <> +struct is_error_code_enum : true_type {}; + +template <> +bool less::operator()(short &&, short &&) const { + return false; } +template <> +struct less::X {}; +} // namespace std diff --git a/clang-tools-extra/test/clang-tidy/checkers/cert/dcl58-cpp.cpp b/clang-tools-extra/test/clang-tidy/checkers/cert/dcl58-cpp.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/cert/dcl58-cpp.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/cert/dcl58-cpp.cpp @@ -1,4 +1,4 @@ -// RUN: %check_clang_tidy %s cert-dcl58-cpp %t -- -- -std=c++1z -I %clang_tidy_headers +// RUN: %check_clang_tidy -std=c++17-or-later %s cert-dcl58-cpp %t -- -- -I %clang_tidy_headers #include "system-header-simulation.h" @@ -15,14 +15,20 @@ } namespace posix { -// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: modification of 'posix' namespace can result in undefined behavior [cert-dcl58-cpp] - namespace vmi { - } +// CHECK-MESSAGES: :[[@LINE+2]]:11: warning: modification of 'posix' namespace can result in undefined behavior [cert-dcl58-cpp] +// CHECK-MESSAGES: :[[@LINE-2]]:11: note: 'posix' namespace opened here +namespace foo { +int foobar; +} } namespace std { -// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: modification of 'std' namespace can - int stdInt; +// CHECK-MESSAGES: :[[@LINE+2]]:5: warning: modification of 'std' namespace +// CHECK-MESSAGES: :[[@LINE-2]]:11: note: 'std' namespace opened here +int stdInt; +// CHECK-MESSAGES: :[[@LINE+2]]:5: warning: modification of 'std' namespace +// CHECK-MESSAGES: :[[@LINE-5]]:11: note: 'std' namespace opened here +int stdInt1; } namespace foobar { @@ -31,37 +37,234 @@ } } +namespace posix { +// CHECK-MESSAGES: :[[@LINE+2]]:11: warning: modification of 'posix' namespace +// CHECK-MESSAGES: :[[@LINE-2]]:11: note: 'posix' namespace opened here +namespace std { +} +} // namespace posix + namespace posix::a { -// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: modification of 'posix' namespace +// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: modification of 'posix' namespace +// CHECK-MESSAGES: :[[@LINE-2]]:11: note: 'posix' namespace opened here } +namespace std { +// no-warning: empty +} // namespace std + +namespace std { +// Warn for non-NamedDecls as well. +// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: modification of 'std' namespace +// CHECK-MESSAGES: :[[@LINE-3]]:11: note: 'std' namespace opened here +static_assert(1 == 1, "non-NamedDecl"); +} // namespace std + enum class MyError { ErrorA, ErrorB }; namespace std { +// no-warning: Class template specialized by a program-defined type. template <> struct is_error_code_enum : std::true_type {}; +// no-warning: Function template specialized by a program-defined type. template<> void swap(MyError &a, MyError &b); } -enum class MyError2 { - Error2A, - Error2B -}; +using ConstBoolPtr = const bool *; namespace std { -// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: modification of 'std' namespace +// class template, builtin type +// CHECK-MESSAGES: :[[@LINE+3]]:8: warning: modification of 'std' namespace +// CHECK-MESSAGES: :[[@LINE-3]]:11: note: 'std' namespace opened here +template <> +struct is_error_code_enum : std::true_type {}; +// function template, builtin type +// CHECK-MESSAGES: :[[@LINE+3]]:6: warning: modification of 'std' namespace +// CHECK-MESSAGES: :[[@LINE-8]]:11: note: 'std' namespace opened here template <> -struct is_error_code_enum : std::true_type {}; +void swap(bool &, bool &); +// CHECK-MESSAGES: :[[@LINE+3]]:6: warning: modification of 'std' namespace +// CHECK-MESSAGES: :[[@LINE-12]]:11: note: 'std' namespace opened here +template <> +void swap(ConstBoolPtr &, ConstBoolPtr &); +} // namespace std -int foobar; +namespace std { +// class template, std type +// CHECK-MESSAGES: :[[@LINE+3]]:8: warning: modification of 'std' namespace +// CHECK-MESSAGES: :[[@LINE-3]]:11: note: 'std' namespace opened here +template <> +struct is_error_code_enum : std::true_type {}; +// function template, std type +// CHECK-MESSAGES: :[[@LINE+3]]:6: warning: modification of 'std' namespace +// CHECK-MESSAGES: :[[@LINE-8]]:11: note: 'std' namespace opened here +template <> +void swap(std::io_errc &, std::io_errc &); +} // namespace std + +// parameter pack, has program-defined type +namespace std { +// no-warning: there is one program-defined type. +template <> +class tuple {}; +} // namespace std + +// parameter pack, only builtin or std type +namespace std { +// Forbid variadic specializations over only `std::` or builtin types. +// CHECK-MESSAGES: :[[@LINE+3]]:7: warning: modification of 'std' namespace +// CHECK-MESSAGES: :[[@LINE-3]]:11: note: 'std' namespace opened here +template <> +class tuple {}; +} // namespace std + +namespace std { +// Test nested standard declarations. +// CHECK-MESSAGES: :[[@LINE+3]]:8: warning: modification of 'std' namespace +// CHECK-MESSAGES: :[[@LINE-3]]:11: note: 'std' namespace opened here +template <> +struct is_error_code_enum : std::true_type {}; +} // namespace std + +namespace std { +// Test nested namespace. +// CHECK-MESSAGES: :[[@LINE+3]]:8: warning: modification of 'std' namespace +// CHECK-MESSAGES: :[[@LINE-3]]:11: note: 'std' namespace opened here +template <> +struct is_error_code_enum : std::true_type {}; +} // namespace std + +// Test member function template specializations. +namespace std { +// CHECK-MESSAGES: :[[@LINE+3]]:18: warning: modification of 'std' namespace +// CHECK_MESSAGES: :[[@LINE-2]]:11: note: 'std' namespace opened here +template <> +bool less::operator()(int &&, float &&) const { + return true; +} +// CHECK-MESSAGES: :[[@LINE+3]]:18: warning: modification of 'std' namespace +// CHECK_MESSAGES: :[[@LINE-8]]:11: note: 'std' namespace opened here +template <> +bool less::operator()(MyError &&, MyError &&) const { + return true; } +} // namespace std + +// Test member class template specializations. +namespace std { +// CHECK-MESSAGES: :[[@LINE+3]]:20: warning: modification of 'std' namespace +// CHECK_MESSAGES: :[[@LINE-2]]:11: note: 'std' namespace opened here +template <> +struct less::X {}; +// CHECK-MESSAGES: :[[@LINE+3]]:20: warning: modification of 'std' namespace +// CHECK_MESSAGES: :[[@LINE-6]]:11: note: 'std' namespace opened here +template <> +struct less::X {}; +// CHECK-MESSAGES: :[[@LINE+3]]:20: warning: modification of 'std' namespace +// CHECK_MESSAGES: :[[@LINE-10]]:11: note: 'std' namespace opened here +template +struct less::X {}; +} // namespace std + +// We did not open the 'std' namespace, but still specialized the member +// function of 'std::less'. +// CHECK-MESSAGES: :[[@LINE+3]]:23: warning: modification of 'std' namespace +// no-note: There is no opening of 'std' namespace, hence no note emitted. +template <> +bool std::less::operator()(int &&, int &&) const { + return true; +} + +namespace SpaceA { +namespace SpaceB { +class MapKey { + int Type = 0; + +public: + MapKey() = default; + int getType() const { return Type; } +}; +} // namespace SpaceB +} // namespace SpaceA + +// no-warning: Specializing for 'std::hash' for a program-defined type. +template <> +struct std::hash<::SpaceA::SpaceB::MapKey> { + // no-warning + unsigned long operator()(const ::SpaceA::SpaceB::MapKey &K) const { + return K.getType(); + } + // no-warning + bool operator()(const ::SpaceA::SpaceB::MapKey &K1, + const ::SpaceA::SpaceB::MapKey &K2) const { + return K1.getType() < K2.getType(); + } +}; + +using myint = int; -using namespace std; +// The type alias declaration is the same as typedef, does not introduce a +// program-defined type. +// CHECK-MESSAGES: :[[@LINE+2]]:13: warning: modification of 'std' namespace +template <> +struct std::hash { + // no-warning: The warning was already reported for the struct itself. + unsigned long operator()(const myint &K) const { + return K; + } + // no-warning: The warning was already reported for the struct itself. + bool operator()(const myint &K1, + const myint &K2) const { + return K1 < K2; + } +}; -int x; +// CHECK-MESSAGES: :[[@LINE+2]]:15: warning: modification of 'std' namespace +template <> +struct ::std::hash { + unsigned long operator()(const long &K) const { + return K; + } +}; +namespace ranges { +namespace detail { +struct diffmax_t {}; +using LongT = long; +} // namespace detail +} // namespace ranges + +namespace std { +// no-warning: specialization with an user-defined type +template <> +struct numeric_limits<::ranges::detail::diffmax_t> { + static constexpr bool is_signed = true; + static constexpr bool is_integer = true; + static constexpr ::ranges::detail::diffmax_t max() noexcept { + return {}; + } +}; +inline constexpr bool numeric_limits<::ranges::detail::diffmax_t>::is_signed; +inline constexpr bool numeric_limits<::ranges::detail::diffmax_t>::is_integer; +} // namespace std + +namespace std { +// specialization with type alias to non-program-defined-type +// CHECK-MESSAGES: :[[@LINE+3]]:8: warning: modification of 'std' namespace +// CHECK_MESSAGES: :[[@LINE-3]]:11: note: 'std' namespace opened here +template <> +struct numeric_limits<::ranges::detail::LongT> { + static constexpr bool is_signed = true; + static constexpr bool is_integer = true; + static constexpr ::ranges::detail::LongT max() noexcept { + return 1; + } +}; +inline constexpr bool numeric_limits<::ranges::detail::LongT>::is_signed; +inline constexpr bool numeric_limits<::ranges::detail::LongT>::is_integer; +} // namespace std