Fix bug in DynTypedMatcher::constructVariadic() that would cause false negatives.

Summary:
DynTypedMatcher::constructVariadic() where the restrict kind of the
different matchers are not related causes the matcher to have a "None"
restrict kind. This causes false negatives for anyOf and eachOf.
Change the logic to get a common ancestor if there is one.
Also added regression tests that fail without the fix.

Reviewers: klimek

Subscribers: klimek, cfe-commits

Differential Revision: http://reviews.llvm.org/D5580

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@219118 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/AST/ASTTypeTraits.h b/include/clang/AST/ASTTypeTraits.h
index 01c5cec599b9d0718683fb1729875e67fd6920bf..efeac563db134d82a60ba65ea57d8dc004da8057 100644 (file)
--- a/include/clang/AST/ASTTypeTraits.h
+++ b/include/clang/AST/ASTTypeTraits.h
@@ -63,6 +63,9 @@ public:
   /// \brief Returns \c true if \c this and \c Other represent the same kind.
   bool isSame(ASTNodeKind Other) const;
 
+  /// \brief Returns \c true only for the default \c ASTNodeKind()
+  bool isNone() const { return KindId == NKI_None; }
+
   /// \brief Returns \c true if \c this is a base kind of (or same as) \c Other.
   /// \param Distance If non-null, used to return the distance between \c this
   /// and \c Other in the class hierarchy.
@@ -76,6 +79,17 @@ public:
     return KindId < Other.KindId;
   }
 
+  /// \brief Return the most derived type between \p Kind1 and \p Kind2.
+  ///
+  /// Return ASTNodeKind() if they are not related.
+  static ASTNodeKind getMostDerivedType(ASTNodeKind Kind1, ASTNodeKind Kind2);
+
+  /// \brief Return the most derived common ancestor between Kind1 and Kind2.
+  ///
+  /// Return ASTNodeKind() if they are not related.
+  static ASTNodeKind getMostDerivedCommonAncestor(ASTNodeKind Kind1,
+                                                  ASTNodeKind Kind2);
+
 private:
   /// \brief Kind ids.
   ///

