Clang plugin that flags implicit conversions from bool

...as they are often enough errors, like a typo in brace placement in

  if (foo == (FOO || bar == BAR) && baz)

or a literal true passed as an argument to a function that rather expects an
integer bit mask, etc.  The plugin is smart enough to detect interaction with
logically boolean return/parameter types of C functions that use [unsigned] int
instead, and knows the relevant boolean typedefs (sal_Bool, gboolean, etc.).

Change-Id: I5f0e4344fe86589bec35a71018c7effdedf85e3e

compilerplugins/clang/implicitboolconversion.cxx

@@ -0,0 +1,480 @@
+/* -*- 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 <algorithm>
+#include <cassert>
+#include <cstddef>
+#include <iterator>
+#include <stack>
+#include <string>
+#include <vector>
+
+#include "plugin.hxx"
+
+template<> struct std::iterator_traits<ExprIterator> {
+    typedef std::ptrdiff_t difference_type;
+    typedef Expr * value_type;
+    typedef Expr const ** pointer;
+    typedef Expr const & reference;
+    typedef std::random_access_iterator_tag iterator_category;
+};
+
+namespace {
+
+bool isBool(Expr const * expr) {
+    QualType t1 { expr->getType() };
+    if (t1->isBooleanType()) {
+        return true;
+    }
+// css::uno::Sequence<sal_Bool> s(1);s[0]=false /*unotools/source/config/configitem.cxx*/:
+if(t1->isSpecificBuiltinType(BuiltinType::UChar))return true;
+    TypedefType const * t2 = t1->getAs<TypedefType>();
+    if (t2 == nullptr) {
+        return false;
+    }
+    std::string name(t2->getDecl()->getNameAsString());
+    return name == "sal_Bool" || name == "FcBool" || name == "UBool"
+        || name == "dbus_bool_t" || name == "gboolean" || name == "hb_bool_t";
+}
+
+bool isBoolExpr(Expr const * expr) {
+    if (isBool(expr)) {
+        return true;
+    }
+    ConditionalOperator const * co = dyn_cast<ConditionalOperator>(expr);
+    if (co != nullptr) {
+        ImplicitCastExpr const * ic1 = dyn_cast<ImplicitCastExpr>(
+            co->getTrueExpr()->IgnoreParens());
+        ImplicitCastExpr const * ic2 = dyn_cast<ImplicitCastExpr>(
+            co->getFalseExpr()->IgnoreParens());
+        if (ic1 != nullptr && ic2 != nullptr
+            && ic1->getType()->isSpecificBuiltinType(BuiltinType::Int)
+            && isBoolExpr(ic1->getSubExpr()->IgnoreParens())
+            && ic2->getType()->isSpecificBuiltinType(BuiltinType::Int)
+            && isBoolExpr(ic2->getSubExpr()->IgnoreParens()))
+        {
+            return true;
+        }
+    }
+    return false;
+}
+
+class ImplicitBoolConversion:
+    public RecursiveASTVisitor<ImplicitBoolConversion>, public loplugin::Plugin
+{
+public:
+    explicit ImplicitBoolConversion(CompilerInstance & compiler):
+        Plugin(compiler) {}
+
+    virtual void run() override
+    { TraverseDecl(compiler.getASTContext().getTranslationUnitDecl()); }
+
+    bool TraverseCallExpr(CallExpr * expr);
+
+    bool TraverseCStyleCastExpr(CStyleCastExpr * expr);
+
+    bool TraverseCXXStaticCastExpr(CXXStaticCastExpr * expr);
+
+    bool TraverseCXXFunctionalCastExpr(CXXFunctionalCastExpr * expr);
+
+    bool TraverseConditionalOperator(ConditionalOperator * expr);
+
+    bool TraverseBinLT(BinaryOperator * expr);
+
+    bool TraverseBinLE(BinaryOperator * expr);
+
+    bool TraverseBinGT(BinaryOperator * expr);
+
+    bool TraverseBinGE(BinaryOperator * expr);
+
+    bool TraverseBinEQ(BinaryOperator * expr);
+
+    bool TraverseBinNE(BinaryOperator * expr);
+
+    bool TraverseBinAssign(BinaryOperator * expr);
+
+    bool TraverseBinAndAssign(CompoundAssignOperator * expr);
+
+    bool TraverseBinOrAssign(CompoundAssignOperator * expr);
+
+    bool TraverseBinXorAssign(CompoundAssignOperator * expr);
+
+    bool TraverseReturnStmt(ReturnStmt * stmt);
+
+    bool TraverseFunctionDecl(FunctionDecl * decl);
+
+    bool VisitImplicitCastExpr(ImplicitCastExpr const * expr);
+
+private:
+    void reportWarning(ImplicitCastExpr const * expr);
+
+    std::stack<std::vector<ImplicitCastExpr const *>> nested;
+    bool externCIntFunctionDefinition = false;
+};
+
+bool ImplicitBoolConversion::TraverseCallExpr(CallExpr * expr) {
+    nested.push(std::vector<ImplicitCastExpr const *>());
+    bool ret = RecursiveASTVisitor::TraverseCallExpr(expr);
+    Decl const * d = expr->getCalleeDecl();
+    bool ext = false;
+    FunctionProtoType const * t = nullptr;
+    if (d != nullptr) {
+        FunctionDecl const * fd = dyn_cast<FunctionDecl>(d);
+        if (fd != nullptr && (fd->isExternC() || fd->isInExternCContext())) {
+            ext = true;
+            PointerType const * pt = dyn_cast<PointerType>(fd->getType());
+            t = (pt == nullptr ? fd->getType() : pt->getPointeeType())
+                ->getAs<FunctionProtoType>();
+        } else {
+            VarDecl const * vd = dyn_cast<VarDecl>(d);
+            if (vd != nullptr && (vd->isExternC() || vd->isInExternCContext()))
+            {
+                ext = true;
+                PointerType const * pt = dyn_cast<PointerType>(vd->getType());
+                t = (pt == nullptr ? vd->getType() : pt->getPointeeType())
+                    ->getAs<FunctionProtoType>();
+            }
+        }
+    }
+    assert(!nested.empty());
+    for (auto i: nested.top()) {
+        if (ext) {
+            auto j = std::find_if(
+                expr->arg_begin(), expr->arg_end(),
+                [&i](Expr * e) { return i == e->IgnoreParens(); });
+            if (j == expr->arg_end()) {
+                reportWarning(i);
+            } else {
+                std::ptrdiff_t n = j - expr->arg_begin();
+                assert(n >= 0);
+                assert(n < t->getNumArgs() || t->isVariadic());
+                if (n < t->getNumArgs()
+                    && !(t->getArgType(n)->isSpecificBuiltinType(
+                             BuiltinType::Int)
+                         || (t->getArgType(n)->isSpecificBuiltinType(
+                                 BuiltinType::UInt))))
+                {
+                    reportWarning(i);
+                }
+            }
+        } else {
+            reportWarning(i);
+        }
+    }
+    nested.pop();
+    return ret;
+}
+
+bool ImplicitBoolConversion::TraverseCStyleCastExpr(CStyleCastExpr * expr) {
+    nested.push(std::vector<ImplicitCastExpr const *>());
+    bool ret = RecursiveASTVisitor::TraverseCStyleCastExpr(expr);
+    assert(!nested.empty());
+    for (auto i: nested.top()) {
+        if (i != expr->getSubExpr()->IgnoreParens()) {
+            reportWarning(i);
+        }
+    }
+    nested.pop();
+    return ret;
+}
+
+bool ImplicitBoolConversion::TraverseCXXStaticCastExpr(CXXStaticCastExpr * expr)
+{
+    nested.push(std::vector<ImplicitCastExpr const *>());
+    bool ret = RecursiveASTVisitor::TraverseCXXStaticCastExpr(expr);
+    assert(!nested.empty());
+    for (auto i: nested.top()) {
+        if (i != expr->getSubExpr()->IgnoreParens()) {
+            reportWarning(i);
+        }
+    }
+    nested.pop();
+    return ret;
+}
+
+bool ImplicitBoolConversion::TraverseCXXFunctionalCastExpr(
+    CXXFunctionalCastExpr * expr)
+{
+    nested.push(std::vector<ImplicitCastExpr const *>());
+    bool ret = RecursiveASTVisitor::TraverseCXXFunctionalCastExpr(expr);
+    assert(!nested.empty());
+    for (auto i: nested.top()) {
+        if (i != expr->getSubExpr()->IgnoreParens()) {
+            reportWarning(i);
+        }
+    }
+    nested.pop();
+    return ret;
+}
+
+bool ImplicitBoolConversion::TraverseConditionalOperator(
+    ConditionalOperator * expr)
+{
+    nested.push(std::vector<ImplicitCastExpr const *>());
+    bool ret = RecursiveASTVisitor::TraverseConditionalOperator(expr);
+    assert(!nested.empty());
+    for (auto i: nested.top()) {
+        if (!((i == expr->getTrueExpr()->IgnoreParens()
+               && isBoolExpr(expr->getFalseExpr()->IgnoreParenImpCasts()))
+              || (i == expr->getFalseExpr()->IgnoreParens()
+                  && isBoolExpr(expr->getTrueExpr()->IgnoreParenImpCasts()))))
+        {
+            reportWarning(i);
+        }
+    }
+    nested.pop();
+    return ret;
+}
+
+bool ImplicitBoolConversion::TraverseBinLT(BinaryOperator * expr) {
+    nested.push(std::vector<ImplicitCastExpr const *>());
+    bool ret = RecursiveASTVisitor::TraverseBinLT(expr);
+    assert(!nested.empty());
+    for (auto i: nested.top()) {
+        if (!((i == expr->getLHS()->IgnoreParens()
+               && isBool(expr->getRHS()->IgnoreParenImpCasts()))
+              || (i == expr->getRHS()->IgnoreParens()
+                  && isBool(expr->getLHS()->IgnoreParenImpCasts()))))
+        {
+            reportWarning(i);
+        }
+    }
+    nested.pop();
+    return ret;
+}
+
+bool ImplicitBoolConversion::TraverseBinLE(BinaryOperator * expr) {
+    nested.push(std::vector<ImplicitCastExpr const *>());
+    bool ret = RecursiveASTVisitor::TraverseBinLE(expr);
+    assert(!nested.empty());
+    for (auto i: nested.top()) {
+        if (!((i == expr->getLHS()->IgnoreParens()
+               && isBool(expr->getRHS()->IgnoreParenImpCasts()))
+              || (i == expr->getRHS()->IgnoreParens()
+                  && isBool(expr->getLHS()->IgnoreParenImpCasts()))))
+        {
+            reportWarning(i);
+        }
+    }
+    nested.pop();
+    return ret;
+}
+
+bool ImplicitBoolConversion::TraverseBinGT(BinaryOperator * expr) {
+    nested.push(std::vector<ImplicitCastExpr const *>());
+    bool ret = RecursiveASTVisitor::TraverseBinGT(expr);
+    assert(!nested.empty());
+    for (auto i: nested.top()) {
+        if (!((i == expr->getLHS()->IgnoreParens()
+               && isBool(expr->getRHS()->IgnoreParenImpCasts()))
+              || (i == expr->getRHS()->IgnoreParens()
+                  && isBool(expr->getLHS()->IgnoreParenImpCasts()))))
+        {
+            reportWarning(i);
+        }
+    }
+    nested.pop();
+    return ret;
+}
+
+bool ImplicitBoolConversion::TraverseBinGE(BinaryOperator * expr) {
+    nested.push(std::vector<ImplicitCastExpr const *>());
+    bool ret = RecursiveASTVisitor::TraverseBinGE(expr);
+    assert(!nested.empty());
+    for (auto i: nested.top()) {
+        if (!((i == expr->getLHS()->IgnoreParens()
+               && isBool(expr->getRHS()->IgnoreParenImpCasts()))
+              || (i == expr->getRHS()->IgnoreParens()
+                  && isBool(expr->getLHS()->IgnoreParenImpCasts()))))
+        {
+            reportWarning(i);
+        }
+    }
+    nested.pop();
+    return ret;
+}
+
+bool ImplicitBoolConversion::TraverseBinEQ(BinaryOperator * expr) {
+    nested.push(std::vector<ImplicitCastExpr const *>());
+    bool ret = RecursiveASTVisitor::TraverseBinEQ(expr);
+    assert(!nested.empty());
+    for (auto i: nested.top()) {
+        if (!((i == expr->getLHS()->IgnoreParens()
+               && isBool(expr->getRHS()->IgnoreParenImpCasts()))
+              || (i == expr->getRHS()->IgnoreParens()
+                  && isBool(expr->getLHS()->IgnoreParenImpCasts()))))
+        {
+            reportWarning(i);
+        }
+    }
+    nested.pop();
+    return ret;
+}
+
+bool ImplicitBoolConversion::TraverseBinNE(BinaryOperator * expr) {
+    nested.push(std::vector<ImplicitCastExpr const *>());
+    bool ret = RecursiveASTVisitor::TraverseBinNE(expr);
+    assert(!nested.empty());
+    for (auto i: nested.top()) {
+        if (!((i == expr->getLHS()->IgnoreParens()
+               && isBool(expr->getRHS()->IgnoreParenImpCasts()))
+              || (i == expr->getRHS()->IgnoreParens()
+                  && isBool(expr->getLHS()->IgnoreParenImpCasts()))))
+        {
+            reportWarning(i);
+        }
+    }
+    nested.pop();
+    return ret;
+}
+
+// /usr/include/gtk-2.0/gtk/gtktogglebutton.h: struct _GtkToggleButton:
+//  guint GSEAL (active) : 1;
+// even though <http://www.gtk.org/api/2.6/gtk/GtkToggleButton.html>:
+//  "active"               gboolean              : Read / Write
+bool ImplicitBoolConversion::TraverseBinAssign(BinaryOperator * expr) {
+    nested.push(std::vector<ImplicitCastExpr const *>());
+    bool ret = RecursiveASTVisitor::TraverseBinAssign(expr);
+    bool ext = false;
+    MemberExpr const * me = dyn_cast<MemberExpr>(expr->getLHS());
+    if (me != nullptr) {
+        FieldDecl const * fd = dyn_cast<FieldDecl>(me->getMemberDecl());
+        if (fd != nullptr && fd->isBitField()
+            && fd->getBitWidthValue(compiler.getASTContext()) == 1)
+        {
+            TypedefType const * t = fd->getType()->getAs<TypedefType>();
+            ext = t != nullptr && t->getDecl()->getNameAsString() == "guint";
+        }
+    }
+    assert(!nested.empty());
+    for (auto i: nested.top()) {
+        if (i != expr->getRHS()->IgnoreParens() || !ext) {
+            reportWarning(i);
+        }
+    }
+    nested.pop();
+    return ret;
+}
+
+bool ImplicitBoolConversion::TraverseBinAndAssign(CompoundAssignOperator * expr)
+{
+    nested.push(std::vector<ImplicitCastExpr const *>());
+    bool ret = RecursiveASTVisitor::TraverseBinAndAssign(expr);
+    assert(!nested.empty());
+    for (auto i: nested.top()) {
+        if (i != expr->getRHS()->IgnoreParens()
+            || !isBool(expr->getLHS()->IgnoreParens()))
+        {
+            reportWarning(i);
+        }
+    }
+    nested.pop();
+    return ret;
+}
+
+bool ImplicitBoolConversion::TraverseBinOrAssign(CompoundAssignOperator * expr)
+{
+    nested.push(std::vector<ImplicitCastExpr const *>());
+    bool ret = RecursiveASTVisitor::TraverseBinOrAssign(expr);
+    assert(!nested.empty());
+    for (auto i: nested.top()) {
+        if (i != expr->getRHS()->IgnoreParens()
+            || !isBool(expr->getLHS()->IgnoreParens()))
+        {
+            reportWarning(i);
+        }
+    }
+    nested.pop();
+    return ret;
+}
+
+bool ImplicitBoolConversion::TraverseBinXorAssign(CompoundAssignOperator * expr)
+{
+    nested.push(std::vector<ImplicitCastExpr const *>());
+    bool ret = RecursiveASTVisitor::TraverseBinXorAssign(expr);
+    assert(!nested.empty());
+    for (auto i: nested.top()) {
+        if (i != expr->getRHS()->IgnoreParens()
+            || !isBool(expr->getLHS()->IgnoreParens()))
+        {
+            reportWarning(i);
+        }
+    }
+    nested.pop();
+    return ret;
+}
+
+bool ImplicitBoolConversion::TraverseReturnStmt(ReturnStmt * stmt) {
+    nested.push(std::vector<ImplicitCastExpr const *>());
+    bool ret = RecursiveASTVisitor::TraverseReturnStmt(stmt);
+    Expr const * expr = stmt->getRetValue();
+    if (expr != nullptr) {
+        ExprWithCleanups const * ec = dyn_cast<ExprWithCleanups>(expr);
+        if (ec != nullptr) {
+            expr = ec->getSubExpr();
+        }
+        expr = expr->IgnoreParens();
+    }
+    assert(!nested.empty());
+    for (auto i: nested.top()) {
+        if (i != expr || !externCIntFunctionDefinition) {
+            reportWarning(i);
+        }
+    }
+    nested.pop();
+    return ret;
+}
+
+bool ImplicitBoolConversion::TraverseFunctionDecl(FunctionDecl * decl) {
+    bool ext = (decl->isExternC() || decl->isInExternCContext())
+        && decl->isThisDeclarationADefinition()
+        && (decl->getResultType()->isSpecificBuiltinType(BuiltinType::Int)
+            || decl->getResultType()->isSpecificBuiltinType(BuiltinType::UInt));
+    if (ext) {
+        assert(!externCIntFunctionDefinition);
+        externCIntFunctionDefinition = true;
+    }
+    bool ret = RecursiveASTVisitor::TraverseFunctionDecl(decl);
+    if (ext) {
+        externCIntFunctionDefinition = false;
+    }
+    return ret;
+}
+
+bool ImplicitBoolConversion::VisitImplicitCastExpr(
+    ImplicitCastExpr const * expr)
+{
+    if (ignoreLocation(expr)) {
+        return true;
+    }
+    if (expr->getSubExprAsWritten()->getType()->isBooleanType()
+        && !isBool(expr))
+    {
+        if (nested.empty()) {
+            reportWarning(expr);
+        } else {
+            nested.top().push_back(expr);
+        }
+    }
+    return true;
+}
+
+void ImplicitBoolConversion::reportWarning(ImplicitCastExpr const * expr) {
+    report(
+        DiagnosticsEngine::Warning,
+        "implicit conversion (%0) from bool to %1", expr->getLocStart())
+        << expr->getCastKindName() << expr->getType() << expr->getSourceRange();
+}
+
+loplugin::Plugin::Registration<ImplicitBoolConversion> X(
+    "implicitboolconversion");
+
+}
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab: */