Fix loplugin:simplifypointertobool for libstdc++ std::shared_ptr

...where the get member function is defined on a std::__shared_ptr base class,
so loplugin:simplifypointertobool used to miss those until now.  (While e.g.
using libc++ on macOS found those cases.)

366d08f2f6d4de922f6099c62bb81b49d89e0a68 "new loplugin:simplifypointertobool"
was mistaken in breaking isSmartPointerType(const clang::Type* t) out of
isSmartPointerType(const Expr* e); c874294ad9fb178df47c66875bfbdec466e39763 "Fix
detection of std::unique_ptr/shared_ptr in loplugin:redundantpointerops" had
introduced that indivisible two-step algorithm on purpose.

The amount of additional hits (on Linux) apparently asked for turning
loplugin:simplifypointertobool into a rewriting plugin.  Which in turn showed
that the naive adivce to just "drop the get()" is not sufficient in places that
are not contextually converted to bool, as those places need to be wrapped in a
bool(...) functional cast now.  If the expression was already wrapped in
parentheses, those could be reused as part of the functional cast, but
implementing that showed that such cases are not yet found at all by the
existing loplugin:simplifypointertobool.  Lets leave that TODO for another
commit.

Besides the changes to compilerplugins/ itself, this change has been generated
fully automatically with the rewriting plugin on Linux.

Change-Id: I83107d6f634fc9ac232986f49044d7017df83e2a
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/94888
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
Tested-by: Jenkins

5 files changed, 349 insertions, 14 deletions

diff --git a/compilerplugins/clang/plugin.cxx b/compilerplugins/clang/plugin.cxx
index 0ffab3ba36bd..5248927e5d84 100644
--- a/compilerplugins/clang/plugin.cxx
+++ b/compilerplugins/clang/plugin.cxx
@@ -837,17 +837,13 @@ bool isSmartPointerType(const Expr* e)
     if (tc1.ClassOrStruct("unique_ptr").StdNamespace()
           || tc1.ClassOrStruct("shared_ptr").StdNamespace())
         return true;
-    return isSmartPointerType(e->getType().getTypePtr());
-}
 
