[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

13–14

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

13–14

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

39

allOf is redudant

46

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

47

this also doesn't look to be needed..

47

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

57

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.

59

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

61

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

65–66

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

96

This is not a Decl but an Expr

155–156

Maybe MatchedDecl->getSourceRange () ?

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

61

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

Good spot.

[carlosgalvezp]

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

61

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

1

Typo

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

public:
   PreferIsaOrDynCastInConditionalsCheck(StringRef Name,
                                         ClangTidyContext *Context)
       : ClangTidyCheck(Name, Context) {}
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
     return LangOpts.CPlusPlus;
   }
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  mutable std::optional<bool> HasIsPresent;
 };
 
 } // namespace clang::tidy::llvm_check
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_PREFERISAORDYNCASTINCONDITIONALSCHECK_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/AST/Decl.h"
+#include "clang/AST/DeclarationName.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace ast_matchers {
 AST_MATCHER(Expr, isMacroID) { return Node.getExprLoc().isMacroID(); }
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);
 }
 
+static bool FindIsaAndPresent(const ASTContext &ctx) {
+  auto LLVMIdent = ctx.Idents.find("llvm");
+  if (LLVMIdent == ctx.Idents.end())
+    return false;
+  auto IsaAndPresent = ctx.Idents.find("isa_and_present");
+  if (IsaAndPresent == ctx.Idents.end())
+    return false;
+  for (const NamedDecl *TopLevelLLVM :
+       ctx.getTranslationUnitDecl()->lookup(LLVMIdent->getValue())) {
+    const auto *NS = dyn_cast<NamespaceDecl>(TopLevelLLVM);
+    if (!NS)
+      continue;
+    for (const NamedDecl *IsaAndPresentDecl :
+         NS->lookup(IsaAndPresent->getValue())) {
+      if (isa<FunctionDecl, FunctionTemplateDecl>(IsaAndPresentDecl))
+        return true;
+    }
+  }
+  return false;
+}
+
 void PreferIsaOrDynCastInConditionalsCheck::check(
     const MatchFinder::MatchResult &Result) {
+  if (!HasIsPresent)
+    HasIsPresent = FindIsaAndPresent(*Result.Context);
   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");
-    Replacement += RHSString.substr(Func->getName().size());
+    StringRef ReplaceFunc =
+        *HasIsPresent ? "isa_and_present" : "isa_and_nonnull";
+
+    std::string Replacement =
+        (ReplaceFunc + RHSString.substr(Func->getName().size())).str();
 
     diag(MatchedDecl->getBeginLoc(),
-         "isa_and_nonnull<> is preferred over an explicit test for null "
+         "%0<> is preferred over an explicit test for null "
          "followed by calling isa<>")
+        << ReplaceFunc
         << 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
-
+// RDUN: %check_clang_tidy -check-suffixes=,NO-PRESENT %s llvm-prefer-isa-or-dyn-cast-in-conditionals %t
+// RUN: %check_clang_tidy -check-suffixes=,PRESENT %s llvm-prefer-isa-or-dyn-cast-in-conditionals %t -- -- -DISA_AND_PRESENT
 struct X;
 struct Y;
 struct Z {
   int foo();
   X *bar();
   X *cast(Y*);
   bool baz(Y*);
 };
 
+namespace llvm {
 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 *);
+#ifdef ISA_AND_PRESENT
+template <class X, class Y>
+bool isa_and_present(Y *);
+#endif
+} // namespace llvm
+using namespace llvm;
 
 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-PRESENT: :[[@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-PRESENT: if (isa_and_present<X>(y))
+  // CHECK-MESSAGES-NO-PRESENT: :[[@LINE-4]]: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-NO-PRESENT: 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()))
+  // CHECK-MESSAGES-PRESENT: :[[@LINE-2]]:7: warning:  isa_and_present<> is preferred
+  // CHECK-FIXES-PRESENT: if (isa_and_present<Y>(z->bar()))
+  // CHECK-MESSAGES-NO-PRESENT: :[[@LINE-4]]:7: warning:  isa_and_nonnull<> is preferred
+  // CHECK-FIXES-NO-PRESENT: 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()))
+  // CHECK-MESSAGES-PRESENT: :[[@LINE-2]]:7: warning: isa_and_present<> is preferred
+  // CHECK-FIXES-PRESENT: if (isa_and_present<Y>(z->bar()))
+  // CHECK-MESSAGES-NO-PRESENT: :[[@LINE-4]]:7: warning: isa_and_nonnull<> is preferred
+  // CHECK-FIXES-NO-PRESENT: 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()))
+  // CHECK-MESSAGES-PRESENT: :[[@LINE-2]]:7: warning: isa_and_present<> is preferred
+  // CHECK-FIXES-PRESENT: if (isa_and_present<Y>(z->bar()))
+  // CHECK-MESSAGES-NO-PRESENT: :[[@LINE-4]]:7: warning: isa_and_nonnull<> is preferred
+  // CHECK-FIXES-NO-PRESENT: 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()))
+  // CHECK-MESSAGES-PRESENT: :[[@LINE-2]]:7: warning: isa_and_present<> is preferred
+  // CHECK-FIXES-PRESENT: if (isa_and_present<Y>(z->bar()))
+  // CHECK-MESSAGES-NO-PRESENT: :[[@LINE-4]]:7: warning: isa_and_nonnull<> is preferred
+  // CHECK-FIXES-NO-PRESENT: if (isa_and_nonnull<Y>(z->bar()))
+
+  if (z->bar() && dyn_cast_if_present<Y>(z->bar()))
+    return true;
+  // CHECK-MESSAGES-PRESENT: :[[@LINE-2]]:7: warning: isa_and_present<> is preferred
+  // CHECK-FIXES-PRESENT: if (isa_and_present<Y>(z->bar()))
+  // CHECK-MESSAGES-NO-PRESENT: :[[@LINE-4]]:7: warning: isa_and_nonnull<> is preferred
+  // CHECK-FIXES-NO-PRESENT: 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());
+  // CHECK-MESSAGES-PRESENT: :[[@LINE-1]]:12: warning: isa_and_present<> is preferred
+  // CHECK-FIXES-PRESENT: bool b = isa_and_present<Y>(z->bar());
+  // CHECK-MESSAGES-NO-PRESENT: :[[@LINE-3]]:12: warning: isa_and_nonnull<> is preferred
+  // CHECK-FIXES-NO-PRESENT: 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;