This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by mgehre on Oct 1 2015, 3:37 PM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

mgehre updated this revision to Diff 36315.Oct 1 2015, 3:37 PM
mgehre retitled this revision from to [clang-tidy] add check cppcoreguidelines-pro-type-static-cast-downcast.
mgehre updated this object.
mgehre added a subscriber: cfe-commits.
sbenza added inline comments.Oct 2 2015, 8:15 AM
test/clang-tidy/cppcoreguidelines-pro-type-static-cast-downcast.cpp
19 ↗(On Diff #36315)

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

47 ↗(On Diff #36315)

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 added inline comments.Oct 2 2015, 8:54 AM
clang-tidy/cppcoreguidelines/ProTypeStaticCastDowncastCheck.cpp
33 ↗(On Diff #36315)

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

42 ↗(On Diff #36315)

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.

51 ↗(On Diff #36315)

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 updated this revision to Diff 36465.Oct 4 2015, 5:44 AM
mgehre marked 3 inline comments as done.

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

mgehre marked 2 inline comments as done.Oct 4 2015, 5:49 AM

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 added inline comments.Oct 5 2015, 7:35 AM
clang-tidy/cppcoreguidelines/ProTypeStaticCastDowncastCheck.cpp
33 ↗(On Diff #36465)

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

46 ↗(On Diff #36465)

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

47 ↗(On Diff #36465)

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 ↗(On Diff #36465)

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 ↗(On Diff #36465)

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

mgehre marked an inline comment as done.Oct 5 2015, 3:14 PM
mgehre added inline comments.
clang-tidy/cppcoreguidelines/ProTypeStaticCastDowncastCheck.cpp
53 ↗(On Diff #36465)

The alternative is to either

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

or

  1. 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 updated this revision to Diff 36567.Oct 5 2015, 4:00 PM
mgehre marked 2 inline comments as done.

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

clang-tidy/cppcoreguidelines/ProTypeStaticCastDowncastCheck.cpp
33 ↗(On Diff #36465)

Source type could be no pointer nor ref like in:

Base B0;
auto R0 = static_cast<Derived&>(B0);
aaron.ballman edited edge metadata.Oct 6 2015, 6:59 AM

On Mon, Oct 5, 2015 at 6:14 PM, Matthias Gehre <M.Gehre@gmx.de> 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

  1. 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
39 ↗(On Diff #36567)

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 added a subscriber: klimek.Oct 6 2015, 7:02 AM

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?

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 updated this revision to Diff 36664.Oct 6 2015, 2:40 PM
mgehre edited edge metadata.

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

mgehre marked an inline comment as done.Oct 6 2015, 2:46 PM

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
39 ↗(On Diff #36567)

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 accepted this revision.Oct 7 2015, 5:45 AM
aaron.ballman edited edge metadata.

On Tue, Oct 6, 2015 at 5:46 PM, Matthias Gehre <M.Gehre@gmx.de> 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

This revision is now accepted and ready to land.Oct 7 2015, 5:45 AM
sbenza added inline comments.Oct 7 2015, 9:59 AM
test/clang-tidy/cppcoreguidelines-pro-type-static-cast-downcast.cpp
28 ↗(On Diff #36664)

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 updated this revision to Diff 37155.Oct 12 2015, 1:15 PM
mgehre marked an inline comment as done.
mgehre edited edge metadata.

Add test for static_cast with const

This revision was automatically updated to reflect the committed changes.