-bool isSmartPointerType(const clang::Type* t)
-{
     // Then check the object type coerced to the type of the get member function, in
     // case the type-as-written is derived from one of these types (tools::SvRef is
     // final, but the rest are not; but note that this will fail when the type-as-
     // written is derived from std::unique_ptr or std::shared_ptr for which the get
     // member function is declared at a base class):
-    auto const tc2 = loplugin::TypeCheck(t);
+    auto const tc2 = loplugin::TypeCheck(e->getType());
     if (tc2.ClassOrStruct("unique_ptr").StdNamespace()
            || tc2.ClassOrStruct("shared_ptr").StdNamespace()
            || tc2.Class("Reference").Namespace("uno").Namespace("star")
diff --git a/compilerplugins/clang/plugin.hxx b/compilerplugins/clang/plugin.hxx
index 2d43371feede..66d22adf97d6 100644
--- a/compilerplugins/clang/plugin.hxx
+++ b/compilerplugins/clang/plugin.hxx
@@ -319,7 +319,6 @@ int derivedFromCount(const CXXRecordDecl* subtypeRecord, const CXXRecordDecl* ba
 // if it contained the 'extern' specifier:
 bool hasExternalLinkage(VarDecl const * decl);
 
-bool isSmartPointerType(const clang::Type*);
 bool isSmartPointerType(const Expr*);
 
 } // namespace
diff --git a/compilerplugins/clang/sharedvisitor/dummyplugin.hxx b/compilerplugins/clang/sharedvisitor/dummyplugin.hxx
index ce5b352384b1..b24157965df2 100644
--- a/compilerplugins/clang/sharedvisitor/dummyplugin.hxx
+++ b/compilerplugins/clang/sharedvisitor/dummyplugin.hxx
@@ -37,8 +37,15 @@ public:
     bool TraverseDecl( Decl* ) { return complain(); }
     bool TraverseLinkageSpecDecl( LinkageSpecDecl* ) { return complain(); }
     bool TraverseStmt( Stmt* ) { return complain(); }
+    bool TraverseIfStmt( IfStmt* ) { return complain(); }
+    bool TraverseWhileStmt( WhileStmt* ) { return complain(); }
+    bool TraverseDoStmt( DoStmt* ) { return complain(); }
+    bool TraverseForStmt( ForStmt* ) { return complain(); }
+    bool TraverseParenExpr( ParenExpr* ) { return complain(); }
     bool TraverseUnaryLNot( UnaryOperator* ) { return complain(); }
     bool TraverseBinLAnd( BinaryOperator* ) { return complain(); }
+    bool TraverseBinLOr( BinaryOperator* ) { return complain(); }
+    bool TraverseConditionalOperator( ConditionalOperator* ) { return complain(); }
     bool TraverseCXXCatchStmt( CXXCatchStmt* ) { return complain(); }
     bool TraverseCXXDestructorDecl( CXXDestructorDecl* ) { return complain(); }
     bool TraverseFunctionDecl( FunctionDecl* ) { return complain(); }
diff --git a/compilerplugins/clang/simplifypointertobool.cxx b/compilerplugins/clang/simplifypointertobool.cxx
index da6581915a1e..4fc4901c3efe 100644
--- a/compilerplugins/clang/simplifypointertobool.cxx
+++ b/compilerplugins/clang/simplifypointertobool.cxx
@@ -7,7 +7,10 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/.
  */
 
+#include <algorithm>
 #include <cassert>
+#include <deque>
+#include <stack>
 #include <string>
 #include <iostream>
 #include <fstream>
@@ -16,6 +19,7 @@
 #include <clang/AST/CXXInheritance.h>
 #include "plugin.hxx"
 #include "check.hxx"
+#include "compat.hxx"
 
 /**
   Simplify boolean expressions involving smart pointers e.g.
@@ -27,11 +31,11 @@
 
 namespace
 {
-class SimplifyPointerToBool : public loplugin::FilteringPlugin<SimplifyPointerToBool>
+class SimplifyPointerToBool : public loplugin::FilteringRewritePlugin<SimplifyPointerToBool>
 {
 public:
     explicit SimplifyPointerToBool(loplugin::InstantiationData const& data)
-        : FilteringPlugin(data)
+        : FilteringRewritePlugin(data)
     {
     }
 
@@ -42,6 +46,280 @@ public:
     }
 
     bool VisitImplicitCastExpr(ImplicitCastExpr const*);
+
+    bool PreTraverseUnaryLNot(UnaryOperator* expr)
+    {
+        contextuallyConvertedExprs_.push_back(expr->getSubExpr()->IgnoreParenImpCasts());
+        return true;
+    }
+
+    bool PostTraverseUnaryLNot(UnaryOperator*, bool)
+    {
+        assert(!contextuallyConvertedExprs_.empty());
+        contextuallyConvertedExprs_.pop_back();
+        return true;
+    }
+
+    bool TraverseUnaryLNot(UnaryOperator* expr)
+    {
+        auto res = PreTraverseUnaryLNot(expr);
+        assert(res);
+        res = FilteringRewritePlugin::TraverseUnaryLNot(expr);
+        PostTraverseUnaryLNot(expr, res);
+        return res;
+    }
+
+    bool PreTraverseBinLAnd(BinaryOperator* expr)
+    {
+        contextuallyConvertedExprs_.push_back(expr->getLHS()->IgnoreParenImpCasts());
+        contextuallyConvertedExprs_.push_back(expr->getRHS()->IgnoreParenImpCasts());
+        return true;
+    }
+
+    bool PostTraverseBinLAnd(BinaryOperator*, bool)
+    {
+        assert(contextuallyConvertedExprs_.size() >= 2);
+        contextuallyConvertedExprs_.pop_back();
+        contextuallyConvertedExprs_.pop_back();
+        return true;
+    }
+
+    bool TraverseBinLAnd(BinaryOperator* expr)
+    {
+        auto res = PreTraverseBinLAnd(expr);
+        assert(res);
+        res = FilteringRewritePlugin::TraverseBinLAnd(expr);
+        PostTraverseBinLAnd(expr, res);
+        return res;
+    }
+
+    bool PreTraverseBinLOr(BinaryOperator* expr)
+    {
+        contextuallyConvertedExprs_.push_back(expr->getLHS()->IgnoreParenImpCasts());
+        contextuallyConvertedExprs_.push_back(expr->getRHS()->IgnoreParenImpCasts());
+        return true;
+    }
+
+    bool PostTraverseBinLOr(BinaryOperator*, bool)
+    {
+        assert(contextuallyConvertedExprs_.size() >= 2);
+        contextuallyConvertedExprs_.pop_back();
+        contextuallyConvertedExprs_.pop_back();
+        return true;
+    }
+
+    bool TraverseBinLOr(BinaryOperator* expr)
+    {
+        auto res = PreTraverseBinLOr(expr);
+        assert(res);
+        res = FilteringRewritePlugin::TraverseBinLOr(expr);
+        PostTraverseBinLOr(expr, res);
+        return res;
+    }
+
+    bool PreTraverseConditionalOperator(ConditionalOperator* expr)
+    {
+        contextuallyConvertedExprs_.push_back(expr->getCond()->IgnoreParenImpCasts());
+        return true;
+    }
+
+    bool PostTraverseConditionalOperator(ConditionalOperator*, bool)
+    {
+        assert(!contextuallyConvertedExprs_.empty());
+        contextuallyConvertedExprs_.pop_back();
+        return true;
+    }
+
+    bool TraverseConditionalOperator(ConditionalOperator* expr)
+    {
+        auto res = PreTraverseConditionalOperator(expr);
+        assert(res);
+        res = FilteringRewritePlugin::TraverseConditionalOperator(expr);
+        PostTraverseConditionalOperator(expr, res);
+        return res;
+    }
+
+    bool PreTraverseIfStmt(IfStmt* stmt)
+    {
+        contextuallyConvertedExprs_.push_back(stmt->getCond()->IgnoreParenImpCasts());
+        return true;
+    }
+
+    bool PostTraverseIfStmt(IfStmt*, bool)
+    {
+        assert(!contextuallyConvertedExprs_.empty());
+        contextuallyConvertedExprs_.pop_back();
+        return true;
+    }
+
+    bool TraverseIfStmt(IfStmt* stmt)
+    {
+        auto res = PreTraverseIfStmt(stmt);
+        assert(res);
+        res = FilteringRewritePlugin::TraverseIfStmt(stmt);
+        PostTraverseIfStmt(stmt, res);
+        return res;
+    }
+
+    bool PreTraverseWhileStmt(WhileStmt* stmt)
+    {
+        contextuallyConvertedExprs_.push_back(stmt->getCond()->IgnoreParenImpCasts());
+        return true;
+    }
+
+    bool PostTraverseWhileStmt(WhileStmt*, bool)
+    {
+        assert(!contextuallyConvertedExprs_.empty());
+        contextuallyConvertedExprs_.pop_back();
+        return true;
+    }
+
+    bool TraverseWhileStmt(WhileStmt* stmt)
+    {
+        auto res = PreTraverseWhileStmt(stmt);
+        assert(res);
+        res = FilteringRewritePlugin::TraverseWhileStmt(stmt);
+        PostTraverseWhileStmt(stmt, res);
+        return res;
+    }
+
+    bool PreTraverseDoStmt(DoStmt* stmt)
+    {
+        contextuallyConvertedExprs_.push_back(stmt->getCond()->IgnoreParenImpCasts());
+        return true;
+    }
+
+    bool PostTraverseDoStmt(DoStmt*, bool)
+    {
+        assert(!contextuallyConvertedExprs_.empty());
+        contextuallyConvertedExprs_.pop_back();
+        return true;
+    }
+
+    bool TraverseDoStmt(DoStmt* stmt)
+    {
+        auto res = PreTraverseDoStmt(stmt);
+        assert(res);
+        res = FilteringRewritePlugin::TraverseDoStmt(stmt);
+        PostTraverseDoStmt(stmt, res);
+        return res;
+    }
+
+    bool PreTraverseForStmt(ForStmt* stmt)
+    {
+        auto const e = stmt->getCond();
+        if (e != nullptr)
+        {
+            contextuallyConvertedExprs_.push_back(e->IgnoreParenImpCasts());
+        }
+        return true;
+    }
+
+    bool PostTraverseForStmt(ForStmt* stmt, bool)
+    {
+        if (stmt->getCond() != nullptr)
+        {
+            assert(!contextuallyConvertedExprs_.empty());
+            contextuallyConvertedExprs_.pop_back();
+        }
+        return true;
+    }
+
+    bool TraverseForStmt(ForStmt* stmt)
+    {
+        auto res = PreTraverseForStmt(stmt);
+        assert(res);
+        res = FilteringRewritePlugin::TraverseForStmt(stmt);
+        PostTraverseForStmt(stmt, res);
+        return res;
+    }
+
+    bool PreTraverseParenExpr(ParenExpr* expr)
+    {
+        parenExprs_.push(expr);
+        return true;
+    }
+
+    bool PostTraverseParenExpr(ParenExpr*, bool)
+    {
+        assert(!parenExprs_.empty());
+        parenExprs_.pop();
+        return true;
+    }
+
+    bool TraverseParenExpr(ParenExpr* expr)
+    {
+        auto res = PreTraverseParenExpr(expr);
+        assert(res);
+        res = FilteringRewritePlugin::TraverseParenExpr(expr);
+        PostTraverseParenExpr(expr, res);
+        return res;
+    }
+
+private:
+    bool isContextuallyConverted(Expr const* expr) const
+    {
+        return std::find(contextuallyConvertedExprs_.begin(), contextuallyConvertedExprs_.end(),
+                         expr)
+               != contextuallyConvertedExprs_.end();
+    }
+
+    // Get the source range starting at the "."or "->" (plus any preceding non-comment white space):
+    SourceRange getCallSourceRange(CXXMemberCallExpr const* expr) const
+    {
+        if (expr->getImplicitObjectArgument() == nullptr)
+        {
+            //TODO: Arguably, such a call of a `get` member function from within some member
+            // function (so that syntactically no caller is mentioned) should already be handled
+            // differently when reporting it (just "drop the get()" does not make sense), instead of
+            // being fitered here:
+            return {};
+        }
+        // CXXMemberCallExpr::getExprLoc happens to return the location following the "." or "->":
+        auto start = compiler.getSourceManager().getSpellingLoc(expr->getExprLoc());
+        if (!start.isValid())
+        {
+            return {};
+        }
+        for (;;)
+        {
+            start = Lexer::GetBeginningOfToken(start.getLocWithOffset(-1),
+                                               compiler.getSourceManager(), compiler.getLangOpts());
+            auto const s = StringRef(compiler.getSourceManager().getCharacterData(start),
+                                     Lexer::MeasureTokenLength(start, compiler.getSourceManager(),
+                                                               compiler.getLangOpts()));
+            if (s.empty() || s.startswith("\\\n"))
+            {
+                continue;
+            }
+            if (s != "." && s != "->")
+            {
+                return {};
+            }
+            break;
+        }
+        for (;;)
+        {
+            auto start1 = Lexer::GetBeginningOfToken(
+                start.getLocWithOffset(-1), compiler.getSourceManager(), compiler.getLangOpts());
+            auto const s = StringRef(compiler.getSourceManager().getCharacterData(start1),
+                                     Lexer::MeasureTokenLength(start1, compiler.getSourceManager(),
+                                                               compiler.getLangOpts()));
+            if (!(s.empty() || s.startswith("\\\n")))
+            {
+                break;
+            }
+            start = start1;
+        }
+        return SourceRange(start,
+                           compiler.getSourceManager().getSpellingLoc(compat::getEndLoc(expr)));
+    }
+
+    //TODO: There are some more places where an expression is contextually converted to bool, but
+    // those are probably not relevant for our needs here.
+    std::deque<Expr const*> contextuallyConvertedExprs_;
+
+    std::stack<ParenExpr const*> parenExprs_;
 };
 
 bool SimplifyPointerToBool::VisitImplicitCastExpr(ImplicitCastExpr const* castExpr)
@@ -58,7 +336,7 @@ bool SimplifyPointerToBool::VisitImplicitCastExpr(ImplicitCastExpr const* castEx
         return true;
     //    castExpr->dump();
     //    methodDecl->getParent()->getTypeForDecl()->dump();
-    if (!loplugin::isSmartPointerType(methodDecl->getParent()->getTypeForDecl()))
+    if (!loplugin::isSmartPointerType(memberCallExpr->getImplicitObjectArgument()))
         return true;
     //    if (isa<CXXOperatorCallExpr>(callExpr))
     //        return true;
@@ -104,15 +382,57 @@ bool SimplifyPointerToBool::VisitImplicitCastExpr(ImplicitCastExpr const* castEx
     //            if (isa<ConditionalOperator>(arg->IgnoreParenImpCasts()))
     //                continue;
     //        }
-    report(DiagnosticsEngine::Warning, "simplify, drop the get()", castExpr->getExprLoc())
-        << castExpr->getSourceRange();
+    if (isContextuallyConverted(memberCallExpr))
+    {
+        if (rewriter)
+        {
+            auto const range = getCallSourceRange(memberCallExpr);
+            if (range.isValid() && removeText(range))
+            {
+                return true;
+            }
+        }
+        report(DiagnosticsEngine::Warning, "simplify, drop the get()", memberCallExpr->getExprLoc())
+            << memberCallExpr->getSourceRange();
+    }
+    else if (!parenExprs_.empty() && parenExprs_.top()->IgnoreImpCasts() == memberCallExpr)
+    {
+        //TODO: attempt rewrite
+        report(DiagnosticsEngine::Warning,
+               "simplify, drop the get() and turn the surrounding parentheses into a functional "
+               "cast to bool",
+               memberCallExpr->getExprLoc())
+            << memberCallExpr->getSourceRange();
+        report(DiagnosticsEngine::Note, "surrounding parentheses here",
+               parenExprs_.top()->getExprLoc())
+            << parenExprs_.top()->getSourceRange();
+    }
+    else
+    {
+        if (rewriter)
+        {
+            auto const loc
+                = compiler.getSourceManager().getSpellingLoc(compat::getBeginLoc(memberCallExpr));
+            auto const range = getCallSourceRange(memberCallExpr);
+            if (loc.isValid() && range.isValid() && insertText(loc, "bool(")
+                && replaceText(range, ")"))
+            {
+                //TODO: atomically only change both or neither
+                return true;
+            }
+        }
+        report(DiagnosticsEngine::Warning,
+               "simplify, drop the get() and wrap the expression in a functional cast to bool",
+               memberCallExpr->getExprLoc())
+            << memberCallExpr->getSourceRange();
+    }
     //        report(DiagnosticsEngine::Note, "method here", param->getLocation())
     //            << param->getSourceRange();
     return true;
 }
 
-loplugin::Plugin::Registration<SimplifyPointerToBool>
-    simplifypointertobool("simplifypointertobool");
+loplugin::Plugin::Registration<SimplifyPointerToBool> simplifypointertobool("simplifypointertobool",
+                                                                            true);
 
 } // namespace
 
diff --git a/compilerplugins/clang/test/simplifypointertobool.cxx b/compilerplugins/clang/test/simplifypointertobool.cxx
index 901262c3d474..18e1da607bb8 100644
--- a/compilerplugins/clang/test/simplifypointertobool.cxx
+++ b/compilerplugins/clang/test/simplifypointertobool.cxx
@@ -11,11 +11,24 @@
 
 void foo();
 
-void test1(std::unique_ptr<int> p2)
+bool test1(std::unique_ptr<int> p2)
 {
     // expected-error@+1 {{simplify, drop the get() [loplugin:simplifypointertobool]}}
     if (p2.get())
         foo();
+    // expected-error@+1 {{simplify, drop the get() and wrap the expression in a functional cast to bool [loplugin:simplifypointertobool]}}
+    bool b1 = p2.get();
+    //TODO:
+    bool b2 = ( // deliberately spread across multiple lines
+        p2.get());
+    return b1 && b2;
+}
+
+void test2(std::shared_ptr<int> p)
+{
+    // expected-error@+1 {{simplify, drop the get() [loplugin:simplifypointertobool]}}
+    if (p.get())
+        foo();
 }
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */