improve loplugin:refcounting

to catch places where we are converting a weak reference to a strong
reference, and then using a pointer to store the result

Change-Id: I69b132907b574e5c6974fadf18bd9658107d3a0d
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/145877
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

compilerplugins/clang/refcounting.cxx

@@ -75,6 +75,7 @@ private:
 
     bool visitTemporaryObjectExpr(Expr const * expr);
     bool isCastingReference(const Expr* expr);
+    bool isCallingGetOnWeakRef(const Expr* expr);
 };
 
 bool containsXInterfaceSubclass(const clang::Type* pType0);
@@ -714,6 +715,17 @@ bool RefCounting::VisitVarDecl(const VarDecl * varDecl) {
                     << pointeeType
                     << varDecl->getSourceRange();
         }
+        if (isCallingGetOnWeakRef(varDecl->getInit()))
+        {
+            auto pointeeType = varDecl->getType()->getPointeeType();
+            if (containsOWeakObjectSubclass(pointeeType))
+                report(
+                    DiagnosticsEngine::Warning,
+                    "weak object being converted to strong, and then the reference dropped, and managed via raw pointer, should be managed via rtl::Reference",
+                    varDecl->getLocation())
+                    << pointeeType
+                    << varDecl->getSourceRange();
+        }
     }
     return true;
 }
@@ -762,6 +774,47 @@ bool RefCounting::isCastingReference(const Expr* expr)
     return true;
 }
 
+/**
+    Look for code like
+        makeFoo().get();
+    or
+        cast<T*>(makeFoo().get().get());
+    or
+        foo.get();
+    where makeFoo() returns a unotools::WeakReference<Foo>
+    and foo is a unotools::WeakReference<Foo> var.
+*/
+bool RefCounting::isCallingGetOnWeakRef(const Expr* expr)
+{
+    expr = expr->IgnoreImplicit();
+    // unwrap the cast (if any)
+    if (auto castExpr = dyn_cast<CastExpr>(expr))
+        expr = castExpr->getSubExpr()->IgnoreImplicit();
+    // unwrap outer get (if any)
+    if (auto memberCallExpr = dyn_cast<CXXMemberCallExpr>(expr))
+    {
+        auto methodDecl = memberCallExpr->getMethodDecl();
+        if (methodDecl && methodDecl->getIdentifier() && methodDecl->getName() == "get")
+        {
+            QualType objectType = memberCallExpr->getImplicitObjectArgument()->getType();
+            if (loplugin::TypeCheck(objectType).Class("Reference").Namespace("rtl"))
+                expr = memberCallExpr->getImplicitObjectArgument()->IgnoreImplicit();
+        }
+    }
+    // check for converting a WeakReference to a strong reference via get()
+    if (auto memberCallExpr = dyn_cast<CXXMemberCallExpr>(expr))
+    {
+        auto methodDecl = memberCallExpr->getMethodDecl();
+        if (methodDecl && methodDecl->getIdentifier() && methodDecl->getName() == "get")
+        {
+            QualType objectType = memberCallExpr->getImplicitObjectArgument()->getType();
+            if (loplugin::TypeCheck(objectType).Class("WeakReference").Namespace("unotools"))
+                return true;
+        }
+    }
+    return false;
+}
+
 bool RefCounting::VisitBinaryOperator(const BinaryOperator * binaryOperator)
 {
     if (ignoreLocation(binaryOperator))
@@ -801,6 +854,21 @@ bool RefCounting::VisitBinaryOperator(const BinaryOperator * binaryOperator)
                 << pointeeType
                 << binaryOperator->getSourceRange();
     }
+    if (isCallingGetOnWeakRef(binaryOperator->getRHS()))
+    {
+        // TODO Very dodgy code, but I see no simple way of fixing it
+        StringRef fileName = getFilenameOfLocation(compiler.getSourceManager().getSpellingLoc(binaryOperator->getBeginLoc()));
+        if (loplugin::isSamePathname(fileName, SRCDIR "/sd/source/ui/view/Outliner.cxx"))
+            return true;
+        auto pointeeType = binaryOperator->getLHS()->getType()->getPointeeType();
+        if (containsOWeakObjectSubclass(pointeeType))
+            report(
+                DiagnosticsEngine::Warning,
+                "weak object being converted to strong, and then the reference dropped, and managed via raw pointer, should be managed via rtl::Reference",
+                binaryOperator->getBeginLoc())
+                << pointeeType
+                << binaryOperator->getSourceRange();
+    }
     return true;
 }
 

compilerplugins/clang/test/refcounting.cxx

@@ -107,11 +107,26 @@ void foo7()
     UnoSubObject* p3 = static_cast<UnoSubObject*>(getConstRef().get());
     (void)p3;
     p3 = static_cast<UnoSubObject*>(getConstRef().get());
-
-    // no warning expected, although, arguably, we should be assigning to a rtl::Reference temporary
-    unotools::WeakReference<UnoObject> weak1;
-    auto pTextObj = dynamic_cast<UnoSubObject*>(weak1.get().get());
-    (void)pTextObj;
 }
 
+const unotools::WeakReference<UnoObject>& getWeakRef();
+void foo8()
+{
+    // expected-error@+1 {{weak object being converted to strong, and then the reference dropped, and managed via raw pointer, should be managed via rtl::Reference [loplugin:refcounting]}}
+    UnoSubObject* p1 = static_cast<UnoSubObject*>(getWeakRef().get().get());
+    (void)p1;
+
+    // expected-error@+1 {{weak object being converted to strong, and then the reference dropped, and managed via raw pointer, should be managed via rtl::Reference [loplugin:refcounting]}}
+    UnoObject* p2 = getWeakRef().get().get();
+    (void)p2;
+
+    unotools::WeakReference<UnoObject> weak1;
+    // expected-error@+1 {{weak object being converted to strong, and then the reference dropped, and managed via raw pointer, should be managed via rtl::Reference [loplugin:refcounting]}}
+    UnoSubObject* p3 = dynamic_cast<UnoSubObject*>(weak1.get().get());
+    (void)p3;
+
+    // expected-error@+1 {{weak object being converted to strong, and then the reference dropped, and managed via raw pointer, should be managed via rtl::Reference [loplugin:refcounting]}}
+    UnoObject* p4 = weak1.get().get();
+    (void)p4;
+}
 /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */

sd/source/ui/view/Outliner.cxx

@@ -1166,7 +1166,7 @@ bool lclIsValidTextObject(const sd::outliner::IteratorPosition& rPosition)
 
 bool isValidVectorGraphicObject(const sd::outliner::IteratorPosition& rPosition)
 {
-    auto* pGraphicObject = dynamic_cast<SdrGrafObj*>(rPosition.mxObject.get().get());
+    rtl::Reference<SdrGrafObj> pGraphicObject = dynamic_cast<SdrGrafObj*>(rPosition.mxObject.get().get());
     if (pGraphicObject)
     {
         auto const& pVectorGraphicData = pGraphicObject->GetGraphic().getVectorGraphicData();

sw/source/uibase/uiview/uivwimp.cxx

@@ -212,7 +212,7 @@ void SwView_Impl::Invalidate()
     GetUNOObject_Impl()->Invalidate();
     for (const auto& xTransferable: mxTransferables)
     {
-        auto pTransferable = xTransferable.get().get();
+        rtl::Reference<SwTransferable> pTransferable = xTransferable.get();
         if(pTransferable)
             pTransferable->Invalidate();
     }