loplugin:referencecasting check for Reference::query

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

9 files changed, 108 insertions, 8 deletions

diff --git a/compilerplugins/clang/referencecasting.cxx b/compilerplugins/clang/referencecasting.cxx
index f23e2f6811cd..aa11bc0738d7 100644
--- a/compilerplugins/clang/referencecasting.cxx
+++ b/compilerplugins/clang/referencecasting.cxx
@@ -60,6 +60,7 @@ public:
 
     bool VisitCXXConstructExpr(const CXXConstructExpr* cce);
     bool VisitCXXMemberCallExpr(const CXXMemberCallExpr* mce);
+    bool VisitCallExpr(const CallExpr*);
 
 private:
     bool CheckForUnnecessaryGet(const Expr*);
@@ -290,6 +291,103 @@ bool ReferenceCasting::VisitCXXMemberCallExpr(const CXXMemberCallExpr* mce)
     return true;
 }
 
+bool ReferenceCasting::VisitCallExpr(const CallExpr* ce)
+{
+    // don't bother processing anything in the Reference.h file. Makes my life easier when debugging this.
+    StringRef aFileName = getFilenameOfLocation(
+        compiler.getSourceManager().getSpellingLoc(compat::getBeginLoc(ce)));
+    if (loplugin::isSamePathname(aFileName, SRCDIR "/include/com/sun/star/uno/Reference.h"))
+        return true;
+    if (loplugin::isSamePathname(aFileName, SRCDIR "/include/com/sun/star/uno/Reference.hxx"))
+        return true;
+
+    // look for calls to Reference<T>::query(x)
+    auto method = dyn_cast_or_null<CXXMethodDecl>(ce->getDirectCallee());
+    if (!method || !method->getIdentifier() || method->getName() != "query")
+        return true;
+    if (ce->getNumArgs() != 1)
+        return true;
+
+    auto methodRecordDecl = dyn_cast<ClassTemplateSpecializationDecl>(method->getParent());
+    if (!methodRecordDecl || !methodRecordDecl->getIdentifier()
+        || methodRecordDecl->getName() != "Reference")
+        return true;
+
+    if (CheckForUnnecessaryGet(ce->getArg(0)))
+        report(DiagnosticsEngine::Warning, "unnecessary get() call", compat::getBeginLoc(ce))
+            << ce->getSourceRange();
+
+    // extract the type parameter passed to the template
+    const RecordType* templateParamType
+        = dyn_cast<RecordType>(methodRecordDecl->getTemplateArgs()[0].getAsType());
+    if (!templateParamType)
+        return true;
+
+    // extract the type of the first parameter passed to the method
+    const Expr* arg0 = ce->getArg(0);
+    if (!arg0)
+        return true;
+
+    // drill down the expression tree till we hit the bottom, because at the top, the type is BaseReference
+    const clang::Type* argType;
+    for (;;)
+    {
+        if (auto castExpr = dyn_cast<CastExpr>(arg0))
+        {
+            arg0 = castExpr->getSubExpr();
+            continue;
+        }
+        if (auto matTempExpr = dyn_cast<MaterializeTemporaryExpr>(arg0))
+        {
+            arg0 = compat::getSubExpr(matTempExpr);
+            continue;
+        }
+        if (auto bindTempExpr = dyn_cast<CXXBindTemporaryExpr>(arg0))
+        {
+            arg0 = bindTempExpr->getSubExpr();
+            continue;
+        }
+        if (auto tempObjExpr = dyn_cast<CXXTemporaryObjectExpr>(arg0))
+        {
+            arg0 = tempObjExpr->getArg(0);
+            continue;
+        }
+        if (auto parenExpr = dyn_cast<ParenExpr>(arg0))
+        {
+            arg0 = parenExpr->getSubExpr();
+            continue;
+        }
+        argType = arg0->getType().getTypePtr();
+        break;
+    }
+
+    const RecordType* argTemplateType = extractTemplateType(argType);
+    if (!argTemplateType)
+        return true;
+
+    CXXRecordDecl* templateParamRD = dyn_cast<CXXRecordDecl>(templateParamType->getDecl());
+    CXXRecordDecl* methodArgRD = dyn_cast<CXXRecordDecl>(argTemplateType->getDecl());
+
+    // querying for XInterface (instead of doing an upcast) has special semantics,
+    // to check for UNO object equivalence.
+    if (templateParamRD->getName() == "XInterface")
+        return true;
+
+    // XShape is used in UNO aggregates in very "entertaining" ways, which means an UNO_QUERY
+    // can return a completely different object, e.g. see SwXShape::queryInterface
+    if (templateParamRD->getName() == "XShape")
+        return true;
+
+    if (methodArgRD->Equals(templateParamRD) || isDerivedFrom(methodArgRD, templateParamRD))
+    {
+        report(DiagnosticsEngine::Warning,
+               "the source reference is already a subtype of the destination reference, just use =",
+               compat::getBeginLoc(ce))
+            << ce->getSourceRange();
+    }
+    return true;
+}
+
 /**
     Check for
         Reference<T>(x.get(), UNO_QUERY)
diff --git a/compilerplugins/clang/test/referencecasting.cxx b/compilerplugins/clang/test/referencecasting.cxx
index 0272bc89cc98..0864aec0f697 100644
--- a/compilerplugins/clang/test/referencecasting.cxx
+++ b/compilerplugins/clang/test/referencecasting.cxx
@@ -19,6 +19,8 @@ void test1(const css::uno::Reference<css::io::XStreamListener>& a)
 {
     // expected-error@+1 {{the source reference is already a subtype of the destination reference, just use = [loplugin:referencecasting]}}
     css::uno::Reference<css::lang::XEventListener> b(a, css::uno::UNO_QUERY);
+    // expected-error@+1 {{the source reference is already a subtype of the destination reference, just use = [loplugin:referencecasting]}}
+    auto c = css::uno::Reference<css::lang::XEventListener>::query(a);
 }
 
 namespace test2
diff --git a/connectivity/source/drivers/file/FDriver.cxx b/connectivity/source/drivers/file/FDriver.cxx
index 16cf60e02cb4..54be5f952429 100644
--- a/connectivity/source/drivers/file/FDriver.cxx
+++ b/connectivity/source/drivers/file/FDriver.cxx
@@ -181,7 +181,7 @@ Reference< XTablesSupplier > SAL_CALL OFileDriver::getDataDefinitionByConnection
         OConnection* pConnection = nullptr;
         for (auto const& elem : m_xConnections)
         {
-            if (static_cast<OConnection*>( Reference< XConnection >::query(elem.get().get()).get() ) == pSearchConnection)
+            if (static_cast<OConnection*>( Reference< XConnection >::query(elem.get()).get() ) == pSearchConnection)
             {
                 pConnection = pSearchConnection;
                 break;
diff --git a/editeng/source/uno/unotext2.cxx b/editeng/source/uno/unotext2.cxx
index 8aa040c97637..1e61337f5471 100644
--- a/editeng/source/uno/unotext2.cxx
+++ b/editeng/source/uno/unotext2.cxx
@@ -234,7 +234,7 @@ void SAL_CALL SvxUnoTextContent::attach( const uno::Reference< text::XTextRange
 
 uno::Reference< text::XTextRange > SAL_CALL SvxUnoTextContent::getAnchor()
 {
-    return uno::Reference< text::XTextRange >::query( mxParentText );
+    return mxParentText;
 }
 
 // XComponent
diff --git a/forms/source/helper/controlfeatureinterception.cxx b/forms/source/helper/controlfeatureinterception.cxx
index 1e0d0fbcd03d..091af550cc29 100644
--- a/forms/source/helper/controlfeatureinterception.cxx
+++ b/forms/source/helper/controlfeatureinterception.cxx
@@ -93,7 +93,7 @@ namespace frm
                 // reconnect the chain
                 if ( xMaster.is() )
                 {
-                    xMaster->setSlaveDispatchProvider( Reference< XDispatchProvider >::query( xSlave ) );
+                    xMaster->setSlaveDispatchProvider( xSlave );
                 }
 
                 // if somebody has registered the same interceptor twice, then we will remove
diff --git a/svx/source/fmcomp/fmgridif.cxx b/svx/source/fmcomp/fmgridif.cxx
index fc8d52735ea6..96f6fc4c2526 100644
--- a/svx/source/fmcomp/fmgridif.cxx
+++ b/svx/source/fmcomp/fmgridif.cxx
@@ -2482,7 +2482,7 @@ void FmXGridPeer::releaseDispatchProviderInterceptor(const Reference< css::frame
             if (xMaster.is())
             {
                 if (xSlave.is())
-                    xMaster->setSlaveDispatchProvider(Reference< css::frame::XDispatchProvider >::query(xSlave));
+                    xMaster->setSlaveDispatchProvider(xSlave);
                 else
                     // it's the first interceptor of the chain, set ourself as slave
                     xMaster->setSlaveDispatchProvider(static_cast<css::frame::XDispatchProvider*>(this));
diff --git a/ucb/source/cacher/dynamicresultsetwrapper.cxx b/ucb/source/cacher/dynamicresultsetwrapper.cxx
index 28dc828944a2..3aa765da316a 100644
--- a/ucb/source/cacher/dynamicresultsetwrapper.cxx
+++ b/ucb/source/cacher/dynamicresultsetwrapper.cxx
@@ -301,7 +301,7 @@ void SAL_CALL DynamicResultSetWrapper::setSource( const Reference< XInterface >
     else if( bStatic )
     {
         Reference< XComponent > xSourceComponent( Source, UNO_QUERY );
-        xSourceComponent->addEventListener( Reference< XEventListener > ::query( xMyListenerImpl ) );
+        xSourceComponent->addEventListener( xMyListenerImpl );
     }
     m_aSourceSet.set();
 }
@@ -354,7 +354,7 @@ void SAL_CALL DynamicResultSetWrapper::setListener( const Reference< XDynamicRes
             throw ListenerAlreadySetException();
 
         m_xListener = Listener;
-        addEventListener( Reference< XEventListener >::query( Listener ) );
+        addEventListener( Listener );
 
         xSource = m_xSource;
         xMyListenerImpl = m_xMyListenerImpl.get();
diff --git a/ucb/source/sorter/sortdynres.cxx b/ucb/source/sorter/sortdynres.cxx
index dc7ad5ea64fc..d2455ca11ec0 100644
--- a/ucb/source/sorter/sortdynres.cxx
+++ b/ucb/source/sorter/sortdynres.cxx
@@ -169,7 +169,7 @@ SortedDynamicResultSet::setListener( const Reference< XDynamicResultSetListener
     if ( mxListener.is() )
         throw ListenerAlreadySetException();
 
-    addEventListener( Reference< XEventListener >::query( Listener ) );
+    addEventListener( Listener );
 
     mxListener = Listener;
 
diff --git a/xmloff/source/draw/ximpstyl.cxx b/xmloff/source/draw/ximpstyl.cxx
index 05ecaf377eec..00d47c5c6f3a 100644
--- a/xmloff/source/draw/ximpstyl.cxx
+++ b/xmloff/source/draw/ximpstyl.cxx
@@ -1288,7 +1288,7 @@ uno::Reference< container::XNameAccess > SdXMLStylesContext::getPageLayouts() co
         }
     }
 
-    return uno::Reference< container::XNameAccess >::query( xLayouts );
+    return xLayouts;
 }