Fixed/improved loplugin:cppunitassertequals

* 994e38e336beeacbd983faafac480afc94d3947e "loplugin: use TypeCheck instead of
getQualifiedNameAsString" had effectively disabled this plugin (Asserter is a
struct, not a namespace).  Fixed that.

* Also improved the checks, for one removing the---expensive---use of
Plugin::parentStmt, for another making the plugin look into (...), !..., and
...&&... expressions.

* However, as the plugin had effectively already been disabled (see above) when
it was switched on generally with 839e53b933322b739a7f534af58c63a2c69af7bd
"compilerplugins: enable loplugin:cppunitassertequals by default", it now hit
way more places than I had initially anticipated.  To keep the amount of work
manageable, midway-through I disabled looking into ...&&... expressions for
now.  That will be enabled (and resulting warnings fixed) in follow-up
commits.

* Checks like

CPPUNIT_ASSERT(a == b)

that actually want to check a specific overloaded operator == implementation,
rather than using such an operator == to actually check that a and b are
equal, can be rewritten as

CPPUNIT_ASSERT(operator ==(a, b))

to avoid false warnings from this plugin.

Change-Id: If3501020e2d150ad0f2454a65a39081e31470c0f

3 files changed, 177 insertions, 44 deletions

diff --git a/compilerplugins/clang/cppunitassertequals.cxx b/compilerplugins/clang/cppunitassertequals.cxx
index 858ad9615cbe..37dff6acca97 100644
--- a/compilerplugins/clang/cppunitassertequals.cxx
+++ b/compilerplugins/clang/cppunitassertequals.cxx
@@ -7,11 +7,6 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/.
  */
 
-#include <cassert>
-#include <string>
-#include <iostream>
-#include <fstream>
-#include <set>
 #include "plugin.hxx"
 #include "check.hxx"
 
@@ -29,74 +24,133 @@ public:
 
     virtual void run() override
     {
-        TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
+        if (compiler.getLangOpts().CPlusPlus) {
+            TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
+        }
     }
 
     bool VisitCallExpr(const CallExpr*);
-    bool VisitBinaryOperator(const BinaryOperator*);
+
 private:
