[clang-tidy] Extend readability-container-size-empty to arbitrary class with size() and empty()

[omtcyfz]

Summary

Implementing feature request.

This patch extends readability-container-size-empty check allowing it to produce warnings not only for stl containers.

[aaron.ballman]

clang-tidy/readability/ContainerSizeEmptyCheck.cpp

33

While it's not entirely unreasonable, it makes me uneasy to assume that anything with a size() and empty() member function must be a container. Is there a way to silence false positives aside from disabling the check entirely?

Perhaps this should instead be a user-configurable list of containers that's prepopulated with STL container names?

Also note: returns(isInteger()) seems a bit permissive. Consider:

bool size() const;

enum E { one };
enum E size() const;

These both will qualify for that matcher, but don't seem like particularly valid indications that the object is a container.

[omtcyfz]

Blacklist enum and bool return types for size().

[omtcyfz]

clang-tidy/readability/ContainerSizeEmptyCheck.cpp

33

Thank you!

Blacklisted these types.

Actually, I believe if someone has bool size() in their codebase there's still a problem...

[Eugene.Zelenko]

Probably check should have options to extend list of containers and also to assume all classes with integer type size() const and bool empty() const as containers. It may be not trivial to find out all custom containers and last option will be helpful to assemble such list.

[aaron.ballman]

In D24349#537350, @Eugene.Zelenko wrote:

Probably check should have options to extend list of containers and also to assume all classes with integer type size() const and bool empty() const as containers. It may be not trivial to find out all custom containers and last option will be helpful to assemble such list.

I think this should actually have two options: one is a list of default-checked containers (default is the list of STL containers we previously had), and the other is a Boolean option for guessing at what might be a container based on function signature (default is true). This gives people a way to silence the diagnostic while still being useful. I would document what function signatures we use as our heuristic, but note that the function signatures we care about may change (for instance, we may decide that const-qualification is unimportant, or that we want to require begin()/end() functions, etc). I think <standard integer type> size() const and bool empty() const are a reasonable heuristic for a first pass.

clang-tidy/readability/ContainerSizeEmptyCheck.cpp

33

Blacklisted these types.

I'm still not certain this is correct. For instance, if someone wants to do uint8_t size() const;, we shouldn't punish them because uint8_t happens to be unsigned char for their platform. But you are correct that we don't want this to work with char32_t, for instance.

I think the best approach is to make a local matcher for the type predicate. We want isInteger() unless it's char (that's not explicitly qualified with signed or unsigned), bool, wchar_t, char16_t, char32_t, or an enumeration type.

Actually, I believe if someone has bool size() in their codebase there's still a problem...

Agreed, though not as part of this check. ;-)

[Eugene.Zelenko]

Should we also check for absence of parameters in size() and empty() as well as const?

[aaron.ballman]

In D24349#537549, @Eugene.Zelenko wrote:

Should we also check for absence of parameters in size() and empty() as well as const?

I think that would be reasonable.

[omtcyfz]

In D24349#537350, @Eugene.Zelenko wrote:

Probably check should have options to extend list of containers and also to assume all classes with integer type size() const and bool empty() const as containers. It may be not trivial to find out all custom containers and last option will be helpful to assemble such list.

I was thinking about that, but I'm not sure people are misusing size() and empty() so much.

In D24349#537552, @aaron.ballman wrote:

In D24349#537549, @Eugene.Zelenko wrote:

Should we also check for absence of parameters in size() and empty() as well as const?

I think that would be reasonable.

I do not agree about the const part. It should be encouraged to use const for these, but I do not think they should be a requirement.

[Eugene.Zelenko]

If size() and empty() change object's state, it may be not equivalent replacement.

[omtcyfz]

In D24349#537589, @Eugene.Zelenko wrote:

If size() and empty() change object's state, it may be not equivalent replacement.

True. But my point is that they are not required to do that if they're just not marked const.

[xazax.hun]

In D24349#537594, @omtcyfz wrote:

In D24349#537589, @Eugene.Zelenko wrote:

If size() and empty() change object's state, it may be not equivalent replacement.

True. But my point is that they are not required to do that if they're just not marked const.

I agree with Eugene. But I think a good consensus could be to warn on the non-const case but do not do the rewrite. What do you think?

[aaron.ballman]

In D24349#537595, @xazax.hun wrote:

In D24349#537594, @omtcyfz wrote:

In D24349#537589, @Eugene.Zelenko wrote:

If size() and empty() change object's state, it may be not equivalent replacement.

True. But my point is that they are not required to do that if they're just not marked const.

I agree with Eugene. But I think a good consensus could be to warn on the non-const case but do not do the rewrite. What do you think?

I think that's reasonable, depending on whether we find false positives with the warning as well (I have a slight concern about size() and empty() being unrelated operations on a non-container class that someone writes).

