[clang-tidy] add new check cppcoreguidelines-pro-type-cstyle-cast

[mgehre]

Summary

This check flags all use of c-style casts that perform a static_cast
downcast, const_cast, or reinterpret_cast.

Use of these casts can violate type safety and cause the program to
access a
variable that is actually of type X to be accessed as if it were of an
unrelated type Z. Note that a C-style (T)expression cast means to
perform
the first of the following that is possible: a const_cast, a
static_cast, a
static_cast followed by a const_cast, a reinterpret_cast, or a
reinterpret_cast followed by a const_cast. This rule bans (T)expression
only when used to perform an unsafe cast.

This rule is part of the "Type safety" profile of the C++ Core
Guidelines, see
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#-type4-dont-use-c-style-texpression-casts-that-would-perform-a-static_cast-downcast-const_cast-or-reinterpret_cast.

[sbenza]

clang-tidy/cppcoreguidelines/ProTypeCstyleCastCheck.cpp

45

Seems to be missing CK_ReinterpretMemberPointer

49

From reading warning this I would think the way to fix the code is to use a reinterpret_cast directly, but that is also against the guidelines.

56

remove blank line

[xazax.hun]

There is already a similar check in the Google package. What are the differences between those two checks? What is the reason we can not just register that check into the core guidelines module?

[sbenza]

In D14096#275902, @xazax.hun wrote:

There is already a similar check in the Google package. What are the differences between those two checks? What is the reason we can not just register that check into the core guidelines module?

That other check discourages c-style cast in favor of C++ style casts, even if it is a reinterpret_cast. It simply replaces the cstyle cast with an equivalent C++ one. It is basically a stylistic check.

This check will warn unsafe cstyle casts, while allowing safe ones like int->uint casts.
This one is a safety related check.

[mgehre]

Review comments: Formating and changed diag messages

[aaron.ballman]

clang-tidy/cppcoreguidelines/ProTypeCstyleCastCheck.cpp

50

"C-style" instead of "c-style"?

58

Missing period to terminate the comment.

75

"C-style"

80

How does this handle a case like:

SomeClass::type *derived = (SomeClass::type *)someBase;

Will it leave the spelling as "SomeClass::type *", or replace it with the underlying type from the typedef? (I suspect it leaves it as-is, but it would be good to have tests for more complex code patterns.)

81

Why do we need this check? Perhaps we want to use IgnoreParenImpCasts() instead?

95

C-style

102

C-style

docs/clang-tidy/checks/cppcoreguidelines-pro-type-cstyle-cast.rst

5

C-style

[mgehre]

clang-tidy/cppcoreguidelines/ProTypeCstyleCastCheck.cpp

80

I copied the fixit part from the google c-style cast check. I will add some more test for it.

[mgehre]

clang-tidy/cppcoreguidelines/ProTypeCstyleCastCheck.cpp

81

If the cast looks like

Derived* D = (Derived*)B;

we need to add parenthesis around B to turn it into

Derived* D = dynamic_cast<Derived*>(B);

If instead we already had

Derived* D = (Derived*)(B);

adding the parenthesis can be skipped.

[mgehre]

Update for review comments

[alexfh]

A peanut gallery comment.

clang-tidy/cppcoreguidelines/ProTypeCstyleCastCheck.cpp

91

Please add braces around the else body.

[mgehre]

Update for review comments: add braces around else

[alexfh]

Looks good with a comment. Thank you for contribution!

test/clang-tidy/cppcoreguidelines-pro-type-cstyle-cast.cpp

15

I think, it makes sense to leave the [cppcoreguidelines-pro-type-cstyle-cast] only once and remove it everywhere else to make the test easier to read.

clang-tidy/cppcoreguidelines/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyCppCoreGuidelinesModule
   CppCoreGuidelinesTidyModule.cpp
   ProBoundsArrayToPointerDecayCheck.cpp
   ProBoundsPointerArithmeticCheck.cpp
   ProTypeConstCastCheck.cpp
+  ProTypeCstyleCastCheck.cpp
   ProTypeReinterpretCastCheck.cpp
   ProTypeStaticCastDowncastCheck.cpp
   ProTypeUnionAccessCheck.cpp
   ProTypeVarargCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyMiscModule
   clangTidyUtils
   clangTooling
   )

clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

 //===--- CppCoreGuidelinesModule.cpp - clang-tidy -------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../misc/AssignOperatorSignatureCheck.h"
 #include "ProBoundsArrayToPointerDecayCheck.h"
 #include "ProBoundsPointerArithmeticCheck.h"
 #include "ProTypeConstCastCheck.h"
