[clang-tidy] add check cppcoreguidelines-pro-type-static-cast-downcast

[mgehre]

Summary

This check flags all usages of static_cast, where a base class is casted
to a derived class.
In those cases, a fixit is provided to convert the cast to a
dynamic_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.

This rule is part of the "Type safety" profile of the C++ Core
Guidelines, see
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#-type2-dont-use-static_cast-downcasts-use-dynamic_cast-instead

Depends on D13313

[sbenza]

test/clang-tidy/cppcoreguidelines-pro-type-static-cast-downcast.cpp

20

This is not a valid dynamic_cast<> because Base is not polymorphic.

48

I don't think we should warn here.
Whether it is a downcast or whether dynamic_cast<> is correct will depend on the specific instantiation.
A fix like this can break valid code.

[aaron.ballman]

clang-tidy/cppcoreguidelines/ProTypeStaticCastDowncastCheck.cpp

34

Will the type dependence check also look through parens, or should this move up a few lines?

43

No need for this or the check above -- getPointeeCXXRecordDecl() already does the right thing for references. You should be able to do:

if (auto *DestDecl = DestType->getPointeeCXXRecordDecl()) {
  // etc
}

Also note that auto DestDecl isn't a good idea as it hides the fact that DestDecl is a pointer from anyone reading the code.

52

Formatting looks off on this; also, I don't think you need the parenthetical as that information is part of the checker name itself.

[mgehre]

Fixed comments. Only show fixit for polymorphic classes. Ignore template instantiations for now

[mgehre]

Thank you for the detailed review.
I disabled checks in template instantiations for now.
Ideally, I would want to see something like:

warning: do not use static_cast to cast from base class to derived class.
note: in instantiation of function template specialization ... requested here ...

This must be without fixit to no break other users of the template (e.g. where other users use non-polymorphic types).
How can I add that "note: in instantiation" line to the diagnostic of the check?

[aaron.ballman]

clang-tidy/cppcoreguidelines/ProTypeStaticCastDowncastCheck.cpp

33

In the event it's not a pointer or a reference, why are you getting the source as a value type?

46

You should elide the braces here and for the else statement.

47

Punctuation nit -- we don't use complete sentences for diagnostics. This should be something like:

do not use static_cast to downcast from a base to a derived class; use dynamic_cast instead

50

I am pretty sure that you can create the replacement from a SourceLocation directly, because SourceRange has a nonexplicit constructor accepting a single SourceLocation.

53

What is the alternative the user should use in this instance?

[mgehre]

clang-tidy/cppcoreguidelines/ProTypeStaticCastDowncastCheck.cpp

53

The alternative is to either

1. change the program logic to not require and downcast of a non-polymorphic type

or

2. add NOLINT to notify your coworkers that this may be the spot where something goes wrong

In the end, these rules together should eliminate some classes of undefined behavior and crashes. If the application still crashes,
you should just need to look at the (hopefully few) places of NOLINT.

Anyway, I didn't make the rules. See Herb Sutters answer here: https://github.com/isocpp/CppCoreGuidelines/issues/270

[mgehre]

Simplify logic to check for BaseToDerived cast. Add test for object to reference cast. Fixed comments

clang-tidy/cppcoreguidelines/ProTypeStaticCastDowncastCheck.cpp

33

Source type could be no pointer nor ref like in:

Base B0;
auto R0 = static_cast<Derived&>(B0);

[aaron.ballman]

On Mon, Oct 5, 2015 at 6:14 PM, Matthias Gehre <[e-mail]> wrote:

mgehre marked an inline comment as done.

Comment at: clang-tidy/cppcoreguidelines/ProTypeStaticCastDowncastCheck.cpp:53
@@ +52,3 @@
+ } else {
+ diag(MatchedCast->getOperatorLoc(), "do not use static_cast to cast from base class to derived class.");

+ }

aaron.ballman wrote:

What is the alternative the user should use in this instance?

The alternative is to either

1. change the program logic to not require and downcast of a non-polymorphic type

or

2. add NOLINT to notify your coworkers that this may be the spot where something goes wrong

In the end, these rules together should eliminate some classes of undefined behavior and crashes. If the application still crashes,
you should just need to look at the (hopefully few) places of NOLINT.

Anyway, I didn't make the rules. See Herb Sutters answer here: https://github.com/isocpp/CppCoreGuidelines/issues/270

