new loplugin:bufferadd

look for OUStringBuffer append sequences that can be turned
into creating an OUString with + operations

Change-Id: Ica840dc096000307b4a105fb4d9ec7588a15ade6
Reviewed-on: https://gerrit.libreoffice.org/80809
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

2 files changed, 491 insertions, 0 deletions

diff --git a/compilerplugins/clang/bufferadd.cxx b/compilerplugins/clang/bufferadd.cxx
new file mode 100644
index 000000000000..deb97bb35c11
--- /dev/null
+++ b/compilerplugins/clang/bufferadd.cxx
@@ -0,0 +1,416 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/*
+ * 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/.
+ */
+#ifndef LO_CLANG_SHARED_PLUGINS
+
+#include <cassert>
+#include <string>
+#include <iostream>
+#include <unordered_set>
+
+#include "plugin.hxx"
+#include "check.hxx"
+#include "clang/AST/CXXInheritance.h"
+#include "clang/AST/StmtVisitor.h"
+
+/**
+    Look for *StringBuffer append sequences which can be converted to *String + sequences.
+*/
+
+namespace
+{
+class BufferAdd : public loplugin::FilteringPlugin<BufferAdd>
+{
+public:
+    explicit BufferAdd(loplugin::InstantiationData const& data)
+        : FilteringPlugin(data)
+    {
+    }
+
+    bool preRun() override
+    {
+        std::string fn(handler.getMainFileName());
+        loplugin::normalizeDotDotInFilePath(fn);
+        if (loplugin::hasPathnamePrefix(fn, SRCDIR "/sal/qa/rtl/oustring/"))
+            return false;
+        if (loplugin::hasPathnamePrefix(fn, SRCDIR "/sal/qa/rtl/oustringbuffer/"))
+            return false;
+        if (loplugin::hasPathnamePrefix(fn, SRCDIR "/sal/qa/rtl/strings/"))
+            return false;
+        if (loplugin::hasPathnamePrefix(fn, SRCDIR "/sal/qa/OStringBuffer/"))
+            return false;
+        // some false +
+        if (loplugin::isSamePathname(fn, SRCDIR "/unoidl/source/sourcetreeprovider.cxx"))
+            return false;
+        if (loplugin::isSamePathname(fn, SRCDIR "/writerfilter/source/dmapper/StyleSheetTable.cxx"))
+            return false;
+        if (loplugin::isSamePathname(fn, SRCDIR "/writerfilter/source/dmapper/GraphicImport.cxx"))
+            return false;
+        return true;
+    }
+
+    virtual void run() override
+    {
+        if (!preRun())
+            return;
+        TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
+        for (auto const& pair : goodMap)
+            if (!isa<ParmVarDecl>(pair.first) &&
+                // reference types have slightly weird behaviour
+                !pair.first->getType()->isReferenceType()
+                && badMap.find(pair.first) == badMap.end())
+                report(DiagnosticsEngine::Warning,
+                       "convert this append sequence into a *String + sequence",
+                       compat::getBeginLoc(pair.first))
+                    << pair.first->getSourceRange();
+    }
+
+    bool VisitStmt(Stmt const*);
+    bool VisitCallExpr(CallExpr const*);
+    bool VisitCXXConstructExpr(CXXConstructExpr const*);
+    bool VisitUnaryOperator(UnaryOperator const*);
+
+private:
+    void findBufferAssignOrAdd(const Stmt* parentStmt, Stmt const*);
+    Expr const* ignore(Expr const*);
+    bool isSideEffectFree(Expr const*);
+    bool isMethodOkToMerge(CXXMemberCallExpr const*);
+    void addToGoodMap(const VarDecl* varDecl, const Stmt* parentStmt);
+
+    std::unordered_map<const VarDecl*, const Stmt*> goodMap;
+    std::unordered_set<const VarDecl*> badMap;
+};
+
+bool BufferAdd::VisitStmt(Stmt const* stmt)
+{
+    if (ignoreLocation(stmt))
+        return true;
+
+    if (!isa<CompoundStmt>(stmt) && !isa<CXXCatchStmt>(stmt) && !isa<CXXForRangeStmt>(stmt)
+        && !isa<CXXTryStmt>(stmt) && !isa<DoStmt>(stmt) && !isa<ForStmt>(stmt) && !isa<IfStmt>(stmt)
+        && !isa<SwitchStmt>(stmt) && !isa<WhileStmt>(stmt))
+        return true;
+
+    for (auto it = stmt->child_begin(); it != stmt->child_end(); ++it)
+        if (*it)
+            findBufferAssignOrAdd(stmt, *it);
+
+    return true;
+}
+
+bool BufferAdd::VisitCallExpr(CallExpr const* callExpr)
+{
+    if (ignoreLocation(callExpr))
+        return true;
+
+    for (unsigned i = 0; i != callExpr->getNumArgs(); ++i)
+    {
+        auto a = ignore(callExpr->getArg(i));
+        if (auto declRefExpr = dyn_cast<DeclRefExpr>(a))
+            if (auto varDecl = dyn_cast<VarDecl>(declRefExpr->getDecl()))
+                badMap.insert(varDecl);
+    }
+    return true;
+}
+
+bool BufferAdd::VisitCXXConstructExpr(CXXConstructExpr const* callExpr)
+{
+    if (ignoreLocation(callExpr))
+        return true;
+
+    for (unsigned i = 0; i != callExpr->getNumArgs(); ++i)
+    {
+        auto a = ignore(callExpr->getArg(i));
+        if (auto declRefExpr = dyn_cast<DeclRefExpr>(a))
+            if (auto varDecl = dyn_cast<VarDecl>(declRefExpr->getDecl()))
+                badMap.insert(varDecl);
+    }
+    return true;
+}
+
+bool BufferAdd::VisitUnaryOperator(const UnaryOperator* unaryOp)
+{
+    if (ignoreLocation(unaryOp))
+        return true;
+    if (unaryOp->getOpcode() != UO_AddrOf)
+        return true;
+    auto a = ignore(unaryOp->getSubExpr());
+    if (auto declRefExpr = dyn_cast<DeclRefExpr>(a))
+        if (auto varDecl = dyn_cast<VarDecl>(declRefExpr->getDecl()))
+            badMap.insert(varDecl);
+    return true;
+}
+
+void BufferAdd::findBufferAssignOrAdd(const Stmt* parentStmt, Stmt const* stmt)
+{
+    if (auto exprCleanup = dyn_cast<ExprWithCleanups>(stmt))
+        stmt = exprCleanup->getSubExpr();
+    if (auto switchCase = dyn_cast<SwitchCase>(stmt))
+        stmt = switchCase->getSubStmt();
+    if (auto declStmt = dyn_cast<DeclStmt>(stmt))
+    {
+        if (declStmt->isSingleDecl())
+            if (auto varDeclLHS = dyn_cast_or_null<VarDecl>(declStmt->getSingleDecl()))
+            {
+                auto tc = loplugin::TypeCheck(varDeclLHS->getType());
+                if (!tc.Class("OUStringBuffer").Namespace("rtl").GlobalNamespace()
+                    && !tc.Class("OStringBuffer").Namespace("rtl").GlobalNamespace())
+                    return;
+                if (varDeclLHS->getStorageDuration() == SD_Static)
+                    return;
+                if (!varDeclLHS->hasInit())
+                    return;
+                auto cxxConstructExpr = dyn_cast<CXXConstructExpr>(ignore(varDeclLHS->getInit()));
+                if (cxxConstructExpr)
+                {
+                    if (cxxConstructExpr->getNumArgs() == 0)
+                    {
+                        addToGoodMap(varDeclLHS, parentStmt);
+                        return;
+                    }
+                    auto tc2 = loplugin::TypeCheck(cxxConstructExpr->getArg(0)->getType());
+                    if (cxxConstructExpr->getArg(0)->getType()->isBuiltinType()
+                        || tc2.LvalueReference().Class("OUStringBuffer")
+                        || tc2.LvalueReference().Class("OStringBuffer")
+                        || tc2.Class("OUStringBuffer") || tc2.Class("OStringBuffer"))
+                    {
+                        badMap.insert(varDeclLHS);
+                        return;
+                    }
+                }
+                if (!isSideEffectFree(varDeclLHS->getInit()))
+                    badMap.insert(varDeclLHS);
+                else
+                    addToGoodMap(varDeclLHS, parentStmt);
+            }
+        return;
+    }
+
+    // check for single calls to buffer method
+
+    if (auto memberCallExpr = dyn_cast<CXXMemberCallExpr>(stmt))
+    {
+        if (auto declRefExprLHS
+            = dyn_cast<DeclRefExpr>(ignore(memberCallExpr->getImplicitObjectArgument())))
+        {
+            auto methodDecl = memberCallExpr->getMethodDecl();
+            if (methodDecl && methodDecl->getIdentifier())
+                if (auto varDeclLHS = dyn_cast<VarDecl>(declRefExprLHS->getDecl()))
+                {
+                    auto tc = loplugin::TypeCheck(varDeclLHS->getType());
+                    if (tc.Class("OUStringBuffer").Namespace("rtl").GlobalNamespace()
+                        || tc.Class("OStringBuffer").Namespace("rtl").GlobalNamespace())
+                    {
+                        if (isMethodOkToMerge(memberCallExpr))
+                            addToGoodMap(varDeclLHS, parentStmt);
+                        else
+                            badMap.insert(varDeclLHS);
+                    }
+                }
+            return;
+        }
+    }
+
+    // now check for chained append calls
+
+    auto expr = dyn_cast<Expr>(stmt);
+    if (!expr)
+        return;
+    auto tc = loplugin::TypeCheck(expr->getType());
+    if (!tc.Class("OUStringBuffer").Namespace("rtl").GlobalNamespace()
+        && !tc.Class("OStringBuffer").Namespace("rtl").GlobalNamespace())
+        return;
+
+    // unwrap the chain (which runs from right to left)
+    const VarDecl* varDeclLHS = nullptr;
+    bool good = true;
+    while (true)
+    {
+        auto memberCallExpr = dyn_cast<CXXMemberCallExpr>(expr);
+        if (!memberCallExpr)
+            break;
+        good &= isMethodOkToMerge(memberCallExpr);
+
+        if (auto declRefExprLHS
+            = dyn_cast<DeclRefExpr>(ignore(memberCallExpr->getImplicitObjectArgument())))
+        {
+            varDeclLHS = dyn_cast<VarDecl>(declRefExprLHS->getDecl());
+            break;
+        }
+        expr = memberCallExpr->getImplicitObjectArgument();
+    }
+
+    if (varDeclLHS)
+    {
+        if (good)
+            addToGoodMap(varDeclLHS, parentStmt);
+        else
+            badMap.insert(varDeclLHS);
+    }
+}
+
+void BufferAdd::addToGoodMap(const VarDecl* varDecl, const Stmt* parentStmt)
+{
+    // check that vars are all inside the same compoundstmt, if they are not, we cannot combine them
+    auto it = goodMap.find(varDecl);
+    if (it != goodMap.end())
+    {
+        if (it->second == parentStmt)
+            return;
+        // don't treat these as parents, otherwise we eliminate .append.append sequences
+        if (isa<MemberExpr>(parentStmt))
+            return;
+        if (isa<CXXMemberCallExpr>(parentStmt))
+            return;
+        badMap.insert(varDecl);
+    }
+    else
+        goodMap.emplace(varDecl, parentStmt);
+}
+
+bool BufferAdd::isMethodOkToMerge(CXXMemberCallExpr const* memberCall)
+{
+    auto methodDecl = memberCall->getMethodDecl();
+    if (methodDecl->getNumParams() == 0)
+        return true;
+    auto tc2 = loplugin::TypeCheck(methodDecl->getParamDecl(0)->getType());
+    if (tc2.LvalueReference().Class("OUStringBuffer")
+        || tc2.LvalueReference().Class("OStringBuffer"))
+        return false;
+
+    auto name = methodDecl->getName();
+    if (name == "appendUninitialized" || name == "setLength" || name == "remove" || name == "insert"
+        || name == "appendAscii" || name == "appendUtf32")
+        return false;
+
+    auto rhs = memberCall->getArg(0);
+
+    if (loplugin::TypeCheck(memberCall->getType())
+            .Class("OStringBuffer")
+            .Namespace("rtl")
+            .GlobalNamespace())
+    {
+        // because we have no OStringLiteral1
+        if (tc2.Char())
+            return false;
+        // Can't see how to make the call to append(sal_Unicode*pStart, sal_Unicode*pEnd) work
+        if (memberCall->getNumArgs() == 2 && loplugin::TypeCheck(rhs->getType()).Pointer())
+            return false;
+    }
+    if (loplugin::TypeCheck(memberCall->getType())
+            .Class("OUStringBuffer")
+            .Namespace("rtl")
+            .GlobalNamespace())
+    {
+        // character literals we do with OUStringBuffer, not variables of type sal_Unicode/char
+        if (tc2.Typedef("sal_Unicode").GlobalNamespace() && !isa<CharacterLiteral>(rhs))
+            return false;
+        // Can't see how to make the call to append(sal_Unicode*pStart, sal_Unicode*pEnd) work
+        if (memberCall->getNumArgs() == 2
+            && loplugin::TypeCheck(memberCall->getArg(0)->getType()).Pointer())
+            return false;
+    }
+    if (!isSideEffectFree(rhs))
+        return false;
+    return true;
+}
+
+Expr const* BufferAdd::ignore(Expr const* expr)
+{
+    return compat::IgnoreImplicit(compat::IgnoreImplicit(expr)->IgnoreParens());
+}
+
+bool BufferAdd::isSideEffectFree(Expr const* expr)
+{
+    expr = ignore(expr);
+    // I don't think the OUStringAppend functionality can handle this efficiently
+    if (isa<ConditionalOperator>(expr))
+        return false;
+    // Multiple statements have a well defined evaluation order (sequence points between them)
+    // but a single expression may be evaluated in arbitrary order;
+    // if there are side effects in one of the sub-expressions that have an effect on another subexpression,
+    // the result may be incorrect, and you don't necessarily notice in tests because the order is compiler-dependent.
+    // for example see commit afd743141f7a7dd05914d0872c9afe079f16fe0c where such a refactoring introduced such a bug.
+    // So only consider simple RHS expressions.
+    if (!expr->HasSideEffects(compiler.getASTContext()))
+        return true;
+
+    // check for chained adds which are side-effect free
+    if (auto operatorCall = dyn_cast<CXXOperatorCallExpr>(expr))
+    {
+        auto op = operatorCall->getOperator();
+        if (op == OO_PlusEqual || op == OO_Plus)
+            if (isSideEffectFree(operatorCall->getArg(0))
+                && isSideEffectFree(operatorCall->getArg(1)))
+                return true;
+    }
+
+    if (auto callExpr = dyn_cast<CallExpr>(expr))
+    {
+        // check for calls through OUString::number/OUString::unacquired
+        if (auto calleeMethodDecl = dyn_cast_or_null<CXXMethodDecl>(callExpr->getCalleeDecl()))
+            if (calleeMethodDecl && calleeMethodDecl->getIdentifier())
+            {
+                auto name = calleeMethodDecl->getName();
+                if (name == "number" || name == "unacquired")
+                {
+                    auto tc = loplugin::TypeCheck(calleeMethodDecl->getParent());
+                    if (tc.Class("OUString") || tc.Class("OString"))
+                    {
+                        if (isSideEffectFree(callExpr->getArg(0)))
+                            return true;
+                    }
+                }
+            }
+        if (auto calleeFunctionDecl = dyn_cast_or_null<FunctionDecl>(callExpr->getCalleeDecl()))
+            if (calleeFunctionDecl && calleeFunctionDecl->getIdentifier())
+            {
+                auto name = calleeFunctionDecl->getName();
+                // check for calls through OUStringToOString
+                if (name == "OUStringToOString" || name == "OStringToOUString")
+                    if (isSideEffectFree(callExpr->getArg(0)))
+                        return true;
+                // whitelist some known-safe methods
+                if (name.endswith("ResId") || name == "GetXMLToken")
+                    if (isSideEffectFree(callExpr->getArg(0)))
+                        return true;
+            }
+    }
+
+    // sometimes we have a constructor call on the RHS
+    if (auto constructExpr = dyn_cast<CXXConstructExpr>(expr))
+    {
+        auto dc = loplugin::DeclCheck(constructExpr->getConstructor());
+        if (dc.MemberFunction().Class("OUString") || dc.MemberFunction().Class("OString")
+            || dc.MemberFunction().Class("OUStringBuffer")
+            || dc.MemberFunction().Class("OStringBuffer"))
+            if (constructExpr->getNumArgs() == 0 || isSideEffectFree(constructExpr->getArg(0)))
+                return true;
+        // Expr::HasSideEffects does not like stuff that passes through OUStringLiteral
+        auto dc2 = loplugin::DeclCheck(constructExpr->getConstructor()->getParent());
+        if (dc2.Struct("OUStringLiteral").Namespace("rtl").GlobalNamespace())
+            return true;
+    }
+
+    // when adding literals, we sometimes get this
+    if (auto functionalCastExpr = dyn_cast<CXXFunctionalCastExpr>(expr))
+    {
+        auto tc = loplugin::TypeCheck(functionalCastExpr->getType());
+        if (tc.Struct("OUStringLiteral").Namespace("rtl").GlobalNamespace())
+            return isSideEffectFree(functionalCastExpr->getSubExpr());
+    }
+
+    return false;
+}
+
+loplugin::Plugin::Registration<BufferAdd> bufferadd("bufferadd");
+}
+
+#endif // LO_CLANG_SHARED_PLUGINS
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/compilerplugins/clang/test/bufferadd.cxx b/compilerplugins/clang/test/bufferadd.cxx
new file mode 100644
index 000000000000..18c6055111a4
--- /dev/null
+++ b/compilerplugins/clang/test/bufferadd.cxx
@@ -0,0 +1,75 @@
+/* -*- 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 <rtl/strbuf.hxx>
+#include <rtl/string.hxx>
+#include <rtl/ustrbuf.hxx>
+#include <rtl/ustring.hxx>
+
+// ---------------------------------------------------------------
+// replaceing OUStringBuffer.append sequences to OUString+
+namespace test1
+{
+void f1()
+{
+    // expected-error@+1 {{convert this append sequence into a *String + sequence [loplugin:bufferadd]}}
+    OUStringBuffer v;
+    v.append("xxx");
+    v.append("xxx");
+}
+void f2()
+{
+    // expected-error@+1 {{convert this append sequence into a *String + sequence [loplugin:bufferadd]}}
+    OUStringBuffer v;
+    v.append("xxx").append("aaaa");
+}
+}
+
+namespace test2
+{
+void f2()
+{
+    // no warning expected
+    OUStringBuffer v;
+    v.append("xxx");
+    if (true)
+        v.append("yyyy");
+}
+void appendTo(OUStringBuffer&);
+void f3()
+{
+    // no warning expected
+    OUStringBuffer v;
+    appendTo(v);
+    v.append("xxx");
+}
+void f4()
+{
+    // no warning expected
+    OUStringBuffer v;
+    v.append("xxx");
+    v.setLength(0);
+}
+void f5()
+{
+    // no warning expected
+    OUStringBuffer v;
+    v.append("xxx");
+    v[1] = 'x';
+}
+void f6()
+{
+    // no warning expected
+    OUStringBuffer noel1("xxx");
+    while (true)
+        noel1.append("ffff").append("aaa");
+}
+}
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */