[clang-tidy] Add portability-std-allocator-const check

[MaskRay]

Summary

Report use of `std::vector<const T>` (and similar containers of const
elements). These are not allowed in standard C++ due to undefined
`std::allocator<const T>`. They do not compile with libstdc++ or MSVC.
Future libc++ will remove the extension (D120996).
See docs/clang-tidy/checks/portability-std-allocator-const.rst for detail.

I have attempted clean-up in a large code base. Here are some statistics:

• 98% are related to the container std::vector, among deque/forward_list/list/multiset/queue/set/stack/vector.
• 24% are related to std::vector<const std::string>.
• Both std::vector<const absl::string_view> and std::vector<const int> contribute 2%. The other contributors spread over various class types.

The check can be useful to other large code bases and may serve as an example
for future libc++ strictness improvement.

[sammccall]

This looks nice, my main substantive question is about matching VarDecl vs TypeLoc.
Maybe there's a good reason to keep it this way but I'd like to understand why.

The rest is nits, feel free to ignore any bits you disagree with.

clang-tools-extra/clang-tidy/portability/PortabilityTidyModule.cpp

28

without the punctuation, the "t" AllocatorConstT/allocator-const-t doesn't seem so obvious IMO, I'd consider dropping it from the check name.

Also the docs and even the implementation are focused on containers, "portability-const-container" might be easier to remember/understand for people not immersed in the details here.

All up to you though.

clang-tools-extra/clang-tidy/portability/StdAllocatorConstTCheck.cpp

25 ↗

