unneeded-cast: Revamp this check a bit

Now warns of unneeded qobject-casts and static_casts too.
Like casting to itself, or casting to base class.

5 files changed, 75 insertions, 12 deletions

diff --git a/src/Checks.h b/src/Checks.h
index 92aa5781..e0a633e8 100644
--- a/src/Checks.h
+++ b/src/Checks.h
@@ -165,7 +165,7 @@ void CheckManager::registerChecks()
     registerCheck(check<OverriddenSignal>("overridden-signal", CheckLevel1,  RegisteredCheck::Option_VisitsDecls));
     registerCheck(check<QHashNamespace>("qhash-namespace", CheckLevel1,  RegisteredCheck::Option_VisitsDecls));
     registerCheck(check<SkippedBaseMethod>("skipped-base-method", CheckLevel1,  RegisteredCheck::Option_VisitsStmts));
-    registerCheck(check<UnneededCast>("unneeded-cast", CheckLevel1,  RegisteredCheck::Option_VisitsStmts));
+    registerCheck(check<UnneededCast>("unneeded-cast", ManualCheckLevel,  RegisteredCheck::Option_VisitsStmts));
     registerCheck(check<CtorMissingParentArgument>("ctor-missing-parent-argument", CheckLevel2,  RegisteredCheck::Option_VisitsDecls));
     registerCheck(check<BaseClassEvent>("base-class-event", CheckLevel2,  RegisteredCheck::Option_VisitsDecls));
     registerCheck(check<CopyablePolymorphic>("copyable-polymorphic", CheckLevel2,  RegisteredCheck::Option_VisitsDecls));
