compiler plugin warning about unwanted silent pointer-to-bool conversions

C/C++ silently converts pointers to booleans, which is presumably seen as
a good idea by lazy people, but it can occassionally silently break code
in strange ways, most notably by selecting a bool overload when no matching
overload for the pointer exists (OUStringBuffer::append() can break like that,
e.g. in ba37e4062f538db7e51d6a64ba544eeddbc567cf, other cases are
8e3bf1598fa95ac8d099e45ae4252e7654a6f590 or 28e4c0250e67a344b4d6088bdca2e680a4bffad0).

So far the plugin checks only conversions in function call arguments, there's
disabled code that could check more places, but I'm not aware so far of any place
where anything actually broke because of that, and it'd require fixups to be
explicit in some places (e.g. 'bool b = returns_pointer();' would require '!= NULL'
added), so for it'll be only the simple various and it can be made more strict if wanted.

Change-Id: I6a5d207daf925e6c2d1bf684060536795ecfcc35

1 files changed, 163 insertions, 0 deletions

diff --git a/compilerplugins/clang/pointertobool.cxx b/compilerplugins/clang/pointertobool.cxx
new file mode 100644
index 000000000000..6988c4f5d93b
--- /dev/null
+++ b/compilerplugins/clang/pointertobool.cxx
@@ -0,0 +1,163 @@
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * Based on LLVM/Clang.
+ *
+ * This file is distributed under the University of Illinois Open Source
+ * License. See LICENSE.TXT for details.
+ *
+ */
+
+#include "plugin.hxx"
+
+#include <clang/AST/ASTContext.h>
+#include <clang/Basic/SourceManager.h>
+#include <clang/Lex/Lexer.h>
+
+namespace loplugin
+{
+
+/*
+This is a compile check.
+
+Check for pointer-to-bool conversions (that are sadly implicit) which are unwanted
+and potentially mistakes.
+
+So far the only places that are checked are passing arguments to functions, as those
+could easily choose a different overload.
+
+The original idea was that the only conversions that are considered safe are
+in conditions (which in turn means also in ||, && and ! operators) and places
+where it's considered unlikely for it to be a problem (or rather, less of a problem
+than explicitly avoiding the warning in the code). The code for this is currently
+commented out (there are a couple of places such as 'bool foo = returns_pointer();'
+that would need modification), possibly enable those later.
+*/
+
+class PointerToBool
+    : public RecursiveASTVisitor< PointerToBool >
+    , public Plugin
+    {
+    public:
+        explicit PointerToBool( CompilerInstance& compiler );
+        void run();
+        bool VisitImplicitCastExpr( const ImplicitCastExpr* expr );
+    private:
+        bool ignoreConversion( const Stmt* stmt );
+    };
+
+PointerToBool::PointerToBool( CompilerInstance& compiler )
+    : Plugin( compiler )
+    {
+    }
+
+void PointerToBool::run()
+    {
+    TraverseDecl( compiler.getASTContext().getTranslationUnitDecl());
+    }
+
+bool PointerToBool::VisitImplicitCastExpr( const ImplicitCastExpr* expr )
+    {
+    if( ignoreLocation( expr ))
+        return true;
+    // Warning about CK_MemberPointerToBoolean would mean warning about
+    // cases there the 'safe bool' idiom is used, so give that such
+    // a conversion is otherwise unlikely anyway, it's probably better
+    // not to warn here at all (at least as long as the 'explicit bool'
+    // from C++11 is not in use).
+    if( expr->getCastKind() == CK_PointerToBoolean )
+        {
+        if( ignoreConversion( expr ))
+            return true;
+        report( DiagnosticsEngine::Warning,
+            "pointer %0 implicitly converted to bool", expr->getLocStart())
+            << expr->getSubExpr()->getType() << expr->getSourceRange();
+        SourceLocation endOfExpression = Lexer::getLocForEndOfToken( expr->getLocEnd(), 0, compiler.getSourceManager(), compiler.getLangOpts());
+        report( DiagnosticsEngine::Note,
+            "explicitly compare to null pointer to silence this warning", endOfExpression )
+            << FixItHint::CreateInsertion( endOfExpression, " != NULL" );
+        }
+    return true;
+    }
+
+bool PointerToBool::ignoreConversion( const Stmt* stmt )
+    {
+#if 1 // less strict version
+    const Stmt* parent = parentStmt( stmt );
+    if( parent == NULL )
+        return true;
+    switch( parent->getStmtClass())
+        {
+        case Stmt::ConditionalOperatorClass:
+            if( stmt == cast< ConditionalOperator >( parent )->getCond())
+                return true;
+            break;
+        case Stmt::BinaryOperatorClass:
+            {
+            const BinaryOperator* binary = cast< BinaryOperator >( parent );
+            if(( binary->getOpcode() == BO_LAnd || binary->getOpcode() == BO_LOr )
+                && ( stmt == binary->getLHS() || stmt == binary->getRHS()))
+                {
+                return true;
+                }
+            break;
+            }
+        case Stmt::UnaryOperatorClass:
+            {
+            const UnaryOperator* unary = cast< UnaryOperator >( parent );
+            if( unary->getOpcode() == UO_LNot && stmt == unary->getSubExpr())
+                return true;
+            break;
+            }
+        default:
+            if( const ExplicitCastExpr* castexpr = dyn_cast< ExplicitCastExpr >( parent ))
+                if( castexpr->getTypeAsWritten()->isBooleanType() && stmt == castexpr->getSubExpr())
+                    return true;
+            if( dyn_cast< CallExpr >( parent ))
+                return false; // The only place where it's not ignored.
+            break;
+        }
+    return ignoreConversion( parent );
+#else // more strict version
+    // Warn only if the expression is not used in a conditional context.
+    const Stmt* parent = parentStmt( stmt );
+    if( parent == NULL ) // Should not happen inside a function, but can happen inside
+        return false;    // ctor initializer list.
+    switch( parent->getStmtClass())
+        {
+        case Stmt::IfStmtClass:
+            return ( stmt == cast< IfStmt >( parent )->getCond());
+        case Stmt::WhileStmtClass:
+            return ( stmt == cast< WhileStmt >( parent )->getCond());
+        case Stmt::DoStmtClass:
+            return ( stmt == cast< DoStmt >( parent )->getCond());
+        case Stmt::ForStmtClass:
+            return ( stmt == cast< ForStmt >( parent )->getCond());
+        case Stmt::ConditionalOperatorClass:
+            return ( stmt == cast< ConditionalOperator >( parent )->getCond());
+        case Stmt::BinaryOperatorClass:
+            {
+            const BinaryOperator* binary = cast< BinaryOperator >( parent );
+            return (( binary->getOpcode() == BO_LAnd || binary->getOpcode() == BO_LOr )
+                && ( stmt == binary->getLHS() || stmt == binary->getRHS()));
+            }
+        case Stmt::UnaryOperatorClass:
+            {
+            const UnaryOperator* unary = cast< UnaryOperator >( parent );
+            return ( unary->getOpcode() == UO_LNot && stmt == unary->getSubExpr());
+            }
+        case Stmt::ExprWithCleanupsClass: // Often happens inside if() condition.
+            return isInConditionalContext( parent );
+        default:
+            if( const ExplicitCastExpr* castexpr = dyn_cast< ExplicitCastExpr >( parent ))
+                if( castexpr->getTypeAsWritten()->isBooleanType() && stmt == castexpr->getSubExpr())
+                    return true;
+            break;
+        }
+    return false;
+#endif
+    }
+
+static Plugin::Registration< PointerToBool > X( "pointertobool" );
+
+} // namespace