loplugin:referencecasting add check for new rtl::Reference operator

rtl::Reference now has a conversion operator to uno::Reference,
so look for places where we can simplify the code and use that.

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

2 files changed, 151 insertions, 55 deletions

diff --git a/compilerplugins/clang/referencecasting.cxx b/compilerplugins/clang/referencecasting.cxx
index aa11bc0738d7..496654237b7d 100644
--- a/compilerplugins/clang/referencecasting.cxx
+++ b/compilerplugins/clang/referencecasting.cxx
@@ -61,16 +61,34 @@ public:
     bool VisitCXXConstructExpr(const CXXConstructExpr* cce);
     bool VisitCXXMemberCallExpr(const CXXMemberCallExpr* mce);
     bool VisitCallExpr(const CallExpr*);
+    bool VisitInitListExpr(const InitListExpr*);
 
 private:
-    bool CheckForUnnecessaryGet(const Expr*);
+    bool CheckForUnnecessaryGet(const Expr*, bool includeRtlReference);
 };
 
-static const RecordType* extractTemplateType(const clang::Type*);
+static const RecordType* extractTemplateType(QualType);
 static bool isDerivedFrom(const CXXRecordDecl* subtypeRecord, const CXXRecordDecl* baseRecord);
 
+bool ReferenceCasting::VisitInitListExpr(const InitListExpr* ile)
+{
+    if (ignoreLocation(ile))
+        return true;
+    for (const Expr* expr : ile->inits())
+    {
+        if (CheckForUnnecessaryGet(expr, /*includeRtlReference*/ true))
+        {
+            report(DiagnosticsEngine::Warning, "unnecessary get() call", compat::getBeginLoc(expr))
+                << expr->getSourceRange();
+            return true;
+        }
+    }
+    return true;
+}
 bool ReferenceCasting::VisitCXXConstructExpr(const CXXConstructExpr* cce)
 {
+    if (ignoreLocation(cce))
+        return true;
     // 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(cce)));
@@ -79,24 +97,46 @@ bool ReferenceCasting::VisitCXXConstructExpr(const CXXConstructExpr* cce)
     if (loplugin::isSamePathname(aFileName, SRCDIR "/include/com/sun/star/uno/Reference.hxx"))
         return true;
 
+    if (cce->getNumArgs() == 0)
+        return true;
+
     // look for calls to the Reference<T>(x, UNO_something) constructor
     auto constructorClass = cce->getConstructor()->getParent();
-    if (!constructorClass->getIdentifier() || constructorClass->getName() != "Reference")
+    auto dc = loplugin::DeclCheck(constructorClass);
+    bool isUnoReference(dc.Class("Reference").Namespace("uno"));
+    bool isRtlReference(dc.Class("Reference").Namespace("rtl").GlobalNamespace());
+    if (!isUnoReference && !isRtlReference)
         return true;
 
-    if (cce->getNumArgs() != 2)
-        return true;
+    if (isUnoReference)
+        if (CheckForUnnecessaryGet(cce->getArg(0), /*includeRtlReference*/ cce->getNumArgs() == 1))
+        {
+            report(DiagnosticsEngine::Warning, "unnecessary get() call",
+                   compat::getBeginLoc(cce->getArg(0)))
+                << cce->getArg(0)->getSourceRange();
+            return true;
+        }
+    if (isRtlReference && cce->getNumArgs() == 1)
+        if (CheckForUnnecessaryGet(cce->getArg(0), /*includeRtlReference*/ true))
+        {
+            report(DiagnosticsEngine::Warning, "unnecessary get() call",
+                   compat::getBeginLoc(cce->getArg(0)))
+                << cce->getArg(0)->getSourceRange();
+            return true;
+        }
 
-    if (CheckForUnnecessaryGet(cce->getArg(0)))
-        report(DiagnosticsEngine::Warning, "unnecessary get() call", compat::getBeginLoc(cce))
-            << cce->getSourceRange();
+    if (isRtlReference)
+        return true;
+    if (isUnoReference && cce->getNumArgs() != 2)
+        return true;
 
-    // ignore the up-casting constructor
-    if (!isa<EnumType>(cce->getConstructor()->getParamDecl(1)->getType()))
+    // ignore the up-casting constructor, which has a std::enable_if second parameter
+    if (isUnoReference && cce->getNumArgs() == 2
+        && !isa<EnumType>(cce->getConstructor()->getParamDecl(1)->getType()))
         return true;
 
     // extract the type parameter passed to the template
-    const RecordType* templateParamType = extractTemplateType(cce->getType().getTypePtr());
+    const RecordType* templateParamType = extractTemplateType(cce->getType());
     if (!templateParamType)
         return true;
 
@@ -106,7 +146,7 @@ bool ReferenceCasting::VisitCXXConstructExpr(const CXXConstructExpr* cce)
         return true;
 
     // drill down the expression tree till we hit the bottom, because at the top, the type is BaseReference
-    const clang::Type* argType;
+    QualType argType;
     for (;;)
     {
         if (auto castExpr = dyn_cast<CastExpr>(constructorArg0))
@@ -134,7 +174,7 @@ bool ReferenceCasting::VisitCXXConstructExpr(const CXXConstructExpr* cce)
             constructorArg0 = parenExpr->getSubExpr();
             continue;
         }
-        argType = constructorArg0->getType().getTypePtr();
+        argType = constructorArg0->getType();
         break;
     }
 
