Page MenuHomePhabricator

[clang-tidy] Warn on functional C-style casts
ClosedPublic

Authored by carlosgalvezp on Nov 23 2021, 2:56 AM.

Details

Summary

The google-readability-casting check is meant to be on par with cpplint's
readability/casting check, according to the documentation. However it
currently does not diagnose functional casts, like:

float x = 1.5F;
int y = int(x);

This is detected by cpplint, however, and the guidelines are clear that such
a cast is only allowed when the type is a class type (constructor call):

You may use cast formats like T(x) only when T is a class type.

Therefore, update the clang-tidy check to check this case.

Diff Detail

Event Timeline

carlosgalvezp created this revision.Nov 23 2021, 2:56 AM
carlosgalvezp requested review of this revision.Nov 23 2021, 2:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2021, 2:56 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
carlosgalvezp edited the summary of this revision. (Show Details)Nov 23 2021, 2:56 AM

Fix numbering of variables.

Fix numbering in variables.

Thanks for the patch.

Just a little bit of feedback but overall I'm happy with the approach taken.

clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
119–120

The majority of checkExpr()'s contents are common to both types, CStyleCastExpr and CXXFunctionalCastExpr.
Only the ReplaceRange = CharSourceRange::getCharRange... and the DestTypeString = Lexer::getSourceText... parts change depending on the Expr type.

What about breaking those two assignments out into their own functions, rather than templating the entire checkExpr() function?

For example, (note: untested code)

clang::CharSourceRange GetReplaceRange(const CStyleCastExpr *CastExpr) {
  return CharSourceRange::getCharRange(
      CastExpr->getLParenLoc(), CastExpr->getSubExprAsWritten()->getBeginLoc());
}

clang::CharSourceRange GetReplaceRange(const CXXFunctionalCastExpr *CastExpr) {
  return CharSourceRange::getCharRange(CastExpr->getBeginLoc(),
                                       CastExpr->getLParenLoc());
}

...

CharSourceRange ReplaceRange =
      isa<CStyleCastExpr>(CastExpr)
          ? GetReplaceRange(dyn_cast<const CStyleCastExpr>(CastExpr))
          : GetReplaceRange(dyn_cast<const CXXFunctionalCastExpr>(CastExpr));

Would something like that work?

164–175

See comment above.

clang-tools-extra/docs/ReleaseNotes.rst
139–140

Single back-ticks are used to do linking. Double back-ticks is probably what you're after.

clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
335

Is a test to check new int(x) worth including? I see that the cpplint guys explicitly filter it out of their results.

clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
120–124

Should this be an if ... else?

carlosgalvezp marked 5 inline comments as done.

Addressed comments.

clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
119–120

Thanks for the suggestion, much cleaner! I've made CastExpr become a ExplicitCastExpr instead (which is common base to both cast classes) to be able to handle only one object.

clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
335

Sure, even though I think technically it's not a cast. At least it's not shown as such in the AST.

carlosgalvezp added inline comments.Nov 27 2021, 9:02 AM
clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
122

One problem I see here is in the future someone adds a 3rd class of casts - then this should become an if-else. That's why I had it like that before, but perhaps it's too "defensive" at this point.

salman-javed-nz accepted this revision.Nov 27 2021, 6:40 PM

LGTM, thanks!

clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
335

It's not a cast in the AST, but it's nice to document in the unit test that we have considered it and that that intend to treat it no differently to how cpplint treats it.

This revision is now accepted and ready to land.Nov 27 2021, 6:40 PM
Quuxplusone added inline comments.
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
342

What about

template<class T> T foo(int i) { return T(i); }
int main() {
    foo<std::vector<int>>(); // valid, OK(!)
    foo<double>(); // valid, not OK
}

What about

struct Widget { Widget(int); };
using T = Widget;
using U = Widget&;
int i = 42;
Widget t = T(i);  // valid, OK?
Widget u = U(i);  // valid C++, should definitely not be OK

https://quuxplusone.github.io/blog/2020/01/22/expression-list-in-functional-cast/

Add extra test for template functions that use the functional cast. It should only warn when T is not a class type.

carlosgalvezp added inline comments.Nov 28 2021, 4:07 AM
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
342

Good point, thanks! I've added the first one to the unit test.

