Suggest automated replacements of C-style casts with C++ casts.

[alexfh]

Summary

This patch implements a subset of possible replacements of C-style
casts with const_cast/static_cast/reinterpret_cast. This should cover a large
portion of cases in real code. Handling of a few more cases may be implemented
eventually.

[djasper]

clang-tidy/google/AvoidCStyleCastsCheck.cpp

38

nit: Grammatically, I'd prefer "needsConstCast"

71

Create a test where the cast is inside a macro. I think you'll want to use the spelling location (or completely abort as the required cast type might actually be different between different macro invocations).

121

Though benign now, don't fall through here.

test/clang-tidy/avoid-c-style-casts.cpp

9

What happens for the following code?

template <typename T> void f(T t) { int i = (int)t; }
f(1);
f(1.0);

(With the first call to f() hopefully leading to a message about a redundant cast and the second call leading to a reinterpret_cast)

[alexfh]

Don't analyze casts in template instantiations. Warn on casts in macros, but don't suggest fixes.

clang-tidy/google/AvoidCStyleCastsCheck.cpp

38

It used to be named like this before a few rounds of refactoring. Renamed.

71

To the moment we completely ignored casts in macros. Maybe we should warn, just refrain from suggesting fixes. I've implemented it this way now, please take a look.

121

We definitely need to turn on -Wimplicit-fallthrough in LLVM/Clang.

test/clang-tidy/avoid-c-style-casts.cpp

9

Good point. Now that I avoid matches in template instantiations, fixes are not suggested for casts to/from dependent types.

[djasper]

Looks good. Nice!

clang-tidy/google/AvoidCStyleCastsCheck.cpp

86

I'd probably prefer "if (!ParenRange ...) return".

clang-tidy/google/AvoidCStyleCastsCheck.cpp

 //===--- AvoidCStyleCastsCheck.cpp - clang-tidy -----------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "AvoidCStyleCastsCheck.h"
+#include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Lex/Lexer.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
 void
 AvoidCStyleCastsCheck::registerMatchers(ast_matchers::MatchFinder *Finder) {
   Finder->addMatcher(
       cStyleCastExpr(
           // Filter out (EnumType)IntegerLiteral construct, which is generated
           // for non-type template arguments of enum types.
           // FIXME: Remove this once this is fixed in the AST.
-          unless(hasParent(substNonTypeTemplateParmExpr()))).bind("cast"),
+          unless(hasParent(substNonTypeTemplateParmExpr())),
+          // Avoid matches in template instantiations.
+          unless(hasAncestor(decl(
+              anyOf(recordDecl(ast_matchers::isTemplateInstantiation()),
+                    functionDecl(ast_matchers::isTemplateInstantiation()))))))
+          .bind("cast"),
       this);
 }
 
+bool needsConstCast(QualType SourceType, QualType DestType) {
+  SourceType = SourceType.getNonReferenceType();
+  DestType = DestType.getNonReferenceType();
+  while (SourceType->isPointerType() && DestType->isPointerType()) {
+    SourceType = SourceType->getPointeeType();
+    DestType = DestType->getPointeeType();
+    if (SourceType.isConstQualified() && !DestType.isConstQualified())
+      return true;
+  }
+  return false;
+}
+
+bool pointedTypesAreEqual(QualType SourceType, QualType DestType) {
+  SourceType = SourceType.getNonReferenceType();
+  DestType = DestType.getNonReferenceType();
+  while (SourceType->isPointerType() && DestType->isPointerType()) {
+    SourceType = SourceType->getPointeeType();
+    DestType = DestType->getPointeeType();
+  }
+  return SourceType.getUnqualifiedType() == DestType.getUnqualifiedType();
+}
 void AvoidCStyleCastsCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *CastExpr = Result.Nodes.getNodeAs<CStyleCastExpr>("cast");
 
-  // Ignore casts in macros for now.
-  if (CastExpr->getLocStart().isMacroID())
-    return;
-
   // Casting to void is an idiomatic way to mute "unused variable" and similar
   // warnings.
   if (CastExpr->getTypeAsWritten()->isVoidType())
     return;
 
-  diag(CastExpr->getLocStart(), "C-style casts are discouraged. Use "
-                                "static_cast/const_cast/reinterpret_cast "
-                                "instead.");
-  // FIXME: Suggest appropriate C++ cast. See [expr.cast] for cast notation
-  // semantics.
+  QualType SourceType =
+      CastExpr->getSubExprAsWritten()->getType().getCanonicalType();
+  QualType DestType = CastExpr->getTypeAsWritten().getCanonicalType();
+
+  auto ParenRange = CharSourceRange::getTokenRange(CastExpr->getLParenLoc(),
+                                                   CastExpr->getRParenLoc());
+  if (SourceType == DestType) {
+    diag(CastExpr->getLocStart(), "Redundant cast to the same type.")
+        << FixItHint::CreateRemoval(ParenRange);
+    return;
+  }
+
+  std::string DestTypeString = CastExpr->getTypeAsWritten().getAsString();
+
+  auto diag_builder =
+      diag(CastExpr->getLocStart(), "C-style casts are discouraged. %0");
+
+  auto ReplaceWithCast = [&](StringRef CastType) {
+    diag_builder << ("Use " + CastType + ".").str();
+    if (ParenRange.getBegin().isFileID() && ParenRange.getEnd().isFileID()) {
+      diag_builder << FixItHint::CreateReplacement(
+                          ParenRange,
+                          (CastType + "<" + DestTypeString + ">(").str())
+                   << FixItHint::CreateInsertion(
+                          Lexer::getLocForEndOfToken(
+                              CastExpr->getSubExprAsWritten()->getLocEnd(), 0,
+                              *Result.SourceManager,
+                              Result.Context->getLangOpts()),
+                          ")");
+    }
+  };
+  // Suggest appropriate C++ cast. See [expr.cast] for cast notation semantics.
+  switch (CastExpr->getCastKind()) {
+  case CK_NoOp:
+    if (needsConstCast(SourceType, DestType) &&
+        pointedTypesAreEqual(SourceType, DestType)) {
+      ReplaceWithCast("const_cast");
+      return;
+    }
+    if (DestType->isReferenceType() &&
+        (SourceType.getNonReferenceType() ==
+             DestType.getNonReferenceType().withConst() ||
+         SourceType.getNonReferenceType() == DestType.getNonReferenceType())) {
+      ReplaceWithCast("const_cast");
+      return;
+    }
+    if (SourceType->isBuiltinType() && DestType->isBuiltinType()) {
+      ReplaceWithCast("static_cast");
+      return;
+    }
+    break;
+  case CK_BitCast:
+    // FIXME: Suggest const_cast<...>(reinterpret_cast<...>(...)) replacement.
+    if (!needsConstCast(SourceType, DestType)) {
+      ReplaceWithCast("reinterpret_cast");
+      return;
+    }
+    break;
+  default:
+    break;
+  }
+
+  diag_builder << "Use static_cast/const_cast/reinterpret_cast.";
 }
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang

test/clang-tidy/avoid-c-style-casts.cpp

-// RUN: clang-tidy -checks=-*,google-readability-casting %s -- | FileCheck %s
-
-// CHECK-NOT: warning:
+// RUN: $(dirname %s)/check_clang_tidy_fix.sh %s google-readability-casting %t
+// REQUIRES: shell
 
 bool g() { return false; }
 
