[clang-tidy] readability-container-size-empty - detect missing usage of .empty() on string_literals

[felix642]

Summary

Detect comparison between string and empty string_literals.

Fixes #64547

[PiotrZSL]

• Release notes entry is missing.
• Commit/Change description should be updated

clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp

179

Probably best would be to write matcher like this:

AST_MATCHER_P(UserDefinedLiteral, hasLiteral, Matcher<Expr>, InnerMatcher) {
   if (const Expr* CookedLiteral  = Node.getCookedLiteral()) {
      return InnerMatcher.matches(CookedLiteral, Finder, Builder);
   }
   return false;
}

and use it instead of hasDescendant

userDefinedLiteral(hasLiteral(stringLiteral(hasSize(0)))),

clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp

791

Add this new line

[felix642]

Fixed tests and addressed comments.

[felix642]

Hi @PiotrZSL, thank you for the feedback.

I have addressed most of your comments, but I'm not sure to understand what you mean by "Commit/Change description should be updated". Would you be able to clarify that for me?

[PiotrZSL]

LGTM, run clang-format on this code....

[felix642]

Clang-format

[rnk]

clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp

26

I discovered that this test started to fail when I landed my revert (0d9919d362a7a70b2a7970861d897ecc47ec9e4d) of f2583f3acf596cc545c8c0e3cb28e712f4ebf21b. I "fixed" the test by changing this to operator""_s in ba52a10fca6fc7b791894c584233db012def68a5, but I'm not sure if that changes the meaning of the test. Please review that or follow up on this.

clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp

 AST_MATCHER(CXXConstructExpr, isDefaultConstruction) {
   return Node.getConstructor()->isDefaultConstructor();
 }
 
 AST_MATCHER(QualType, isIntegralType) {
   return Node->isIntegralType(Finder->getASTContext());
 }
 
+AST_MATCHER_P(UserDefinedLiteral, hasLiteral,
+              clang::ast_matchers::internal::Matcher<Expr>, InnerMatcher) {
+  if (const Expr *CookedLiteral = Node.getCookedLiteral()) {
+    return InnerMatcher.matches(*CookedLiteral, Finder, Builder);
+  }
+  return false;
+}
+
 } // namespace ast_matchers
 namespace tidy::readability {
 
 using utils::isBinaryOrTernary;
 
 ContainerSizeEmptyCheck::ContainerSizeEmptyCheck(StringRef Name,
                                                  ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
Finder->addMatcher(
                    hasMemberName("size"))),
                WrongUse,
                unless(hasAncestor(
                    cxxMethodDecl(ofClass(equalsBoundNode("container"))))))
           .bind("SizeCallExpr"),
       this);
 
   // Comparison to empty string or empty constructor.