-    void checkExpr(const Stmt* stmt);
+    void checkExpr(
+        SourceRange range, StringRef name, Expr const * expr, bool negated);
+
+    void reportEquals(SourceRange range, StringRef name, bool negative);
 };
 
 bool CppunitAssertEquals::VisitCallExpr(const CallExpr* callExpr)
 {
-    if (ignoreLocation(callExpr)) {
+    auto const decl = callExpr->getDirectCallee();
+    if (decl == nullptr
+        || !(loplugin::DeclCheck(decl).Function("failIf").Struct("Asserter")
+             .Namespace("CppUnit").GlobalNamespace()))
+    {
         return true;
     }
-    if (callExpr->getDirectCallee() == nullptr) {
+    // Don't use callExpr->getLocStart() or callExpr->getExprLoc(), as those
+    // fall into a nested use of the CPPUNIT_NS macro; CallExpr::getRParenLoc
+    // happens to be readily available and cause good results:
+    auto loc = callExpr->getRParenLoc();
+    while (compiler.getSourceManager().isMacroArgExpansion(loc)) {
+        loc = compiler.getSourceManager().getImmediateMacroCallerLoc(loc);
+    }
+    if (!compiler.getSourceManager().isMacroBodyExpansion(loc)
+        || ignoreLocation(
+            compiler.getSourceManager().getImmediateMacroCallerLoc(loc)))
+    {
         return true;
     }
-    const FunctionDecl* functionDecl = callExpr->getDirectCallee()->getCanonicalDecl();
-    if (functionDecl->getOverloadedOperator() != OO_EqualEqual) {
+    auto name = Lexer::getImmediateMacroName(
+        loc, compiler.getSourceManager(), compiler.getLangOpts());
+    if (name != "CPPUNIT_ASSERT" && name != "CPPUNIT_ASSERT_MESSAGE") {
         return true;
     }
-    checkExpr(callExpr);
-    return true;
-}
-
-bool CppunitAssertEquals::VisitBinaryOperator(const BinaryOperator* binaryOp)
-{
-    if (ignoreLocation(binaryOp)) {
+    if (decl->getNumParams() != 3) {
+        report(
+            DiagnosticsEngine::Warning,
+            ("TODO: suspicious CppUnit::Asserter::failIf call with %0"
+             " parameters"),
+            callExpr->getExprLoc())
+            << decl->getNumParams() << callExpr->getSourceRange();
         return true;
     }
-    if (binaryOp->getOpcode() != BO_EQ) {
+    auto const e1 = callExpr->getArg(0)->IgnoreParenImpCasts();
+    Expr const * e2 = nullptr;
+    if (auto const e3 = dyn_cast<UnaryOperator>(e1)) {
+        if (e3->getOpcode() == UO_LNot) {
+            e2 = e3->getSubExpr();
+        }
+    } else if (auto const e4 = dyn_cast<CXXOperatorCallExpr>(e1)) {
+        if (e4->getOperator() == OO_Exclaim) {
+            e2 = e4->getArg(0);
+        }
+    }
+    if (e2 == nullptr) {
+        report(
+            DiagnosticsEngine::Warning,
+            ("TODO: suspicious CppUnit::Asserter::failIf call not wrapping"
+             " !(...)"),
+            callExpr->getExprLoc())
+            << callExpr->getSourceRange();
         return true;
     }
-    checkExpr(binaryOp);
+    auto range = compiler.getSourceManager().getImmediateExpansionRange(loc);
+    checkExpr(
+        SourceRange(range.first, range.second), name,
+        e2->IgnoreParenImpCasts(), false);
     return true;
 }
 
-/*
- * check that we are not a compound expression
- */
-void CppunitAssertEquals::checkExpr(const Stmt* stmt)
+void CppunitAssertEquals::checkExpr(
+    SourceRange range, StringRef name, Expr const * expr, bool negated)
 {
-    const Stmt* parent = parentStmt(stmt);
-    if (!parent || !isa<ParenExpr>(parent)) {
-        return;
-    }
-    parent = parentStmt(parent);
-    if (!parent || !isa<UnaryOperator>(parent)) {
-        return;
-    }
-    parent = parentStmt(parent);
-    if (!parent || !isa<CallExpr>(parent)) {
+    if (auto const e = dyn_cast<UnaryOperator>(expr)) {
+        if (e->getOpcode() == UO_LNot) {
+            checkExpr(
+                range, name, e->getSubExpr()->IgnoreParenImpCasts(), !negated);
+        }
         return;
     }
-    const CallExpr* callExpr = dyn_cast<CallExpr>(parent);
-    if (!callExpr || callExpr->getDirectCallee() == nullptr) {
+    if (auto const e = dyn_cast<BinaryOperator>(expr)) {
+        auto const op = e->getOpcode();
+        if ((!negated && op == BO_EQ) || (negated && op == BO_NE)) {
+            reportEquals(range, name, op == BO_NE);
+            return;
+        }
+#if 0 // TODO: enable later
+        if ((!negated && op == BO_LAnd) || (negated && op == BO_LOr)) {
+            report(
+                DiagnosticsEngine::Warning,
+                "rather split into two %0", e->getExprLoc())
+                << name << range;
+            return;
+        }
+#endif
         return;
     }
-    const FunctionDecl* functionDecl = callExpr->getDirectCallee()->getCanonicalDecl();
-    if (!functionDecl ||
-        !loplugin::DeclCheck(functionDecl).Function("failIf").Namespace("Asserter").Namespace("CppUnit").GlobalNamespace())
-    {
+    if (auto const e = dyn_cast<CXXOperatorCallExpr>(expr)) {
+        auto const op = e->getOperator();
+        if ((!negated && op == OO_EqualEqual)
+            || (negated && op == OO_ExclaimEqual))
+        {
+            reportEquals(range, name, op == OO_ExclaimEqual);
+            return;
+        }
         return;
     }
+}
+
+void CppunitAssertEquals::reportEquals(
+    SourceRange range, StringRef name, bool negative)
+{
     report(
-        DiagnosticsEngine::Warning, "rather call CPPUNIT_ASSERT_EQUALS",
-        stmt->getSourceRange().getBegin())
-      << stmt->getSourceRange();
+        DiagnosticsEngine::Warning,
+        ("rather call"
+         " %select{CPPUNIT_ASSERT_EQUAL|CPPUNIT_ASSERT_EQUAL_MESSAGE}0 (or"
+         " rewrite as an explicit operator %select{==|!=}1 call when the"
+         " operator itself is the topic)"),
+        range.getBegin())
+        << (name == "CPPUNIT_ASSERT_MESSAGE") << negative << range;
 }
 
 loplugin::Plugin::Registration< CppunitAssertEquals > X("cppunitassertequals");
diff --git a/compilerplugins/clang/test/cppunitassertequals.cxx b/compilerplugins/clang/test/cppunitassertequals.cxx
new file mode 100644
index 000000000000..239de58853b3
--- /dev/null
+++ b/compilerplugins/clang/test/cppunitassertequals.cxx
@@ -0,0 +1,56 @@
+/* -*- 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 "sal/config.h"
+
+#include "cppunit/TestAssert.h"
+
+#include "cppunitassertequals.hxx"
+
+#define TEST1 CPPUNIT_ASSERT(b1 == b2)
+#define TEST2(x) x
+
+void test(bool b1, bool b2, OUString const & s1, OUString const & s2, T t) {
+    CppUnit::Asserter::failIf(b1,"");
+#if 0 // TODO: enable later
+    CPPUNIT_ASSERT(b1 && b2); // expected-error {{rather split into two CPPUNIT_ASSERT [loplugin:cppunitassertequals]}}
+    CPPUNIT_ASSERT((b1 && b2)); // expected-error {{rather split into two CPPUNIT_ASSERT [loplugin:cppunitassertequals]}}
+    CPPUNIT_ASSERT(!(b1 || b2)); // expected-error {{rather split into two CPPUNIT_ASSERT [loplugin:cppunitassertequals]}}
+    CPPUNIT_ASSERT(!(b1 && b2));
+    CPPUNIT_ASSERT(!!(b1 && b2)); // expected-error {{rather split into two CPPUNIT_ASSERT [loplugin:cppunitassertequals]}}
+    CPPUNIT_ASSERT_MESSAGE("", b1 && b2); // expected-error {{rather split into two CPPUNIT_ASSERT_MESSAGE [loplugin:cppunitassertequals]}}
+#endif
+    CPPUNIT_ASSERT(b1 == b2); // expected-error {{rather call CPPUNIT_ASSERT_EQUAL (or rewrite as an explicit operator == call when the operator itself is the topic) [loplugin:cppunitassertequals]}}
+    CPPUNIT_ASSERT(b1 != b2);
+    CPPUNIT_ASSERT((b1 == b2)); // expected-error {{rather call CPPUNIT_ASSERT_EQUAL (or rewrite as an explicit operator == call when the operator itself is the topic) [loplugin:cppunitassertequals]}}
+    CPPUNIT_ASSERT(!(b1 != b2)); // expected-error {{rather call CPPUNIT_ASSERT_EQUAL (or rewrite as an explicit operator != call when the operator itself is the topic) [loplugin:cppunitassertequals]}}
+    CPPUNIT_ASSERT(!(b1 == b2));
+    CPPUNIT_ASSERT(!!(b1 == b2)); // expected-error {{rather call CPPUNIT_ASSERT_EQUAL (or rewrite as an explicit operator == call when the operator itself is the topic) [loplugin:cppunitassertequals]}}
+    CPPUNIT_ASSERT_MESSAGE("", b1 == b2); // expected-error {{rather call CPPUNIT_ASSERT_EQUAL_MESSAGE (or rewrite as an explicit operator == call when the operator itself is the topic) [loplugin:cppunitassertequals]}}
+    CPPUNIT_ASSERT(s1 == s2); // expected-error {{rather call CPPUNIT_ASSERT_EQUAL (or rewrite as an explicit operator == call when the operator itself is the topic) [loplugin:cppunitassertequals]}}
+    CPPUNIT_ASSERT(s1 != s2);
+    CPPUNIT_ASSERT((s1 == s2)); // expected-error {{rather call CPPUNIT_ASSERT_EQUAL (or rewrite as an explicit operator == call when the operator itself is the topic) [loplugin:cppunitassertequals]}}
+    CPPUNIT_ASSERT(!(s1 != s2)); // expected-error {{rather call CPPUNIT_ASSERT_EQUAL (or rewrite as an explicit operator != call when the operator itself is the topic) [loplugin:cppunitassertequals]}}
+    CPPUNIT_ASSERT(!(s1 == s2));
+    CPPUNIT_ASSERT(!!(s1 == s2)); // expected-error {{rather call CPPUNIT_ASSERT_EQUAL (or rewrite as an explicit operator == call when the operator itself is the topic) [loplugin:cppunitassertequals]}}
+    TEST1; // expected-error {{rather call CPPUNIT_ASSERT_EQUAL (or rewrite as an explicit operator == call when the operator itself is the topic) [loplugin:cppunitassertequals]}}
+    TEST2(CPPUNIT_ASSERT(b1 == b2)); // expected-error {{rather call CPPUNIT_ASSERT_EQUAL (or rewrite as an explicit operator == call when the operator itself is the topic) [loplugin:cppunitassertequals]}}
+    TEST2(TEST1); // expected-error {{rather call CPPUNIT_ASSERT_EQUAL (or rewrite as an explicit operator == call when the operator itself is the topic) [loplugin:cppunitassertequals]}}
+
+    // Useful when testing an equality iterator itself:
+    CPPUNIT_ASSERT(operator ==(s1, s1));
+    CPPUNIT_ASSERT(t.operator ==(t));
+
+    // There might even be good reasons(?) not to warn inside explicit casts:
+    CPPUNIT_ASSERT(bool(b1 && b2));
+    CPPUNIT_ASSERT(bool(b1 == b2));
+    CPPUNIT_ASSERT(bool(s1 == s2));
+}
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
diff --git a/compilerplugins/clang/test/cppunitassertequals.hxx b/compilerplugins/clang/test/cppunitassertequals.hxx
new file mode 100644
index 000000000000..b844c8387e67
--- /dev/null
+++ b/compilerplugins/clang/test/cppunitassertequals.hxx
@@ -0,0 +1,23 @@
+/* -*- 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/.
+ */
+
+#ifndef INCLUDED_COMPILERPLUGINS_CLANG_TEST_CPPUNITASSERTEQUALS_HXX
+#define INCLUDED_COMPILERPLUGINS_CLANG_TEST_CPPUNITASSERTEQUALS_HXX
+
+#include "sal/config.h"
+
+#include "rtl/ustring.hxx"
+
+struct T { bool operator ==(T); };
+
+void test(bool b1, bool b2, OUString const & s1, OUString const & s2, T t);
+
+#endif
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */