Exclude matchers which can have multiple results

[aaron.ballman]

lib/ASTMatchers/Dynamic/Registry.cpp

604

excludedMatchers -> ExcludedMatchers

604

Please add comments that explain why these are excluded and under what circumstances this list should be updated to add or remove items.

Also: how do we ensure this list stays properly updated? We sometimes miss updating RegistryMaps(), so I'm a bit concerned that adding a second list in a different location will be inviting trouble.

[steveire]

I acknowledge and share the future-proofing concern.

We could possibly use something trait-based instead and put the trait beside the matcher definition in ASTMatchers.h, but that doesn't really solve the problem. It only moves the problem.

[steveire]

Update

[aaron.ballman]

In D54404#1295426, @steveire wrote:

I acknowledge and share the future-proofing concern.

We could possibly use something trait-based instead and put the trait beside the matcher definition in ASTMatchers.h, but that doesn't really solve the problem. It only moves the problem.

If we could somehow incorporate it into the matcher definition itself, though, it means we don't have two lists of things to keep updated. You're right that it doesn't eliminate the problem -- we still have to know to ask the question when reviewing new matchers (so perhaps something that requires you to explicitly opt-in/out would be better).

Adding @sbenza in case he has ideas.

lib/ASTMatchers/Dynamic/Registry.cpp

604

The comment is a bit better but still not enough to tell me what's common to all of these matchers. It sort of reads to me like "all the matchers with overloads" since overloaded matchers can be ambiguous with particular arguments, but that's not what I think you're going for here.

For instance, I don't see how the anything() matcher can be ambiguous when used with particular arguments -- it doesn't accept arguments and it matches literally everything.

[steveire]

Let's discuss it on IRC at some point and see if we can come up with wording.

[sbenza]

In D54404#1296224, @aaron.ballman wrote:

In D54404#1295426, @steveire wrote:

I acknowledge and share the future-proofing concern.

We could possibly use something trait-based instead and put the trait beside the matcher definition in ASTMatchers.h, but that doesn't really solve the problem. It only moves the problem.

If we could somehow incorporate it into the matcher definition itself, though, it means we don't have two lists of things to keep updated. You're right that it doesn't eliminate the problem -- we still have to know to ask the question when reviewing new matchers (so perhaps something that requires you to explicitly opt-in/out would be better).

Adding @sbenza in case he has ideas.

We could put something in MatcherDescriptor to indicate this. However, I am still not sure what property are we looking for here.

lib/ASTMatchers/Dynamic/Registry.cpp

606

I don't think we are allowed to have non-trivial static storage duration objects like this.
You have to use llvm::ManagedStatic. Or just make it a raw array.

624

I'm not sure what goes in this list.
hasAnyName is here but not hasName.
What is ambiguous about hasAnyName?

[aaron.ballman]

lib/ASTMatchers/Dynamic/Registry.cpp

606

IIRC, that's only needed for global statics, not local statics. Local statics are less terrifying than their global brethren.

[steveire]

lib/ASTMatchers/Dynamic/Registry.cpp

624

I have a follow-up which adds output showing that hasName can be used. See

http://ec2-52-14-16-249.us-east-2.compute.amazonaws.com:10240/z/_X9mnw

If there was an entry there for hasAnyName, what would go in it?

[aaron.ballman]

lib/ASTMatchers/Dynamic/Registry.cpp

624

If there was an entry there for hasAnyName, what would go in it?

Presumably the same as hasName(), though for the purposes of that list, I could see why it would be a bit odd to list it.

It almost feels like this isn't about ambiguity of the matchers (at least, not always) so much as it is about sensibility within a "the following are related matchers" list due to there being many different ways for matchers to relate. For instance, a related matcher could be functionDecl(hasAnyParameter(anything())), but you might not want to list that because it's an open-ended problem to generate all such cases and it has very limited value to list them. Is that a better way for me to think about this?

[steveire]

lib/ASTMatchers/Dynamic/Registry.cpp

624

it is about sensibility ... Is that a better way for me to think about this?

Yes. The list attempts to exclude things that don't make sense to show for matcher output.

[aaron.ballman]

lib/ASTMatchers/Dynamic/Registry.cpp

