[clang-tidy] Add new checker: llvm-prefer-isa-or-dyn-cast-in-conditionals

[hintonda]

Summary

Looks at conditionals and finds cases of `cast<>`, which will
assert rather than return a null pointer, and `dyn_cast<>` where
the return value is not captured. Additionally, finds cases that
match the pattern `var.foo() && isa<X>(var.foo())`, where the
method is called twice and could be expensive.

.. code-block:: c++

// Finds cases like these:
if (auto x = cast<X>(y)) <...>
if (cast<X>(y)) <...>

// But not cases like these:
if (auto f = cast<Z>(y)->foo()) <...>
if (cast<Z>(y)->foo()) <...>

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h

21

Please use using. See modernize-use-using.

clang-tools-extra/docs/ReleaseNotes.rst

136

Please enclose cast<> in ``.

clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst

7

Please enclose cast<> in ``. Same below.

8

Please fix double space.

10

Please fix double dot.

[hintonda]

clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h

21

Normally I'd agree, but since the llvm checkers live in the llvm namespace, including any llvm checker prior to this header will prevent clang from finding llvm::SmallSet. As you can see, the other llvm classes don't need the llvm:: prefix, but that's because clang/include/clang/Basic/LLVM.h already has a bunch of using statements.

So, this code won't compile without this change, or the addition of using llvm::SmallSet; to clang/include/clang/Basic/LLVM.h. Should I make that change first? Then just remove the llvm:: prefix?

[hintonda]

clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h

21

Oh, sorry, didn't read closely enough. I can get rid of the typedef.

[hintonda]

• Address comments.

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h

17

It'll be easier to use llvm::SmallSet in using statement.

clang-tools-extra/docs/ReleaseNotes.rst

136

