[ASTMatchers] Add matchers for `CXXDeleteExpr`

[njames93]

Summary

Add deletes traveral matcher which matches on the expression being delete
Extend CXXNewExpr::isArray matcher to work for CXXDeleteExpr. Currently this only matches array delete expressions as written.

[aaron.ballman]

clang/include/clang/ASTMatchers/ASTMatchers.h

7621

Why add this instead of continuing to use has()? (I worry a bit that we'll want to add a verb for different subexpression matchers and it won't scale well or we won't add verbs for different subexpressions and this'll introduce a new inconsistency.)

clang/include/clang/ASTMatchers/ASTMatchersInternal.h

163

Do we need/want this interface to consider whether the user is matching things spelled in source or not?

clang/lib/ASTMatchers/Dynamic/Registry.cpp

269

Why removing this?

[njames93]

clang/include/clang/ASTMatchers/ASTMatchers.h

7621

This change is about lowering the barrier to entry. While has can be very powerful, its interface isn't newcomer friendly.
You can put any matcher in, that may not make sense or ever be capable of actually matching but it will happily compile without emitting any warning. using deletes forces some type checking on the argument you provide.
The explicit names are also much better for documentation and API discovery.
When running in clang query with completions this is what appears at the top of the list

clang-query> m cxxDeleteExpr(
Matcher<CXXDeleteExpr> deletes(Matcher<Expr>)
Matcher<CXXDeleteExpr> isArray()

A similar case is made for the matchers documentation.

clang/include/clang/ASTMatchers/ASTMatchersInternal.h

163

From what I can see the only time isArrayFrom and isArrayFromAsWritten differentiate is if normal delete operator is used on an arrayType. However that behaviour isn't standard afaik and was only added to be consistent with gcc and edg. If that case comes up we emit a fixit to add the [].
Maybe there is merit to check traversal kind before assuming tho.

clang/lib/ASTMatchers/Dynamic/Registry.cpp

269

Accident. Will re add.

[aaron.ballman]

clang/include/clang/ASTMatchers/ASTMatchers.h

7621

Thanks! I agree with your assessment about this being a more user-friendly interface. My concerns are more mundane though. IIRC, each time we add a new matcher, we slow down compilation for the entire Clang project by a nontrivial amount because of the large amount of template instantiations involved. Because of that, we usually only add matchers when we find there's a need for them (as opposed to generating matchers for everything possible). So I worry that we'll slow down compile times and increase build sizes for a relatively uncommon AST matcher that isn't strictly necessary, and that it'll be a slippery slope to do more of this. That said, I've not measured the slowdown and perhaps things have improved here.

@klimek may have more context or thoughts on this.

clang/include/clang/ASTMatchers/ASTMatchersInternal.h

163

I think it's defensible either way, so I leave it to whatever you and @steveire think is appropriate.

[njames93]

clang/include/clang/ASTMatchers/ASTMatchers.h

7621

That is a good point, maybe though if we felt so inclined. We could actually split up the Matcher definitions from their declarations. It may not work as nicely with PolymorphicMatchers but given around 3/4 of the matchers aren't, It could have a positive impact on compile time.

clang-tools-extra/clang-tidy/bugprone/BoolPointerImplicitConversionCheck.cpp

if (!match(findAll(unaryOperator(hasOperatorName("*"),
              *Result.Context)
            .empty() ||
       // FIXME: We should still warn if the paremater is implicitly converted to
       // bool.
       !match(
            findAll(callExpr(hasAnyArgument(ignoringParenImpCasts(RefMatcher)))),
            *If, *Result.Context)
            .empty() ||
-      !match(
-           findAll(cxxDeleteExpr(has(ignoringParenImpCasts(expr(RefMatcher))))),
+      !match(findAll(cxxDeleteExpr(
+                 deletes(ignoringParenImpCasts(expr(RefMatcher))))),
              *If, *Result.Context)
            .empty())
     return;
 
   Check.diag(Ref->getBeginLoc(),
              "dubious check of 'bool *' against 'nullptr', did "
              "you mean to dereference it?")
       << FixItHint::CreateInsertion(Ref->getBeginLoc(), "*");
 }

clang-tools-extra/clang-tidy/readability/DeleteNullPointerCheck.cpp

 
 namespace clang {
 namespace tidy {
 namespace readability {
 
 void DeleteNullPointerCheck::registerMatchers(MatchFinder *Finder) {
   const auto DeleteExpr =
       cxxDeleteExpr(
-          has(declRefExpr(to(decl(equalsBoundNode("deletedPointer"))))))
+          deletes(declRefExpr(to(decl(equalsBoundNode("deletedPointer"))))))
           .bind("deleteExpr");
 
   const auto DeleteMemberExpr =
-      cxxDeleteExpr(has(memberExpr(hasDeclaration(
+      cxxDeleteExpr(deletes(memberExpr(hasDeclaration(
                         fieldDecl(equalsBoundNode("deletedMemberPointer"))))))
           .bind("deleteMemberExpr");
 
   const auto PointerExpr = anyOf(
       declRefExpr(to(decl().bind("deletedPointer"))),
       memberExpr(hasDeclaration(fieldDecl().bind("deletedMemberPointer"))));
 
   const auto BinaryPointerCheckCondition = binaryOperator(hasOperands(

clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp

auto IsSusbstituted = qualType(anyOf(
       substTemplateTypeParmType(), hasDescendant(substTemplateTypeParmType())));
 
   auto UniquePtrWithDefaultDelete = classTemplateSpecializationDecl(
       hasName("std::unique_ptr"),
       hasTemplateArgument(1, refersToType(qualType(hasDeclaration(cxxRecordDecl(
                                  hasName("std::default_delete")))))));
 
   Finder->addMatcher(
-      cxxDeleteExpr(has(ignoringParenImpCasts(cxxMemberCallExpr(
+      cxxDeleteExpr(deletes(ignoringParenImpCasts(cxxMemberCallExpr(
                         on(expr(hasType(UniquePtrWithDefaultDelete),
                                 unless(hasType(IsSusbstituted)))
                                .bind("uptr")),
                         callee(cxxMethodDecl(hasName("release")))))))
           .bind("delete"),
       this);
 }
 

clang/docs/LibASTMatchersReference.html

Given
   S(int) -> S<true> // #5
   explicit S(double) -> S<false> // #6
 cxxConstructorDecl(isExplicit()) will match #2 and #8, but not #1, #7 or #9.
 cxxConversionDecl(isExplicit()) will match #4, but not #3.
 cxxDeductionGuideDecl(isExplicit()) will match #6, but not #5.
 </pre></td></tr>
 
 
+<tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1CXXDeleteExpr.html">CXXDeleteExpr</a>&gt;</td><td class="name" onclick="toggle('isArray1')"><a name="isArray1Anchor">isArray</a></td><td></td></tr>
+<tr><td colspan="4" class="doc" id="isArray1"><pre>Matches array new and delete expressions.
+
+Given:
+  MyClass *p1 = new MyClass[10];
+  delete[] p1;
+cxxNewExpr(isArray())
+  matches the expression 'new MyClass[10]'.
+cxxDeleteExpr(isArray())
+  matches the expression 'delete[] p1'.
+</pre></td></tr>
+
+
 <tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1CXXDependentScopeMemberExpr.html">CXXDependentScopeMemberExpr</a>&gt;</td><td class="name" onclick="toggle('hasMemberName0')"><a name="hasMemberName0Anchor">hasMemberName</a></td><td>std::string N</td></tr>
 <tr><td colspan="4" class="doc" id="hasMemberName0"><pre>Matches template-dependent, but known, member names.
 
 In template declarations, dependent members are not resolved and so can
 not be matched to particular named declarations.
 
 This matcher allows to match on the known name of members.
 
Given
    public:
     void x();
   };
   matches A::x but not B::x
 </pre></td></tr>
 
 
 <tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1CXXNewExpr.html">CXXNewExpr</a>&gt;</td><td class="name" onclick="toggle('isArray0')"><a name="isArray0Anchor">isArray</a></td><td></td></tr>
-<tr><td colspan="4" class="doc" id="isArray0"><pre>Matches array new expressions.
+<tr><td colspan="4" class="doc" id="isArray0"><pre>Matches array new and delete expressions.
 
 Given:
   MyClass *p1 = new MyClass[10];
+  delete[] p1;
 cxxNewExpr(isArray())
   matches the expression 'new MyClass[10]'.
+cxxDeleteExpr(isArray())
+  matches the expression 'delete[] p1'.
 </pre></td></tr>
 
 
 <tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1CXXOperatorCallExpr.html">CXXOperatorCallExpr</a>&gt;</td><td class="name" onclick="toggle('hasAnyOperatorName1')"><a name="hasAnyOperatorName1Anchor">hasAnyOperatorName</a></td><td>StringRef, ..., StringRef</td></tr>
 <tr><td colspan="4" class="doc" id="hasAnyOperatorName1"><pre>Matches operator expressions (binary or unary) that have any of the
 specified names.
 
    hasAnyOperatorName("+", "-")
Given
   };
 cxxRecordDecl(has(cxxConstructorDecl(hasAnyConstructorInitializer(
     withInitializer(integerLiteral(equals(1)))))))
   matches Foo
 with withInitializer matching (1)
 </pre></td></tr>
 
 
+<tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1CXXDeleteExpr.html">CXXDeleteExpr</a>&gt;</td><td class="name" onclick="toggle('deletes0')"><a name="deletes0Anchor">deletes</a></td><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1Expr.html">Expr</a>&gt; InnerMatcher</td></tr>
+<tr><td colspan="4" class="doc" id="deletes0"><pre>Matchers the expression being deleted.
+
+Given:
+  int *Ptr = nullptr;
+  delete Ptr;
+cxxDeleteExpr(deletes(declRefExpr(ignoringImpCasts(to(varDecl(hasName("Ptr")))))))
+  matches the expression 'delete Ptr'.
+</pre></td></tr>
+
+
 <tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1CXXDependentScopeMemberExpr.html">CXXDependentScopeMemberExpr</a>&gt;</td><td class="name" onclick="toggle('hasObjectExpression2')"><a name="hasObjectExpression2Anchor">hasObjectExpression</a></td><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1Expr.html">Expr</a>&gt; InnerMatcher</td></tr>
 <tr><td colspan="4" class="doc" id="hasObjectExpression2"><pre>Matches a member expression where the object expression is matched by a
 given matcher. Implicit object expressions are included; that is, it matches
 use of implicit `this`.
 
 Given
   struct X {
     int m;

clang/include/clang/ASTMatchers/ASTMatchers.h

 ///
 /// A matcher such as
 ///   parmVarDecl(hasInitializer(anything()))
 /// is equivalent to parmVarDecl(hasDefaultArgument()).
 AST_MATCHER(ParmVarDecl, hasDefaultArgument) {
   return Node.hasDefaultArg();
 }
 
-/// Matches array new expressions.
+/// Matches array new and delete expressions.
 ///
 /// Given:
 /// \code
 ///   MyClass *p1 = new MyClass[10];
+///   delete[] p1;
 /// \endcode
 /// cxxNewExpr(isArray())
 ///   matches the expression 'new MyClass[10]'.
-AST_MATCHER(CXXNewExpr, isArray) {
-  return Node.isArray();
+/// cxxDeleteExpr(isArray())
+///   matches the expression 'delete[] p1'.
+AST_POLYMORPHIC_MATCHER(isArray,
+                        AST_POLYMORPHIC_SUPPORTED_TYPES(CXXNewExpr,
+                                                        CXXDeleteExpr)) {
+  return isArrayOperator(Node);
 }
 
 /// Matches placement new expression arguments.
 ///
 /// Given:
 /// \code
 ///   MyClass *p1 = new (Storage, 16) MyClass();
 /// \endcode

 /// \endcode
 /// cxxNewExpr(hasArraySize(integerLiteral(equals(10))))
 ///   matches the expression 'new MyClass[10]'.
 AST_MATCHER_P(CXXNewExpr, hasArraySize, internal::Matcher<Expr>, InnerMatcher) {
   return Node.isArray() && *Node.getArraySize() &&
          InnerMatcher.matches(**Node.getArraySize(), Finder, Builder);
 }
 
+/// Matchers the expression being deleted.
+///
+/// Given:
+/// \code
+///   int *Ptr = nullptr;
+///   delete Ptr;
+/// \endcode
+/// cxxDeleteExpr(deletes(declRefExpr(ignoringImpCasts(to(varDecl(hasName("Ptr")))))))
+///   matches the expression 'delete Ptr'.
+AST_MATCHER_P_OVERLOAD(CXXDeleteExpr, deletes, internal::Matcher<Expr>,
+                       InnerMatcher, 1) {
+  return InnerMatcher.matches(*Node.getArgument(), Finder, Builder);
+}
+
 /// Matches a class declaration that is defined.
 ///
 /// Example matches x (matcher = cxxRecordDecl(hasDefinition()))
 /// \code
 /// class x {};
 /// class y;
 /// \endcode
 AST_MATCHER(CXXRecordDecl, hasDefinition) {

clang/include/clang/ASTMatchers/ASTMatchersInternal.h

 inline clang::AccessSpecifier getAccessSpecifier(const Decl &Node) {
   return Node.getAccess();
 }
 
 inline clang::AccessSpecifier getAccessSpecifier(const CXXBaseSpecifier &Node) {
   return Node.getAccessSpecifier();
 }
 
+/// Unifies obtaining the whether as new or delete operator operates on an
+/// array.
+inline bool isArrayOperator(const CXXNewExpr &Node) { return Node.isArray(); }
+
+inline bool isArrayOperator(const CXXDeleteExpr &Node) {
+  return Node.isArrayFormAsWritten();
+}
+
 /// Internal version of BoundNodes. Holds all the bound nodes.
 class BoundNodesMap {
 public:
   /// Adds \c Node to the map with key \c ID.
   ///
   /// The node's base type should be in NodeBaseType or it will be unaccessible.
   void addNode(StringRef ID, const DynTypedNode &DynNode) {
     NodeMap[std::string(ID)] = DynNode;

clang/lib/ASTMatchers/Dynamic/Registry.cpp

RegistryMaps::RegistryMaps() {
   REGISTER_MATCHER(decompositionDecl);
   REGISTER_MATCHER(declCountIs);
   REGISTER_MATCHER(declRefExpr);
   REGISTER_MATCHER(declStmt);
   REGISTER_MATCHER(declaratorDecl);
   REGISTER_MATCHER(decltypeType);
   REGISTER_MATCHER(deducedTemplateSpecializationType);
   REGISTER_MATCHER(defaultStmt);
+  REGISTER_MATCHER(deletes);
   REGISTER_MATCHER(dependentSizedArrayType);
   REGISTER_MATCHER(designatedInitExpr);
   REGISTER_MATCHER(designatorCountIs);
   REGISTER_MATCHER(doStmt);
   REGISTER_MATCHER(eachOf);
   REGISTER_MATCHER(elaboratedType);
   REGISTER_MATCHER(enumConstantDecl);
   REGISTER_MATCHER(enumDecl);
RegistryMaps::RegistryMaps() {
   REGISTER_MATCHER(hasAnyOperatorName);
   REGISTER_MATCHER(hasAnyOverloadedOperatorName);
   REGISTER_MATCHER(hasAnyParameter);
   REGISTER_MATCHER(hasAnyPlacementArg);
   REGISTER_MATCHER(hasAnySelector);
   REGISTER_MATCHER(hasAnySubstatement);
   REGISTER_MATCHER(hasAnyTemplateArgument);
   REGISTER_MATCHER(hasAnyUsingShadowDecl);
-  REGISTER_MATCHER(hasArgument);
   REGISTER_MATCHER(hasArgumentOfType);
   REGISTER_MATCHER(hasArraySize);
   REGISTER_MATCHER(hasAttr);
   REGISTER_MATCHER(hasAutomaticStorageDuration);
   REGISTER_MATCHER(hasBase);
   REGISTER_MATCHER(hasBinding);
   REGISTER_MATCHER(hasBitWidth);
   REGISTER_MATCHER(hasBody);

clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

 
 TEST_P(ASTMatchersTest, IsArray) {
   if (!GetParam().isCXX()) {
     return;
   }
 
   EXPECT_TRUE(matches("struct MyClass {}; MyClass *p1 = new MyClass[10];",
                       cxxNewExpr(isArray())));
+
+  EXPECT_TRUE(
+      matches("void foo(int* p) { delete[] p; }", cxxDeleteExpr(isArray())));
+
+  EXPECT_TRUE(
+      notMatches("void foo(int* p) { delete p; }", cxxDeleteExpr(isArray())));
 }
 
 TEST_P(ASTMatchersTest, HasArraySize) {
   if (!GetParam().isCXX()) {
     return;
   }
 
   EXPECT_TRUE(matches("struct MyClass {}; MyClass *p1 = new MyClass[10];",

clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

TEST(CXXNewExpr, PlacementArgs) {
   EXPECT_TRUE(notMatches(R"(
     void* operator new(decltype(sizeof(void*)), void*);
     int *foo(void* Storage) {
       return new int;
     })",
                          IsPlacementNew));
 }
 
+TEST(CXXDeleteExpr, Deletes) {
+  StatementMatcher IsDeletingPtrVar = cxxDeleteExpr(
+      deletes(ignoringImpCasts(declRefExpr(to(varDecl(hasName("Ptr")))))));
+
+  EXPECT_TRUE(matches("void foo(int* Ptr) { delete Ptr; }", IsDeletingPtrVar));
+  EXPECT_TRUE(notMatches(R"(
+    struct Foo{
+      int* Address;
+    };
+    void Bar(Foo& Ptr) {
+      delete Ptr.Address;
+    }
+  )",
+                         IsDeletingPtrVar));
+
+  EXPECT_TRUE(notMatches(R"(
+    struct Foo{
+      int* Ptr;
+    };
+    void Bar(Foo& Ptr) {
+      delete Ptr.Ptr;
+    }
+  )",
+                         IsDeletingPtrVar));
+}
+
 } // namespace ast_matchers
 } // namespace clang