@@ -155,19 +195,20 @@ bool ReferenceCasting::VisitCXXConstructExpr(const CXXConstructExpr* cce)
     if (templateParamRD->getName() == "XShape")
         return true;
 
-    if (auto declRefExpr = dyn_cast<DeclRefExpr>(cce->getArg(1)))
-    {
-        // no warning expected, used to reject null references
-        if (auto enumConstantDecl = dyn_cast<EnumConstantDecl>(declRefExpr->getDecl()))
+    if (cce->getNumArgs() == 2)
+        if (auto declRefExpr = dyn_cast<DeclRefExpr>(cce->getArg(1)))
         {
-            if (enumConstantDecl->getName() == "UNO_SET_THROW")
-                return true;
-            if (enumConstantDecl->getName() == "UNO_QUERY_THROW")
-                return true;
-            if (enumConstantDecl->getName() == "SAL_NO_ACQUIRE")
-                return true;
+            // no warning expected, used to reject null references
+            if (auto enumConstantDecl = dyn_cast<EnumConstantDecl>(declRefExpr->getDecl()))
+            {
+                if (enumConstantDecl->getName() == "UNO_SET_THROW")
+                    return true;
+                if (enumConstantDecl->getName() == "UNO_QUERY_THROW")
+                    return true;
+                if (enumConstantDecl->getName() == "SAL_NO_ACQUIRE")
+                    return true;
+            }
         }
-    }
 
     if (constructorArgRD->Equals(templateParamRD)
         || isDerivedFrom(constructorArgRD, templateParamRD))