Regarding the second check, I'm not sure if it's the scope of this check. This check does not care whether the constructor of the class is implicit or not - if you use a class type, then you are calling the constructor so it's fine. Same goes when it's a reference - in my opinion this check is not concerned with that.

I definitely see the problems that can arise from the example that you posted, but maybe it fits better as a separate check in the bugprone category? This check (google-readability-casting) is focused only about avoiding C-style casting, i.e. it's a readability/style/modernize matter IMO. If cpplint is not diagnosing that, I don't think this check should either.

carlosgalvezp added inline comments.Nov 28 2021, 5:55 AM
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
342

It seems I should be able to just add the second example as a test and clang-tidy would warn but, what should be the fixit for it? A static_cast<U> would lead to compiler error (which I personally would gladly take, but I don't know in general if we want clang-tidy to "fix" code leading to compiler errors"). Adding an ad-hoc message for this particular error seems out of the scope of a "readability" check.

What do you guys think?

Personally I think it's best to merge this as is, allowing people to e.g. replace cpplint with clang-tidy (one static analyzer to rule them all!), and add more functionality in a separate, focused patch. Looking forward to hearing your thoughts :)

clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
342

It seems I should be able to just add the second example as a test and clang-tidy would warn but, what should be the fixit for it?

If I run the second example, but with old style C casts instead:

Input:

struct Widget { Widget(int); };
using T = Widget;
using U = Widget&;
int i = 42;
Widget t = (T)(i);
Widget u = (U)(i);

Output after fixits:

struct Widget { Widget(int); };
using T = Widget;
using U = Widget&;
int i = 42;
Widget t = T(i);
Widget u = (U)(i);

I guess the fix Widget t = T(i); is OK as it is covered by this exception:

You may use cast formats like T(x) only when T is a class type.

For the Widget u = (U)(i); line, clang-tidy has warned about it but not offered a fix.

salman-javed-nz added a comment.EditedNov 29 2021, 2:12 AM

I think the primary goal is satisfied - in all cases the cast is identified and a warning is generated.

For the Widget& case, a warning is generated but no fixit is offered, but that isn't any worse than the existing C-style cast fixits.
It does sound like a case where offering no fix is better than offering a fix that makes things worse.
If a fixit were to be implemented, a new check sounds like the best place to host it, along with other hidden reinterpret_cast scenarios that we can think of.

What would be the right fixit for that anyway?
Widget u = U(i); --> Widget u = static_cast<T>(i); ?

Quuxplusone added inline comments.Nov 29 2021, 6:06 AM
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
342

What would be the right fixit for that anyway?
Widget u = U(i); --> Widget u = static_cast<T>(i); ?

No, this is a reinterpret_cast, so it would be

Widget u = reinterpret_cast<U>(i);

at least in C++. I don't know about C, but I imagine the problem doesn't come up.

(If the programmer looks at this line and says "oh geez, that's wrong," well, he'll either fix it or file a task to revisit weird reinterpret_casts in the codebase. If the programmer thinks the cast is correct, then personally I'd hope he rewrites it as Widget u = *reinterpret_cast<Widget*>(&i); for clarity, but that's not a clang-tidy issue.)

Relevant: "fixits versus suppression mechanisms" https://quuxplusone.github.io/blog/2020/09/02/wparentheses/ reinterpret_cast is a suppression mechanism; I infer that you're casting about for a fixit, which won't exist in this case.

Added Widget test case (reusing the S2 struct instead of adding a new struct Widget)

carlosgalvezp added inline comments.Nov 29 2021, 7:28 AM
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
342

Added test case, currently it provides a generic comment.

Thanks a lot for the explanation, this was eye-opening to me. I only thought of reinterpret casts when using pointers, but of course references are kind of the same thing :)

Let me know if you are happy with the patch!

Quuxplusone accepted this revision.Nov 29 2021, 7:48 AM

Marking "accepted" for the record; but my checkmark means merely "I'm not intending to block this," not "I claim the authority to say you should land this." :)

clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
166

