[clang-tidy] Fix `readability-container-size-empty` check for smart pointers

[fwolff]

Summary

Fixes PR#51776. If the object on which size() is called has an overloaded operator->, the span for E will include the ->, and since the return type of operator-> is a pointer, readability-container-size-empty will try to add another arrow, leading to the nonsensical vp->->empty() suggestion. This patch fixes this behavior.

[aaron.ballman]

LGTM! Can you also add a release note for the fix?

[Sockke]

Could you please add a test case where the smart pointer object is dereferenced before calling size()? E.g. return (*ptr).size() == 0;. I think this change doesn't work on this test case.

[fwolff]

In D115124#3209199, @Sockke wrote:

Could you please add a test case where the smart pointer object is dereferenced before calling size()? E.g. return (*ptr).size() == 0;. I think this change doesn't work on this test case.

This is not entirely true; what you're seeing is an existing, unrelated bug: https://godbolt.org/z/9zEfdrPW8

In any case, I've fixed it, added a test for it, and added a bullet point to the release notes.

[Sockke]

Thanks for improving this check! I think it looks great and safe enough. It would be better to turn !(*ptr).empty() into !ptr->empty(), but I have no particular opinions at this point. Let's see if @aaron.ballman has any comments.

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

void ContainerSizeEmptyCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *E =
       MemberCallObject
           ? MemberCallObject
           : (Pointee ? Pointee : Result.Nodes.getNodeAs<Expr>("STLObject"));
   FixItHint Hint;
   std::string ReplacementText = std::string(
       Lexer::getSourceText(CharSourceRange::getTokenRange(E->getSourceRange()),
                            *Result.SourceManager, getLangOpts()));
-  if (isBinaryOrTernary(E) || isa<UnaryOperator>(E)) {
+  const auto *OpCallExpr = dyn_cast<CXXOperatorCallExpr>(E);
+  if (isBinaryOrTernary(E) || isa<UnaryOperator>(E) ||
+      (OpCallExpr && (OpCallExpr->getOperator() == OO_Star))) {
     ReplacementText = "(" + ReplacementText + ")";
   }
-  if (E->getType()->isPointerType())
+  if (OpCallExpr &&
+      OpCallExpr->getOperator() == OverloadedOperatorKind::OO_Arrow) {
+    // This can happen if the object is a smart pointer. Don't add anything
+    // because a '->' is already there (PR#51776), just call the method.
+    ReplacementText += "empty()";
+  } else if (E->getType()->isPointerType())
     ReplacementText += "->empty()";
   else
     ReplacementText += ".empty()";
 
   if (BinCmp) {
     if (BinCmp->getOperator() == OO_ExclaimEqual) {
       ReplacementText = "!" + ReplacementText;
     }

clang-tools-extra/docs/ReleaseNotes.rst

 
 - Improved :doc:`performance-inefficient-vector-operation
   <clang-tidy/checks/performance-inefficient-vector-operation>` to work when
   the vector is a member of a structure.
 
 - Fixed nonsensical suggestion of :doc:`altera-struct-pack-align
   <clang-tidy/checks/altera-struct-pack-align>` check for empty structs.
 
+- Fixed incorrect suggestions for :doc:`readability-container-size-empty
+  <clang-tidy/checks/readability-container-size-empty>` when smart pointers are involved.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

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

public:
   unsigned size() const { return 0; }
   // Does not have `.empty()`
 };
 
 void instantiator() {
   instantiatedTemplateWithSizeCall<TypeWithSize>();
   instantiatedTemplateWithSizeCall<std::vector<int>>();
 }
+
+namespace std {
+template <typename T>
+struct unique_ptr {
+  T *operator->() const;
+  T &operator*() const;
+};
+} // namespace std
+
+bool call_through_unique_ptr(const std::unique_ptr<std::vector<int>> &ptr) {
+  return ptr->size() > 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used
+  // CHECK-MESSAGES: :9:8: note: method 'vector'::empty() defined here
+  // CHECK-FIXES: {{^  }}return !ptr->empty();
+}
+
+bool call_through_unique_ptr_deref(const std::unique_ptr<std::vector<int>> &ptr) {
+  return (*ptr).size() > 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used
+  // CHECK-MESSAGES: :9:8: note: method 'vector'::empty() defined here
+  // CHECK-FIXES: {{^  }}return !(*ptr).empty();
+}