@@ -182,6 +223,8 @@ bool ReferenceCasting::VisitCXXConstructExpr(const CXXConstructExpr* cce)
 
 bool ReferenceCasting::VisitCXXMemberCallExpr(const CXXMemberCallExpr* mce)
 {
+    if (ignoreLocation(mce))
+        return true;
     // 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(mce)));
@@ -190,21 +233,29 @@ bool ReferenceCasting::VisitCXXMemberCallExpr(const CXXMemberCallExpr* mce)
     if (loplugin::isSamePathname(aFileName, SRCDIR "/include/com/sun/star/uno/Reference.hxx"))
         return true;
 
+    if (mce->getNumArgs() == 0)
+        return true;
+
     // look for calls to the Reference<T>.set(x, UNO_QUERY) constructor
     auto method = mce->getMethodDecl();
     if (!method || !method->getIdentifier() || method->getName() != "set")
         return true;
-    if (mce->getNumArgs() != 2)
-        return true;
 
     auto methodRecordDecl = dyn_cast<ClassTemplateSpecializationDecl>(mce->getRecordDecl());
     if (!methodRecordDecl || !methodRecordDecl->getIdentifier()
         || methodRecordDecl->getName() != "Reference")
         return true;
 
-    if (CheckForUnnecessaryGet(mce->getArg(0)))
-        report(DiagnosticsEngine::Warning, "unnecessary get() call", compat::getBeginLoc(mce))
-            << mce->getSourceRange();
+    if (CheckForUnnecessaryGet(mce->getArg(0), /*includeRtlReference*/ mce->getNumArgs() == 1))
+    {
+        report(DiagnosticsEngine::Warning, "unnecessary get() call",
+               compat::getBeginLoc(mce->getArg(0)))
+            << mce->getArg(0)->getSourceRange();
+        return true;
+    }
+
+    if (mce->getNumArgs() != 2)
+        return true;
 
     // extract the type parameter passed to the template
     const RecordType* templateParamType
@@ -218,7 +269,7 @@ bool ReferenceCasting::VisitCXXMemberCallExpr(const CXXMemberCallExpr* mce)
         return true;
 
     // drill down the expression tree till we hit the bottom, because at the top, the type is BaseReference
-    const clang::Type* argType;
+    QualType argType;
     for (;;)
     {
         if (auto castExpr = dyn_cast<CastExpr>(arg0))
@@ -246,7 +297,7 @@ bool ReferenceCasting::VisitCXXMemberCallExpr(const CXXMemberCallExpr* mce)
             arg0 = parenExpr->getSubExpr();
             continue;
         }
-        argType = arg0->getType().getTypePtr();
+        argType = arg0->getType();
         break;
     }
 
@@ -267,19 +318,20 @@ bool ReferenceCasting::VisitCXXMemberCallExpr(const CXXMemberCallExpr* mce)
     if (templateParamRD->getName() == "XShape")
         return true;
 
-    if (auto declRefExpr = dyn_cast<DeclRefExpr>(mce->getArg(1)))
-    {
-        // no warning expected, used to reject null references
-        if (auto enumConstantDecl = dyn_cast<EnumConstantDecl>(declRefExpr->getDecl()))
+    if (mce->getNumArgs() == 2)
+        if (auto declRefExpr = dyn_cast<DeclRefExpr>(mce->getArg(1)))
         {
-            if (enumConstantDecl->getName() == "UNO_SET_THROW")
-                return true;
-            if (enumConstantDecl->getName() == "UNO_QUERY_THROW")
-                return true;
-            if (enumConstantDecl->getName() == "SAL_NO_ACQUIRE")
-                return true;
+            // no warning expected, used to reject null references
+            if (auto enumConstantDecl = dyn_cast<EnumConstantDecl>(declRefExpr->getDecl()))
+            {
+                if (enumConstantDecl->getName() == "UNO_SET_THROW")
+                    return true;
+                if (enumConstantDecl->getName() == "UNO_QUERY_THROW")
+                    return true;
+                if (enumConstantDecl->getName() == "SAL_NO_ACQUIRE")
+                    return true;
+            }
         }
-    }
 
     if (methodArgRD->Equals(templateParamRD) || isDerivedFrom(methodArgRD, templateParamRD))
     {
@@ -293,6 +345,8 @@ bool ReferenceCasting::VisitCXXMemberCallExpr(const CXXMemberCallExpr* mce)
 
 bool ReferenceCasting::VisitCallExpr(const CallExpr* ce)
 {
+    if (ignoreLocation(ce))
+        return true;
     // 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)));
@@ -313,9 +367,10 @@ bool ReferenceCasting::VisitCallExpr(const CallExpr* ce)
         || methodRecordDecl->getName() != "Reference")
         return true;
 
-    if (CheckForUnnecessaryGet(ce->getArg(0)))
-        report(DiagnosticsEngine::Warning, "unnecessary get() call", compat::getBeginLoc(ce))
-            << ce->getSourceRange();
+    if (CheckForUnnecessaryGet(ce->getArg(0), /*includeRtlReference*/ true))
+        report(DiagnosticsEngine::Warning, "unnecessary get() call",
+               compat::getBeginLoc(ce->getArg(0)))
+            << ce->getArg(0)->getSourceRange();
 
     // extract the type parameter passed to the template
     const RecordType* templateParamType
@@ -329,7 +384,7 @@ bool ReferenceCasting::VisitCallExpr(const CallExpr* ce)
         return true;
 
     // drill down the expression tree till we hit the bottom, because at the top, the type is BaseReference
-    const clang::Type* argType;
+    QualType argType;
     for (;;)
     {
         if (auto castExpr = dyn_cast<CastExpr>(arg0))
@@ -357,7 +412,7 @@ bool ReferenceCasting::VisitCallExpr(const CallExpr* ce)
             arg0 = parenExpr->getSubExpr();
             continue;
         }
-        argType = arg0->getType().getTypePtr();
+        argType = arg0->getType();
         break;
     }
 