diff --git a/src/Utils.cpp b/src/Utils.cpp
index 10d89e99..af6507b8 100644
--- a/src/Utils.cpp
+++ b/src/Utils.cpp
@@ -57,6 +57,13 @@ CXXRecordDecl * Utils::namedCastInnerDecl(CXXNamedCastExpr *staticOrDynamicCast)
 {
     Expr *e = staticOrDynamicCast->getSubExpr();
     if (!e) return nullptr;
+    if (auto implicitCast = dyn_cast<ImplicitCastExpr>(e)) {
+        // Sometimes it's automatically cast to base
+        if (implicitCast->getCastKind() == CK_DerivedToBase) {
+            e = implicitCast->getSubExpr();
+        }
+    }
+
     QualType qt = e->getType();
     const Type *t = qt.getTypePtrOrNull();
     if (!t) return nullptr;
diff --git a/src/checks/level1/README-unneeded-cast.md b/src/checks/level1/README-unneeded-cast.md
index a649e326..97af458f 100644
--- a/src/checks/level1/README-unneeded-cast.md
+++ b/src/checks/level1/README-unneeded-cast.md
@@ -1,13 +1,17 @@
-# dynamic-cast
+# unneeded-cast
+
+Finds unneeded qobject_cast, static_cast and dynamic_casts.
+Warns when you're casting to base or casting to the same type, which doesn't require
+any explicit cast.
+
+Also warns when you're using dynamic_cast for QObjects. qobject_cast is prefered.
 
-Finds places where a dynamic cast is redundant or when dynamic casting to base class, which is unneeded.
-Optionally it can also find places where a qobject_cast should be used instead.
 
 #### Example
 
     Foo *a = ...;
-    Foo *b = dynamic_cast<Foo*>(a);
+    Foo *b = qobject_cast<Foo*>(a);
 
 
-If you prefer to use qobject_cast instead of dynamic_cast you can catch those cases with:
-`export CLAZY_EXTRA_OPTIONS="unneeded-cast-qobject`
+To shut the warnings about using qobject_cast over dynamic cast you can set:
+`export CLAZY_EXTRA_OPTIONS="unneeded-cast-prefer-dynamic-cast-over-qobject"`
diff --git a/src/checks/level1/unneeded-cast.cpp b/src/checks/level1/unneeded-cast.cpp
index 4e8f790f..29f39620 100644
--- a/src/checks/level1/unneeded-cast.cpp
+++ b/src/checks/level1/unneeded-cast.cpp
@@ -26,10 +26,13 @@
 #include "Utils.h"
 #include "QtUtils.h"
 #include "TypeUtils.h"
+#include "HierarchyUtils.h"
+#include "ClazyContext.h"
 
 #include <clang/AST/DeclCXX.h>
 #include <clang/AST/ExprCXX.h>
 
+using namespace llvm;
 using namespace clang;
 
 UnneededCast::UnneededCast(const std::string &name, ClazyContext *context)
@@ -41,15 +44,41 @@ void UnneededCast::VisitStmt(clang::Stmt *stm)
 {
     if (handleNamedCast(dyn_cast<CXXNamedCastExpr>(stm)))
         return;
+
+    handleQObjectCast(stm);
 }
 
 bool UnneededCast::handleNamedCast(CXXNamedCastExpr *namedCast)
 {
-    CXXRecordDecl *castFrom = namedCast ? Utils::namedCastInnerDecl(namedCast) : nullptr;
-    if (!castFrom)
+    if (!namedCast)
         return false;
 
     const bool isDynamicCast = isa<CXXDynamicCastExpr>(namedCast);
+    const bool isStaticCast = isDynamicCast ? false : isa<CXXStaticCastExpr>(namedCast);
+
+    if (!isDynamicCast && !isStaticCast)
+        return false;
+
+    if (namedCast->getLocStart().isMacroID())
+        return false;
+
+    CXXRecordDecl *castFrom = namedCast ? Utils::namedCastInnerDecl(namedCast) : nullptr;
+    if (!castFrom || !castFrom->hasDefinition() || std::distance(castFrom->bases_begin(), castFrom->bases_end()) > 1)
+        return false;
+
+    if (isStaticCast) {
+        if (auto implicitCast = dyn_cast<ImplicitCastExpr>(namedCast->getSubExpr())) {
+            if (implicitCast->getCastKind() == CK_NullToPointer) {
+                // static_cast<Foo*>(0) is OK, and sometimes needed
+                return false;
+            }
+        }
+
+        // static_cast to base is needed in ternary operators
+        if (clazy::getFirstParentOfType<ConditionalOperator>(m_context->parentMap, namedCast) != nullptr)
+            return false;
+    }
+
     if (isDynamicCast && !isOptionSet("prefer-dynamic-cast-over-qobject") && clazy::isQObject(castFrom))
         emitWarning(namedCast->getLocStart(), "Use qobject_cast rather than dynamic_cast");
 
@@ -57,11 +86,32 @@ bool UnneededCast::handleNamedCast(CXXNamedCastExpr *namedCast)
     if (!castTo)
         return false;
 
+    return maybeWarn(namedCast, castFrom, castTo);
+}
+
+bool UnneededCast::handleQObjectCast(Stmt *stm)
+{
+    CXXRecordDecl *castTo = nullptr;
+    CXXRecordDecl *castFrom = nullptr;
+
+    if (!clazy::is_qobject_cast(stm, &castTo, &castFrom))
+        return false;
+
+    return maybeWarn(stm, castFrom, castTo);
+}
+
+bool UnneededCast::maybeWarn(Stmt *stmt, CXXRecordDecl *castFrom, CXXRecordDecl *castTo)
+{
+    castFrom = castFrom->getCanonicalDecl();
+    castTo = castTo->getCanonicalDecl();
+
     if (castFrom == castTo) {
-        emitWarning(namedCast->getLocStart(), "Casting to itself");
+        emitWarning(stmt->getLocStart(), "Casting to itself");
+        return true;
     } else if (TypeUtils::derivesFrom(/*child=*/castFrom, castTo)) {
-        emitWarning(namedCast->getLocStart(), "explicitly casting to base is unnecessary");
+        emitWarning(stmt->getLocStart(), "explicitly casting to base is unnecessary");
+        return true;
     }
 
-    return true;
+    return false;
 }
diff --git a/src/checks/level1/unneeded-cast.h b/src/checks/level1/unneeded-cast.h
index 4701e381..ffb22dd3 100644
--- a/src/checks/level1/unneeded-cast.h
+++ b/src/checks/level1/unneeded-cast.h
@@ -42,6 +42,8 @@ public:
     void VisitStmt(clang::Stmt *stm) override;
 private:
     bool handleNamedCast(clang::CXXNamedCastExpr *);
+    bool handleQObjectCast(clang::Stmt *);
+    bool maybeWarn(clang::Stmt *, clang::CXXRecordDecl *from, clang::CXXRecordDecl *to);
 };
 
 #endif