[clang-tidy] Update llvm-prefer-isa-or-dyn-cast-in-conditionals with new syntax

[njames93]

Summary

In D123901, the or_null, and_nonnull templates were deprecated intended to be replaced with if_present and and_present.
In light of this, The clang-tidy check that enforces correct use of isa and dyn_cast should also be updated with this syntax.

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

322

Please use double back-ticks.

[njames93]

Fix documentation.

[bzcheeseman]

This is great, thank you for doing this! I'm not a competent reviewer for the actual clang-tidy code changes but the +1 for the idea :)

[whisperity]

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

12–13

Will the tests pass properly once the fixes are applied, even though the replaced code refers to a symbol (isa_and_present) that is not declared in the TU?

[whisperity]

In D131319#3708667, @bzcheeseman wrote:

This is great, thank you for doing this! I'm not a competent reviewer for the actual clang-tidy code changes but the +1 for the idea :)

The problem with the approval here is that a single approval will turn the patch into a fully approved state, which breaks the dashboards for people added to the patch (i.e., other reviewers will think the patch is already approved, and thus perhaps will not consider putting effort into reviewing it!).

However, I think you should try the Award Token option from the menu on the right! Somewhere the awarded tokens should show up on the patch, tallying up support!

[bzcheeseman]

In D131319#3709671, @whisperity wrote:

In D131319#3708667, @bzcheeseman wrote:

This is great, thank you for doing this! I'm not a competent reviewer for the actual clang-tidy code changes but the +1 for the idea :)

The problem with the approval here is that a single approval will turn the patch into a fully approved state, which breaks the dashboards for people added to the patch (i.e., other reviewers will think the patch is already approved, and thus perhaps will not consider putting effort into reviewing it!).

However, I think you should try the Award Token option from the menu on the right! Somewhere the awarded tokens should show up on the patch, tallying up support!

Ah I had no idea, thanks for pointing that out. I was looking at the Code Review document (https://llvm.org/docs/CodeReview.html), I'll put up a patch to add a short section on this.

[njames93]

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

12–13

The current tests would fail if ran once the fixes are applied as isa_and_nonnull doesn't exist in this TU. Most tidy checks (for better or worse) don't check for existence of an identifier that is expected to be there when making replacements.

[njames93]

Added dynamic checking for existence of isa_and_present before suggesting it as a replacement

[njames93]

Fix wrong patch upload

[PiotrZSL]

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

37

allOf is redudant

44–51

no need for allOf, just put those unless directly under callExpr

45

this also doesn't look to be needed..

45

maybe check argument count first argumentCountIs(1)
it could be cheaper than hasAnyName

55

Let me quote You: "Rather than calling traverse, the canoncial way to handle this is by overriding the getCheckTraversalKind virtual function to return TK_IgnoreUnlessSpelledInSource"
In this case just return TK_AsIs.

57

Change name of this any, on first look I mistook this for a anything()

59

no need for allOf, all those matchers are variadic...

63–64

I think that hasRHS(ignoringImpCasts(CallExpression)) should do a trick.

71

This is not a Decl but an Expr

126–127

Maybe MatchedDecl->getSourceRange () ?

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

61 ↗

(On Diff #512269)

no need for mutable, its used only form Check that isn't const.
Maybe better HasIsPresentCache ?

[njames93]

@PiotrZSL Thank you for your review, however we tend to only comment on code that has been changed by the patch. You have made some good points about parts of this check, but they can be addressed in an NFC commit.

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

61 ↗

(On Diff #512269)

Good spot.

[carlosgalvezp]

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

61 ↗

(On Diff #512269)

Comment not yet addressed, do you intend to?

clang-tools-extra/docs/ReleaseNotes.rst

323

Would it make more sense to point to an actual commit in the repo? A review may or may not have been merged.

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

2

Typo

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

auto Condition = hasCondition(implicitCastExpr(has(
                 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(unless(isMacroID()), unless(cxxMemberCallExpr()),
                 allOf(callee(namedDecl(hasAnyName("isa", "cast", "cast_or_null",
-                                                "dyn_cast", "dyn_cast_or_null"))
+                                                  "cast_if_present", "dyn_cast",
+                                                  "dyn_cast_or_null",
+                                                  "dyn_cast_if_present"))
                                  .bind("func")),
-                    hasArgument(
-                        0,
-                        mapAnyOf(declRefExpr, cxxMemberCallExpr).bind("arg")))))
+                      hasArgument(0, mapAnyOf(declRefExpr, cxxMemberCallExpr)
+                                         .bind("arg")))))
           .bind("rhs");
 
   Finder->addMatcher(
       traverse(TK_AsIs,
                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");
if (const auto *MatchedDecl = Result.Nodes.getNodeAs<CallExpr>("assign")) {
 
     if (ArgString != LHSString)
       return;
 
     StringRef RHSString(Lexer::getSourceText(
         CharSourceRange::getTokenRange(RHS->getSourceRange()),
         *Result.SourceManager, getLangOpts()));
 
-    std::string Replacement("isa_and_nonnull");
+    std::string Replacement("isa_and_present");
     Replacement += RHSString.substr(Func->getName().size());
 
     diag(MatchedDecl->getBeginLoc(),
-         "isa_and_nonnull<> is preferred over an explicit test for null "
+         "isa_and_present<> is preferred over an explicit test for null "
          "followed by calling isa<>")
         << FixItHint::CreateReplacement(SourceRange(MatchedDecl->getBeginLoc(),
                                                     MatchedDecl->getEndLoc()),
                                         Replacement);
   }
 }
 
 } // namespace tidy::llvm_check
 } // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

- Improved :doc:`modernize-concat-nested-namespaces
   <clang-tidy/checks/modernize/concat-nested-namespaces>` to fix incorrect fixes when
   using macro between namespace declarations and false positive when using namespace
   with attributes.
 
 - Fixed a false positive in :doc:`performance-no-automatic-move
   <clang-tidy/checks/performance/no-automatic-move>` when warning would be
   emitted for a const local variable to which NRVO is applied.
 
+- Updated :doc:`llvm-prefer-isa-or-dyn-cast-in-conditionals
+  <clang-tidy/checks/llvm/prefer-isa-or-dyn-cast-in-conditionals>` check to
+  use the ``*and_present`` and ``*if_present`` templates added in 
+  `D123901 <https://reviews.llvm.org/D123901>`_.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

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

.. code-block:: c++
   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())) {}
+  if (isa_and_present<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/checkers/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 *);
+template <class X, class Y>
+X *dyn_cast_if_present(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))
bool foo(Y *y, Z *z) {
   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))
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: isa_and_present<> 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_present<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()))
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  isa_and_present<> is preferred
+  // CHECK-FIXES: if (isa_and_present<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()))
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: isa_and_present<> is preferred
+  // CHECK-FIXES: if (isa_and_present<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()))
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: isa_and_present<> is preferred
+  // CHECK-FIXES: if (isa_and_present<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()))
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: isa_and_present<> is preferred
+  // CHECK-FIXES: if (isa_and_present<Y>(z->bar()))
+
+  if (z->bar() && dyn_cast_if_present<Y>(z->bar()))
+    return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: isa_and_present<> is preferred
+  // CHECK-FIXES: if (isa_and_present<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());
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: isa_and_present<> is preferred
+  // CHECK-FIXES: bool b = isa_and_present<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;