@@ -393,9 +448,9 @@ bool ReferenceCasting::VisitCallExpr(const CallExpr* ce)
         Reference<T>(x.get(), UNO_QUERY)
     because sometimes simplifying that means the main purpose of this plugin can kick in.
  */
-bool ReferenceCasting::CheckForUnnecessaryGet(const Expr* expr)
+bool ReferenceCasting::CheckForUnnecessaryGet(const Expr* expr, bool includeRtlReference)
 {
-    expr = expr->IgnoreImplicit();
+    expr = compat::IgnoreImplicit(expr);
     auto cxxMemberCallExpr = dyn_cast<CXXMemberCallExpr>(expr);
     if (!cxxMemberCallExpr)
         return false;
@@ -407,7 +462,12 @@ bool ReferenceCasting::CheckForUnnecessaryGet(const Expr* expr)
 
     if (!loplugin::TypeCheck(expr->getType()).Pointer())
         return false;
-    if (!loplugin::DeclCheck(methodDecl->getParent()).Class("Reference").Namespace("uno"))
+    auto dc = loplugin::DeclCheck(methodDecl->getParent());
+    if (dc.Class("Reference").Namespace("uno"))
+        ; // ok
+    else if (includeRtlReference && dc.Class("Reference").Namespace("rtl"))
+        ; // ok
+    else
         return false;
 
     StringRef aFileName = getFilenameOfLocation(
@@ -418,7 +478,7 @@ bool ReferenceCasting::CheckForUnnecessaryGet(const Expr* expr)
     return true;
 }
 
-static const RecordType* extractTemplateType(const clang::Type* cceType)
+static const RecordType* extractTemplateType(QualType cceType)
 {
     // check for passing raw pointer to interface case
     if (cceType->isPointerType())
@@ -426,6 +486,8 @@ static const RecordType* extractTemplateType(const clang::Type* cceType)
         auto pointeeType = cceType->getPointeeType();
         if (auto elaboratedType = dyn_cast<ElaboratedType>(pointeeType))
             pointeeType = elaboratedType->desugar();
+        if (auto substTemplateTypeParmType = dyn_cast<SubstTemplateTypeParmType>(pointeeType))
+            pointeeType = substTemplateTypeParmType->desugar();
         if (auto recordType = dyn_cast<RecordType>(pointeeType))
             return recordType;
     }
@@ -433,7 +495,7 @@ static const RecordType* extractTemplateType(const clang::Type* cceType)
     // extract Foo from Reference<Foo>
     if (auto subst = dyn_cast<SubstTemplateTypeParmType>(cceType))
     {
-        if (auto recType = dyn_cast<RecordType>(subst->desugar().getTypePtr()))
+        if (auto recType = dyn_cast<RecordType>(subst->desugar()))
         {
             if (auto ctsd = dyn_cast<ClassTemplateSpecializationDecl>(recType->getDecl()))
             {
@@ -445,16 +507,16 @@ static const RecordType* extractTemplateType(const clang::Type* cceType)
     }
 
     if (auto elaboratedType = dyn_cast<ElaboratedType>(cceType))
-        cceType = elaboratedType->desugar().getTypePtr();
+        cceType = elaboratedType->desugar();
     auto cceTST = dyn_cast<TemplateSpecializationType>(cceType);
     if (!cceTST)
         return NULL;
     if (cceTST->getNumArgs() != 1)
         return NULL;
     const TemplateArgument& cceTA = cceTST->getArg(0);
-    const clang::Type* templateParamType = cceTA.getAsType().getTypePtr();
+    QualType templateParamType = cceTA.getAsType();
     if (auto elaboratedType = dyn_cast<ElaboratedType>(templateParamType))
-        templateParamType = elaboratedType->desugar().getTypePtr();
+        templateParamType = elaboratedType->desugar();
     return dyn_cast<RecordType>(templateParamType);
 }
 
diff --git a/compilerplugins/clang/test/referencecasting.cxx b/compilerplugins/clang/test/referencecasting.cxx
index 0864aec0f697..bb52cfbfdd17 100644
--- a/compilerplugins/clang/test/referencecasting.cxx
+++ b/compilerplugins/clang/test/referencecasting.cxx
@@ -8,12 +8,15 @@
  */
 
 #include "sal/config.h"
+#include "config_clang.h"
 
+#include "com/sun/star/uno/Sequence.hxx"
 #include "com/sun/star/uno/XInterface.hpp"
 #include "com/sun/star/io/XStreamListener.hpp"
 #include "com/sun/star/lang/XTypeProvider.hpp"
 #include "com/sun/star/lang/XComponent.hpp"
 #include "cppuhelper/weak.hxx"
+#include "rtl/ref.hxx"
 
 void test1(const css::uno::Reference<css::io::XStreamListener>& a)
 {
@@ -79,6 +82,37 @@ void test(css::uno::Reference<css::io::XStreamListener> l)
     // expected-error@+1 {{unnecessary get() call [loplugin:referencecasting]}}
     a.set(l.get(), css::uno::UNO_QUERY);
 }
+
+class FooStream : public css::io::XStreamListener
+{
+    virtual ~FooStream();
+};
+void test(rtl::Reference<FooStream> l)
+{
+    // expected-error@+1 {{unnecessary get() call [loplugin:referencecasting]}}
+    css::uno::Reference<css::io::XStreamListener> a(l.get());
+    // expected-error@+1 {{the source reference is already a subtype of the destination reference, just use = [loplugin:referencecasting]}}
+    a.set(l.get(), css::uno::UNO_QUERY);
+    // expected-error@+1 {{unnecessary get() call [loplugin:referencecasting]}}
+    a.set(l.get());
+    // expected-error@+1 {{the source reference is already a subtype of the destination reference, just use = [loplugin:referencecasting]}}
+    css::uno::Reference<css::io::XStreamListener> b(l.get(), css::uno::UNO_QUERY);
+    // no warning expected
+    css::uno::Reference<css::lang::XTypeProvider> c(l.get(), css::uno::UNO_QUERY);
+    // no warning expected
+    css::uno::Reference<css::io::XStreamListener> a2 = l;
+    (void)a2;
+}
+// not should about the exact version I should use here,
+// clang 5.0.2 visits the CXXConstructorExpr inside the initializer, while clang 11 does not
+#if CLANG_VERSION >= 60000
+css::uno::Sequence<css::uno::Reference<css::io::XStreamListener>> getContinuations()
+{
+    rtl::Reference<FooStream> noel1;
+    // expected-error@+1 {{unnecessary get() call [loplugin:referencecasting]}}
+    return { noel1.get() };
+}
+#endif
 }
 
 namespace test8