-  const auto WrongComparend = anyOf(
-      stringLiteral(hasSize(0)), cxxConstructExpr(isDefaultConstruction()),
+  const auto WrongComparend =
+      anyOf(stringLiteral(hasSize(0)),
+            userDefinedLiteral(hasLiteral(stringLiteral(hasSize(0)))),
+            cxxConstructExpr(isDefaultConstruction()),
             cxxUnresolvedConstructExpr(argumentCountIs(0)));
   // Match the object being compared.
   const auto STLArg =
       anyOf(unaryOperator(
                 hasOperatorName("*"),
                 hasUnaryOperand(
                     expr(hasType(pointsTo(ValidContainer))).bind("Pointee"))),
             expr(hasType(ValidContainer)).bind("STLObject"));
 

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`bugprone-inc-dec-in-conditions
 
   Detects when a variable is both incremented/decremented and referenced inside
   a complex condition and suggests moving them outside to avoid ambiguity in
   the variable's value.
 
 - New :doc:`bugprone-incorrect-enable-if
   <clang-tidy/checks/bugprone/incorrect-enable-if>` check.
 
-  Detects incorrect usages of ``std::enable_if`` that don't name the nested 
+  Detects incorrect usages of ``std::enable_if`` that don't name the nested
   ``type`` type.
 
 - New :doc:`bugprone-multi-level-implicit-pointer-conversion
   <clang-tidy/checks/bugprone/multi-level-implicit-pointer-conversion>` check.
 
   Detects implicit conversions between pointers of different levels of
   indirection.
 

 - Improved :doc:`performance-faster-string-find
   <clang-tidy/checks/performance/faster-string-find>` check to properly escape
   single quotes.
 
 - Improved :doc:`performance-noexcept-swap
   <clang-tidy/checks/performance/noexcept-swap>` check to enforce a stricter
   match with the swap function signature, eliminating false-positives.
 
+- Improved :doc:`readability-container-size-empty
+  <clang-tidy/checks/readability/container-size-empty>` check to
+  detect comparison between string and empty string literals.
+
 - Improved :doc:`readability-identifier-naming
   <clang-tidy/checks/readability/identifier-naming>` check to emit proper
   warnings when a type forward declaration precedes its definition.
 
 - Improved :doc:`readability-implicit-bool-conversion
   <clang-tidy/checks/readability/implicit-bool-conversion>` check to take
   do-while loops into account for the `AllowIntegerConditions` and
   `AllowPointerConditions` options.

clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp

template <typename T> struct set {
   set();
   bool operator==(const set<T>& other) const;
   bool operator!=(const set<T>& other) const;
   unsigned long size() const;
   bool empty() const;
 };
 }
 
+namespace string_literals{
+string operator""s(const char *, size_t);
+}
+
 }
 
 template <typename T>
 class TemplatedContainer {
 public:
   bool operator==(const TemplatedContainer<T>& other) const;
   bool operator!=(const TemplatedContainer<T>& other) const;
   int size() const;

 // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used
 // CHECK-MESSAGES: :12:8: note: method 'vector'::empty() defined here
 // CHECK-FIXES: {{^  }}return !C.empty() ? 0 : 1;
 }
 
 constexpr Lazy L;
 static_assert(!L.size(), "");
 // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: the 'empty' method should be used
-// CHECK-MESSAGES: :90:18: note: method 'Lazy'::empty() defined here
+// CHECK-MESSAGES: :94:18: note: method 'Lazy'::empty() defined here
 // CHECK-FIXES: {{^}}static_assert(L.empty(), "");
 
 struct StructWithLazyNoexcept {
   void func() noexcept(L.size());
 };
 
 #define CHECKSIZE(x) if (x.size()) {}
 // CHECK-FIXES: #define CHECKSIZE(x) if (x.size()) {}
template <typename T> void f() {
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: the 'empty' method should be used
   // CHECK-MESSAGES: :12:8: note: method 'vector'::empty() defined here
   // CHECK-FIXES: CHECKSIZE(v);
 
   TemplatedContainer<T> templated_container;
   if (templated_container.size())
     ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}if (!templated_container.empty()){{$}}
   if (templated_container != TemplatedContainer<T>())
     ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // 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: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: CHECKSIZE(templated_container);
 }
 
 void g() {
   f<int>();
   f<double>();
   f<char *>();
 }
bool neverInstantiatedTemplate() {
   // CHECK-MESSAGES: :[[@LINE-2]]:8: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
   // CHECK-MESSAGES: :12:8: note: method 'vector'::empty() defined here
   // CHECK-FIXES: {{^  }}if (v.empty()){{$}}
 
   TemplatedContainer<T> templated_container;
   if (templated_container.size())
     ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}if (!templated_container.empty()){{$}}
   if (templated_container != TemplatedContainer<T>())
     ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}if (!templated_container.empty()){{$}}
   // CHECK-FIXES-NEXT: ;
   while (templated_container.size())
     ;
   // CHECK-MESSAGES: :[[@LINE-2]]:10: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}while (!templated_container.empty()){{$}}
 
   do {
   }
   while (templated_container.size());
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}while (!templated_container.empty());
 
   for (; templated_container.size();)
     ;
   // CHECK-MESSAGES: :[[@LINE-2]]:10: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}for (; !templated_container.empty();){{$}}
 
   if (true && templated_container.size())
     ;
   // CHECK-MESSAGES: :[[@LINE-2]]:15: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}if (true && !templated_container.empty()){{$}}
 
   if (true || templated_container.size())
     ;
   // CHECK-MESSAGES: :[[@LINE-2]]:15: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}if (true || !templated_container.empty()){{$}}
 
   if (!templated_container.size())
     ;
   // CHECK-MESSAGES: :[[@LINE-2]]:8: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}if (templated_container.empty()){{$}}
 
   bool b1 = templated_container.size();
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}bool b1 = !templated_container.empty();
 
   bool b2(templated_container.size());
   // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}bool b2(!templated_container.empty());
 
   auto b3 = static_cast<bool>(templated_container.size());
   // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}auto b3 = static_cast<bool>(!templated_container.empty());
 
   auto b4 = (bool)templated_container.size();
   // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}auto b4 = (bool)!templated_container.empty();
 
   auto b5 = bool(templated_container.size());
   // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}auto b5 = bool(!templated_container.empty());
 
   takesBool(templated_container.size());
   // We don't detect this one because we don't know the parameter of takesBool
   // until the type of templated_container.size() is known
 
   return templated_container.size();
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}return !templated_container.empty();
 }
 
 template <typename TypeRequiresSize>
 void instantiatedTemplateWithSizeCall() {
   TypeRequiresSize t;
   // The instantiation of the template with std::vector<int> should not
   // result in changing the template, because we don't know that

 };
 
 typedef std::array<int, 10U> Array;
 
 bool testArraySize(const Array& value) {
   return value.size() == 0U;
 // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
 // CHECK-FIXES: {{^  }}return value.empty();{{$}}
-// CHECK-MESSAGES: :731:8: note: method 'array'::empty() defined here
+// CHECK-MESSAGES: :735:8: note: method 'array'::empty() defined here
 }
 
 bool testArrayCompareToEmpty(const Array& value) {
   return value == std::array<int, 10U>();
 }
 
 bool testDummyType(const DummyType& value) {
   return value == DummyType();
 // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used to check for emptiness instead of comparing to an empty object [readability-container-size-empty]
 // CHECK-FIXES: {{^  }}return value.empty();{{$}}
-// CHECK-MESSAGES: :741:8: note: method 'DummyType'::empty() defined here
+// CHECK-MESSAGES: :745:8: note: method 'DummyType'::empty() defined here
 }
 
 bool testIgnoredDummyType(const IgnoredDummyType& value) {
   return value == IgnoredDummyType();
 }
+
+bool testStringLiterals(const std::string& s)
+{
+  using namespace std::string_literals;
+  return s == ""s;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used
+  // CHECK-FIXES: {{^  }}return s.empty()
+}
+
+bool testNotEmptyStringLiterals(const std::string& s)
+{
+  using namespace std::string_literals;
+  return s == "foo"s;
+}