+#include "ProTypeCstyleCastCheck.h"
 #include "ProTypeReinterpretCastCheck.h"
 #include "ProTypeStaticCastDowncastCheck.h"
 #include "ProTypeUnionAccessCheck.h"
 #include "ProTypeVarargCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace cppcoreguidelines {
 
 /// A module containing checks of the C++ Core Guidelines
 class CppCoreGuidelinesModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ProBoundsArrayToPointerDecayCheck>(
         "cppcoreguidelines-pro-bounds-array-to-pointer-decay");
     CheckFactories.registerCheck<ProBoundsPointerArithmeticCheck>(
         "cppcoreguidelines-pro-bounds-pointer-arithmetic");
     CheckFactories.registerCheck<ProTypeConstCastCheck>(
         "cppcoreguidelines-pro-type-const-cast");
+    CheckFactories.registerCheck<ProTypeCstyleCastCheck>(
+        "cppcoreguidelines-pro-type-cstyle-cast");
     CheckFactories.registerCheck<ProTypeReinterpretCastCheck>(
         "cppcoreguidelines-pro-type-reinterpret-cast");
     CheckFactories.registerCheck<ProTypeStaticCastDowncastCheck>(
         "cppcoreguidelines-pro-type-static-cast-downcast");
     CheckFactories.registerCheck<ProTypeUnionAccessCheck>(
         "cppcoreguidelines-pro-type-union-access");
     CheckFactories.registerCheck<ProTypeVarargCheck>(
         "cppcoreguidelines-pro-type-vararg");

clang-tidy/cppcoreguidelines/ProTypeCstyleCastCheck.h

+//===--- ProTypeCstyleCastCheck.h - clang-tidy-------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_PRO_TYPE_CSTYLE_CAST_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_PRO_TYPE_CSTYLE_CAST_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// This check flags all use of C-style casts that perform a static_cast
+/// downcast, const_cast, or reinterpret_cast.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-pro-type-cstyle-cast.html
+class ProTypeCstyleCastCheck : public ClangTidyCheck {
+public:
+  ProTypeCstyleCastCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_PRO_TYPE_CSTYLE_CAST_H

clang-tidy/cppcoreguidelines/ProTypeCstyleCastCheck.cpp

+//===--- ProTypeCstyleCastCheck.cpp - clang-tidy---------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "ProTypeCstyleCastCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+
+static 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;
+}
+
+void ProTypeCstyleCastCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  Finder->addMatcher(
+      cStyleCastExpr(unless(isInTemplateInstantiation())).bind("cast"), this);
+}
+
+void ProTypeCstyleCastCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedCast = Result.Nodes.getNodeAs<CStyleCastExpr>("cast");
+
+  if (MatchedCast->getCastKind() == CK_BitCast ||
+      MatchedCast->getCastKind() == CK_LValueBitCast ||
+      MatchedCast->getCastKind() == CK_IntegralToPointer ||
+      MatchedCast->getCastKind() == CK_PointerToIntegral ||
+      MatchedCast->getCastKind() == CK_ReinterpretMemberPointer) {
+    diag(MatchedCast->getLocStart(),
+         "do not use C-style cast to convert between unrelated types");
+    return;
+  }
+
+  QualType SourceType = MatchedCast->getSubExpr()->getType();
+
+  if (MatchedCast->getCastKind() == CK_BaseToDerived) {
+    const auto *SourceDecl = SourceType->getPointeeCXXRecordDecl();
+    if (!SourceDecl) // The cast is from object to reference.
+      SourceDecl = SourceType->getAsCXXRecordDecl();
+    if (!SourceDecl)
+      return;
+
+    if (SourceDecl->isPolymorphic()) {
+      // Leave type spelling exactly as it was (unlike
+      // getTypeAsWritten().getAsString() which would spell enum types 'enum
+      // X').
+      StringRef DestTypeString = Lexer::getSourceText(
+          CharSourceRange::getTokenRange(
+              MatchedCast->getLParenLoc().getLocWithOffset(1),
+              MatchedCast->getRParenLoc().getLocWithOffset(-1)),
+          *Result.SourceManager, Result.Context->getLangOpts());
+
+      auto diag_builder = diag(
+          MatchedCast->getLocStart(),
+          "do not use C-style cast to downcast from a base to a derived class; "
+          "use dynamic_cast instead");
+
+      const Expr *SubExpr =
+          MatchedCast->getSubExprAsWritten()->IgnoreImpCasts();
+      std::string CastText = ("dynamic_cast<" + DestTypeString + ">").str();
+      if (!isa<ParenExpr>(SubExpr)) {
+        CastText.push_back('(');
+        diag_builder << FixItHint::CreateInsertion(
+            Lexer::getLocForEndOfToken(SubExpr->getLocEnd(), 0,
+                                       *Result.SourceManager,
+                                       Result.Context->getLangOpts()),
+            ")");
+      }
+      auto ParenRange = CharSourceRange::getTokenRange(
+          MatchedCast->getLParenLoc(), MatchedCast->getRParenLoc());
+      diag_builder << FixItHint::CreateReplacement(ParenRange, CastText);
+    } else
+      diag(
+          MatchedCast->getLocStart(),
+          "do not use C-style cast to downcast from a base to a derived class");
+    return;
+  }
+
+  if (MatchedCast->getCastKind() == CK_NoOp &&
+      needsConstCast(SourceType, MatchedCast->getType())) {
+    diag(MatchedCast->getLocStart(),
+         "do not use C-style cast to cast away constness");
+  }
+}
+
+} // namespace tidy
+} // namespace clang

docs/clang-tidy/checks/cppcoreguidelines-pro-type-cstyle-cast.rst

+cppcoreguidelines-pro-type-cstyle-cast
+======================================
+
+This check flags all use of C-style casts that perform a static_cast downcast, const_cast, or reinterpret_cast.
+
+Use of these casts can violate type safety and cause the program to access a
+variable that is actually of type X to be accessed as if it were of an
+unrelated type Z. Note that a C-style (T)expression cast means to perform
+the first of the following that is possible: a const_cast, a static_cast, a
+static_cast followed by a const_cast, a reinterpret_cast, or a
+reinterpret_cast followed by a const_cast. This rule bans (T)expression
+only when used to perform an unsafe cast.
+
+This rule is part of the "Type safety" profile of the C++ Core Guidelines, see
+https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#-type4-dont-use-c-style-texpression-casts-that-would-perform-a-static_cast-downcast-const_cast-or-reinterpret_cast.

docs/clang-tidy/checks/list.rst

 List of clang-tidy Checks
 =========================
 
 .. toctree::
    cert-setlongjmp
    cert-variadic-function-def
    cppcoreguidelines-pro-bounds-array-to-pointer-decay
    cppcoreguidelines-pro-bounds-pointer-arithmetic
    cppcoreguidelines-pro-type-const-cast
+   cppcoreguidelines-pro-type-cstyle-cast
    cppcoreguidelines-pro-type-reinterpret-cast
    cppcoreguidelines-pro-type-static-cast-downcast
    cppcoreguidelines-pro-type-union-access
    cppcoreguidelines-pro-type-vararg
    google-build-explicit-make-pair
    google-build-namespaces
    google-build-using-namespace
    google-explicit-constructor

test/clang-tidy/cppcoreguidelines-pro-type-cstyle-cast.cpp

+// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-cstyle-cast %t
+
+void reinterpretcast() {
+  int i = 0;
+  void *j;
+  j = (int*)j;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
+}
+
+void constcast() {
+  int* i;
+  const int* j;
+  i = (int*)j;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use C-style cast to cast away constness [cppcoreguidelines-pro-type-cstyle-cast]
+  j = (const int*)i; // OK, const added
+  (void)j; // OK, not a const_cast
+}
+
+void const_and_reinterpret() {
+  int* i;
+  const void* j;
+  i = (int*)j;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
+}
+
+class Base {
+};
+
+class Derived : public Base {
+};
+
+class Base2 {
+};
+
+class MultiDerived : public Base, public Base2 {
+};
+
+class PolymorphicBase {
+public:
+  virtual ~PolymorphicBase();
+};
+
+class PolymorphicDerived : public PolymorphicBase {
+};
+
+class PolymorphicMultiDerived : public Base, public PolymorphicBase {
+};
+
+void pointers() {
+
+  auto P0 = (Derived*)new Base();
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: do not use C-style cast to downcast from a base to a derived class [cppcoreguidelines-pro-type-cstyle-cast]
+
+  const Base* B0;
+  auto PC0 = (const Derived*)(B0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: do not use C-style cast to downcast from a base to a derived class [cppcoreguidelines-pro-type-cstyle-cast]
+
+  auto P1 = (Base*)new Derived(); // OK, upcast to a public base
+  auto P2 = (Base*)new MultiDerived(); // OK, upcast to a public base
+  auto P3 = (Base2*)new MultiDerived(); // OK, upcast to a public base
+}
+
+void pointers_polymorphic() {
+
+  auto PP0 = (PolymorphicDerived*)new PolymorphicBase();
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: do not use C-style cast to downcast from a base to a derived class; use dynamic_cast instead [cppcoreguidelines-pro-type-cstyle-cast]
+  // CHECK-FIXES: auto PP0 = dynamic_cast<PolymorphicDerived*>(new PolymorphicBase());
+
+  const PolymorphicBase* B0;
+  auto PPC0 = (const PolymorphicDerived*)B0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: do not use C-style cast to downcast from a base to a derived class; use dynamic_cast instead [cppcoreguidelines-pro-type-cstyle-cast]
+  // CHECK-FIXES: auto PPC0 = dynamic_cast<const PolymorphicDerived*>(B0);
+
+
+  auto B1 = (PolymorphicBase*)new PolymorphicDerived(); // OK, upcast to a public base
+  auto B2 = (PolymorphicBase*)new PolymorphicMultiDerived(); // OK, upcast to a public base
+  auto B3 = (Base*)new PolymorphicMultiDerived(); // OK, upcast to a public base
+}
+
+void arrays() {
+  Base ArrayOfBase[10];
+  auto A0 = (Derived*)ArrayOfBase;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: do not use C-style cast to downcast from a base to a derived class [cppcoreguidelines-pro-type-cstyle-cast]
+}
+
+void arrays_polymorphic() {
+  PolymorphicBase ArrayOfPolymorphicBase[10];
+  auto AP0 = (PolymorphicDerived*)ArrayOfPolymorphicBase;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: do not use C-style cast to downcast from a base to a derived class; use dynamic_cast instead
+  // CHECK-FIXES: auto AP0 = dynamic_cast<PolymorphicDerived*>(ArrayOfPolymorphicBase);
+}
+
+void references() {
+  Base B0;
+  auto R0 = (Derived&)B0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: do not use C-style cast to downcast from a base to a derived class [cppcoreguidelines-pro-type-cstyle-cast]
+  Base& RefToBase = B0;
+  auto R1 = (Derived&)RefToBase;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: do not use C-style cast to downcast from a base to a derived class [cppcoreguidelines-pro-type-cstyle-cast]
+
+  const Base& ConstRefToBase = B0;
+  auto RC1 = (const Derived&)ConstRefToBase;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: do not use C-style cast to downcast from a base to a derived class [cppcoreguidelines-pro-type-cstyle-cast]
+
+
+  Derived RD1;
+  auto R2 = (Base&)RD1; // OK, upcast to a public base
+}
+
+void references_polymorphic() {
+  PolymorphicBase B0;
+  auto RP0 = (PolymorphicDerived&)B0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: do not use C-style cast to downcast from a base to a derived class; use dynamic_cast instead
+  // CHECK-FIXES: auto RP0 = dynamic_cast<PolymorphicDerived&>(B0);
+
+  PolymorphicBase& RefToPolymorphicBase = B0;
+  auto RP1 = (PolymorphicDerived&)RefToPolymorphicBase;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: do not use C-style cast to downcast from a base to a derived class; use dynamic_cast instead [cppcoreguidelines-pro-type-cstyle-cast]
+  // CHECK-FIXES: auto RP1 = dynamic_cast<PolymorphicDerived&>(RefToPolymorphicBase);
+
+  const PolymorphicBase& ConstRefToPolymorphicBase = B0;
+  auto RPC2 = (const PolymorphicDerived&)(ConstRefToPolymorphicBase);
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: do not use C-style cast to downcast from a base to a derived class; use dynamic_cast instead [cppcoreguidelines-pro-type-cstyle-cast]
+  // CHECK-FIXES: auto RPC2 = dynamic_cast<const PolymorphicDerived&>(ConstRefToPolymorphicBase);
+
+  PolymorphicDerived d1;
+  auto RP2 = (PolymorphicBase&)d1; // OK, upcast to a public base
+}
+
+template<class B, class D>
+void templ() {
+  auto B0 = (B*)new D();
+}
+
+void templ_bad_call() {
+  templ<Derived, Base>(); //FIXME: this should trigger a warning
+}
+
+void templ_good_call() {
+  templ<Base, Derived>(); // OK, upcast to a public base
+}