-void f(int a, double b) {
+struct X {};
+struct Y : public X {};
+
+void f(int a, double b, const char *cpc, const void *cpv, X *pX) {
+  const char *cpc2 = (const char*)cpc;
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: Redundant cast to the same type. [google-readability-casting]
+  // CHECK-FIXES: const char *cpc2 = cpc;
+
+  char *pc = (char*)cpc;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: C-style casts are discouraged. Use const_cast. {{.*}}
+  // CHECK-FIXES: char *pc = const_cast<char *>(cpc);
+
+  char *pc2 = (char*)(cpc + 33);
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: C-style casts are discouraged. Use const_cast. {{.*}}
+  // CHECK-FIXES: char *pc2 = const_cast<char *>((cpc + 33));
+
+  const char &crc = *cpc;
+  char &rc = (char&)crc;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: C-style casts are discouraged. Use const_cast. {{.*}}
+  // CHECK-FIXES: char &rc = const_cast<char &>(crc);
+
+  char &rc2 = (char&)*cpc;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: C-style casts are discouraged. Use const_cast. {{.*}}
+  // CHECK-FIXES: char &rc2 = const_cast<char &>(*cpc);
+
+  char ** const* const* ppcpcpc;
+  char ****ppppc = (char****)ppcpcpc;
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: C-style casts are discouraged. Use const_cast. {{.*}}
+  // CHECK-FIXES: char ****ppppc = const_cast<char ****>(ppcpcpc);
+
   int b1 = (int)b;
-  // CHECK: :[[@LINE-1]]:12: warning: C-style casts are discouraged. Use static_cast{{.*}}
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: C-style casts are discouraged. Use static_cast. [google-readability-casting]
+  // CHECK-FIXES: int b1 = static_cast<int>(b);
+
+  Y *pB = (Y*)pX;
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: C-style casts are discouraged. Use static_cast/const_cast/reinterpret_cast. [google-readability-casting]
+  Y &rB = (Y&)*pX;
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: C-style casts are discouraged. Use static_cast/const_cast/reinterpret_cast. [google-readability-casting]
+
+  const char *pc3 = (const char*)cpv;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: C-style casts are discouraged. Use reinterpret_cast. [google-readability-casting]
+  // CHECK-FIXES: const char *pc3 = reinterpret_cast<const char *>(cpv);
 
-  // CHECK-NOT: warning:
+  char *pc4 = (char*)cpv;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: C-style casts are discouraged. Use static_cast/const_cast/reinterpret_cast. [google-readability-casting]
+  // CHECK-FIXES: char *pc4 = (char*)cpv;
+
+  // CHECK-MESSAGES-NOT: warning:
   int b2 = int(b);
   int b3 = static_cast<double>(b);
   int b4 = b;
   double aa = a;
   (void)b2;
   return (void)g();
 }
 
-// CHECK-NOT: warning:
+template <typename T>
+void template_function(T t, int n) {
+  int i = (int)t;
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: C-style casts are discouraged. Use static_cast/const_cast/reinterpret_cast.
+  // CHECK-FIXES: int i = (int)t;
+  int j = (int)n;
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: Redundant cast to the same type.
+  // CHECK-FIXES: int j = n;
+}
+
+template <typename T>
+struct TemplateStruct {
+  void f(T t, int n) {
+    int k = (int)t;
+    // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: C-style casts are discouraged. Use static_cast/const_cast/reinterpret_cast.
+    // CHECK-FIXES: int k = (int)t;
+    int l = (int)n;
+    // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: Redundant cast to the same type.
+    // CHECK-FIXES: int l = n;
+  }
+};
+
+void test_templates() {
+  template_function(1, 42);
+  template_function(1.0, 42);
+  TemplateStruct<int>().f(1, 42);
+  TemplateStruct<double>().f(1.0, 42);
+}
+
+#define CAST(type, value) (type)(value)
+void macros(double d) {
+  int i = CAST(int, d);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: C-style casts are discouraged. Use static_cast.
+  // CHECK-FIXES: #define CAST(type, value) (type)(value)
+  // CHECK-FIXES: int i = CAST(int, d);
+}
+
 enum E { E1 = 1 };
 template <E e>
-struct A { static const E ee = e; };
+struct A {
+  // Usage of template argument e = E1 is represented as (E)1 in the AST for
+  // some reason. We have a special treatment of this case to avoid warnings
+  // here.
+  static const E ee = e;
+};
 struct B : public A<E1> {};