This wasn't a comment on the rule so much as a comment on the diagnostic not being very helpful.In this case, you're telling the user to not do something, but it is unclear how the user would structure their code to silence this diagnostic. Perhaps there is no way to word this to give the user a clue, but we should at least try. If I got this diagnostic as it is now, I would scratch my head and quickly decide to ignore it.

Comment at: clang-tidy/cppcoreguidelines/ProTypeStaticCastDowncastCheck.cpp:33
@@ +32,3 @@
+ const auto *SourceDecl = SourceType->getPointeeCXXRecordDecl();
+ if(!SourceDecl)

+ SourceDecl = SourceType->getAsCXXRecordDecl();

aaron.ballman wrote:

In the event it's not a pointer or a reference, why are you getting the source as a value type?

Source type could be no pointer nor ref like in:

Base B0;
auto R0 = static_cast<Derived&>(B0);

Ah, interesting! I was guessing that case would have had an lvalue reference type for B0, but I'm guessing it's already gone through lvalue to rvalue conversion before hitting the cast node (or my intuition is just off).

test/clang-tidy/cppcoreguidelines-pro-type-static-cast-downcast.cpp

40

No need to have the [cppcoreguidelines-pro-type-static-cast-downcast] part of the message on anything but the first diagnostic in the file. (This reduces clutter, but we test it one time just to make sure it's there).

[klimek]

In D13368#260669, @aaron.ballman wrote:

This wasn't a comment on the rule so much as a comment on the diagnostic not being very helpful.In this case, you're telling the user to not do something, but it is unclear how the user would structure their code to silence this diagnostic. Perhaps there is no way to word this to give the user a clue, but we should at least try. If I got this diagnostic as it is now, I would scratch my head and quickly decide to ignore it.

The cpp core guidelines are written in a way that they should be referenceable by links - do we want to add links to the relevant portions of the core guidelines from the clang-tidy checks?

[aaron.ballman]

In D13368#260672, @klimek wrote:

In D13368#260669, @aaron.ballman wrote:

This wasn't a comment on the rule so much as a comment on the diagnostic not being very helpful.In this case, you're telling the user to not do something, but it is unclear how the user would structure their code to silence this diagnostic. Perhaps there is no way to word this to give the user a clue, but we should at least try. If I got this diagnostic as it is now, I would scratch my head and quickly decide to ignore it.

The cpp core guidelines are written in a way that they should be referenceable by links - do we want to add links to the relevant portions of the core guidelines from the clang-tidy checks?

I'd be hesitant to do that. It would add a lot of verbiage to diagnostics that are likely to be chatty, and if the links ever go dead mid-release cycle for us, we're stuck looking bad with no way to fix it. CERT's guidelines are also linkable in the same fashion (as would be hypothetical checks for MISRA, JSF, etc), and I would have the same hesitation for those as well due to the potential dead link issue.

I think that having the links within the user-facing documentation is a must-have though (and something we've been pretty good about thus far) because those can be updated live from ToT. So perhaps a permanent short link to our own documentation might be useful (if a bit obtuse since our docs mostly just point to other docs elsewhere)? I'd still be a bit worried about expired short links or something, but maybe we already host a service for this sort of thing?

[mgehre]

Remove trailing [cppcoreguidelines-pro-type-static-cast-downcast] on tests

[mgehre]

I cannot think of any way to improve the usefulness of the diagnostic in the non-polymorphic case.
For now, this patch has a link to the CppCoreGuidelines document in the docs/**/*.rst file.

Also, this check is not enabled by default. One has to explicitly enable it, and may at that
point reason about the usefulness of the CppCoreGuidelines and this check.

test/clang-tidy/cppcoreguidelines-pro-type-static-cast-downcast.cpp

40

I can remove it on the remaining messages that end in "use dynamic_cast instead". I need to keep it in the non-polymorphic case
to make sure that they are not followed by "use dynamic_cast instead".

[aaron.ballman]

On Tue, Oct 6, 2015 at 5:46 PM, Matthias Gehre <[e-mail]> wrote:

mgehre marked an inline comment as done.
mgehre added a comment.

I cannot think of any way to improve the usefulness of the diagnostic in the non-polymorphic case.
For now, this patch has a link to the CppCoreGuidelines document in the docs/**/*.rst file.

Neither can I, so I guess it stands for right now.

Also, this check is not enabled by default. One has to explicitly enable it, and may at that
point reason about the usefulness of the CppCoreGuidelines and this check.

That is true, but enabling this check doesn't always require thoughtfulness on the part of the user. They can enable the check with -checks=corecppguidelines-* for instance. However, they can at least find the documentation from the check failures and hopefully go from there.

Comment at: test/clang-tidy/cppcoreguidelines-pro-type-static-cast-downcast.cpp:39
@@ +38,3 @@
+ auto PP0 = static_cast<PolymorphicDerived*>(new PolymorphicBase());
+ // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: do not use static_cast to downcast from a base to a derived class; use dynamic_cast instead [cppcoreguidelines-pro-type-static-cast-downcast]

+ // CHECK-FIXES: auto PP0 = dynamic_cast<PolymorphicDerived*>(new PolymorphicBase());

aaron.ballman wrote:

No need to have the [cppcoreguidelines-pro-type-static-cast-downcast] part of the message on anything but the first diagnostic in the file. (This reduces clutter, but we test it one time just to make sure it's there).

I can remove it on the remaining messages that end in "use dynamic_cast instead". I need to keep it in the non-polymorphic case
to make sure that they are not followed by "use dynamic_cast instead".

Ah, that's an excellent point.

Patch LGTM! Since Samuel had comments as well, I will wait for his sign-off before committing.

~Aaron

[sbenza]

test/clang-tidy/cppcoreguidelines-pro-type-static-cast-downcast.cpp

29

I just noticed that we don't have any test with

static_cast<const Derived*>(...)  or static_cast<const Derived&>

We should make sure that works.

[mgehre]

Add test for static_cast with const

clang-tidy/cppcoreguidelines/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyCppCoreGuidelinesModule
   CppCoreGuidelinesTidyModule.cpp
   ProTypeReinterpretCastCheck.cpp
+  ProTypeStaticCastDowncastCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   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 "ProTypeReinterpretCastCheck.h"
+#include "ProTypeStaticCastDowncastCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace cppcoreguidelines {
 
 class CppCoreGuidelinesModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ProTypeReinterpretCastCheck>(
         "cppcoreguidelines-pro-type-reinterpret-cast");
+    CheckFactories.registerCheck<ProTypeStaticCastDowncastCheck>(
+        "cppcoreguidelines-pro-type-static-cast-downcast");
   }
 };
 
 // Register the LLVMTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<CppCoreGuidelinesModule>
     X("cppcoreguidelines-module", "Adds checks for the C++ Core Guidelines.");
 
 } // namespace cppcoreguidelines
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the CppCoreGuidelinesModule.
 volatile int CppCoreGuidelinesModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tidy/cppcoreguidelines/ProTypeStaticCastDowncastCheck.h

+//===--- ProTypeStaticCastDowncastCheck.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_STATIC_CAST_DOWNCAST_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_PRO_TYPE_STATIC_CAST_DOWNCAST_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// Checks for usages of static_cast, where a base class is downcasted to a derived class.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-pro-type-static-cast-downcast.html
+class ProTypeStaticCastDowncastCheck : public ClangTidyCheck {
+public:
+  ProTypeStaticCastDowncastCheck(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_STATIC_CAST_DOWNCAST_H
+

clang-tidy/cppcoreguidelines/ProTypeStaticCastDowncastCheck.cpp

+//===--- ProTypeStaticCastDowncastCheck.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 "ProTypeStaticCastDowncastCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+
+void ProTypeStaticCastDowncastCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  Finder->addMatcher(cxxStaticCastExpr(
+                      unless(isInTemplateInstantiation())
+                    ).bind("cast"), this);
+}
+
+void ProTypeStaticCastDowncastCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedCast = Result.Nodes.getNodeAs<CXXStaticCastExpr>("cast");
+
+  QualType SourceType = MatchedCast->getSubExpr()->getType();
+  const auto *SourceDecl = SourceType->getPointeeCXXRecordDecl();
+  if(!SourceDecl)
+    SourceDecl = SourceType->getAsCXXRecordDecl();
+  if(!SourceDecl)
+    return;
+
+  QualType DestType = MatchedCast->getTypeAsWritten();
+  const auto *DestDecl = DestType->getPointeeCXXRecordDecl();
+  if(!DestDecl)
+    return;
+
+  if (!DestDecl->isDerivedFrom(SourceDecl))
+    return;
+
+  if (SourceDecl->isPolymorphic()) {
+    diag(MatchedCast->getOperatorLoc(), "do not use static_cast to cast from base class to derived class. "
+      "Use dynamic_cast instead.")
+      << FixItHint::CreateReplacement(
+          {MatchedCast->getOperatorLoc(), MatchedCast->getOperatorLoc()},
+          "dynamic_cast");
+  } else {
+    diag(MatchedCast->getOperatorLoc(), "do not use static_cast to cast from base class to derived class.");
+  }
+}
+
+} // namespace tidy
+} // namespace clang
+

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