[omtcyfz]

In D24349#537624, @aaron.ballman wrote:

I think that's reasonable, depending on whether we find false positives with the warning as well (I have a slight concern about size() and empty() being unrelated operations on a non-container class that someone writes).

Agreed. I'd like to try LLVM as an example of a huge codebase tomorrow to see how many false-positives there actually are.

Generally, I can see the point of requiring const and providing options to reduce the set of allowed classes to std::container's, but at the same time my thoughts are as follows:

1. Chances of people misusing C++ so much are very low. That's something I can check tomorrow just by running the check on few large open source codebases to see whether it's true.
2. If people are misusing C++ it's their fault anyway. One can do whatever he likes to do. One can rewrite all interfaces to STL containers and make size() and empty() most sophisticated functions the world has ever seen while performing whatever strange operations inside of them. Thus, I am not very supportive of the idea "let's try to think about all weird cornercases".

I'm very curious to see how many false-positives there would be in LLVM codebase, for example. I'll try it tomorrow.

[Prazek]

clang-tidy/readability/ContainerSizeEmptyCheck.cpp

34

Would be nice to have 'isStrictlyInteger' matcher to do this thing.

[alexfh]

Thank you for the patch!

In D24349#537500, @aaron.ballman wrote:

In D24349#537350, @Eugene.Zelenko wrote:

Probably check should have options to extend list of containers and also to assume all classes with integer type size() const and bool empty() const as containers. It may be not trivial to find out all custom containers and last option will be helpful to assemble such list.

I think this should actually have two options: one is a list of default-checked containers (default is the list of
STL containers we previously had), and the other is a Boolean option for guessing at what might be a
container based on function signature (default is true). This gives people a way to silence the diagnostic
while still being useful.

While I understand your concerns, I would suggest to wait for actual bug reports before introducing more options. The duck typing approach worked really well for readability-redundant-smartptr-get and I expect it to work equally well here. The constraints checked here are pretty strict and should make the check safe.

I would document what function signatures we use as our heuristic, but note that the function signatures we care about may change (for instance, we may decide that const-qualification is unimportant, or that we want to require begin()/end() functions, etc). I think <standard integer type> size() const and bool empty() const are a reasonable heuristic for a first pass.

Yep, documentation should be updated accordingly.

clang-tidy/readability/ContainerSizeEmptyCheck.cpp

33

Same here, let's wait for a real user asking to support his real container class that defines uint8_t size() const;.

143

Since we're expanding the set of supported classes, it makes sense to include the name of the class to the diagnostic message.

[aaron.ballman]

In D24349#538098, @alexfh wrote:

Thank you for the patch!

In D24349#537500, @aaron.ballman wrote:

In D24349#537350, @Eugene.Zelenko wrote:

Probably check should have options to extend list of containers and also to assume all classes with integer type size() const and bool empty() const as containers. It may be not trivial to find out all custom containers and last option will be helpful to assemble such list.

I think this should actually have two options: one is a list of default-checked containers (default is the list of
STL containers we previously had), and the other is a Boolean option for guessing at what might be a
container based on function signature (default is true). This gives people a way to silence the diagnostic
while still being useful.

While I understand your concerns, I would suggest to wait for actual bug reports before introducing more options. The duck typing approach worked really well for readability-redundant-smartptr-get and I expect it to work equally well here. The constraints checked here are pretty strict and should make the check safe.

That means there is no way to silence this check on false positives aside from disabling it entirely, which is not a particularly good user experience. However, I do agree that if we constrain the heuristic appropriately, the number of false positives should be low.

clang-tidy/readability/ContainerSizeEmptyCheck.cpp

33

I'm not certain I agree with that (I don't like adding new functionality that is known to be imprecise, especially when dealing with heuristic-based checking), but I don't have that strong of feelings.

[omtcyfz]

Restricted size() and empty() functions a little bit more.

[omtcyfz]

Allow inheritance for size() and empty().

[alexfh]

In D24349#538102, @aaron.ballman wrote:

In D24349#538098, @alexfh wrote:

Thank you for the patch!

In D24349#537500, @aaron.ballman wrote:

In D24349#537350, @Eugene.Zelenko wrote:

Probably check should have options to extend list of containers and also to assume all classes with integer type size() const and bool empty() const as containers. It may be not trivial to find out all custom containers and last option will be helpful to assemble such list.

I think this should actually have two options: one is a list of default-checked containers (default is the list of
STL containers we previously had), and the other is a Boolean option for guessing at what might be a
container based on function signature (default is true). This gives people a way to silence the diagnostic
while still being useful.

While I understand your concerns, I would suggest to wait for actual bug reports before introducing more options. The duck typing approach worked really well for readability-redundant-smartptr-get and I expect it to work equally well here. The constraints checked here are pretty strict and should make the check safe.

