loplugin:vclwidgets check for assigning from VclPt<T> to T*

Inspired by a recent bug report where we were assigning the result
of VclPtr<T>::Create to a raw pointer.

As a consequence, we also need to change various methods that were
returning newly created Window subclasses via raw pointer, to
instead return those via VclPtr

Change-Id: I8118e0195a5b2b4780e646cfb0e151692e54ae2b
Reviewed-on: https://gerrit.libreoffice.org/31318
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
Tested-by: Noel Grandin <noel.grandin@collabora.co.uk>
(cherry picked from commit e6ffb539ee232ea0c679928ff456c1cf97429f63)

2 files changed, 150 insertions, 20 deletions

diff --git a/compilerplugins/clang/test/vclwidgets.cxx b/compilerplugins/clang/test/vclwidgets.cxx
new file mode 100644
index 000000000000..c18c775c054a
--- /dev/null
+++ b/compilerplugins/clang/test/vclwidgets.cxx
@@ -0,0 +1,86 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4; fill-column: 100 -*- */
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+#include <sal/config.h>
+#include <vcl/vclreferencebase.hxx>
+
+struct Widget : public VclReferenceBase
+{
+    VclPtr<Widget> mpParent;
+
+    void widget1() // expected-error {{Unreferenced externally visible function definition [loplugin:unreffun]}}
+    {
+        // test that we ignore assignments from a member field
+        Widget* p = mpParent;
+        (void)p;
+        // test against false+
+        p = true ? mpParent.get() : nullptr;
+    }
+
+    ~Widget() override
+    {
+        disposeOnce();
+    }
+
+    void dispose() override
+    {
+        mpParent.clear();
+        VclReferenceBase::dispose();
+    }
+};
+
+VclPtr<Widget> f()
+{
+    return nullptr;
+}
+
+Widget* g()
+{
+    return nullptr;
+}
+
+// test the variable init detection
+void bar() // expected-error {{Unreferenced externally visible function definition [loplugin:unreffun]}}
+{
+    Widget* p = f(); // expected-error {{assigning a returned-by-value VclPtr<T> to a T* variable is dodgy, should be assigned to a VclPtr. If you know that the RHS does not return a newly created T, then add a '.get()' to the RHS [loplugin:vclwidgets]}}
+    (void)p;
+    Widget* q = g();
+    (void)q;
+    Widget* r = nullptr;
+    (void)r;
+}
+
+// test the assignment detection
+void bar2() // expected-error {{Unreferenced externally visible function definition [loplugin:unreffun]}}
+{
+    Widget* p;
+    p = nullptr;
+    p = f(); // expected-error {{assigning a returned-by-value VclPtr<T> to a T* variable is dodgy, should be assigned to a VclPtr. If you know that the RHS does not return a newly created T, then add a '.get()' to the RHS [loplugin:vclwidgets]}}
+    (void)p;
+    Widget* q;
+    q = g();
+    (void)q;
+}
+
+
+// test against false+
+
+template<class T>
+T * get() { return nullptr; }
+
+void bar3() // expected-error {{Unreferenced externally visible function definition [loplugin:unreffun]}}
+{
+    Widget* p;
+    p = get<Widget>();
+}
+
+
+
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
diff --git a/compilerplugins/clang/vclwidgets.cxx b/compilerplugins/clang/vclwidgets.cxx
index d6007a65537b..b373d93cec98 100644
--- a/compilerplugins/clang/vclwidgets.cxx
+++ b/compilerplugins/clang/vclwidgets.cxx
@@ -46,7 +46,7 @@ public:
     bool VisitCXXConstructExpr(const CXXConstructExpr *);
     bool VisitBinaryOperator(const BinaryOperator *);
 private:
-    void checkAssignmentForVclPtrToRawConversion(const Type* lhsType, const Expr* rhs);
+    void checkAssignmentForVclPtrToRawConversion(const SourceLocation& sourceLoc, const Type* lhsType, const Expr* rhs);
     bool isDisposeCallingSuperclassDispose(const CXXMethodDecl* pMethodDecl);
     bool mbCheckingMemcpy = false;
 };
@@ -251,13 +251,15 @@ bool VCLWidgets::VisitBinaryOperator(const BinaryOperator * binaryOperator)
     if ( !binaryOperator->isAssignmentOp() ) {
         return true;
     }
-    checkAssignmentForVclPtrToRawConversion(binaryOperator->getLHS()->getType().getTypePtr(), binaryOperator->getRHS());
+    SourceLocation spellingLocation = compiler.getSourceManager().getSpellingLoc(
+                          binaryOperator->getLocStart());
+    checkAssignmentForVclPtrToRawConversion(spellingLocation, binaryOperator->getLHS()->getType().getTypePtr(), binaryOperator->getRHS());
     return true;
 }
 
 // Look for places where we are accidentally assigning a returned-by-value VclPtr<T> to a T*, which generally
 // ends up in a use-after-free.
