[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

23 ↗

(On Diff #192214)

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

6 ↗

(On Diff #192214)

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

7 ↗

(On Diff #192214)

Please fix double space.

9 ↗

(On Diff #192214)

Please fix double dot.

[hintonda]

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

23 ↗

(On Diff #192214)

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

23 ↗

(On Diff #192214)

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 ↗

(On Diff #192234)

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 ↗

(On Diff #192234)

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 ↗

(On Diff #192234)

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 ↗

(On Diff #192238)

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

35–36 ↗

(On Diff #192238)

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 ↗

(On Diff #192238)

Spurious semicolon?

[Eugene.Zelenko]

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

8 ↗

(On Diff #192238)

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

73 ↗

(On Diff #192756)

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

74 ↗

(On Diff #192756)

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

82 ↗

(On Diff #192756)

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

83 ↗

(On Diff #192756)

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

90 ↗

(On Diff #192756)

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

91 ↗

(On Diff #192756)

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

101 ↗

(On Diff #192756)

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

104 ↗

(On Diff #192756)

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

107 ↗

(On Diff #192756)

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

[hintonda]

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

73 ↗

(On Diff #192756)

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

38 ↗

(On Diff #192778)

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

60 ↗

(On Diff #192778)

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()

62–64 ↗

(On Diff #192778)

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

90–91 ↗

(On Diff #192778)

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.

96 ↗

(On Diff #192778)

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)

112–116 ↗

(On Diff #192778)

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

145 ↗

(On Diff #192778)

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

73 ↗

(On Diff #192756)

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".

[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 ↗

(On Diff #192778)

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

145 ↗

(On Diff #192778)

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

38 ↗

(On Diff #192778)

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

145 ↗

(On Diff #192778)

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.

103–104 ↗

(On Diff #192939)

Spurious code that can be removed?

118 ↗

(On Diff #192939)

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

125–126 ↗

(On Diff #192939)

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

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

21 ↗

(On Diff #192939)

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

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

22 ↗

(On Diff #192939)

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.

48 ↗

(On Diff #192939)

Same here.

64 ↗

(On Diff #192939)

and here

[hintonda]

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

21 ↗

(On Diff #192939)

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 ↗

(On Diff #192939)

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 ↗

(On Diff #192939)

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

125–126 ↗

(On Diff #192939)

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

145 ↗

(On Diff #192778)

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

18 ↗

(On Diff #193176)

@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

7

Indentation is not needed here. Same for following lines.

[hintonda]

• Remove spaces in docs.

[aaron.ballman]

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

18 ↗

(On Diff #193176)

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

145 ↗

(On Diff #192778)

/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

145 ↗

(On Diff #192778)

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

145 ↗

(On Diff #192778)

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

18 ↗

(On Diff #193176)

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

145 ↗

(On Diff #192778)

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.

145 ↗

(On Diff #192778)

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

18 ↗

(On Diff #193176)

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

18 ↗

(On Diff #193176)

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

126

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

38–40

I think this comment is now out of date.

[hintonda]

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

38–40

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

27–28

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

27–28

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

27–28

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

27–28

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

27–28

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/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyLLVMModule
   HeaderGuardCheck.cpp
   IncludeOrderCheck.cpp
   LLVMTidyModule.cpp
+  PreferIsaOrDynCastInConditionalsCheck.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 "HeaderGuardCheck.h"
 #include "IncludeOrderCheck.h"
+#include "PreferIsaOrDynCastInConditionalsCheck.h"
 #include "TwineLocalCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace llvm {
 
 class LLVMModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<LLVMHeaderGuardCheck>("llvm-header-guard");
     CheckFactories.registerCheck<IncludeOrderCheck>("llvm-include-order");
+    CheckFactories.registerCheck<PreferIsaOrDynCastInConditionalsCheck>(
+        "llvm-prefer-isa-or-dyn-cast-in-conditionals");
     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",

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

+//===--- PreferIsaOrDynCastInConditionalsCheck.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 "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace llvm {
+
+/// \brief Looks at conditionals and finds and replaces cases of ``cast<>``, which will
+/// assert rather than return a null pointer, and ``dyn_cast<>`` where
+/// the return value is not captured.  Additionally, finds and replaces cases that match the
+/// pattern ``var && isa<X>(var)``, where ``var`` is evaluated twice.
+///
+/// Finds cases like these:
+/// \code
+///  if (auto x = cast<X>(y)) {}
+///  // is replaced by:
+///  if (auto x = dyn_cast<X>(y)) {}
+///
+///  if (cast<X>(y)) {}
+///  // is replaced by:
+///  if (isa<X>(y)) {}
+///
+///  if (dyn_cast<X>(y)) {}
+///  // is replaced by:
+///  if (isa<X>(y)) {}
+///
+///  if (var && isa<T>(var)) {}
+///  // is replaced by:
+///  if (isa_and_nonnull<T>(var.foo())) {}
+/// \endcode
+///
+///  // Other cases are ignored, e.g.:
+/// \code
+///  if (auto f = cast<Z>(y)->foo()) {}
+///  if (cast<Z>(y)->foo()) {}
+///  if (X.cast(y)) {}
+/// \endcode
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/llvm-prefer-isa-or-dyn-cast-in-conditionals.html
+class PreferIsaOrDynCastInConditionalsCheck : public ClangTidyCheck {
+public:
+  PreferIsaOrDynCastInConditionalsCheck(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/PreferIsaOrDynCastInConditionalsCheck.cpp

+//===--- PreferIsaOrDynCastInConditionalsCheck.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 "PreferIsaOrDynCastInConditionalsCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace ast_matchers {
+AST_MATCHER(Expr, isMacroID) { return Node.getExprLoc().isMacroID(); }
+} // namespace ast_matchers
+
+namespace tidy {
+namespace llvm {
+
+void PreferIsaOrDynCastInConditionalsCheck::registerMatchers(
+    MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  auto Condition = hasCondition(implicitCastExpr(has(
+      callExpr(
+          allOf(unless(isMacroID()), unless(cxxMemberCallExpr()),
+                anyOf(callee(namedDecl(hasName("cast"))),
+                      callee(namedDecl(hasName("dyn_cast")).bind("dyn_cast")))))
+          .bind("call"))));
+
+  auto Any = anyOf(
+      has(declStmt(containsDeclaration(
+          0,
+          varDecl(hasInitializer(
+              callExpr(allOf(unless(isMacroID()), unless(cxxMemberCallExpr()),
+                             callee(namedDecl(hasName("cast")))))
+                  .bind("assign")))))),
+      Condition);
+
+  auto CallExpression =
+      callExpr(
+          allOf(unless(isMacroID()), unless(cxxMemberCallExpr()),
+                allOf(callee(namedDecl(anyOf(hasName("isa"), hasName("cast"),
+                                             hasName("cast_or_null"),
+                                             hasName("dyn_cast"),
+                                             hasName("dyn_cast_or_null")))
+                                 .bind("func")),
+                      hasArgument(0, anyOf(declRefExpr().bind("arg"),
+                                           cxxMemberCallExpr().bind("arg"))))))
+          .bind("rhs");
+
+  Finder->addMatcher(
+      stmt(anyOf(ifStmt(Any), whileStmt(Any), doStmt(Condition),
+                 binaryOperator(
+                     allOf(unless(isExpansionInFileMatching(
+                               "llvm/include/llvm/Support/Casting.h")),
+                           hasOperatorName("&&"),
+                           hasLHS(implicitCastExpr().bind("lhs")),
+                           hasRHS(anyOf(implicitCastExpr(has(CallExpression)),
+                                        CallExpression))))
+                     .bind("and"))),
+      this);
+}
+
+void PreferIsaOrDynCastInConditionalsCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  if (const auto *MatchedDecl = Result.Nodes.getNodeAs<CallExpr>("assign")) {
+    SourceLocation StartLoc = MatchedDecl->getCallee()->getExprLoc();
+    SourceLocation EndLoc =
+        StartLoc.getLocWithOffset(StringRef("cast").size() - 1);
+
+    diag(MatchedDecl->getBeginLoc(),
+         "cast<> in conditional will assert rather than return a null pointer")
+        << FixItHint::CreateReplacement(SourceRange(StartLoc, EndLoc),
+                                        "dyn_cast");
+  } else if (const auto *MatchedDecl =
+                 Result.Nodes.getNodeAs<CallExpr>("call")) {
+    SourceLocation StartLoc = MatchedDecl->getCallee()->getExprLoc();
+    SourceLocation EndLoc =
+        StartLoc.getLocWithOffset(StringRef("cast").size() - 1);
+
+    StringRef Message =
+        "cast<> in conditional will assert rather than return a null pointer";
+    if (Result.Nodes.getNodeAs<NamedDecl>("dyn_cast"))
+      Message = "return value from dyn_cast<> not used";
+
+    diag(MatchedDecl->getBeginLoc(), Message)
+        << FixItHint::CreateReplacement(SourceRange(StartLoc, EndLoc), "isa");
+  } else if (const auto *MatchedDecl =
+                 Result.Nodes.getNodeAs<BinaryOperator>("and")) {
+    const auto *LHS = Result.Nodes.getNodeAs<ImplicitCastExpr>("lhs");
+    const auto *RHS = Result.Nodes.getNodeAs<CallExpr>("rhs");
+    const auto *Arg = Result.Nodes.getNodeAs<Expr>("arg");
+    const auto *Func = Result.Nodes.getNodeAs<NamedDecl>("func");
+
+    assert(LHS && "LHS is null");
+    assert(RHS && "RHS is null");
+    assert(Arg && "Arg is null");
+    assert(Func && "Func is null");
+
+    StringRef LHSString(Lexer::getSourceText(
+        CharSourceRange::getTokenRange(LHS->getSourceRange()),
+        *Result.SourceManager, getLangOpts()));
+
+    StringRef ArgString(Lexer::getSourceText(
+        CharSourceRange::getTokenRange(Arg->getSourceRange()),
+        *Result.SourceManager, getLangOpts()));
+
+    if (ArgString != LHSString)
+      return;
+
+    StringRef RHSString(Lexer::getSourceText(
+        CharSourceRange::getTokenRange(RHS->getSourceRange()),
+        *Result.SourceManager, getLangOpts()));
+
+    std::string Replacement("isa_and_nonnull");
+    Replacement += RHSString.substr(Func->getName().size());
+
+    diag(MatchedDecl->getBeginLoc(),
+         "isa_and_nonnull<> is preferred over an explicit test for null "
+         "followed by calling isa<>")
+        << FixItHint::CreateReplacement(SourceRange(MatchedDecl->getBeginLoc(),
+                                                    MatchedDecl->getEndLoc()),
+                                        Replacement);
+  }
+}
+
+} // namespace llvm
+} // namespace tidy
+} // namespace clang

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-prefer-isa-or-dyn-cast-in-conditionals
+  <clang-tidy/checks/llvm-prefer-isa-or-dyn-cast-in-conditionals>` check.
+
+  Looks at conditionals and finds and replaces cases of ``cast<>``,
+  which will assert rather than return a null pointer, and
+  ``dyn_cast<>`` where the return value is not captured. Additionally,
+  finds and replaces cases that match the pattern ``var &&
+  isa<X>(var)``, where ``var`` is evaluated twice.
+
 - 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-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-header-guard
    llvm-include-order
    llvm-namespace-comment
+   llvm-prefer-isa-or-dyn-cast-in-conditionals
    llvm-twine-local
    misc-definitions-in-headers
    misc-misplaced-const
    misc-new-delete-overloads
    misc-non-copyable-objects
    misc-non-private-member-variables-in-classes
    misc-redundant-expression
    misc-static-assert

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

+.. title:: clang-tidy - llvm-prefer-isa-or-dyn-cast-in-conditionals
+
+llvm-prefer-isa-or-dyn-cast-in-conditionals
+===========================================
+
+Looks at conditionals and finds and replaces cases of ``cast<>``,
+which will assert rather than return a null pointer, and
+``dyn_cast<>`` where the return value is not captured. Additionally,
+finds and replaces cases that match the pattern ``var &&
+isa<X>(var)``, where ``var`` is evaluated twice.
+
+.. code-block:: c++
+
+  // Finds these:
+  if (auto x = cast<X>(y)) {}
+  // is replaced by:
+  if (auto x = dyn_cast<X>(y)) {}
+
+  if (cast<X>(y)) {}
+  // is replaced by:
+  if (isa<X>(y)) {}
+
+  if (dyn_cast<X>(y)) {}
+  // is replaced by:
+  if (isa<X>(y)) {}
+
+  if (var && isa<T>(var)) {}
+  // is replaced by:
+  if (isa_and_nonnull<T>(var.foo())) {}
+
+  // Other cases are ignored, e.g.:
+  if (auto f = cast<Z>(y)->foo()) {}
+  if (cast<Z>(y)->foo()) {}
+  if (X.cast(y)) {}

clang-tools-extra/test/clang-tidy/llvm-prefer-isa-or-dyn-cast-in-conditionals.cpp

+// RUN: %check_clang_tidy %s llvm-prefer-isa-or-dyn-cast-in-conditionals %t
+
+struct X;
+struct Y;
+struct Z {
+  int foo();
+  X *bar();
+  X *cast(Y*);
+  bool baz(Y*);
+};
+
+template <class X, class Y>
+bool isa(Y *);
+template <class X, class Y>
+X *cast(Y *);
+template <class X, class Y>
+X *dyn_cast(Y *);
+template <class X, class Y>
+X *dyn_cast_or_null(Y *);
+
+bool foo(Y *y, Z *z) {
+  if (auto x = cast<X>(y))
+    return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: cast<> in conditional will assert rather than return a null pointer [llvm-prefer-isa-or-dyn-cast-in-conditionals]
+  // CHECK-FIXES: if (auto x = dyn_cast<X>(y))
+
+  while (auto x = cast<X>(y))
+    break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:19: warning: cast<> in conditional
+  // CHECK-FIXES: while (auto x = dyn_cast<X>(y))
+
+  if (cast<X>(y))
+    return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: cast<> in conditional
+  // CHECK-FIXES: if (isa<X>(y))
+
+  while (cast<X>(y))
+    break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning: cast<> in conditional
+  // CHECK-FIXES: while (isa<X>(y))
+
+  do {
+    break;
+  } while (cast<X>(y));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: cast<> in conditional
+  // CHECK-FIXES: while (isa<X>(y));
+
+  if (dyn_cast<X>(y))
+    return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: return value from dyn_cast<> not used [llvm-prefer-isa-or-dyn-cast-in-conditionals]
+  // CHECK-FIXES: if (isa<X>(y))
+
+  while (dyn_cast<X>(y))
+    break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning: return value from dyn_cast<> not used
+  // CHECK-FIXES: while (isa<X>(y))
+
+  do {
+    break;
+  } while (dyn_cast<X>(y));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: return value from dyn_cast<> not used
+  // CHECK-FIXES: while (isa<X>(y));
+
+  if (y && isa<X>(y))
+    return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: isa_and_nonnull<> is preferred over an explicit test for null followed by calling isa<> [llvm-prefer-isa-or-dyn-cast-in-conditionals]
+  // CHECK-FIXES: if (isa_and_nonnull<X>(y))
+
+  if (z->bar() && isa<Y>(z->bar()))
+    return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  isa_and_nonnull<> is preferred
+  // CHECK-FIXES: if (isa_and_nonnull<Y>(z->bar()))
+
+  if (z->bar() && cast<Y>(z->bar()))
+    return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: isa_and_nonnull<> is preferred
+  // CHECK-FIXES: if (isa_and_nonnull<Y>(z->bar()))
+
+  if (z->bar() && dyn_cast<Y>(z->bar()))
+    return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: isa_and_nonnull<> is preferred
+  // CHECK-FIXES: if (isa_and_nonnull<Y>(z->bar()))
+
+  if (z->bar() && dyn_cast_or_null<Y>(z->bar()))
+    return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: isa_and_nonnull<> is preferred
+  // CHECK-FIXES: if (isa_and_nonnull<Y>(z->bar()))
+
+  bool b = z->bar() && cast<Y>(z->bar());
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: isa_and_nonnull<> is preferred
+  // CHECK-FIXES: bool b = isa_and_nonnull<Y>(z->bar());
+
+  // These don't trigger a warning.
+  if (auto x = cast<Z>(y)->foo())
+    return true;
+  if (auto x = z->cast(y))
+    return true;
+  while (auto x = cast<Z>(y)->foo())
+    break;
+  if (cast<Z>(y)->foo())
+    return true;
+  if (z->cast(y))
+    return true;
+  while (cast<Z>(y)->foo())
+    break;
+  if (y && cast<X>(z->bar()))
+    return true;
+  if (z && cast<Z>(y)->foo())
+    return true;
+  bool b2 = y && cast<X>(z);
+  if(z->cast(y))
+    return true;
+  if (z->baz(cast<Y>(z)))
+    return true;
+
+#define CAST(T, Obj) cast<T>(Obj)
+#define AUTO_VAR_CAST(X, Y, Z) auto X = cast<Y>(Z)
+#define ISA(T, Obj) isa<T>(Obj)
+#define ISA_OR_NULL(T, Obj) Obj &&isa<T>(Obj)
+
+  // Macros don't trigger warning.
+  if (auto x = CAST(X, y))
+    return true;
+  if (AUTO_VAR_CAST(x, X, z))
+    return true;
+  if (z->bar() && ISA(Y, z->bar()))
+    return true;
+  if (ISA_OR_NULL(Y, z->bar()))
+    return true;
+
+  return false;
+}