624

Okay, so this isn't really about ambiguity as I was thinking of it (I was thinking of it in terms of "calling this matcher with this argument would be ambiguous"). Perhaps a comment like:

When matcher output is enabled, a list of related matchers is displayed to the user suggesting other matchers that would also be true for the matched construct. This is a list of matchers to exclude when considering those related matchers. Add elements to this list when the relationship to other matchers is impossible to determine or expected to be uninteresting to the user.

For instance, when matching functionDecl() to a function declaration with a single parameter, it would not be interesting for the user to know that functionDecl(hasParameter(anything())) also matches. Given that the anything() matcher is unlikely to be of interest in any matcher relationship, it is listed below as one of the matchers to exclude.

(Feel free to reword, come up with more compelling examples, etc.)

lib/ASTMatchers/Dynamic/Registry.cpp

for (const auto &M : RegistryData->constructors()) {
     }
   }
 }
 
 std::vector<MatchingMatcher>
 Registry::getMatchingMatchers(ast_type_traits::ASTNodeKind StaticType) {
   std::vector<MatchingMatcher> Result;
 
+  // Exclude matchers which can't unambiguously
+  // be used with particular arguments.
+  static std::vector<StringRef> ExcludedMatchers{
+      "allOf",
+      "anyOf",
+      "anything",
+      "containsDeclaration",
+      "eachOf",
+      "equalsNode",
+      "findAll",
+      "forEach",
+      "forEachConstructorInitializer",
+      "forEachDescendant",
+      "forEachOverridden",
+      "forEachSwitchCase",
+      "has",
+      "hasAncestor",
+      "hasAnyArgument",
+      "hasAnyConstructorInitializer",
+      "hasAnyDeclaration",
+      "hasAnyName",
+      "hasAnyParameter",
+      "hasAnySelector",
+      "hasAnySubstatement",
+      "hasAnyTemplateArgument",
+      "hasAnyUsingShadowDecl",
+      "hasArgumentOfType",
+      "hasDescendant",
+      "hasEitherOperand",
+      "hasMethod",
+      "hasParent",
+      "isExpansionInFileMatching",
+      "isSameOrDerivedFrom",
+      "matchesName",
+      "matchesSelector",
+      "unless"};
+  assert(std::is_sorted(ExcludedMatchers.begin(), ExcludedMatchers.end()));
+
   std::vector<ArgKind> AcceptedTypes{StaticType};
 
   processAcceptableMatchers(
-      AcceptedTypes,
-      [&Result](StringRef Name, const MatcherDescriptor &,
-                std::set<ASTNodeKind> &, std::vector<std::vector<ArgKind>>,
-                unsigned) { Result.emplace_back((Name + "()").str()); });
+      AcceptedTypes, [&Result](StringRef Name, const MatcherDescriptor &,
+                               std::set<ASTNodeKind> &,
+                               std::vector<std::vector<ArgKind>>, unsigned) {
+        if (!std::binary_search(ExcludedMatchers.begin(),
+                                ExcludedMatchers.end(), Name)) {
+          Result.emplace_back((Name + "()").str());
+        }
+      });
 
   return Result;
 }
 
 std::vector<MatcherCompletion>
 Registry::getMatcherCompletions(ArrayRef<ArgKind> AcceptedTypes) {
   std::vector<MatcherCompletion> Completions;
 

unittests/ASTMatchers/Dynamic/RegistryTest.cpp

TEST_F(RegistryTest, MatchingMatchers) {
 
   EXPECT_TRUE(!Contains(Matchers, "decl()"));
   EXPECT_TRUE(!Contains(Matchers, "namedDecl()"));
   EXPECT_TRUE(!Contains(Matchers, "valueDecl()"));
   EXPECT_TRUE(!Contains(Matchers, "declaratorDecl()"));
   EXPECT_TRUE(!Contains(Matchers, "functionDecl()"));
 
   EXPECT_TRUE(Contains(Matchers, "cxxMethodDecl()"));
+
+  EXPECT_TRUE(!Contains(Matchers, "has()"));
 }
 
 } // end anonymous namespace
 } // end namespace dynamic
 } // end namespace ast_matchers
 } // end namespace clang