(On Diff #422404)

If creating allocator<const> is always invalid, why be so careful here about which containers we're matching?

27 ↗

(On Diff #422404)

we can tell the "not caught" containers from the list below, but it'd be useful for the comment to say why

28 ↗

(On Diff #422404)

coupling this check to what libc++ currently supports makes it harder to understand, what's the cost of including unordered_set in this list anyway?

30 ↗

(On Diff #422404)

Matching VarDecl specifically is an interesting choice. Maybe a good one: it does a good job of ensuring we diagnose at an appropriate place and only once.

Other tempting choices are:

• typeloc(loc(...)) - requires the type to be spelled somewhere, I think this can work even in template instantiations
• expr(hasType(...)) - problem is it's likely to find multiple matches for the same root cause

My guess is matching the TypeLoc would work better: catching some extra cases without much downside.

31 ↗

(On Diff #422404)

Maybe you'd want to handle cases where the type is buried a bit, like vector& and vector* etc.

Matching TypeLoc would take care of this for you, as matchers will see the nested TypeLoc.

50 ↗

(On Diff #422404)

I'm a bit concerned that the second half:

• is jargon that will confuse some people
• excludes MSVC (are there others?)
• will be inaccurate once libc++ changes

Also nits: "undefined" might be confusingly close to "not defined" in the "no visible definition" sense, and "remove const to work with" reads a little oddly because it's a person that's removing but the code that's working.

What about "container using std::allocator<const T> is invalid; remove const"
or if focusing on the libc++ situation: "container using std::allocator<const T> is a libc++ extension; remove const for compatibility with other standard libraries"?

clang-tools-extra/docs/clang-tidy/checks/portability-std-allocator-const-t.rst

6 ↗

(On Diff #422404)

this seems a bit standard-ese for the intro paragraph.

Maybe add a paragraph before it: "This check reports use of vector<const T> (and similar containers of const elements). These are not allowed in standard C++, and should usually be vector<T> instead."

11 ↗

(On Diff #422404)

It's a little confusing to call out only libstdc++ here without context when libc++ is the odd-one out.

Maybe first mention that libc++ supports this as an extension that may be removed in the future?

And if we're mentioning libstdc++'s behavior we should probably mention MS STL too which is similar to libstdc++: https://godbolt.org/z/c4o5nc66v

21 ↗

(On Diff #422404)

again, I'd probably phrase this as "only compiled with libc++"

[Eugene.Zelenko]

Shouldn't this check be enabled only when libstdc++ is used?

clang-tools-extra/docs/clang-tidy/checks/portability-std-allocator-const-t.rst

6 ↗

(On Diff #422404)

Please synchronize first sentence with Release Notes.

[ldionne]

In D123655#3448472, @Eugene.Zelenko wrote:

Shouldn't this check be enabled only when libstdc++ is used?

No, because we would like to enforce this in libc++ as well!

Thanks @MaskRay for pursuing this!

[ldionne]

clang-tools-extra/docs/clang-tidy/checks/portability-std-allocator-const-t.rst

11 ↗

(On Diff #422404)

I think I'd rather not mention libc++ at all -- I don't know that we ever documented that extension, and since we're actively trying to remove it, it might be better to just not say anything. Or we could say "It works on libc++, but it's non-standard and it may not work in the future".

[MaskRay]

address comments

[MaskRay]

clang-tools-extra/clang-tidy/portability/PortabilityTidyModule.cpp

28

Renamed to portability-std-allocator-const. The intention is to catch std::allocator. If the check gets improved to catch more cases, we won't need to rename it from std-container-const to std-allocator-const :)

clang-tools-extra/clang-tidy/portability/StdAllocatorConstTCheck.cpp

30 ↗

(On Diff #422404)

Thanks for Sam's offline help. I've got a form which is able to match many cases:)

50 ↗

(On Diff #422404)

Thanks for the suggestion! Adopted the libc++ oriented diagnostic.

[sammccall]

LG from my side. It won't catch all cases, but I think it gets the common ones.

Up to you/Louis how best to describe the libc++ behavior.

clang-tools-extra/test/clang-tidy/checkers/portability-std-allocator-const.cpp

66

maybe using my_vector2 = my_vector;, with no extra diagnostics

77

This case will always be an error, but isn't diagnosed until you see an instantiation.

If it's important to catch these standalone e.g. when analyzing headers, you could adapt the matcher like:

hasUnqualifiedDesugaredType(anyOf(
  recordDecl(...), // current case
  templateSpecializationType( // when dependent, TST is canonical
    templateArgumentCountIs(1), // std::vector<const dependent>
    hasTemplateArgument(0, refersToType(qualType(isConstQualified())))
  )
))

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/portability-std-allocator-const.rst

7

Please omit This check. Also please make first sentence same as in Release Notes.

[MaskRay]

Catch std::vector<const dependent>

[MaskRay]

improve test.
mention in the diagnostic this is a deprecated libc++ extension

clang-tools-extra/test/clang-tidy/checkers/portability-std-allocator-const.cpp

77

Thanks for Sam's offline help! Incorporated this to the matcher.

[thakis]

Looks like this breaks tests on windows: http://45.33.8.238/win/56334/step_8.txt

Please take a look and revert for now if it takes a while to fix.

[thakis]

Reverted in a29d9ba1f584745c1a169da5d08f10b27623cfb0 for now.

[thakis]

Err, actually in dd47ab750b584a320bf61d58a3965a03f3871c1b

[sammccall]

Thanks Nico!

@MaskRay my guess is this is related to implicit -fdelayed-template-parsing on windows, which causes non-instantiated templates not really to be parsed.
If that's the case, best just to add -fno-delayed-template-parsing to the test and accept that these cases will be missed on windows.

clang-tools-extra/clang-tidy/portability/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
 add_clang_library(clangTidyPortabilityModule
   PortabilityTidyModule.cpp
   RestrictSystemIncludesCheck.cpp
   SIMDIntrinsicsCheck.cpp
+  StdAllocatorConstCheck.cpp
 
   LINK_LIBS
   clangTidy
   clangTidyUtils
 
   DEPENDS
   omp_gen
   )

clang-tools-extra/clang-tidy/portability/PortabilityTidyModule.cpp

 //===--- PortabilityTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "RestrictSystemIncludesCheck.h"
 #include "SIMDIntrinsicsCheck.h"
+#include "StdAllocatorConstCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace portability {
 
 class PortabilityModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<RestrictSystemIncludesCheck>(
         "portability-restrict-system-includes");
     CheckFactories.registerCheck<SIMDIntrinsicsCheck>(
         "portability-simd-intrinsics");
+    CheckFactories.registerCheck<StdAllocatorConstCheck>(
+        "portability-std-allocator-const");
   }
 };
 
 // Register the PortabilityModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<PortabilityModule>
     X("portability-module", "Adds portability-related checks.");
 
 } // namespace portability
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the PortabilityModule.
 volatile int PortabilityModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tools-extra/clang-tidy/portability/StdAllocatorConstCheck.h

+//===--- StdAllocatorConstT.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_PORTABILITY_STDALLOCATORCONSTCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PORTABILITY_STDALLOCATORCONSTCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace portability {
+
+/// Report use of ``std::vector<const T>`` (and similar containers of const
+/// elements). These are not allowed in standard C++ due to undefined
+/// ``std::allocator<const T>``. They do not compile with libstdc++ or MSVC.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/portability-std-allocator-const.html
+class StdAllocatorConstCheck : public ClangTidyCheck {
+public:
+  StdAllocatorConstCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace portability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PORTABILITY_STDALLOCATORCONSTCHECK_H

clang-tools-extra/clang-tidy/portability/StdAllocatorConstCheck.cpp

+//===-- StdAllocatorConstCheck.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 "StdAllocatorConstCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace portability {
+
+void StdAllocatorConstCheck::registerMatchers(MatchFinder *Finder) {
+  // Match std::allocator<const T>.
+  auto allocatorConst =
+      recordType(hasDeclaration(classTemplateSpecializationDecl(
+          hasName("::std::allocator"),
+          hasTemplateArgument(0, refersToType(qualType(isConstQualified()))))));
+
+  auto hasContainerName =
+      hasAnyName("::std::vector", "::std::deque", "::std::list",
+                 "::std::multiset", "::std::set", "::std::unordered_multiset",
+                 "::std::unordered_set", "::absl::flat_hash_set");
+
+  // Match `std::vector<const T> var;` and other common containers like deque,
+  // list, and absl::flat_hash_set. Containers like queue and stack use deque
+  // but do not directly use std::allocator as a template argument, so they
+  // aren't caught.
+  Finder->addMatcher(
+      typeLoc(
+          templateSpecializationTypeLoc(),
+          loc(hasUnqualifiedDesugaredType(anyOf(
+              recordType(hasDeclaration(classTemplateSpecializationDecl(
+                  hasContainerName,
+                  anyOf(
+                      hasTemplateArgument(1, refersToType(allocatorConst)),
+                      hasTemplateArgument(2, refersToType(allocatorConst)),
+                      hasTemplateArgument(3, refersToType(allocatorConst)))))),
+              // Match std::vector<const dependent>
+              templateSpecializationType(
+                  templateArgumentCountIs(1),
+                  hasTemplateArgument(
+                      0, refersToType(qualType(isConstQualified()))),
+                  hasDeclaration(namedDecl(hasContainerName)))))))
+          .bind("type_loc"),
+      this);
+}
+
+void StdAllocatorConstCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *T = Result.Nodes.getNodeAs<TypeLoc>("type_loc");
+  if (!T)
+    return;
+  // Exclude TypeLoc matches in STL headers.
+  if (isSystem(Result.Context->getSourceManager().getFileCharacteristic(
+          T->getBeginLoc())))
+    return;
+
+  diag(T->getBeginLoc(),
+       "container using std::allocator<const T> is a deprecated libc++ "
+       "extension; remove const for compatibility with other standard "
+       "libraries");
+}
+
+} // namespace portability
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`bugprone-shared-ptr-array-mismatch <clang-tidy/checks/bugprone-shared-ptr-array-mismatch>` check.
 
   Finds initializations of C++ shared pointers to non-array type that are initialized with an array.
 
 - New :doc:`modernize-macro-to-enum
   <clang-tidy/checks/modernize-macro-to-enum>` check.
 
   Replaces groups of adjacent macros with an unscoped anonymous enum.
 
+- New :doc:`portability-std-allocator-const <clang-tidy/checks/portability-std-allocator-const>` check.
+
+  Report use of ``std::vector<const T>`` (and similar containers of const
+  elements). These are not allowed in standard C++ due to undefined
+  ``std::allocator<const T>``. They do not compile with libstdc++ or MSVC.
+  Future libc++ will remove the extension (`D120996
+  <https://reviews.llvm.org/D120996>`).
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
 - New alias :doc:`cppcoreguidelines-macro-to-enum
   <clang-tidy/checks/cppcoreguidelines-macro-to-enum>` to :doc:`modernize-macro-to-enum
   <clang-tidy/checks/modernize-macro-to-enum>` was added.
 
 Changes in existing checks

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `performance-no-int-to-ptr <performance-no-int-to-ptr.html>`_,
    `performance-noexcept-move-constructor <performance-noexcept-move-constructor.html>`_, "Yes"
    `performance-trivially-destructible <performance-trivially-destructible.html>`_, "Yes"
    `performance-type-promotion-in-math-fn <performance-type-promotion-in-math-fn.html>`_, "Yes"
    `performance-unnecessary-copy-initialization <performance-unnecessary-copy-initialization.html>`_, "Yes"
    `performance-unnecessary-value-param <performance-unnecessary-value-param.html>`_, "Yes"
    `portability-restrict-system-includes <portability-restrict-system-includes.html>`_, "Yes"
    `portability-simd-intrinsics <portability-simd-intrinsics.html>`_,
+   `portability-std-allocator-const <portability-std-allocator-const.html>`_,
    `readability-avoid-const-params-in-decls <readability-avoid-const-params-in-decls.html>`_, "Yes"
    `readability-braces-around-statements <readability-braces-around-statements.html>`_, "Yes"
    `readability-const-return-type <readability-const-return-type.html>`_, "Yes"
    `readability-container-contains <readability-container-contains.html>`_, "Yes"
    `readability-container-data-pointer <readability-container-data-pointer.html>`_, "Yes"
    `readability-container-size-empty <readability-container-size-empty.html>`_, "Yes"
    `readability-convert-member-functions-to-static <readability-convert-member-functions-to-static.html>`_, "Yes"
    `readability-delete-null-pointer <readability-delete-null-pointer.html>`_, "Yes"

clang-tools-extra/docs/clang-tidy/checks/portability-std-allocator-const.rst

+.. title:: clang-tidy - portability-std-allocator-const
+
+portability-std-allocator-const
+===============================
+
+Report use of ``std::vector<const T>`` (and similar containers of const
+elements). These are not allowed in standard C++, and should usually be
+``std::vector<T>`` instead."
+
+Per C++ ``[allocator.requirements.general]``: "T is any cv-unqualified object
+type", ``std::allocator<const T>`` is undefined. Many standard containers use
+``std::allocator`` by default and therefore their ``const T`` instantiations are
+undefined.
+
+libc++ defines ``std::allocator<const T>`` as an extension which will be removed
+in the future.
+
+libstdc++ and MSVC do not support ``std::allocator<const T>``:
+
+.. code:: c++
+
+  // libstdc++ has a better diagnostic since https://gcc.gnu.org/bugzilla/show_bug.cgi?id=48101
+  std::deque<const int> deque; // error: static assertion failed: std::deque must have a non-const, non-volatile value_type
+  std::set<const int> set; // error: static assertion failed: std::set must have a non-const, non-volatile value_type
+  std::vector<int* const> vector; // error: static assertion failed: std::vector must have a non-const, non-volatile value_type
+
+  // MSVC
+  // error C2338: static_assert failed: 'The C++ Standard forbids containers of const elements because allocator<const T> is ill-formed.'
+
+Code bases only compiled with libc++ may accrue such undefined usage. This
+check finds such code and prevents backsliding while clean-up is ongoing.

clang-tools-extra/test/clang-tidy/checkers/portability-std-allocator-const.cpp

+// RUN: %check_clang_tidy -std=c++11-or-later %s portability-std-allocator-const %t --
+
+namespace std {
+typedef unsigned size_t;
+
+template <class T>
+class allocator {};
+template <class T>
+class hash {};
+template <class T>
+class equal_to {};
+template <class T>
+class less {};
+
+template <class T, class A = std::allocator<T>>
+class deque {};
+template <class T, class A = std::allocator<T>>
+class forward_list {};
+template <class T, class A = std::allocator<T>>
+class list {};
+template <class T, class A = std::allocator<T>>
+class vector {};
+
+template <class K, class C = std::less<K>, class A = std::allocator<K>>
+class multiset {};
+template <class K, class C = std::less<K>, class A = std::allocator<K>>
+class set {};
+template <class K, class H = std::hash<K>, class Eq = std::equal_to<K>, class A = std::allocator<K>>
+class unordered_multiset {};
+template <class K, class H = std::hash<K>, class Eq = std::equal_to<K>, class A = std::allocator<K>>
+class unordered_set {};
+
+template <class T, class C = std::deque<T>>
+class stack {};
+} // namespace std
+
+namespace absl {
+template <class K, class H = std::hash<K>, class Eq = std::equal_to<K>, class A = std::allocator<K>>
+class flat_hash_set {};
+} // namespace absl
+
+template <class T>
+class allocator {};
+
+void simple(const std::vector<const char> &v, std::deque<const short> *d) {
+  // CHECK-MESSAGES: [[#@LINE-1]]:24: warning: container using std::allocator<const T> is a deprecated libc++ extension; remove const for compatibility with other standard libraries
+  // CHECK-MESSAGES: [[#@LINE-2]]:52: warning: container
+  std::list<const long> l;
+  // CHECK-MESSAGES: [[#@LINE-1]]:8: warning: container
+
+  std::multiset<int *const> ms;
+  // CHECK-MESSAGES: [[#@LINE-1]]:8: warning: container
+  std::set<const std::hash<int>> s;
+  // CHECK-MESSAGES: [[#@LINE-1]]:8: warning: container
+  std::unordered_multiset<int *const> ums;
+  // CHECK-MESSAGES: [[#@LINE-1]]:8: warning: container
+  std::unordered_set<const int> us;
+  // CHECK-MESSAGES: [[#@LINE-1]]:8: warning: container
+
+  absl::flat_hash_set<const int> fhs;
+  // CHECK-MESSAGES: [[#@LINE-1]]:9: warning: container
+
+  using my_vector = std::vector<const int>;
+  // CHECK-MESSAGES: [[#@LINE-1]]:26: warning: container
+  my_vector v1;
+  using my_vector2 = my_vector;
+
+  std::vector<int> neg1;
+  std::vector<const int *> neg2;                     // not const T
+  std::vector<const int, allocator<const int>> neg3; // not use std::allocator<const T>
+  std::allocator<const int> a;                       // not caught, but rare
+  std::forward_list<const int> forward;              // not caught, but rare
+  std::stack<const int> stack;                       // not caught, but rare
+}
+
+template <class T>
+void temp1() {
+  std::vector<const T> v;
+  // CHECK-MESSAGES: [[#@LINE-1]]:8: warning: container
+
+  std::vector<T> neg1;
+  std::forward_list<const T> neg2;
+}
+void use_temp1() { temp1<int>(); }
+
+template <class T>
+void temp2() {
+  // Match std::vector<const dependent> for the uninstantiated temp2.
+  std::vector<const T> v;
+  // CHECK-MESSAGES: [[#@LINE-1]]:8: warning: container
+
+  std::vector<T> neg1;
+  std::forward_list<const T> neg2;
+}