[clang-tidy] Improve check cert-dcl58-cpp.

[balazske]

Summary

Detect template specializations that should be handled specially.
In some cases it is allowed to extend the std namespace with
template specializations.

[aaron.ballman]

clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp

29

Might as well give this a better name so it's less similar to List.

92

This one is missing the system header check, but I wonder if that can be handled once in the call to addMatcher() instead of needing to be sprinkled here?

135

You should add the newline back.

[balazske]

address review comments

[aaron.ballman]

LGTM with some minor nits.

clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp

80–81

This one can go as well, right?

clang-tools-extra/test/clang-tidy/checkers/cert/dcl58-cpp.cpp

209

Can you add a comment above this to mention that an alias declaration is the same as a typedef, and does not introduce a user-defined type, which is why the below fails.

256

Same mention here about an alias declaration not being a user-defined type.

[balazske]

Added comments, removed unneeded AST matcher code.

[njames93]

This appears to be causing a crash with friend declarations. The crash happens during matching, see https://github.com/llvm/llvm-project/issues/56902

clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp

 //===--- DontModifyStdNamespaceCheck.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 "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<TemplateArgument>, InnerMatcher) {
+  ArrayRef<TemplateArgument> Args =
+      clang::ast_matchers::internal::getTemplateSpecializationArgs(Node);
+  for (const auto &Arg : Args) {
+    if (Arg.getKind() != TemplateArgument::Pack)
+      continue;
+    ArrayRef<TemplateArgument> 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"),
+  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)));
+
+  // 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.
+
+  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);
+
+  Finder->addMatcher(decl(anyOf(BadNonTemplateSpecializationDecl,
+                                BadClassTemplateSpec, BadInnerClassTemplateSpec,
+                                BadFunctionTemplateSpec, BadMemberFunctionSpec),
+                          unless(isExpansionInSystemHeader()))
+                         .bind("decl"),
                      this);
 }
+} // namespace cert
+} // namespace tidy
+} // namespace clang
 
-void DontModifyStdNamespaceCheck::check(
-    const MatchFinder::MatchResult &Result) {
-  const auto *N = Result.Nodes.getNodeAs<NamespaceDecl>("nmspc");
+static const NamespaceDecl *getTopLevelLexicalNamespaceDecl(const Decl *D) {
+  const NamespaceDecl *LastNS = nullptr;
+  while (D) {
+    if (const auto *NS = dyn_cast<NamespaceDecl>(D))
+      LastNS = NS;
+    D = dyn_cast_or_null<Decl>(D->getLexicalDeclContext());
+  }
+  return LastNS;
+}
 
-  // Only consider top level namespaces.
-  if (N->getParent() != Result.Context->getTranslationUnitDecl())
+void clang::tidy::cert::DontModifyStdNamespaceCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *D = Result.Nodes.getNodeAs<Decl>("decl");
+  const auto *NS = Result.Nodes.getNodeAs<NamespaceDecl>("nmspc");
+  if (!D || !NS)
     return;
 
-  diag(N->getLocation(),
+  diag(D->getLocation(),
        "modification of %0 namespace can result in undefined behavior")
-      << N;
+      << 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;
+  }
 }
-
-} // namespace cert
-} // namespace tidy
-} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

- Fixed bugs in :doc:`bugprone-use-after-move
   <clang-tidy/checks/bugprone/use-after-move>`:
 
   - Treat a move in a lambda capture as happening in the function that defines
     the lambda, not within the body of the lambda (as we were previously doing
     erroneously).
 
   - Don't emit an erroneous warning on self-moves.
 
+- Improved :doc:`cert-dcl58-cpp
+  <clang-tidy/checks/cert/dcl58-cpp>` check.
+
+  The check now detects explicit template specializations that are handled specially.
+
 - Made :doc:`cert-oop57-cpp <clang-tidy/checks/cert/oop57-cpp>` more sensitive
   by checking for an arbitrary expression in the second argument of ``memset``.
 
 - Made the fix-it of :doc:`cppcoreguidelines-init-variables
   <clang-tidy/checks/cppcoreguidelines/init-variables>` use ``false`` to initialize
   boolean variables.
 
 - Improved :doc:`cppcoreguidelines-prefer-member-initializer

clang-tools-extra/docs/clang-tidy/checks/cert/dcl58-cpp.rst

 .. title:: clang-tidy - cert-dcl58-cpp
 
 cert-dcl58-cpp
 ==============
 
 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<long> { // 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<MyData> { // no warning: specialization with user-defined type
+    unsigned long operator()(const MyData &K) const {
+      return K.data;
+    }
+  };
+
+  namespace std {
+    template <>
+    void swap<bool>(bool &a, bool &b); // warning: modification of 'std' namespace can result in undefined behavior
+
+    template <>
+    bool less<void>::operator()<MyData &&, MyData &&>(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
 <https://www.securecoding.cert.org/confluence/display/cplusplus/DCL58-CPP.+Do+not+modify+the+standard+namespaces>`_.

clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/system-header-simulation.h

 template <bool B>
 using bool_constant = integral_constant<bool, B>;
 using true_type = bool_constant<true>;
 using false_type = bool_constant<false>;
 
 template<class T>
 struct is_error_code_enum : false_type {};
 
-template<class T>
+template <class T>
 void swap(T &a, T &b);
+
+enum class io_errc {
+  stream = 1,
+};
+
+template <class... Types>
+class tuple;
+
+template <typename T = void>
+class less;
+
+template <>
+class less<void> {
+public:
+  template <typename T, typename U>
+  bool operator()(T &&Lhs, U &&Rhs) const {
+    return static_cast<T &&>(Lhs) < static_cast<U &&>(Rhs);
+  }
+  template <typename A, typename B = int>
+  struct X {};
+};
+
+template <class Key>
+struct hash;
+
+template <class T>
+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 &, short &){};
+
+template <>
+struct is_error_code_enum<short> : true_type {};
+
+template <>
+bool less<void>::operator()<short &&, short &&>(short &&, short &&) const {
+  return false;
 }
 
+template <>
+struct less<void>::X<short> {};
+} // namespace std

clang-tools-extra/test/clang-tidy/checkers/cert/dcl58-cpp.cpp

-// 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"
 
 namespace A {
   namespace B {
     int b;
   }
 }
 
 namespace A {
   namespace B {
     int c;
   }
 }
 
 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
+// CHECK-MESSAGES: :[[@LINE+2]]:5: warning: modification of 'std' namespace
+// CHECK-MESSAGES: :[[@LINE-5]]:11: note: 'std' namespace opened here
+int stdInt1;
 }
 
 namespace foobar {
   namespace std {
     int bar;
   }
 }
 
+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<MyError> : std::true_type {};
 
+// no-warning: Function template specialized by a program-defined type.
 template<>
 void swap<MyError>(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<bool> : 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 <>
+void swap<bool>(bool &, bool &);
+// CHECK-MESSAGES: :[[@LINE+3]]:6: warning: modification of 'std' namespace
+// CHECK-MESSAGES: :[[@LINE-12]]:11: note: 'std' namespace opened here
 template <>
-struct is_error_code_enum<MyError2> : std::true_type {};
+void swap<ConstBoolPtr>(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::io_errc> : 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 &, std::io_errc &);
+} // namespace std
+
+// parameter pack, has program-defined type
+namespace std {
+// no-warning: there is one program-defined type.
+template <>
+class tuple<int, MyError, std::io_errc> {};
+} // 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<int, const std::io_errc, float> {};
+} // 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::Outer::Inner> : 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::detail::X> : 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<void>::operator()<int &&, float &&>(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<void>::operator()<MyError &&, MyError &&>(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<void>::X<bool> {};
+// CHECK-MESSAGES: :[[@LINE+3]]:20: warning: modification of 'std' namespace
+// CHECK_MESSAGES: :[[@LINE-6]]:11: note: 'std' namespace opened here
+template <>
+struct less<void>::X<MyError> {};
+// CHECK-MESSAGES: :[[@LINE+3]]:20: warning: modification of 'std' namespace
+// CHECK_MESSAGES: :[[@LINE-10]]:11: note: 'std' namespace opened here
+template <typename T>
+struct less<void>::X<MyError, T> {};
+} // 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<void>::operator()<int &&, int &&>(int &&, int &&) const {
+  return true;
 }
 
-using namespace std;
+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();
+  }
+};
 
-int x;
+using myint = int;
 
+// 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<myint> {
+  // 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;
+  }
+};
+
+// CHECK-MESSAGES: :[[@LINE+2]]:15: warning: modification of 'std' namespace
+template <>
+struct ::std::hash<long> {
+  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