loplugin:refcounting check for one more case

where we might be holding something newly created by pointer
instead of by *::Reference

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

8 files changed, 104 insertions, 15 deletions

diff --git a/accessibility/source/standard/vclxaccessibletoolbox.cxx b/accessibility/source/standard/vclxaccessibletoolbox.cxx
index acd2c4f18899..a04685730c85 100644
--- a/accessibility/source/standard/vclxaccessibletoolbox.cxx
+++ b/accessibility/source/standard/vclxaccessibletoolbox.cxx
@@ -593,8 +593,8 @@ void VCLXAccessibleToolBox::ProcessWindowEvent( const VclWindowEvent& rVclWindow
             if ( pWin && pWin->GetParent() &&
                  pWin->GetParent()->GetType() == WindowType::TOOLBOX )
             {
-                VCLXAccessibleToolBox* pParent = static_cast< VCLXAccessibleToolBox* >(
-                    pWin->GetParent()->GetAccessible()->getAccessibleContext().get() );
+                auto pParentAccContext = pWin->GetParent()->GetAccessible()->getAccessibleContext();
+                VCLXAccessibleToolBox* pParent = static_cast< VCLXAccessibleToolBox* >( pParentAccContext.get() );
                 if ( pParent )
                     pParent->ReleaseSubToolBox(static_cast<ToolBox *>(pWin.get()));
             }
diff --git a/canvas/source/opengl/ogl_canvashelper.cxx b/canvas/source/opengl/ogl_canvashelper.cxx
index d64e1ba1d7f0..0484f710ae53 100644
--- a/canvas/source/opengl/ogl_canvashelper.cxx
+++ b/canvas/source/opengl/ogl_canvashelper.cxx
@@ -679,7 +679,8 @@ namespace oglcanvas
             ScopedVclPtrInstance< VirtualDevice > pVDev;
             pVDev->EnableOutput(false);
 
-            CanvasFont* pFont=dynamic_cast<CanvasFont*>(xLayoutetText->getFont().get());
+            auto pLayoutFont = xLayoutetText->getFont();
+            CanvasFont* pFont=dynamic_cast<CanvasFont*>(pLayoutFont.get());
             const rendering::StringContext& rTxt=xLayoutetText->getText();
             if( pFont && rTxt.Length )
             {
diff --git a/chart2/source/controller/main/ChartController_Window.cxx b/chart2/source/controller/main/ChartController_Window.cxx
index a8d71e903fd3..afd4de8a5e75 100644
--- a/chart2/source/controller/main/ChartController_Window.cxx
+++ b/chart2/source/controller/main/ChartController_Window.cxx
@@ -863,7 +863,8 @@ void ChartController::execute_MouseButtonUp( const MouseEvent& rMEvt )
                             m_xUndoManager );
 
                         bool bChanged = false;
-                        ChartModel* pModel = dynamic_cast<ChartModel*>(getModel().get());
+                        css::uno::Reference< css::frame::XModel > xModel = getModel();
+                        ChartModel* pModel = dynamic_cast<ChartModel*>(xModel.get());
                         assert(pModel);
                         if ( eObjectType == OBJECTTYPE_LEGEND )
                             bChanged = DiagramHelper::switchDiagramPositioningToExcludingPositioning( *pModel, false , true );
@@ -2090,7 +2091,8 @@ void ChartController::sendPopupRequest(OUString const & rCID, tools::Rectangle a
 
     OUString sPivotTableName = xPivotTableDataProvider->getPivotTableName();
 
-    PopupRequest* pPopupRequest = dynamic_cast<PopupRequest*>(pChartModel->getPopupRequest().get());
+    css::uno::Reference<css::awt::XRequestCallback> xPopupRequest = pChartModel->getPopupRequest();
+    PopupRequest* pPopupRequest = dynamic_cast<PopupRequest*>(xPopupRequest.get());
     if (!pPopupRequest)
         return;
 
diff --git a/compilerplugins/clang/refcounting.cxx b/compilerplugins/clang/refcounting.cxx
index 319a23b83a9b..9157a1910add 100644
--- a/compilerplugins/clang/refcounting.cxx
+++ b/compilerplugins/clang/refcounting.cxx
@@ -73,6 +73,7 @@ private:
                            const RecordDecl* parent, const std::string& rDeclName);
 
     bool visitTemporaryObjectExpr(Expr const * expr);
+    bool isCastingReference(const Expr* expr);
 };
 
 bool containsXInterfaceSubclass(const clang::Type* pType0);
@@ -693,6 +694,56 @@ bool RefCounting::VisitVarDecl(const VarDecl * varDecl) {
                     << pointeeType
                     << varDecl->getSourceRange();
         }