That means there is no way to silence this check on false positives aside from disabling it entirely, which is not a particularly good user experience. However, I do agree that if we constrain the heuristic appropriately, the number of false positives should be low.

Just checked: readability-container-size-empty with the patch finds a few thousand new warnings related to about 400 unique container names in our code. A representative sample shows no false positives and we've found just a single false negative (that triggers the check without the patch) - a container's size() method was not const. Which makes me pretty sure that:

1. The new algorithm is much more consistent and useful than the whitelist approach (gathering and maintaining a whitelist of a few hundred names is also not fun at all).
2. Given the size of our code and the number of third-party libraries, possibility of false positives is extremely low.
3. If there are reports of false positives, we could go with a blacklist approach instead, but I don't think this will ever be needed.

clang-tidy/readability/ContainerSizeEmptyCheck.cpp

33

I don't think we can come up with a proper solution until we see a real world example. If we want to find one faster, we can allow all character types and wait for reports of false positives ;)

docs/clang-tidy/checks/readability-container-size-empty.rst

18 ↗

(On Diff #70822)

The check

18 ↗

(On Diff #70822)

Double backquotes should be used.

21 ↗

(On Diff #70822)

nit: space before c++.

23 ↗

(On Diff #70822)

Explain the constraints size_type should satisfy.

[omtcyfz]

Address another round of comments.

[omtcyfz]

Messed up with the last diff; fix that + clang-format the check.

[alexfh]

LG

clang-tidy/readability/ContainerSizeEmptyCheck.cpp

35

You could probably combine the two returns matchers. Something like this: returns(qualType(isInteger(), unless(booleanType()))).

[omtcyfz]

Combine two returns matchers.

clang-tidy/readability/ContainerSizeEmptyCheck.cpp

ContainerSizeEmptyCheck::ContainerSizeEmptyCheck(StringRef Name,
     : ClangTidyCheck(Name, Context) {}
 
 void ContainerSizeEmptyCheck::registerMatchers(MatchFinder *Finder) {
   // Only register the matchers for C++; the functionality currently does not
   // provide any benefit to other languages, despite being benign.
   if (!getLangOpts().CPlusPlus)
     return;
 
-  const auto stlContainer = hasAnyName(
-      "array", "basic_string", "deque", "forward_list", "list", "map",
-      "multimap", "multiset", "priority_queue", "queue", "set", "stack",
-      "unordered_map", "unordered_multimap", "unordered_multiset",
-      "unordered_set", "vector");
+  const auto validContainer = namedDecl(
+      has(functionDecl(isPublic(), hasName("size"), returns(isInteger()))),
+      has(functionDecl(isPublic(), hasName("empty"), returns(booleanType()))));
 
   const auto WrongUse = anyOf(
       hasParent(binaryOperator(
                     matchers::isComparisonOperator(),
                     hasEitherOperand(ignoringImpCasts(anyOf(
                         integerLiteral(equals(1)), integerLiteral(equals(0))))))
                     .bind("SizeBinaryOp")),
       hasParent(implicitCastExpr(
           hasImplicitDestinationType(booleanType()),
           anyOf(
               hasParent(unaryOperator(hasOperatorName("!")).bind("NegOnSize")),
               anything()))),
       hasParent(explicitCastExpr(hasDestinationType(booleanType()))));
 
   Finder->addMatcher(
       cxxMemberCallExpr(
-          on(expr(anyOf(hasType(namedDecl(stlContainer)),
-                        hasType(pointsTo(namedDecl(stlContainer))),
-                        hasType(references(namedDecl(stlContainer)))))
+          on(expr(anyOf(hasType(namedDecl(validContainer)),
+                        hasType(pointsTo(namedDecl(validContainer))),
+                        hasType(references(namedDecl(validContainer)))))
                  .bind("STLObject")),
           callee(cxxMethodDecl(hasName("size"))), WrongUse)
           .bind("SizeCallExpr"),
       this);
 }
 
 void ContainerSizeEmptyCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *MemberCall =
if (BinaryOp) { // Determine the correct transformation.
     if (const auto *UnaryOp =
             Result.Nodes.getNodeAs<UnaryOperator>("NegOnSize"))
       Hint = FixItHint::CreateReplacement(UnaryOp->getSourceRange(),
                                           ReplacementText);
     else
       Hint = FixItHint::CreateReplacement(MemberCall->getSourceRange(),
                                           "!" + ReplacementText);
   }
   diag(MemberCall->getLocStart(), "the 'empty' method should be used to check "
                                   "for emptiness instead of 'size'")
       << Hint;
 }
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang

test/clang-tidy/readability-container-size-empty.cpp

 inline namespace __v2 {
 template <typename T> struct set {
   set();
   unsigned long size() const;
   bool empty() const;
 };
 }
 
-
 }
 
+template <typename T>
+class TemplatedContainer {
+public:
+  int size();
+  bool empty();
+};
+
+template <typename T>
+class PrivateEmpty {
+public:
+  int size();
+private:
+  bool empty();
+};
+
 int main() {
   std::set<int> intSet;
   std::string str;
   std::wstring wstr;
   str.size() + 0;
   str.size() - 0;
   0 + str.size();
   0 - str.size();
int main() {
 
   delete vect3;
 
   const std::vector<int> &vect4 = vect2;
   if (vect4.size() == 0)
     ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
   // CHECK-FIXES: {{^  }}if (vect4.empty()){{$}}
+
+  TemplatedContainer<void> templated_container;
+  if (templated_container.size() == 0)
+    ;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-FIXES: {{^  }}if (templated_container.empty()){{$}}
+  if (templated_container.size() != 0)
+    ;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-FIXES: {{^  }}if (!templated_container.empty()){{$}}
+  if (0 == templated_container.size())
+    ;
+  // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: the 'empty' method should be used
+  // CHECK-FIXES: {{^  }}if (templated_container.empty()){{$}}
+  if (0 != templated_container.size())
+    ;
+  // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: the 'empty' method should be used
+  // CHECK-FIXES: {{^  }}if (!templated_container.empty()){{$}}
+  if (templated_container.size() > 0)
+    ;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-FIXES: {{^  }}if (!templated_container.empty()){{$}}
+  if (0 < templated_container.size())
+    ;
+  // CHECK-MESSAGES: :[[@LINE-2]]:11: warning: the 'empty' method should be used
+  // CHECK-FIXES: {{^  }}if (!templated_container.empty()){{$}}
+  if (templated_container.size() < 1)
+    ;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-FIXES: {{^  }}if (templated_container.empty()){{$}}
+  if (1 > templated_container.size())
+    ;
+  // CHECK-MESSAGES: :[[@LINE-2]]:11: warning: the 'empty' method should be used
+  // CHECK-FIXES: {{^  }}if (templated_container.empty()){{$}}
+  if (templated_container.size() >= 1)
+    ;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-FIXES: {{^  }}if (!templated_container.empty()){{$}}
+  if (1 <= templated_container.size())
+    ;
+  // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: the 'empty' method should be used
+  // CHECK-FIXES: {{^  }}if (!templated_container.empty()){{$}}
+  if (templated_container.size() > 1) // no warning
+    ;
+  if (1 < templated_container.size()) // no warning
+    ;
+  if (templated_container.size() <= 1) // no warning
+    ;
+  if (1 >= templated_container.size()) // no warning
+    ;
+  if (!templated_container.size())
+    ;
+  // CHECK-MESSAGES: :[[@LINE-2]]:8: warning: the 'empty' method should be used
+  // CHECK-FIXES: {{^  }}if (templated_container.empty()){{$}}
+  if (templated_container.size())
+    ;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-FIXES: {{^  }}if (!templated_container.empty()){{$}}
+
+  if (templated_container.empty())
+    ;
+
+  // No warnings expected.
+  PrivateEmpty<void> private_empty;
+  if (private_empty.size() == 0)
+    ;
+  if (private_empty.size() != 0)
+    ;
+  if (0 == private_empty.size())
+    ;
+  if (0 != private_empty.size())
+    ;
+  if (private_empty.size() > 0)
+    ;
+  if (0 < private_empty.size())
+    ;
+  if (private_empty.size() < 1)
+    ;
+  if (1 > private_empty.size())
+    ;
+  if (private_empty.size() >= 1)
+    ;
+  if (1 <= private_empty.size())
+    ;
+  if (private_empty.size() > 1)
+    ;
+  if (1 < private_empty.size())
+    ;
+  if (private_empty.size() <= 1)
+    ;
+  if (1 >= private_empty.size())
+    ;
+  if (!private_empty.size())
+    ;
+  if (private_empty.size())
+    ;
 }
 
 #define CHECKSIZE(x) if (x.size())
 // CHECK-FIXES: #define CHECKSIZE(x) if (x.size())
 
 template <typename T> void f() {
   std::vector<T> v;
   if (v.size())
     ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
   // CHECK-FIXES: {{^  }}if (!v.empty()){{$}}
   // CHECK-FIXES-NEXT: ;
   CHECKSIZE(v);
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: the 'empty' method should be used
   // CHECK-MESSAGES: CHECKSIZE(v);
+
+  TemplatedContainer<T> templated_container;
+  if (templated_container.size())
+    ;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-FIXES: {{^  }}if (!templated_container.empty()){{$}}
+  // CHECK-FIXES-NEXT: ;
+  CHECKSIZE(templated_container);
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: the 'empty' method should be used
+  // CHECK-MESSAGES: CHECKSIZE(templated_container);
 }
 
 void g() {
   f<int>();
   f<double>();
   f<char *>();
 }