-void VCLWidgets::checkAssignmentForVclPtrToRawConversion(const Type* lhsType, const Expr* rhs)
+void VCLWidgets::checkAssignmentForVclPtrToRawConversion(const SourceLocation& spellingLocation, const Type* lhsType, const Expr* rhs)
 {
     if (!lhsType || !isa<PointerType>(lhsType)) {
         return;
@@ -265,32 +267,72 @@ void VCLWidgets::checkAssignmentForVclPtrToRawConversion(const Type* lhsType, co
     if (!rhs) {
         return;
     }
-    // lots of null checking for something weird going in SW that tends to crash clang with:
-    // const clang::ExtQualsTypeCommonBase *clang::QualType::getCommonPtr() const: Assertion `!isNull() && "Cannot retrieve a NULL type pointer"'
-    if (rhs->getType().getTypePtrOrNull()) {
-        if (const PointerType* pt = dyn_cast<PointerType>(rhs->getType())) {
-             const Type* pointeeType = pt->getPointeeType().getTypePtrOrNull();
-             if (pointeeType && !isa<SubstTemplateTypeParmType>(pointeeType)) {
-                return;
-             }
-        }
+    StringRef filename = compiler.getSourceManager().getFilename(spellingLocation);
+    if (filename == SRCDIR "/include/rtl/ref.hxx") {
+        return;
     }
     const CXXRecordDecl* pointeeClass = lhsType->getPointeeType()->getAsCXXRecordDecl();
     if (!isDerivedFromVclReferenceBase(pointeeClass)) {
         return;
     }
-    const ExprWithCleanups* exprWithCleanups = dyn_cast<ExprWithCleanups>(rhs);
-    if (!exprWithCleanups) {
+
+    // if we have T* on the LHS and VclPtr<T> on the RHS, we expect to see either
+    // an ImplicitCastExpr
+    // or a ExprWithCleanups and then an ImplicitCastExpr
+    if (auto implicitCastExpr = dyn_cast<ImplicitCastExpr>(rhs)) {
+        if (implicitCastExpr->getCastKind() != CK_UserDefinedConversion) {
+            return;
+        }
+        rhs = rhs->IgnoreCasts();
+    } else if (auto exprWithCleanups = dyn_cast<ExprWithCleanups>(rhs)) {
+        if (auto implicitCastExpr = dyn_cast<ImplicitCastExpr>(exprWithCleanups->getSubExpr())) {
+            if (implicitCastExpr->getCastKind() != CK_UserDefinedConversion) {
+                return;
+            }
+            rhs = exprWithCleanups->IgnoreCasts();
+        } else {
+            return;
+        }
+    } else {
+        return;
+    }
+    if (isa<CXXNullPtrLiteralExpr>(rhs)) {
         return;
     }
-    const ImplicitCastExpr* implicitCast = dyn_cast<ImplicitCastExpr>(exprWithCleanups->getSubExpr());
-    if (!implicitCast) {
+    if (isa<CXXThisExpr>(rhs)) {
         return;
     }
-    //rhs->getType().dump();
+
+    // ignore assignments from a member field to a local variable, to avoid unnecessary refcounting traffic
+    if (auto callExpr = dyn_cast<CXXMemberCallExpr>(rhs)) {
+        if (auto calleeMemberExpr = dyn_cast<MemberExpr>(callExpr->getCallee())) {
+            if ((calleeMemberExpr = dyn_cast<MemberExpr>(calleeMemberExpr->getBase()->IgnoreImpCasts()))) {
+                if (isa<FieldDecl>(calleeMemberExpr->getMemberDecl())) {
+                    return;
+                }
+            }
+        }
+    }
+
+    // ignore assignments from a local variable to a local variable, to avoid unnecessary refcounting traffic
+    if (auto callExpr = dyn_cast<CXXMemberCallExpr>(rhs)) {
+        if (auto calleeMemberExpr = dyn_cast<MemberExpr>(callExpr->getCallee())) {
+            if (auto declRefExpr = dyn_cast<DeclRefExpr>(calleeMemberExpr->getBase()->IgnoreImpCasts())) {
+                if (isa<VarDecl>(declRefExpr->getDecl())) {
+                    return;
+                }
+            }
+        }
+    }
+    if (auto declRefExpr = dyn_cast<DeclRefExpr>(rhs->IgnoreImpCasts())) {
+        if (isa<VarDecl>(declRefExpr->getDecl())) {
+             return;
+        }
+    }
+
     report(
         DiagnosticsEngine::Warning,
-        "assigning a returned-by-value VclPtr<T> to a T* variable is dodgy, should be assigned to a VclPtr",
+        "assigning a returned-by-value VclPtr<T> to a T* variable is dodgy, should be assigned to a VclPtr. If you know that the RHS does not return a newly created T, then add a '.get()' to the RHS",
          rhs->getSourceRange().getBegin())
         << rhs->getSourceRange();
 }
@@ -302,10 +344,12 @@ bool VCLWidgets::VisitVarDecl(const VarDecl * pVarDecl) {
     if (isa<ParmVarDecl>(pVarDecl)) {
         return true;
     }
+    SourceLocation spellingLocation = compiler.getSourceManager().getSpellingLoc(
+                          pVarDecl->getLocStart());
     if (pVarDecl->getInit()) {
-        checkAssignmentForVclPtrToRawConversion(pVarDecl->getType().getTypePtr(), pVarDecl->getInit());
+        checkAssignmentForVclPtrToRawConversion(spellingLocation, pVarDecl->getType().getTypePtr(), pVarDecl->getInit());
     }
-    StringRef aFileName = compiler.getSourceManager().getFilename(compiler.getSourceManager().getSpellingLoc(pVarDecl->getLocStart()));
+    StringRef aFileName = compiler.getSourceManager().getFilename(spellingLocation);
     if (aFileName == SRCDIR "/include/vcl/vclptr.hxx")
         return true;
     if (aFileName == SRCDIR "/vcl/source/window/layout.cxx")