+        if (isCastingReference(varDecl->getInit()))
+        {
+            // TODO false+ code
+            StringRef fileName = getFilenameOfLocation(compiler.getSourceManager().getSpellingLoc(compat::getBeginLoc(varDecl)));
+            if (loplugin::isSamePathname(fileName, SRCDIR "/sw/source/core/unocore/unotbl.cxx"))
+                return true;
+            auto pointeeType = varDecl->getType()->getPointeeType();
+            if (containsOWeakObjectSubclass(pointeeType))
+                report(
+                    DiagnosticsEngine::Warning,
+                    "cppu::OWeakObject subclass %0 being managed via raw pointer, should be managed via rtl::Reference",
+                    varDecl->getLocation())
+                    << pointeeType
+                    << varDecl->getSourceRange();
+        }
+    }
+    return true;
+}
+
+/**
+    Look for code like
+        static_cast<FooChild*>(makeFoo().get());
+    where makeFoo() returns a Reference<Foo>
+*/
+bool RefCounting::isCastingReference(const Expr* expr)
+{
+    expr = compat::IgnoreImplicit(expr);
+    auto castExpr = dyn_cast<CastExpr>(expr);
+    if (!castExpr)
+        return false;
+    auto memberCallExpr = dyn_cast<CXXMemberCallExpr>(castExpr->getSubExpr());
+    if (!memberCallExpr)
+        return false;
+    if (!memberCallExpr->getMethodDecl()->getIdentifier() || memberCallExpr->getMethodDecl()->getName() != "get")
+        return false;
+    QualType objectType = memberCallExpr->getImplicitObjectArgument()->getType();
+    if (!loplugin::TypeCheck(objectType).Class("Reference"))
+        return false;
+    // ignore "x.get()" where x is a var
+    auto obj = compat::IgnoreImplicit(memberCallExpr->getImplicitObjectArgument());
+    if (isa<DeclRefExpr>(obj) || isa<MemberExpr>(obj))
+        return false;
+    // if the foo in foo().get() returns "rtl::Reference<T>&" then the variable
+    // we are assigning to does not __have__ to be Reference, since the method called
+    // must already be holding a reference.
+    if (auto callExpr = dyn_cast<CallExpr>(obj))
+    {
+        if (auto callMethod = callExpr->getDirectCallee())
+            if (callMethod->getReturnType()->isReferenceType())
+                return false;
     }
     return true;
 }
@@ -706,14 +757,14 @@ bool RefCounting::VisitBinaryOperator(const BinaryOperator * binaryOperator)
     if (!binaryOperator->getLHS()->getType()->isPointerType())
         return true;
 
-    // deliberately does not want to keep track at the allocation site
-    StringRef fileName = getFilenameOfLocation(compiler.getSourceManager().getSpellingLoc(compat::getBeginLoc(binaryOperator)));
-    if (loplugin::isSamePathname(fileName, SRCDIR "/vcl/unx/generic/dtrans/X11_selection.cxx"))
-        return true;
-
     auto newExpr = dyn_cast<CXXNewExpr>(compat::IgnoreImplicit(binaryOperator->getRHS()));
     if (newExpr)
     {
+        // deliberately does not want to keep track at the allocation site
+        StringRef fileName = getFilenameOfLocation(compiler.getSourceManager().getSpellingLoc(compat::getBeginLoc(binaryOperator)));
+        if (loplugin::isSamePathname(fileName, SRCDIR "/vcl/unx/generic/dtrans/X11_selection.cxx"))
+            return true;
+
         auto pointeeType = binaryOperator->getLHS()->getType()->getPointeeType();
         if (containsOWeakObjectSubclass(pointeeType))
         {
@@ -725,6 +776,17 @@ bool RefCounting::VisitBinaryOperator(const BinaryOperator * binaryOperator)
                 << binaryOperator->getSourceRange();
         }
     }
+    if (isCastingReference(binaryOperator->getRHS()))
+    {
+        auto pointeeType = binaryOperator->getLHS()->getType()->getPointeeType();
+        if (containsOWeakObjectSubclass(pointeeType))
+            report(
+                DiagnosticsEngine::Warning,
+                "cppu::OWeakObject subclass %0 being managed via raw pointer, should be managed via rtl::Reference",
+                compat::getBeginLoc(binaryOperator))
+                << pointeeType
+                << binaryOperator->getSourceRange();
+    }
     return true;
 }
 
diff --git a/compilerplugins/clang/test/refcounting.cxx b/compilerplugins/clang/test/refcounting.cxx
index 4c133023b0b8..7ab830fc913b 100644
--- a/compilerplugins/clang/test/refcounting.cxx
+++ b/compilerplugins/clang/test/refcounting.cxx
@@ -27,6 +27,9 @@ public:
 struct UnoObject : public cppu::OWeakObject
 {
 };
+struct UnoSubObject : public UnoObject
+{
+};
 
 //
 // Note, getting duplicate warnings for some reason I cannot fathom
@@ -94,5 +97,24 @@ rtl::Reference<UnoObject> foo6()
     // no warning expected
     return new UnoObject;
 }
+const rtl::Reference<UnoObject>& getConstRef();
+void foo7()
+{
+    // expected-error@+1 {{cppu::OWeakObject subclass 'UnoSubObject' being managed via raw pointer, should be managed via rtl::Reference [loplugin:refcounting]}}
+    UnoSubObject* p1 = static_cast<UnoSubObject*>(foo6().get());
+    (void)p1;
+    // expected-error@+1 {{cppu::OWeakObject subclass 'UnoSubObject' being managed via raw pointer, should be managed via rtl::Reference [loplugin:refcounting]}}
+    p1 = static_cast<UnoSubObject*>(foo6().get());
+
+    rtl::Reference<UnoObject> u2;
+    // no warning expected
+    UnoSubObject* p2 = static_cast<UnoSubObject*>(u2.get());
+    (void)p2;
+    p2 = static_cast<UnoSubObject*>(u2.get());
+    // no warning expected
+    UnoSubObject* p3 = static_cast<UnoSubObject*>(getConstRef().get());
+    (void)p3;
+    p3 = static_cast<UnoSubObject*>(getConstRef().get());
+}
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
diff --git a/sc/source/ui/Accessibility/AccessibleSpreadsheet.cxx b/sc/source/ui/Accessibility/AccessibleSpreadsheet.cxx
index aee01834103e..457adf6d0213 100644
--- a/sc/source/ui/Accessibility/AccessibleSpreadsheet.cxx
+++ b/sc/source/ui/Accessibility/AccessibleSpreadsheet.cxx
@@ -493,8 +493,9 @@ void ScAccessibleSpreadsheet::Notify( SfxBroadcaster& rBC, const SfxHint& rHint
                 if(aNewCell.Tab() != maActiveCell.Tab())
                 {
                     aEvent.EventId = AccessibleEventId::PAGE_CHANGED;
+                    auto pAccParent = getAccessibleParent();
                     ScAccessibleDocument *pAccDoc =
-                        static_cast<ScAccessibleDocument*>(getAccessibleParent().get());
+                        static_cast<ScAccessibleDocument*>(pAccParent.get());
                     if(pAccDoc)
                     {
                         pAccDoc->CommitChange(aEvent);
diff --git a/sd/source/ui/dlg/sdtreelb.cxx b/sd/source/ui/dlg/sdtreelb.cxx
index 764301858ad4..1afad1a87d48 100644
--- a/sd/source/ui/dlg/sdtreelb.cxx
+++ b/sd/source/ui/dlg/sdtreelb.cxx
@@ -612,7 +612,8 @@ void SdPageObjsTLV::AddShapeToTransferable (
             if ( ! (xFrameAccess->getByIndex(nIndex) >>= xFrame))
                 continue;
 
-            ::sd::DrawController* pController = dynamic_cast<sd::DrawController*>(xFrame->getController().get());
+            auto xController = xFrame->getController();
+            ::sd::DrawController* pController = dynamic_cast<sd::DrawController*>(xController.get());
             if (pController == nullptr)
                 continue;
             ::sd::ViewShellBase* pBase = pController->GetViewShellBase();
diff --git a/writerfilter/source/ooxml/OOXMLFastHelper.hxx b/writerfilter/source/ooxml/OOXMLFastHelper.hxx
index b68baee63b96..89bbf0c850b6 100644
--- a/writerfilter/source/ooxml/OOXMLFastHelper.hxx
+++ b/writerfilter/source/ooxml/OOXMLFastHelper.hxx
@@ -28,7 +28,7 @@ template <class T>
 class OOXMLFastHelper
 {
 public:
-    static OOXMLFastContextHandler* createAndSetParentAndDefine
+    static rtl::Reference<OOXMLFastContextHandler> createAndSetParentAndDefine
     (OOXMLFastContextHandler * pHandler, sal_uInt32 nToken, Id nId, Id nDefine);
 
     static void newProperty(OOXMLFastContextHandler * pHandler,
@@ -36,9 +36,9 @@ public:
 };
 
 template <class T>
-OOXMLFastContextHandler* OOXMLFastHelper<T>::createAndSetParentAndDefine (OOXMLFastContextHandler * pHandler, sal_uInt32 nToken, Id nId, Id nDefine)
+rtl::Reference<OOXMLFastContextHandler> OOXMLFastHelper<T>::createAndSetParentAndDefine (OOXMLFastContextHandler * pHandler, sal_uInt32 nToken, Id nId, Id nDefine)
 {
-    OOXMLFastContextHandler * pTmp = new T(pHandler);
+    rtl::Reference<OOXMLFastContextHandler> pTmp = new T(pHandler);
 
     pTmp->setToken(nToken);
     pTmp->setId(nId);