+cppcoreguidelines-pro-type-static-cast-downcast
+===============================================
+
+This check flags all usages of static_cast, where a base class is casted to a derived class.
+In those cases, a fixit is provided to convert the cast to a dynamic_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.
+
+This rule is part of the "Type safety" profile of the C++ Core Guidelines, see
+https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#-type2-dont-use-static_cast-downcasts-use-dynamic_cast-instead

docs/clang-tidy/checks/list.rst

 List of clang-tidy Checks
 =========================
 
 .. toctree::
    cppcoreguidelines-pro-type-reinterpret-cast
+   cppcoreguidelines-pro-type-static-cast-downcast
    google-build-explicit-make-pair
    google-build-namespaces
    google-build-using-namespace
    google-explicit-constructor
    google-global-names-in-headers
    google-readability-braces-around-statements
    google-readability-casting
    google-readability-function-size

test/clang-tidy/cppcoreguidelines-pro-type-static-cast-downcast.cpp

+// RUN: %python %S/check_clang_tidy.py %s cppcoreguidelines-pro-type-static-cast-downcast %t
+
+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 = static_cast<Derived*>(new Base());
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: do not use static_cast to cast from base class to derived class. [cppcoreguidelines-pro-type-static-cast-downcast]
+
+  auto P1 = static_cast<Base*>(new Derived()); // OK, upcast to a public base
+  auto P2 = static_cast<Base*>(new MultiDerived()); // OK, upcast to a public base
+  auto P3 = static_cast<Base2*>(new MultiDerived()); // OK, upcast to a public base
+}
+
+void pointers_polymorphic() {
+
+  auto PP0 = static_cast<PolymorphicDerived*>(new PolymorphicBase());
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: do not use static_cast to cast from base class to derived class. Use dynamic_cast instead. [cppcoreguidelines-pro-type-static-cast-downcast]
+  // CHECK-FIXES: auto PP0 = dynamic_cast<PolymorphicDerived*>(new PolymorphicBase());
+
+  auto B1 = static_cast<PolymorphicBase*>(new PolymorphicDerived()); // OK, upcast to a public base
+  auto B2 = static_cast<PolymorphicBase*>(new PolymorphicMultiDerived()); // OK, upcast to a public base
+  auto B3 = static_cast<Base*>(new PolymorphicMultiDerived()); // OK, upcast to a public base
+}
+
+void arrays() {
+  Base ArrayOfBase[10];
+  auto A0 = static_cast<Derived*>(ArrayOfBase);
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: do not use static_cast to cast from base class to derived class. [cppcoreguidelines-pro-type-static-cast-downcast]
+}
+
+void arrays_polymorphic() {
+  PolymorphicBase ArrayOfPolymorphicBase[10];
+  auto AP0 = static_cast<PolymorphicDerived*>(ArrayOfPolymorphicBase);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: do not use static_cast to cast from base class to derived class. Use dynamic_cast instead. [cppcoreguidelines-pro-type-static-cast-downcast]
+  // CHECK-FIXES: auto AP0 = dynamic_cast<PolymorphicDerived*>(ArrayOfPolymorphicBase);
+}
+
+void references() {
+  Base B0;
+  Base& RefToBase = B0;
+  auto R0 = static_cast<Derived&>(RefToBase);
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: do not use static_cast to cast from base class to derived class. [cppcoreguidelines-pro-type-static-cast-downcast]
+
+  Derived RD1;
+  auto R1 = static_cast<Base&>(RD1); // OK, upcast to a public base
+}
+
+void references_polymorphic() {
+  PolymorphicBase B0;
+  PolymorphicBase& RefToPolymorphicBase = B0;
+  auto RP0 = static_cast<PolymorphicDerived&>(RefToPolymorphicBase);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: do not use static_cast to cast from base class to derived class. Use dynamic_cast instead. [cppcoreguidelines-pro-type-static-cast-downcast]
+  // CHECK-FIXES: auto RP0 = dynamic_cast<PolymorphicDerived&>(RefToPolymorphicBase);
+
+  PolymorphicDerived d1;
+  auto RP1 = static_cast<PolymorphicBase&>(d1); // OK, upcast to a public base
+}
+
+template<class B, class D>
+void templ() {
+  auto B0 = static_cast<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
+}