IMO, this repeated conditional (compare lines 119–122) should be factored out into the body of the helper function getDestTypeString (rather than being repeated at every call-site), and getDestTypeString should take a const ExplicitCastExpr * instead of having two overloads. (Notice that you never use the overloading for anything: everywhere you call into the overload set, you do so with a non-dependent dyn_cast wrapped in a ?:, indicating that you don't really want overloading at all.)

StringRef DestTypeString = getDestTypeString(SM, getLangOpts(), CastExpr);
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
358–360

FWIW, I'd prefer to instantiate the same function template in both cases (because that's the interesting case for practical purposes — a template that's only instantiated once doesn't pose a problem for the programmer). But I get that you're doing this because it's easier to express the expected output.

369

It has also just now occurred to me that it might be interesting to see what happens with this warning in a SFINAE context, e.g.

template<class T> auto is_castable(int i) -> decltype(T(i), void(), true) { return true; }

but I suppose none of the things we could do there would be remotely sensible, so you'd just be testing basically that clang-tidy doesn't crash in that case. Anyway, no action needed.

carlosgalvezp marked 6 inline comments as done.

Moved conditionals inside function body.

clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
166

Updated to move the conditional into the function. I cannot avoid the casting though, because there is no common base class of CXXFunctionalCastExpr and CStyleCastExpr that has the methods getLParenLoc and so on, so that's why I need the type explicitly instead of invoking those methods from the base class.

The original solution used templates instead of overloading for this, but @salman-javed-nz suggested overloading instead. I think that's a bit easier to read IMO, with small functions having a single responsibility.

Let me know if you like the update :)

clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
358–360

Yeah I'd prefer that too, but I wasn't sure how to set expectations on the same line for different inputs.

Marking "accepted" for the record; but my checkmark means merely "I'm not intending to block this," not "I claim the authority to say you should land this." :)

Thanks! I was recently told that this is not recommended, as the patch no longer shows the status "Needs review to proceed" and reviewers might not be able to see it immediately in their dashboards, thus delaying review.

Quuxplusone added inline comments.Nov 29 2021, 9:31 AM
clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
101–110

I actually meant to move the conditional inside the body, like this. ^
Now there's only one function, and the reader doesn't have to do mental overload resolution to figure out what the caller's behavior is intended to be.

clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
318

Pre-existing: This looks accidental.

Quuxplusone added inline comments.Nov 29 2021, 9:35 AM
clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
63–79

Ditto here:

static clang::CharSourceRange
getReplaceRange(const ExplicitCastExpr *Expr) {
  if (const auto *CastExpr = dyn_cast<CStyleCastExpr>(Expr)) {
    return CharSourceRange::getCharRange(
        CastExpr->getLParenLoc(), CastExpr->getSubExprAsWritten()->getBeginLoc());
  } else if (const auto *CastExpr = dyn_cast<CXXFunctionalCastExpr>(Expr)) {
    return CharSourceRange::getCharRange(CastExpr->getBeginLoc(),
                                         CastExpr->getLParenLoc());
  } else {
    // however Clang spells "unreachable"
  }
}

Besides saving some reader-brain-cells, this also makes it clearer that there's a potential null dereference in the old code (if both dyn_casts fail) that we're hoping is unreachable. This way, we have a place we can annotate that with assert(false) or whatever, instead of just dereferencing null.

Move logic into single functions.

carlosgalvezp marked 2 inline comments as done.Nov 29 2021, 9:49 AM
clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp
63–73

Looks much better than my suggested change. Good stuff.

clang-tools-extra/docs/ReleaseNotes.rst
139

Should this be like the

:doc:`bugprone-suspicious-memory-comparison
  <clang-tidy/checks/bugprone-suspicious-memory-comparison>`

a few lines above.

Nothing else that comes to mind that I want to see.
@Quuxplusone are you OK with the latest round of diffs?

clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
318

Isn't this intentional? Isn't it saying what the CHECK-MESSAGES should look like in the future once the FIXME is done?

LGTM!

clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp
318

Ah, I guess that must be it.
(C HECK-FIXES: ... is a really subtle idiom for FIXME the line above should become: ..., but OK!)

carlosgalvezp marked an inline comment as done.

Fixed doc.

Thanks a lot for the review! I've fixed the last comment by @salman-javed-nz . Since that was the last standing comment I'll push :)

This revision was landed with ongoing or failed builds.Nov 29 2021, 11:31 PM
This revision was automatically updated to reflect the committed changes.