diff --git a/lib/AST/ASTTypeTraits.cpp b/lib/AST/ASTTypeTraits.cpp
index 56915b96406e718c38a647f4b294fde3f87a0800..ec0671ceb1b5fd969143a4ff5678d22ecc578f22 100644 (file)
--- a/lib/AST/ASTTypeTraits.cpp
+++ b/lib/AST/ASTTypeTraits.cpp
@@ -62,6 +62,22 @@ bool ASTNodeKind::isBaseOf(NodeKindId Base, NodeKindId Derived,
 
 StringRef ASTNodeKind::asStringRef() const { return AllKindInfo[KindId].Name; }
 
+ASTNodeKind ASTNodeKind::getMostDerivedType(ASTNodeKind Kind1,
+                                            ASTNodeKind Kind2) {
+  if (Kind1.isBaseOf(Kind2)) return Kind2;
+  if (Kind2.isBaseOf(Kind1)) return Kind1;
+  return ASTNodeKind();
+}
+
+ASTNodeKind ASTNodeKind::getMostDerivedCommonAncestor(ASTNodeKind Kind1,
+                                                      ASTNodeKind Kind2) {
+  NodeKindId Parent = Kind1.KindId;
+  while (!isBaseOf(Parent, Kind2.KindId, nullptr) && Parent != NKI_None) {
+    Parent = AllKindInfo[Parent].ParentId;
+  }
+  return ASTNodeKind(Parent);
+}
+
 ASTNodeKind ASTNodeKind::getFromNode(const Decl &D) {
   switch (D.getKind()) {
 #define DECL(DERIVED, BASE)                                                    \

diff --git a/lib/ASTMatchers/ASTMatchersInternal.cpp b/lib/ASTMatchers/ASTMatchersInternal.cpp
index 4cff381f3f4abb3c7e56a40f83002c303a58411a..ec60f0d9498fb49201936250e7f8b70b8e58aa79 100644 (file)
--- a/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ b/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -64,28 +64,6 @@ class IdDynMatcher : public DynMatcherInterface {
   const IntrusiveRefCntPtr<DynMatcherInterface> InnerMatcher;
 };
 
-/// \brief Return the most derived type between \p Kind1 and \p Kind2.
-///
-/// Return the null type if they are not related.
-ast_type_traits::ASTNodeKind getMostDerivedType(
-    const ast_type_traits::ASTNodeKind Kind1,
-    const ast_type_traits::ASTNodeKind Kind2) {
-  if (Kind1.isBaseOf(Kind2)) return Kind2;
-  if (Kind2.isBaseOf(Kind1)) return Kind1;
-  return ast_type_traits::ASTNodeKind();
-}
-
-/// \brief Return the least derived type between \p Kind1 and \p Kind2.
-///
-/// Return the null type if they are not related.
-static ast_type_traits::ASTNodeKind getLeastDerivedType(
-    const ast_type_traits::ASTNodeKind Kind1,
-    const ast_type_traits::ASTNodeKind Kind2) {
-  if (Kind1.isBaseOf(Kind2)) return Kind1;
-  if (Kind2.isBaseOf(Kind1)) return Kind2;
-  return ast_type_traits::ASTNodeKind();
-}
-
 }  // namespace
 
 DynTypedMatcher DynTypedMatcher::constructVariadic(
@@ -98,7 +76,8 @@ DynTypedMatcher DynTypedMatcher::constructVariadic(
     assert(Result.SupportedKind.isSame(M.SupportedKind) &&
            "SupportedKind must match!");
     Result.RestrictKind =
-        getLeastDerivedType(Result.RestrictKind, M.RestrictKind);
+        ast_type_traits::ASTNodeKind::getMostDerivedCommonAncestor(
+            Result.RestrictKind, M.RestrictKind);
   }
   Result.Implementation = new VariadicMatcher(Func, std::move(InnerMatchers));
   return Result;
@@ -108,7 +87,8 @@ DynTypedMatcher DynTypedMatcher::dynCastTo(
     const ast_type_traits::ASTNodeKind Kind) const {
   auto Copy = *this;
   Copy.SupportedKind = Kind;
-  Copy.RestrictKind = getMostDerivedType(Kind, RestrictKind);
+  Copy.RestrictKind =
+      ast_type_traits::ASTNodeKind::getMostDerivedType(Kind, RestrictKind);
   return Copy;
 }
 

diff --git a/unittests/AST/ASTTypeTraitsTest.cpp b/unittests/AST/ASTTypeTraitsTest.cpp
index 0e73f7616923f80ed85ef91fc9ca544684d4fe47..eeb01ccad1dfe7f44996334f73416b50107ec8ca 100644 (file)
--- a/unittests/AST/ASTTypeTraitsTest.cpp
+++ b/unittests/AST/ASTTypeTraitsTest.cpp
@@ -26,6 +26,12 @@ template <typename T> static ASTNodeKind DNT() {
   return ASTNodeKind::getFromNodeKind<T>();
 }
 
+TEST(ASTNodeKind, IsNone) {
+  EXPECT_TRUE(ASTNodeKind().isNone());
+  EXPECT_FALSE(DNT<Decl>().isNone());
+  EXPECT_FALSE(DNT<VarDecl>().isNone());
+}
+
 TEST(ASTNodeKind, Bases) {
   EXPECT_TRUE(DNT<Decl>().isBaseOf(DNT<VarDecl>()));
   EXPECT_FALSE(DNT<Decl>().isSame(DNT<VarDecl>()));
@@ -60,6 +66,39 @@ TEST(ASTNodeKind, DiffBase) {
   EXPECT_FALSE(DNT<Type>().isSame(DNT<QualType>()));
 }
 
+TEST(ASTNodeKind, MostDerivedType) {
+  EXPECT_TRUE(DNT<BinaryOperator>().isSame(
+      ASTNodeKind::getMostDerivedType(DNT<Expr>(), DNT<BinaryOperator>())));
+  EXPECT_TRUE(DNT<BinaryOperator>().isSame(
+      ASTNodeKind::getMostDerivedType(DNT<BinaryOperator>(), DNT<Expr>())));
+  EXPECT_TRUE(DNT<VarDecl>().isSame(
+      ASTNodeKind::getMostDerivedType(DNT<VarDecl>(), DNT<VarDecl>())));
+
+  // Not related. Returns nothing.
+  EXPECT_TRUE(
+      ASTNodeKind::getMostDerivedType(DNT<IfStmt>(), DNT<VarDecl>()).isNone());
+  EXPECT_TRUE(ASTNodeKind::getMostDerivedType(DNT<IfStmt>(),
+                                              DNT<BinaryOperator>()).isNone());
+}
+
+TEST(ASTNodeKind, MostDerivedCommonAncestor) {
+  EXPECT_TRUE(DNT<Expr>().isSame(ASTNodeKind::getMostDerivedCommonAncestor(
+      DNT<Expr>(), DNT<BinaryOperator>())));
+  EXPECT_TRUE(DNT<Expr>().isSame(ASTNodeKind::getMostDerivedCommonAncestor(
+      DNT<BinaryOperator>(), DNT<Expr>())));
+  EXPECT_TRUE(DNT<VarDecl>().isSame(ASTNodeKind::getMostDerivedCommonAncestor(
+      DNT<VarDecl>(), DNT<VarDecl>())));
+
+  // A little related. Returns the ancestor.
+  EXPECT_TRUE(
+      DNT<NamedDecl>().isSame(ASTNodeKind::getMostDerivedCommonAncestor(
+          DNT<CXXMethodDecl>(), DNT<RecordDecl>())));
+
+  // Not related. Returns nothing.
+  EXPECT_TRUE(ASTNodeKind::getMostDerivedCommonAncestor(
+                  DNT<IfStmt>(), DNT<VarDecl>()).isNone());
+}
+
 struct Foo {};
 
 TEST(ASTNodeKind, UnknownKind) {

diff --git a/unittests/ASTMatchers/ASTMatchersTest.cpp b/unittests/ASTMatchers/ASTMatchersTest.cpp
index 4eb4c0d6727de438bc1f55310c1ebb971d0bb941..a7969340ba8a03f3532cd4f31051440bef916195 100644 (file)
--- a/unittests/ASTMatchers/ASTMatchersTest.cpp
+++ b/unittests/ASTMatchers/ASTMatchersTest.cpp
@@ -460,6 +460,11 @@ TEST(DeclarationMatcher, MatchAnyOf) {
   EXPECT_TRUE(matches("class U {};", XOrYOrZOrUOrV));
   EXPECT_TRUE(matches("class V {};", XOrYOrZOrUOrV));
   EXPECT_TRUE(notMatches("class A {};", XOrYOrZOrUOrV));
+
+  StatementMatcher MixedTypes = stmt(anyOf(ifStmt(), binaryOperator()));
+  EXPECT_TRUE(matches("int F() { return 1 + 2; }", MixedTypes));
+  EXPECT_TRUE(matches("int F() { if (true) return 1; }", MixedTypes));
+  EXPECT_TRUE(notMatches("int F() { return 1; }", MixedTypes));
 }
 
 TEST(DeclarationMatcher, MatchHas) {

diff --git a/unittests/ASTMatchers/Dynamic/RegistryTest.cpp b/unittests/ASTMatchers/Dynamic/RegistryTest.cpp
index f8a9430f169dc462232140ef1f91e1e970dcb14c..824b310442a3cb160d37968602c0dbb3d5026fe6 100644 (file)
--- a/unittests/ASTMatchers/Dynamic/RegistryTest.cpp
+++ b/unittests/ASTMatchers/Dynamic/RegistryTest.cpp
@@ -347,7 +347,7 @@ TEST_F(RegistryTest, VariadicOp) {
       "anyOf",
       constructMatcher("recordDecl",
                        constructMatcher("hasName", std::string("Foo"))),
-      constructMatcher("namedDecl",
+      constructMatcher("functionDecl",
                        constructMatcher("hasName", std::string("foo"))))
       .getTypedMatcher<Decl>();