Please use double, not single `.

[hintonda]

clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h

17

Unfortunately, that won't compile.

The reason is that the llvm checkers live in namespace clang::clang-tidy::llvm, so if you include an llvm checker header, which opens the clang::clang-tidy::llvm namespace, clang doesn't know whether to look in llvm or clang::clang-tidy::llvm since you are in clang::clang-tidy.

The error does suggest using ::llvm::SmallSet, but that seems sorta klugey.

Since this looks ugly, how about I add using llvm::SmallSet; to clang/include/clang/LLVM.h?

[hintonda]

clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h

17

Btw, the reason this never caused a problem before was that the first llvm checker header included in clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp was HeaderGuardCheck.h, which only include HeaderGuardCheck.h before opening the new clang::clang-tidy::llvm namespace. Once I added this checker, which was included before HeaderGuardCheck.h, it made the new llvm namespace was ambiguous within clang-tidy, hence the error.

[hintonda]

• Address additional comments, and move to LLVM.h.

[aaron.ballman]

Should this check also try to find this pattern:

if (dyn_cast<Frobble>(Bobble))
  ...

in order to recommend the proper replacement:

if (isa<Frobble>(Bobble))
  ...

I ask because the name llvm-avoid-cast-in-conditional sounds like it would also cover this situation and I run into it during code reviews with some frequency (more often than I run into cast<> being used in a conditional).

clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp

20

No need to register this matcher if C++ isn't enabled.

35–36

clang-tidy diagnostics should not be complete sentences; you should make this ungrammatical. How about: cast<> in conditional will assert rather than return a null pointer?

37

Spurious semicolon?

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst

8

This cast<> still needs to be enclosed in ``.

[hintonda]

In D59802#1443340, @aaron.ballman wrote:

Should this check also try to find this pattern:

if (dyn_cast<Frobble>(Bobble))
  ...

in order to recommend the proper replacement:

if (isa<Frobble>(Bobble))
  ...

I ask because the name llvm-avoid-cast-in-conditional sounds like it would also cover this situation and I run into it during code reviews with some frequency (more often than I run into cast<> being used in a conditional).

Yes, I can add that, and provide a fix-it too. Thanks...

[hintonda]

In D59802#1443451, @hintonda wrote:

In D59802#1443340, @aaron.ballman wrote:

Should this check also try to find this pattern:

if (dyn_cast<Frobble>(Bobble))
  ...

in order to recommend the proper replacement:

if (isa<Frobble>(Bobble))
  ...

I ask because the name llvm-avoid-cast-in-conditional sounds like it would also cover this situation and I run into it during code reviews with some frequency (more often than I run into cast<> being used in a conditional).

Yes, I can add that, and provide a fix-it too. Thanks...

I did a quick grep and found a few of these, but also found the _or_null<> variety. Guess they'll need to stay the same since isa<> can't handle nulls.

Btw, I also found the same pattern used for while(), so I'll add that too. Here's a sample of the patterns I'm seeing:

./lldb/source/Plugins/ExpressionParser/Clang/ASTResultSynthesizer.cpp:213: while (dyn_cast<NullStmt>(last_stmt)) {
./clang/lib/CodeGen/CodeGenModule.cpp:1390: if (dyn_cast_or_null<NamedDecl>(D)) . // <--- this one's okay
./clang/lib/CodeGen/CodeGenModule.cpp:3950: if (dyn_cast<llvm::CallInst>(callSite)) {

[aaron.ballman]

In D59802#1443515, @hintonda wrote:

In D59802#1443451, @hintonda wrote:

In D59802#1443340, @aaron.ballman wrote:

Should this check also try to find this pattern:

if (dyn_cast<Frobble>(Bobble))
  ...

in order to recommend the proper replacement:

if (isa<Frobble>(Bobble))
  ...

I ask because the name llvm-avoid-cast-in-conditional sounds like it would also cover this situation and I run into it during code reviews with some frequency (more often than I run into cast<> being used in a conditional).

Yes, I can add that, and provide a fix-it too. Thanks...

I did a quick grep and found a few of these, but also found the _or_null<> variety. Guess they'll need to stay the same since isa<> can't handle nulls.

Would it make sense to transform if (dyn_cast_or_null<T>(Obj)) into if (Obj && isa<T>(Obj)) or are there bad transformations from that?

Btw, I also found the same pattern used for while(), so I'll add that too. Here's a sample of the patterns I'm seeing:

./lldb/source/Plugins/ExpressionParser/Clang/ASTResultSynthesizer.cpp:213: while (dyn_cast<NullStmt>(last_stmt)) {

Hah, good catch!

./clang/lib/CodeGen/CodeGenModule.cpp:1390: if (dyn_cast_or_null<NamedDecl>(D)) . // <--- this one's okay

I think this could be expressed as if (D && isa<NamedDecl>(D)), no?

[hintonda]

In D59802#1443527, @aaron.ballman wrote:

Would it make sense to transform if (dyn_cast_or_null<T>(Obj)) into if (Obj && isa<T>(Obj)) or are there bad transformations from that?

Sure, that sounds reasonable. I only see about 12 cases across all repos, so it isn't that common. Whereas the idiom you present is used quite often.

I haven't looked yet, but wouldn't the use of isa_or_null<> be more efficient in cases like this?

./clang/lib/Sema/AnalysisBasedWarnings.cpp:401: if (B->getTerminator() && isa<CXXTryStmt>(B->getTerminator()))
./clang/lib/AST/Expr.cpp:2734: if (MCE->getMethodDecl() && isa<CXXConversionDecl>(MCE->getMethodDecl()))

Granted, there aren't a lot of these.

Btw, I also found the same pattern used for while(), so I'll add that too. Here's a sample of the patterns I'm seeing:

./lldb/source/Plugins/ExpressionParser/Clang/ASTResultSynthesizer.cpp:213: while (dyn_cast<NullStmt>(last_stmt)) {

Hah, good catch!

./clang/lib/CodeGen/CodeGenModule.cpp:1390: if (dyn_cast_or_null<NamedDecl>(D)) . // <--- this one's okay

I think this could be expressed as if (D && isa<NamedDecl>(D)), no?

[hintonda]

• Address additional comments.

[hintonda]

• Added dyn_cast and dyn_cast_or_null, and provided fixit's for all of them.

[hintonda]

I looked at the IR generated at -O2, and found that while if (isa<X>(y)) is a modest win over if (dyn_cast<X>(y), if (dyn_cast_or_null<X>(y)) generates exactly the same IR that if(y && isa<X>(y)) does. Also, if y is actually an expression that makes a function call, it's more expensive because it will make the call twice.

So I don't seen any reason to replace dyn_cast_or_null<> in conditionals.

[hintonda]

• Removed the dyn_cast_or_null replacements.

[hintonda]

• Add replacement of y && cast<X>(y) with dyn_cast_or_null<X>(y), which is at least as efficient, and can be a big win if y is a function call.

[hintonda]

• Remove commented code.

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp

74

Please don't use auto where type is not obvious.

75

Please don't use auto where type is not obvious.

83

Please don't use auto where type is not obvious.

84

Please don't use auto where type is not obvious.

91

Please don't use auto where type is not obvious.

92

Please don't use auto where type is not obvious.

102

Please don't use auto where type is not obvious.

105

Please don't use auto where type is not obvious.

108

Please don't use auto where type is not obvious.

[hintonda]

clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp

74

Happy to make the change, but thought the get.*Loc() methods were obvious?

[Eugene.Zelenko]

In D59802#1447108, @hintonda wrote:

It's good idea to follow modernize-use-auto convention without introducing exceptions.

[hintonda]

In D59802#1447127, @Eugene.Zelenko wrote:

In D59802#1447108, @hintonda wrote:

It's good idea to follow modernize-use-auto convention without introducing exceptions.

No worries. I'm just not sure I fully grok them.

[hintonda]

• Remove spurious auto's.

[hintonda]

If the dyn_cast_or_null<> replacement is acceptable, I'll update the documentation and warning message -- suggestions are appreciated.

[hintonda]

• Add isa and dyn_cast when matching for dyn_cast_or_null replacement.

[aaron.ballman]

In D59802#1445566, @hintonda wrote:

I looked at the IR generated at -O2, and found that while if (isa<X>(y)) is a modest win over if (dyn_cast<X>(y), if (dyn_cast_or_null<X>(y)) generates exactly the same IR that if(y && isa<X>(y)) does. Also, if y is actually an expression that makes a function call, it's more expensive because it will make the call twice.

So I don't seen any reason to replace dyn_cast_or_null<> in conditionals.

Mostly because I think it's more clear with isa<> because that's really what it's checking. However, I could see not doing an automatic transform in the case where the expression argument is something expensive, like a function call. I could also see this as an impetus for adding isa_or_null<> as a separate commit.

clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp

39

This seems like a fair amount of code duplication -- can this be cleaned up using some local variables for the inner matchers?

61

I think the expr() matcher is too general. This will trigger on something like if (foo && isa<T>(bar)) won't it?

I'd also like to see tests for code like:

foo && cast<T>(foo)->bar();

I don't think this pattern should diagnose because you cannot safely replace it with dyn_cast<T>(foo)->bar()

63–65

I do not think we should diagnose code that uses obj && isa<T>(obj) -- that's very reasonable code.

74

Generally, no. There are a fair number of different location types and it's not obvious which type you're getting. We usually mean obvious as in "this type is an iterator, but the concrete type of the iterator isn't that important" or "the type is exactly spelled out as a template argument".

91–92

Should we be concerned about fix-it interactions with macros (here and elsewhere in the check)? We usually suppress fix-its if the replacement range is somewhere within a macro expansion.

97

What if the reason we matched was because the cast was written using a macro of a different name? e.g., #define LONGER_IDENTIFIER(T, Obj) cast<T>(Obj)

113–117

Can this be handled at the matcher level? IIRC we have isExpansionInFileMatching() or something like that.

146

This diagnostic doesn't tell the user what they've done wrong with the code or why this is a better choice.

[aaron.ballman]

clang-tools-extra/clang-tidy/tool/run-clang-tidy.py

256–258 ↗

(On Diff #192778)

Is this related to your patch? If so, why is it needed?

clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h

21

Personally, I prefer using ::llvm::SmallSet to this change. It makes it very clear what namespace the type lives in.

[hintonda]

• Address comments.

[hintonda]

In D59802#1447560, @aaron.ballman wrote:

In D59802#1445566, @hintonda wrote:

I looked at the IR generated at -O2, and found that while if (isa<X>(y)) is a modest win over if (dyn_cast<X>(y), if (dyn_cast_or_null<X>(y)) generates exactly the same IR that if(y && isa<X>(y)) does. Also, if y is actually an expression that makes a function call, it's more expensive because it will make the call twice.

So I don't seen any reason to replace dyn_cast_or_null<> in conditionals.

Mostly because I think it's more clear with isa<> because that's really what it's checking. However, I could see not doing an automatic transform in the case where the expression argument is something expensive, like a function call. I could also see this as an impetus for adding isa_or_null<> as a separate commit.

My last patch only changes if(y && isa<X>(y)) if y is a CXXMemberCallExpr, so I hope that addresses your concern.

clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp

146

Yes, but I'm not yet sure what it should say. Was sorta hoping for a suggestion.

[hintonda]

• Improve warning message.

[hintonda]

• Added matcher, and reordered tests.

[hintonda]

In D59802#1448574, @hintonda wrote:

• Added matcher, and reordered tests.

Commandline ate my ticks... ;-(

Should be added isMacroID matcher.

[aaron.ballman]

clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp

39

I'm still hoping that some of this duplication can be reduced, btw.

104–105

Spurious code that can be removed?

119

Aside from the fix-it, this code is identical to the block above. Can be combined.

126–127

There are zero test cases that seem to trigger this diagnostic text.

146

Do you have any evidence that this situation happens in practice? I kind of feel like this entire branch could be eliminated from this patch unless it actually catches problems that happen.

clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h

21

Can you add back the ::llvm:: qualifier on the StringRef type?

clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp

23

Please spell out the full diagnostic the first time it occur in the test case - it's fine to abbreviate subsequent diagnostics, but we like to see at least one exact match per diagnostic.

49

Same here.

65

and here

[hintonda]

clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h

21

I could do that, but do you mean just this case of StringRef, of all four? And what about SourceLocation and SourceManager?

[aaron.ballman]

clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h

21

If any changes are needed in this file at all, I'd prefer to keep them minimal and self-consistent, so only this instance. However, I wouldn't be opposed to a consistency cleanup in this file as a separate patch with NFC.

[hintonda]

clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h

21

Since fixing the bug in this file is orthogonal to this patch, I'll address it in it's own patch.

[hintonda]

clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp

126–127

Sorry for any confusion, but there are actually 3 tests for this below. That'll be made clearer once I address your comments below, and spell out the entire diagnostic message on the first instance.

[xbolva00]

Do we have any checker to recommend llvm functions over std ?

e.g. llvm::sort, llvm::all_of, ...

[Eugene.Zelenko]

In D59802#1450185, @xbolva00 wrote:

Do we have any checker to recommend llvm functions over std ?

e.g. llvm::sort, llvm::all_of, ...

No. Actually LLVM coding conventions support is very limited. See https://clang.llvm.org/extra/clang-tidy/checks/list.html.

I think check for auto usage is useful too.

[hintonda]

clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp

146

Yes, here are a few from clang/lib -- let me know if you think it's worth it or not to keep this:

• DiagnosticName: llvm-avoid-cast-in-conditional FileOffset: 305293 FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/SemaTemplate.cpp Message: method 'getAsTemplateDecl' is called twice and could be expensive Replacements:
  • FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/SemaTemplate.cpp Length: 93 Offset: 305293 ReplacementText: dyn_cast_or_null<TemplateTemplateParmDecl>(Name.getAsTemplateDecl())
• DiagnosticName: llvm-avoid-cast-in-conditional FileOffset: 153442 FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/AST/ASTContext.cpp Message: method 'getAsTemplateDecl' is called twice and could be expensive Replacements:
  • FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/AST/ASTContext.cpp Length: 92 Offset: 153442 ReplacementText: dyn_cast_or_null<TypeAliasTemplateDecl>(Template.getAsTemplateDecl())
• DiagnosticName: llvm-avoid-cast-in-conditional FileOffset: 97556 FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/AST/Expr.cpp Message: method 'getMethodDecl' is called twice and could be expensive Replacements:
  • FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/AST/Expr.cpp Length: 68 Offset: 97556 ReplacementText: dyn_cast_or_null<CXXConversionDecl>(MCE->getMethodDecl())
• DiagnosticName: llvm-avoid-cast-in-conditional FileOffset: 301950 FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/SemaInit.cpp Message: method 'get' is called twice and could be expensive Replacements:
  • FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/SemaInit.cpp Length: 49 Offset: 301950 ReplacementText: dyn_cast_or_null<InitListExpr>(CurInit.get())
• DiagnosticName: llvm-avoid-cast-in-conditional FileOffset: 14335 FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/AnalysisBasedWarnings.cpp Message: method 'operator bool' is called twice and could be expensive Replacements:
  • FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/AnalysisBasedWarnings.cpp Length: 57 Offset: 14335 ReplacementText: dyn_cast_or_null<CXXTryStmt>(B->getTerminator())
• DiagnosticName: llvm-avoid-cast-in-conditional FileOffset: 15997 FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/AnalysisBasedWarnings.cpp Message: method 'operator bool' is called twice and could be expensive Replacements:
  • FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/AnalysisBasedWarnings.cpp Length: 55 Offset: 15997 ReplacementText: dyn_cast_or_null<CXXTryStmt>(B.getTerminator())
• DiagnosticName: llvm-avoid-cast-in-conditional FileOffset: 9492 FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h Message: method 'sexpr' is called twice and could be expensive Replacements:
  • FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h Length: 39 Offset: 9492 ReplacementText: dyn_cast_or_null<til::Undefined>(sexpr())
• DiagnosticName: llvm-avoid-cast-in-conditional FileOffset: 9572 FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h Message: method 'sexpr' is called twice and could be expensive Replacements:
  • FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h Length: 38 Offset: 9572 ReplacementText: dyn_cast_or_null<til::Wildcard>(sexpr())
• DiagnosticName: llvm-avoid-cast-in-conditional FileOffset: 9492 FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h Message: method 'sexpr' is called twice and could be expensive Replacements:
  • FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h Length: 39 Offset: 9492 ReplacementText: dyn_cast_or_null<til::Undefined>(sexpr())
• DiagnosticName: llvm-avoid-cast-in-conditional FileOffset: 9572 FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h Message: method 'sexpr' is called twice and could be expensive Replacements:
  • FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h Length: 38 Offset: 9572 ReplacementText: dyn_cast_or_null<til::Wildcard>(sexpr())

[hintonda]

• Defer HeaderFileExtionsUtils.h bugfix to separate patch.
• Simplify code per comments.

[hintonda]

clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp

19

@aaron.ballman: This matcher seems genuinely useful. What do you think about moving it to ASTMatchers.h?

[hintonda]

• Renamed and updated docs.
• Fix formatting.

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-isa-or-dyn-cast-in-conditionals.rst

6 ↗

(On Diff #193376)

Indentation is not needed here. Same for following lines.

[hintonda]

• Remove spaces in docs.

[aaron.ballman]

clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp

19

I think that adding something like this might be a good idea. We don't have any notion of source locations in the AST matching syntax currently, and I'm not certain whether that's a good thing or not here. I'm mildly uncomfortable that this matcher operates on an Expr but then internally uses a source location from that expression and I wonder if we would rather introduce source location matching. For instance, what if the user cares about the difference between getExprLoc() and getBeginLoc() for some reason?

[aaron.ballman]

clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp

146

/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/SemaTemplate.cpp Message: method 'getAsTemplateDecl' is called twice and could be expensive Replacements:

This one is a false positive and the fix is incorrect. The original code is:

Diag(TemplateNameLoc, diag::err_not_class_template_specialization)
  << (Name.getAsTemplateDecl() &&
      isa<TemplateTemplateParmDecl>(Name.getAsTemplateDecl()));

The rest look to be plausible true positives, but not a single case looks like there is much expense involved in the expression, and honestly, several of the replacement expressions would feel a bit "worse" to me, despite being equivalent. For instance, code like: bool Foo = bar && isa<T>(bar); reads more clearly to me than bool Foo = dyn_cast_or_null<T>(bar); because the former looks like a bool initializer while the latter looks like the user meant to write T *Foo instead of bool Foo.

I think this part of the check is contentious and I'd rather leave it out for now so that we can get the rest of the check in. I'd be especially curious to know whether the community would prefer to see us leave this situation alone, introduce isa_or_null<T>(), or use dyn_cast_or_null<T>().

[hintonda]

clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp

146

How is that a false positive? In

Diag(TemplateNameLoc, diag::err_not_class_template_specialization)
  << (Name.getAsTemplateDecl() &&
      isa<TemplateTemplateParmDecl>(Name.getAsTemplateDecl()));

Name.getAsTemplateDecl() is called twice. First to see if returned nullptr or not, then to see if the pointer satisfied isa<TemplateTemplateParmDecl>(Name.getAsTemplateDecl()).

As for whether or not it's worth having, I'll defer to you. Most cases are relatively cheap, but it can be expensive. I'll go back and rerun it on all of clang+llvm and find the expensive ones if you're interested, but it takes a really long time to run on my laptop, so let me know what you'd prefer.

[hintonda]

clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp

146

Btw, I'll draft a message to the list and ask about isa_or_null, et al, in the morning. I didn't want to add stuff like that in this patch, but would be happy to use it if I got the green light to add it in another.

Thanks again for the feedback -- especially on the matchers.

[hintonda]

clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp

19

Well, you can attach it to whatever you want, so I'm not sure that's a problem. Alternatively, you have to check each location yourself. In my case, that was multiple places, so putting it in the matcher cleaned up the code.

I need to verify it, but it seems that it triggered when any macros were in the range, but I need to look closer into that to understand the behavior. I'll check it out and get back to you.

[aaron.ballman]

clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp

146

How is that a false positive?

Sorry! You are correct, I was being imprecise. I meant that the fix here is definitely bad and it demonstrates one of the things I was concerned about for the check. I also meant that the diagnostic firing here is suggesting that this code is wrong when in fact, naively conforming to the check would make the code incorrect. One possible solution to this is for the check to also include static_cast<bool>(<expr>), but I'm not convinced that's great either.

As for whether or not it's worth having, I'll defer to you. Most cases are relatively cheap, but it can be expensive. I'll go back and rerun it on all of clang+llvm and find the expensive ones if you're interested, but it takes a really long time to run on my laptop, so let me know what you'd prefer.

I went through all of the places in the list you posted above and did not see a single case where this would make a lot of difference in terms of performance. In every case it's a function call to a simple accessor, effectively.

146

Btw, I'll draft a message to the list and ask about isa_or_null, et al, in the morning. I didn't want to add stuff like that in this patch, but would be happy to use it if I got the green light to add it in another.

Yup, agreed! I think the best approach right now is to see if isa_or_null<> gets some traction before moving forward with this part of the patch. You can either stall this patch until that discussion looks like it's getting consensus, or you can remove the functionality from this patch and land it sooner, then add it back in if isa_or_null<> gets adopted.

Thanks again for the feedback -- especially on the matchers.

My pleasure!

[aaron.ballman]

clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp

19

Well, you can attach it to whatever you want, so I'm not sure that's a problem.

How does a caller of this AST matcher indicate that they want it to match on Node.getBeingLoc().isMacroID() instead of what's written now? If the answer is "they write a different matcher", then that's what I think could be a problem. There's a lot of ways to get a source location (and a lot of different kinds of source locations), and I think matching on those can be very useful functionality. So I think the idea of the matcher is good, I'm just not certain whether we want it to look like AST_MATCHER(Expr, isMacroID); or AST_MATCHER(SourceLocation, isMacroID); or something else.

[hintonda]

clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp

19

I'll workup a separate patch with tests and examples, and see if I can allay your concerns.

[hintonda]

• Use new isa_and_nonnull, and cleanup code.

[hintonda]

Explicitly check for implicitCastExpr and add additional negative matching test.

[aaron.ballman]

clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp

125 ↗

(On Diff #194017)

How about 'isa_and_nonnull<>' is preferred over an explicit test for null followed by calling 'isa<>'?

clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.h

37–39 ↗

(On Diff #194017)

I think this comment is now out of date.

[hintonda]

clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.h

37–39 ↗

(On Diff #194017)

Thanks, I'll fix that.

I'll give the RFC a few more days, but not sure it's been definitive. A few no votes, but there are always some of those. I sorta like isa_nonnull<T> for brevity, so I'll mention that why I follow-up on that thread. Btw, the isa_and_nonnull<T> has been in for a week: r357761, and nothing on that thread leads me to believe I should revert it.

[hintonda]

• Update isa_and_nonnull diagnostic message, and fix typo.

[aaron.ballman]

LGTM aside from a nit and the ultimate name for the isa_and_nonnull<> API.

clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp

29–30

Please keep this list sorted alphabetically.

[hintonda]

In D59802#1467363, @aaron.ballman wrote:

LGTM aside from a nit and the ultimate name for the isa_and_nonnull<> API.

Thanks. I'll ping the list again at the end of the week, then make whatever change is necessary.

Btw, the current iteration replaces all v && isa<T>(v) instances with isa_and_nonnull<T>(v). Is that still okay, or should it just do method calls?

clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp

29–30

I think this is where add_new_check.py put it, but I'll alphabetize.

[hintonda]

• Alphabetize registration calls.

[aaron.ballman]

In D59802#1467430, @hintonda wrote:

In D59802#1467363, @aaron.ballman wrote:

LGTM aside from a nit and the ultimate name for the isa_and_nonnull<> API.

Thanks. I'll ping the list again at the end of the week, then make whatever change is necessary.

Btw, the current iteration replaces all v && isa<T>(v) instances with isa_and_nonnull<T>(v). Is that still okay, or should it just do method calls?

I think it's fine to replace all instances. We can relax the check if it seems like onerous behavior in practice.

clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp

29–30

Huh, I thought that script kept everything alphabetized? That may be worth investigating (don't feel obligated though).

[hintonda]

clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp

29–30

I'm sure it's something I did inadvertently, but can't really remember the details.

[hintonda]

clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp

29–30

Looks like rename_check.py definitely does not alphabetize. I'll try to address than in another patch. Thanks for pointing this out.

[hintonda]

@aaron.ballman, I just ran it over llvm/lib, including all in-tree headers, and it seems to work fine. However, it did miss this one:

• if (V && isa<Instruction>(V) && (EntInst = cast<Instruction>(V)) &&

+ if (isa_and_nonnull<Instruction>(V) && (EntInst = cast<Instruction>(V)) &&

It got the first, but not the second. Not sure how to pick that one up. Even ran it a second time on just that file, but still didn't pick it up. Any ideas?

[aaron.ballman]

In D59802#1474300, @hintonda wrote:

@aaron.ballman, I just ran it over llvm/lib, including all in-tree headers, and it seems to work fine. However, it did miss this one:

• if (V && isa<Instruction>(V) && (EntInst = cast<Instruction>(V)) &&

+ if (isa_and_nonnull<Instruction>(V) && (EntInst = cast<Instruction>(V)) &&

It got the first, but not the second. Not sure how to pick that one up. Even ran it a second time on just that file, but still didn't pick it up. Any ideas?

I don't think it's a critical case to cover for the check, but yeah, it looks like that code really wants to be (EntInst = dyn_cast_or_null<Instruction>(V)). I think that looking for a pattern to handle this case would be tricky though and given how infrequent it seems to show up in the code base, I am not too worried. Someone applying the check will still get a notice for the Var && isa<Type>(Var) part of the expression, so they can hopefully see there's more related cleanup to be done.

[hintonda]

In D59802#1475596, @aaron.ballman wrote:

In D59802#1474300, @hintonda wrote:

@aaron.ballman, I just ran it over llvm/lib, including all in-tree headers, and it seems to work fine. However, it did miss this one:

• if (V && isa<Instruction>(V) && (EntInst = cast<Instruction>(V)) &&

+ if (isa_and_nonnull<Instruction>(V) && (EntInst = cast<Instruction>(V)) &&

It got the first, but not the second. Not sure how to pick that one up. Even ran it a second time on just that file, but still didn't pick it up. Any ideas?

I don't think it's a critical case to cover for the check, but yeah, it looks like that code really wants to be (EntInst = dyn_cast_or_null<Instruction>(V)). I think that looking for a pattern to handle this case would be tricky though and given how infrequent it seems to show up in the code base, I am not too worried. Someone applying the check will still get a notice for the Var && isa<Type>(Var) part of the expression, so they can hopefully see there's more related cleanup to be done.

Okay, thanks for looking. I'll go ahead and land this today, and if I think of a good way to handle this case, I'll update it later.

Thanks again for all your help...

clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.h

+//===--- AvoidCastInConditionalCheck.h - clang-tidy -------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_AVOIDCASTINCONDITIONALCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_AVOIDCASTINCONDITIONALCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace llvm {
+
+/// \brief Finds cases of cast<> used in conditional statements which will
+/// assert on failure in Debug builds.
+///
+/// Finds cases like these:
+/// \code
+///   if (auto x = cast<X>(y)) <...>
+///   if (cast<X>(y)) <...>
+/// \endcode
+/// But not cases like these:
+/// \code
+///   if (auto f = cast<Z>(y)->foo()) <...>
+///   if (cast<Z>(y)->foo()) <...>
+/// \endcode
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/llvm-avoid-cast-in-conditional.html
+class AvoidCastInConditionalCheck : public ClangTidyCheck {
+public:
+  AvoidCastInConditionalCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace llvm
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_AVOIDCASTINCONDITIONALCHECK_H

clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp

+//===--- AvoidCastInConditionalCheck.cpp - clang-tidy ---------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "AvoidCastInConditionalCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace llvm {
+
+void AvoidCastInConditionalCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      ifStmt(anyOf(
+          hasConditionVariableStatement(declStmt(containsDeclaration(
+              0, varDecl(hasInitializer(
+                     callExpr(callee(namedDecl(hasName("cast"))))
+                         .bind("cast-call")))))),
+          has(implicitCastExpr(has(callExpr(callee(namedDecl(hasName("cast"))))
+                                       .bind("cast-call")))))),
+      this);
+}
+
+void AvoidCastInConditionalCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  if (const auto *MatchedDecl = Result.Nodes.getNodeAs<CallExpr>("cast-call")) {
+    diag(MatchedDecl->getBeginLoc(),
+         "Found cast<> in conditional statement; consider using dyn_cast<> if "
+         "it can fail, or removal of conditional if it can't.");
+    ;
+  }
+}
+
+} // namespace llvm
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/llvm/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyLLVMModule
+  AvoidCastInConditionalCheck.cpp
   HeaderGuardCheck.cpp
   IncludeOrderCheck.cpp
   LLVMTidyModule.cpp
   TwineLocalCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyReadabilityModule
   clangTidyUtils
   clangTooling
   )

clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp

 //===--- LLVMTidyModule.cpp - clang-tidy ----------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../readability/NamespaceCommentCheck.h"
+#include "AvoidCastInConditionalCheck.h"
 #include "HeaderGuardCheck.h"
 #include "IncludeOrderCheck.h"
 #include "TwineLocalCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace llvm {
 
 class LLVMModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<AvoidCastInConditionalCheck>(
+        "llvm-avoid-cast-in-conditional");
     CheckFactories.registerCheck<LLVMHeaderGuardCheck>("llvm-header-guard");
     CheckFactories.registerCheck<IncludeOrderCheck>("llvm-include-order");
     CheckFactories.registerCheck<readability::NamespaceCommentCheck>(
         "llvm-namespace-comment");
     CheckFactories.registerCheck<TwineLocalCheck>("llvm-twine-local");
   }
 };
 
 // Register the LLVMTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<LLVMModule> X("llvm-module",
                                                   "Adds LLVM lint checks.");
 
 } // namespace llvm
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the LLVMModule.
 volatile int LLVMModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h

 //===--- HeaderFileExtensionsUtils.h - clang-tidy----------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_HEADER_FILE_EXTENSIONS_UTILS_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_HEADER_FILE_EXTENSIONS_UTILS_H
 
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringRef.h"
 
 namespace clang {
 namespace tidy {
 namespace utils {
 
-typedef llvm::SmallSet<llvm::StringRef, 5> HeaderFileExtensionsSet;
+using HeaderFileExtensionsSet = SmallSet<StringRef, 5>;
 
 /// \brief Checks whether expansion location of \p Loc is in header file.
 bool isExpansionLocInHeaderFile(
     SourceLocation Loc, const SourceManager &SM,
     const HeaderFileExtensionsSet &HeaderFileExtensions);
 
 /// \brief Checks whether presumed location of \p Loc is in header file.
 bool isPresumedLocInHeaderFile(

clang-tools-extra/docs/ReleaseNotes.rst

 - The `Acronyms` and `IncludeDefaultAcronyms` options for the
   :doc:`objc-property-declaration <clang-tidy/checks/objc-property-declaration>`
   check have been removed.
 
 - The :doc:`modernize-use-override
   <clang-tidy/checks/modernize-use-override>` now supports `OverrideSpelling`
   and `FinalSpelling` options.
 
+- New :doc:`llvm-avoid-cast-in-conditional
+  <clang-tidy/checks/llvm-avoid-cast-in-conditional>` check.
+
+  Finds cases of ``cast<>`` used as condition in conditional statements
+  which will assert on failure in Debug builds.
+
 - New :doc:`openmp-exception-escape
   <clang-tidy/checks/openmp-exception-escape>` check.
 
   Analyzes OpenMP Structured Blocks and checks that no exception escapes
   out of the Structured Block it was thrown in.
 
 - New :doc:`openmp-use-default-none
   <clang-tidy/checks/openmp-use-default-none>` check.

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. toctree::
    hicpp-use-auto (redirects to modernize-use-auto) <hicpp-use-auto>
    hicpp-use-emplace (redirects to modernize-use-emplace) <hicpp-use-emplace>
    hicpp-use-equals-default (redirects to modernize-use-equals-default) <hicpp-use-equals-default>
    hicpp-use-equals-delete (redirects to modernize-use-equals-delete) <hicpp-use-equals-delete>
    hicpp-use-noexcept (redirects to modernize-use-noexcept) <hicpp-use-noexcept>
    hicpp-use-nullptr (redirects to modernize-use-nullptr) <hicpp-use-nullptr>
    hicpp-use-override (redirects to modernize-use-override) <hicpp-use-override>
    hicpp-vararg (redirects to cppcoreguidelines-pro-type-vararg) <hicpp-vararg>
+   llvm-avoid-cast-in-conditional
    llvm-header-guard
    llvm-include-order
    llvm-namespace-comment
    llvm-twine-local
    misc-definitions-in-headers
    misc-misplaced-const
    misc-new-delete-overloads
    misc-non-copyable-objects

clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst

+.. title:: clang-tidy - llvm-avoid-cast-in-conditional
+
+llvm-avoid-cast-in-conditional
+==============================
+
+Finds cases of ``cast<>`` used as the condition of a conditional
+statements which will assert on failure in Debug builds. The use of
+cast<> implies that the operation cannot fail, and should not be used
+as the condition of a conditional statement.
+
+.. code-block:: c++
+
+  // Finds cases like these:
+  if (auto x = cast<X>(y)) <...>
+  if (cast<X>(y)) <...>
+
+  // But not cases like these:
+  if (auto f = cast<Z>(y)->foo()) <...>
+  if (cast<Z>(y)->foo()) <...>

clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp

+// RUN: %check_clang_tidy %s llvm-avoid-cast-in-conditional %t
+
+struct X;
+struct Y;
+struct Z {
+  int foo();
+};
+
+template <class X, class Y>
+X* cast(Y*);
+
+bool foo(Y* y) {
+  if (auto x = cast<X>(y))
+    // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: {{.*llvm-avoid-cast-in-conditional}}
+    return true;
+  if (cast<X>(y))
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning:  {{.*llvm-avoid-cast-in-conditional}}
+    return true;
+
+  // Does not trigger warning.
+  if (cast<Z>(y)->foo())
+    return true;
+  return false;
+}
+

clang/include/clang/Basic/LLVM.h

 namespace llvm {
   // ADT's.
   class StringRef;
   class Twine;
   class VersionTuple;
   template<typename T> class ArrayRef;
   template<typename T> class MutableArrayRef;
   template<typename T> class OwningArrayRef;
+  template <typename T, unsigned N, typename C> class SmallSet;
   template<unsigned InternalLen> class SmallString;
   template<typename T, unsigned N> class SmallVector;
   template<typename T> class SmallVectorImpl;
   template<typename T> class Optional;
   template <class T> class Expected;
 
   template<typename T>
   struct SaveAndRestore;
namespace clang {
 
   // ADT's.
   using llvm::ArrayRef;
   using llvm::MutableArrayRef;
   using llvm::None;
   using llvm::Optional;
   using llvm::OwningArrayRef;
   using llvm::SaveAndRestore;
+  using llvm::SmallSet;
   using llvm::SmallString;
   using llvm::SmallVector;
   using llvm::SmallVectorImpl;
   using llvm::StringRef;
   using llvm::Twine;
   using llvm::VersionTuple;
 
   // Error handling.