new loplugin:shouldreturnbool

look for methods returning only 1 and/or 0, which (most of the time)
should be returning bool.

Off by default, because some of this is a matter of taste

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

2 files changed, 281 insertions, 0 deletions

diff --git a/compilerplugins/clang/shouldreturnbool.cxx b/compilerplugins/clang/shouldreturnbool.cxx
new file mode 100644
index 000000000000..d993bf25f21e
--- /dev/null
+++ b/compilerplugins/clang/shouldreturnbool.cxx
@@ -0,0 +1,250 @@
+/* -*- 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/.
+ */
+
+#include <string>
+#include <set>
+#include <iostream>
+
+#include "check.hxx"
+#include "plugin.hxx"
+#include "functionaddress.hxx"
+
+/*
+ Look for functions that only return 1 and/or 0, which sometimes indicates that the
+ function should be returning bool.
+
+ Note that is partly a question of taste and code style, which is why this plugin is off by default.
+*/
+
+namespace
+{
+class ShouldReturnBool : public loplugin::FunctionAddress<ShouldReturnBool>
+{
+public:
+    explicit ShouldReturnBool(loplugin::InstantiationData const& data)
+        : loplugin::FunctionAddress<ShouldReturnBool>(data)
+    {
+    }
+
+    virtual void run() override
+    {
+        if (!compiler.getLangOpts().CPlusPlus)
+            return;
+        StringRef fn(compiler.getSourceManager()
+                         .getFileEntryForID(compiler.getSourceManager().getMainFileID())
+                         ->getName());
+        // functions used as function pointers
+        if (loplugin::isSamePathname(fn, SRCDIR "/sal/rtl/alloc_cache.cxx"))
+            return;
+        // false +, slightly odd usage, but not wrong
+        if (loplugin::isSamePathname(fn, SRCDIR "/libreofficekit/qa/tilebench/tilebench.cxx")
+            || loplugin::isSamePathname(fn, SRCDIR "/smoketest/libtest.cxx"))
+            return;
+        // uses the Unix convention of "non-zero return indicates error"
+        if (loplugin::isSamePathname(fn, SRCDIR "/idlc/source/idlcproduce.cxx"))
+            return;
+        // template magic
+        if (loplugin::isSamePathname(fn, SRCDIR "/vcl/source/gdi/bmpfast.cxx"))
+            return;
+        // fine
+        if (loplugin::isSamePathname(fn, SRCDIR "/svl/unx/source/svdde/ddedummy.cxx"))
+            return;
+        if (loplugin::isSamePathname(fn, SRCDIR "/vcl/source/opengl/OpenGLHelper.cxx"))
+            return;
+        if (loplugin::isSamePathname(fn, SRCDIR "/svtools/source/misc/imap2.cxx"))
+            return;
+        if (loplugin::isSamePathname(fn, SRCDIR "/svx/source/dialog/docrecovery.cxx"))
+            return;
+        if (loplugin::isSamePathname(fn, SRCDIR "/hwpfilter/source/lexer.cxx"))
+            return;
+        if (loplugin::isSamePathname(fn, SRCDIR "/hwpfilter/source/grammar.cxx"))
+            return;
+        if (loplugin::isSamePathname(
+                fn, SRCDIR "/connectivity/source/drivers/odbc/ODatabaseMetaDataResultSet.cxx"))
+            return;
+        if (loplugin::isSamePathname(fn, SRCDIR "/dbaccess/source/ui/browser/dsEntriesNoExp.cxx"))
+            return;
+        if (loplugin::isSamePathname(fn, SRCDIR "/lotuswordpro/source/filter/explode.cxx"))
+            return;
+        if (loplugin::isSamePathname(fn, SRCDIR "/filter/source/graphicfilter/ipict/ipict.cxx"))
+            return;
+        if (loplugin::isSamePathname(fn, SRCDIR "/sc/source/core/data/dptabsrc.cxx"))
+            return;
+        if (loplugin::isSamePathname(fn, SRCDIR "/sc/source/ui/docshell/docsh3.cxx"))
+            return;
+        if (loplugin::isSamePathname(fn, SRCDIR "/sd/source/ui/dlg/masterlayoutdlg.cxx"))
+            return;
+        if (loplugin::isSamePathname(fn, SRCDIR "/sd/source/filter/ppt/pptinanimations.cxx"))
+            return;
+        if (loplugin::isSamePathname(fn, SRCDIR "/vcl/unx/generic/app/i18n_im.cxx"))
+            return;
+
+        // callback
+        if (loplugin::isSamePathname(fn, SRCDIR "/sax/source/expatwrap/sax_expat.cxx"))
+            return;
+        if (loplugin::isSamePathname(fn, SRCDIR "/xmlsecurity/source/xmlsec/xmlstreamio.cxx"))
+            return;
+        if (loplugin::isSamePathname(fn, SRCDIR "/sw/source/filter/ww8/ww8par.cxx"))
+            return;
+        if (loplugin::isSamePathname(fn, SRCDIR "/sw/source/filter/ww8/ww8par2.cxx"))
+            return;
+        if (loplugin::isSamePathname(fn, SRCDIR "/sw/source/filter/ww8/ww8par5.cxx"))
+            return;
+        // SaxWriterHelper::writeSequence a little weird
+        if (loplugin::isSamePathname(fn, SRCDIR "/sax/source/expatwrap/saxwriter.cxx"))
+            return;
+        // main function
+        if (loplugin::isSamePathname(fn, SRCDIR "/xmlsecurity/workben/pdfverify.cxx"))
+            return;
+
+        TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
+
+        for (auto functionDecl : problemFunctions)
+        {
+            auto canonicalDecl = functionDecl->getCanonicalDecl();
+            if (getFunctionsWithAddressTaken().find(canonicalDecl)
+                != getFunctionsWithAddressTaken().end())
+                continue;
+            report(DiagnosticsEngine::Warning,
+                   "only returning one or zero is an indication you want to return bool",
+                   functionDecl->getLocStart())
+                << functionDecl->getSourceRange();
+            if (canonicalDecl->getLocation() != functionDecl->getLocation())
+            {
+                report(DiagnosticsEngine::Note, "canonical function declaration here",
+                       canonicalDecl->getLocStart())
+                    << canonicalDecl->getSourceRange();
+            }
+        }
+    }
+
+    bool TraverseFunctionDecl(FunctionDecl*);
+    bool TraverseCXXMethodDecl(CXXMethodDecl*);
+    bool VisitReturnStmt(ReturnStmt const*);
+
+private:
+    bool mbInsideFunction = false;
+    bool mbFunctionOnlyReturningOneOrZero = false;
+    std::unordered_set<FunctionDecl const*> problemFunctions;
+
+    bool IsInteresting(FunctionDecl const*);
+    void Report(FunctionDecl const*) const;
+    bool isExprOneOrZero(Expr const*) const;
+};
+
+bool ShouldReturnBool::TraverseFunctionDecl(FunctionDecl* functionDecl)
+{
+    bool ret;
+    if (IsInteresting(functionDecl))
+    {
+        mbInsideFunction = true;
+        mbFunctionOnlyReturningOneOrZero = true;
+        ret = loplugin::FunctionAddress<ShouldReturnBool>::TraverseFunctionDecl(functionDecl);
+        mbInsideFunction = false;
+        if (mbFunctionOnlyReturningOneOrZero)
+            problemFunctions.insert(functionDecl);
+    }
+    else
+        ret = loplugin::FunctionAddress<ShouldReturnBool>::TraverseFunctionDecl(functionDecl);
+    return ret;
+}
+
+bool ShouldReturnBool::TraverseCXXMethodDecl(CXXMethodDecl* methodDecl)
+{
+    bool ret;
+    if (IsInteresting(methodDecl))
+    {
+        mbInsideFunction = true;
+        mbFunctionOnlyReturningOneOrZero = true;
+        ret = loplugin::FunctionAddress<ShouldReturnBool>::TraverseCXXMethodDecl(methodDecl);
+        mbInsideFunction = false;
+        if (mbFunctionOnlyReturningOneOrZero)
+            problemFunctions.insert(methodDecl);
+    }
+    else
+        ret = loplugin::FunctionAddress<ShouldReturnBool>::TraverseCXXMethodDecl(methodDecl);
+    return ret;
+}
+
+bool ShouldReturnBool::IsInteresting(FunctionDecl const* functionDecl)
+{
+    if (ignoreLocation(functionDecl))
+        return false;
+    // ignore stuff that forms part of the stable URE interface
+    if (isInUnoIncludeFile(functionDecl))
+        return false;
+    if (functionDecl->getTemplatedKind() != FunctionDecl::TK_NonTemplate)
+        return false;
+    if (!functionDecl->isThisDeclarationADefinition())
+        return false;
+    if (functionDecl->isMain())
+        return false;
+    if (functionDecl->isExternC() || functionDecl->isInExternCContext())
+        return false;
+    auto methodDecl = dyn_cast<CXXMethodDecl>(functionDecl);
+    if (methodDecl && methodDecl->isVirtual())
+        return false;
+    auto tc = loplugin::TypeCheck(functionDecl->getReturnType());
+    if (tc.AnyBoolean() || tc.Void())
+        return false;
+    auto returnType = functionDecl->getReturnType();
+    if (returnType->isEnumeralType() || !returnType->getUnqualifiedDesugaredType()->isIntegerType())
+        return false;
+    // Ignore functions that contains #ifdef-ery
+    if (containsPreprocessingConditionalInclusion(functionDecl->getSourceRange()))
+        return false;
+
+    // not sure what basegfx is doing here
+    StringRef fileName{ compiler.getSourceManager().getFilename(functionDecl->getLocation()) };
+    if (loplugin::isSamePathname(fileName, SRCDIR "/include/basegfx/range/basicrange.hxx"))
+        return false;
+    // false +
+    if (loplugin::isSamePathname(fileName, SRCDIR "/include/svl/macitem.hxx"))
+        return false;
+    if (loplugin::isSamePathname(fileName, SRCDIR "/lotuswordpro/source/filter/lwpcharsetmgr.hxx"))
+        return false;
+    if (loplugin::isSamePathname(fileName, SRCDIR "/sc/inc/dptabsrc.hxx"))
+        return false;
+
+    return true;
+}
+
+bool ShouldReturnBool::VisitReturnStmt(const ReturnStmt* returnStmt)
+{
+    if (!mbInsideFunction)
+        return true;
+    if (!returnStmt->getRetValue())
+        return true;
+    if (loplugin::TypeCheck(returnStmt->getRetValue()->getType()).AnyBoolean())
+        return true;
+    if (!isExprOneOrZero(returnStmt->getRetValue()))
+        mbFunctionOnlyReturningOneOrZero = false;
+    return true;
+}
+
+bool ShouldReturnBool::isExprOneOrZero(const Expr* arg) const
+{
+    arg = arg->IgnoreParenCasts();
+    // ignore this, it seems to trigger an infinite recursion
+    if (isa<UnaryExprOrTypeTraitExpr>(arg))
+    {
+        return false;
+    }
+    APSInt x1;
+    if (arg->EvaluateAsInt(x1, compiler.getASTContext()))
+    {
+        return x1 == 1 || x1 == 0;
+    }
+    return false;
+}
+
+loplugin::Plugin::Registration<ShouldReturnBool> X("shouldreturnbool", false);
+}
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/compilerplugins/clang/test/shouldreturnbool.cxx b/compilerplugins/clang/test/shouldreturnbool.cxx
new file mode 100644
index 000000000000..03a698e30ed4
--- /dev/null
+++ b/compilerplugins/clang/test/shouldreturnbool.cxx
@@ -0,0 +1,31 @@
+/* -*- 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/.
+ */
+
+int foo1(char ch)
+{
+    // expected-error@-2 {{only returning one or zero is an indication you want to return bool [loplugin:shouldreturnbool]}}
+    if (ch == 'x')
+        return 1;
+    return 0;
+}
+
+long foo2()
+{
+    // expected-error@-2 {{only returning one or zero is an indication you want to return bool [loplugin:shouldreturnbool]}}
+    return 1;
+}
+
+enum Enum1
+{
+    NONE
+};
+
+Enum1 foo3() { return Enum1::NONE; }
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */