loplugin:unusedfields improve search for unused collection fields

look for collection-like fields that are never added to, and are
therefore effectively unused

Change-Id: Id52c5500ea5e3d2436fb5915aebb86278bf2d925
Reviewed-on: https://gerrit.libreoffice.org/60661
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

2 files changed, 104 insertions, 18 deletions

diff --git a/compilerplugins/clang/test/unusedfields.cxx b/compilerplugins/clang/test/unusedfields.cxx
index fe81c88ed205..a6b1ec625ec3 100644
--- a/compilerplugins/clang/test/unusedfields.cxx
+++ b/compilerplugins/clang/test/unusedfields.cxx
@@ -10,6 +10,7 @@
 #include <vector>
 #include <ostream>
 #include <com/sun/star/uno/Any.hxx>
+#include <com/sun/star/uno/Sequence.hxx>
 
 struct Foo
 // expected-error@-1 {{read m_foo1 [loplugin:unusedfields]}}
@@ -172,4 +173,28 @@ struct ReadOnlyAnalysis3
     }
 };
 
+// Verify the special logic for container fields that only contains mutations that
+// add elements.
+struct ReadOnlyAnalysis4
+// expected-error@-1 {{read m_readonly [loplugin:unusedfields]}}
+// expected-error@-2 {{read m_readwrite [loplugin:unusedfields]}}
+// expected-error@-3 {{write m_readwrite [loplugin:unusedfields]}}
+// expected-error@-4 {{read m_readonlyCss [loplugin:unusedfields]}}
+{
+    std::vector<int> m_readonly;
+    std::vector<int> m_readwrite;
+    css::uno::Sequence<sal_Int32> m_readonlyCss;
+
+    void func1()
+    {
+        int x = m_readonly[0];
+        (void)x;
+        *m_readonly.begin() = 1;
+
+        m_readwrite.push_back(0);
+
+        x = m_readonlyCss.getArray()[0];
+    }
+};
+
 /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
diff --git a/compilerplugins/clang/unusedfields.cxx b/compilerplugins/clang/unusedfields.cxx
index 5ba6ac3c1cd6..bcb2db202cec 100644
--- a/compilerplugins/clang/unusedfields.cxx
+++ b/compilerplugins/clang/unusedfields.cxx
@@ -159,9 +159,10 @@ public:
 private:
     MyFieldInfo niceName(const FieldDecl*);
     void checkTouchedFromOutside(const FieldDecl* fieldDecl, const Expr* memberExpr);
-    void checkWriteOnly(const FieldDecl* fieldDecl, const Expr* memberExpr);
-    void checkReadOnly(const FieldDecl* fieldDecl, const Expr* memberExpr);
+    void checkIfReadFrom(const FieldDecl* fieldDecl, const Expr* memberExpr);
+    void checkIfWrittenTo(const FieldDecl* fieldDecl, const Expr* memberExpr);
     bool isSomeKindOfZero(const Expr* arg);
+    bool checkForWriteWhenUsingCollectionType(const CXXMethodDecl * calleeMethodDecl);
     bool IsPassedByNonConst(const FieldDecl* fieldDecl, const Stmt * child, CallerWrapper callExpr,
                                         CalleeWrapper calleeFunctionDecl);
     llvm::Optional<CalleeWrapper> getCallee(CallExpr const *);
@@ -453,14 +454,14 @@ bool UnusedFields::VisitMemberExpr( const MemberExpr* memberExpr )
 
     checkTouchedFromOutside(fieldDecl, memberExpr);
 
-    checkWriteOnly(fieldDecl, memberExpr);
+    checkIfReadFrom(fieldDecl, memberExpr);
 
-    checkReadOnly(fieldDecl, memberExpr);
+    checkIfWrittenTo(fieldDecl, memberExpr);
 
     return true;
 }
 
-void UnusedFields::checkWriteOnly(const FieldDecl* fieldDecl, const Expr* memberExpr)
+void UnusedFields::checkIfReadFrom(const FieldDecl* fieldDecl, const Expr* memberExpr)
 {
     if (insideMoveOrCopyOrCloneDeclParent || insideStreamOutputOperator)
     {
@@ -656,15 +657,10 @@ void UnusedFields::checkWriteOnly(const FieldDecl* fieldDecl, const Expr* member
     if (bPotentiallyReadFrom)
     {
         readFromSet.insert(fieldInfo);
-        if (fieldInfo.fieldName == "nNextElementNumber")
-        {
-            parent->dump();
-            memberExpr->dump();
-        }
     }
 }
 
-void UnusedFields::checkReadOnly(const FieldDecl* fieldDecl, const Expr* memberExpr)
+void UnusedFields::checkIfWrittenTo(const FieldDecl* fieldDecl, const Expr* memberExpr)
 {
     if (insideMoveOrCopyOrCloneDeclParent)
     {
@@ -749,10 +745,10 @@ void UnusedFields::checkReadOnly(const FieldDecl* fieldDecl, const Expr* memberE
             {
                 // if calling a non-const operator on the field
                 auto calleeMethodDecl = callee->getAsCXXMethodDecl();
-                if (calleeMethodDecl
-                    && operatorCallExpr->getArg(0) == child && !calleeMethodDecl->isConst())
+                if (calleeMethodDecl && operatorCallExpr->getArg(0) == child)
                 {
-                    bPotentiallyWrittenTo = true;
+                    if (!calleeMethodDecl->isConst())
+                        bPotentiallyWrittenTo = checkForWriteWhenUsingCollectionType(calleeMethodDecl);
                 }
                 else if (IsPassedByNonConst(fieldDecl, child, operatorCallExpr, *callee))
                 {
@@ -773,13 +769,13 @@ void UnusedFields::checkReadOnly(const FieldDecl* fieldDecl, const Expr* memberE
                 if (tmp->isBoundMemberFunction(compiler.getASTContext())) {
                     tmp = dyn_cast<MemberExpr>(tmp)->getBase();
                 }
-                if (cxxMemberCallExpr->getImplicitObjectArgument() == tmp
-                     && !calleeMethodDecl->isConst())
+                if (cxxMemberCallExpr->getImplicitObjectArgument() == tmp)
                 {
-                    bPotentiallyWrittenTo = true;
+                    if (!calleeMethodDecl->isConst())
+                        bPotentiallyWrittenTo = checkForWriteWhenUsingCollectionType(calleeMethodDecl);
                     break;
                 }
-                if (IsPassedByNonConst(fieldDecl, child, cxxMemberCallExpr, CalleeWrapper(calleeMethodDecl)))
+                else if (IsPassedByNonConst(fieldDecl, child, cxxMemberCallExpr, CalleeWrapper(calleeMethodDecl)))
                     bPotentiallyWrittenTo = true;
             }
             else
@@ -888,6 +884,71 @@ void UnusedFields::checkReadOnly(const FieldDecl* fieldDecl, const Expr* memberE
     }
 }
 
+// return true if this not a collection type, or if it is a collection type, and we might be writing to it
+bool UnusedFields::checkForWriteWhenUsingCollectionType(const CXXMethodDecl * calleeMethodDecl)
+{
+    auto const tc = loplugin::TypeCheck(calleeMethodDecl->getParent());
+    bool listLike = false, setLike = false, mapLike = false, cssSequence = false;
+    if (tc.Class("deque").StdNamespace()
+        || tc.Class("list").StdNamespace()
+        || tc.Class("queue").StdNamespace()
+        || tc.Class("vector").StdNamespace())
+    {
+        listLike = true;
+    }
+    else if (tc.Class("set").StdNamespace()
+        || tc.Class("unordered_set").StdNamespace())
+    {
+        setLike = true;
+    }
+    else if (tc.Class("map").StdNamespace()
+        || tc.Class("unordered_map").StdNamespace())
+    {
+        mapLike = true;
+    }
+    else if (tc.Class("Sequence").Namespace("uno").Namespace("star").Namespace("sun").Namespace("com").GlobalNamespace())
+    {
+        cssSequence = true;
+    }
+    else
+        return true;
+
+    if (calleeMethodDecl->isOverloadedOperator())
+    {
+        auto oo = calleeMethodDecl->getOverloadedOperator();
+        if (oo == OO_Equal)
+            return true;
+        // This is operator[]. We only care about things that add elements to the collection.
+        // if nothing modifies the size of the collection, then nothing useful
+        // is stored in it.
+        if (listLike)
+            return false;
+        return true;
+    }
+
+    auto name = calleeMethodDecl->getName();
+    if (listLike || setLike || mapLike)
+    {
+        if (name == "reserve" || name == "shrink_to_fit" || name == "clear"
+            || name == "erase" || name == "pop_back" || name == "pop_front"
+            || name == "front" || name == "back" || name == "data"
+            || name == "remove" || name == "remove_if"
+            || name == "unique" || name == "sort"
+            || name == "begin" || name == "end"
+            || name == "rbegin" || name == "rend"
+            || name == "at" || name == "find" || name == "equal_range"
+            || name == "lower_bound" || name == "upper_bound")
+            return false;
+    }
+    if (cssSequence)
+    {
+        if (name == "getArray" || name == "begin" || name == "end")
+            return false;
+    }
+
+    return true;
+}
+
 bool UnusedFields::IsPassedByNonConst(const FieldDecl* fieldDecl, const Stmt * child, CallerWrapper callExpr,
                                          CalleeWrapper calleeFunctionDecl)
 {