[ASTMatchers] Matchers that take enumerations args provide hints with invalid arguments

[njames93]

Summary

This adds support for giving hints when using dynamic matchers with enum args. Take this query, I couldn't figure out why the matcher wasn't working:
(Turns out it needed to be "IntegralToBoolean", but thats another bug itself)

clang-query> match implicitCastExpr(hasCastKind("CK_IntegralToBoolean"))
1:1: Error parsing argument 1 for matcher implicitCastExpr.
1:18: Error building matcher hasCastKind.
1:30: Incorrect type for arg 1. (Expected = string) != (Actual = String)

With this patch the new behaviour looks like this:

clang-query> match implicitCastExpr(hasCastKind("CK_IntegralToBoolean"))
1:1: Error parsing argument 1 for matcher implicitCastExpr.
1:18: Error building matcher hasCastKind.
1:30: Unknown value 'CK_IntegralToBoolean' for arg 1; did you mean 'IntegralToBoolean'

There are no test cases for this yet as there simply isn't any infrastructure for testing errors reported when parsing args that I can see.

[njames93]

• Remove format artefacts

[aaron.ballman]

Thank you for working on this, I think it's very useful functionality! I'd appreciate seeing some unit test coverage for these changes.

clang/lib/ASTMatchers/Dynamic/CMakeLists.txt

20

We usually try to keep these lists in alphabetical order. Would you mind hoisting this up a bit (feel free to change VariantValue.cpp as a driveby if you want, or just leave it there)?

clang/lib/ASTMatchers/Dynamic/Marshallers.cpp

11

Elide braces, and please use ~0U instead of -1u.

17–18

Is this case possible to hit? I would imagine that if the search time matches anything in the allowed list, we would have not needed to call getBestGuess() in the first place?

[njames93]

• Added test cases and addressed nits.

[njames93]

clang/lib/ASTMatchers/Dynamic/Marshallers.cpp

17–18

This one shouldn't be, I put an assert in for sanity though. The one below with the prefix can be hit though.

[aaron.ballman]

LGTM, thank you!

clang/include/clang/ASTMatchers/Dynamic/Diagnostics.h

enum ErrorType {
     ET_None = 0,
 
     ET_RegistryMatcherNotFound = 1,
     ET_RegistryWrongArgCount = 2,
     ET_RegistryWrongArgType = 3,
     ET_RegistryNotBindable = 4,
     ET_RegistryAmbiguousOverload = 5,
     ET_RegistryValueNotFound = 6,
+    ET_RegistryUnknownEnumWithReplace = 7,
 
     ET_ParserStringError = 100,
     ET_ParserNoOpenParen = 101,
     ET_ParserNoCloseParen = 102,
     ET_ParserNoComma = 103,
     ET_ParserNoCode = 104,
     ET_ParserNotAMatcher = 105,
     ET_ParserInvalidToken = 106,

clang/lib/ASTMatchers/Dynamic/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
 )
 
 # The registry source file ends up generating a lot of sections for each
 # matcher. Each matcher appears to get a vtable and several methods. Each
 # method needs .text, .pdata, .xdata, and .debug sections, adding to the
 # section multiplier. By default MSVC has a 2^16 limit on the number of
 # sections in an object file, and this needs more than that.
 if (MSVC)
   set_source_files_properties(Registry.cpp PROPERTIES COMPILE_FLAGS /bigobj)
 endif()
 
 add_clang_library(clangDynamicASTMatchers
   Diagnostics.cpp
-  VariantValue.cpp
+  Marshallers.cpp
   Parser.cpp
   Registry.cpp
+  VariantValue.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   )

clang/lib/ASTMatchers/Dynamic/Diagnostics.cpp

case Diagnostics::ET_RegistryWrongArgType:
     return "Incorrect type for arg $0. (Expected = $1) != (Actual = $2)";
   case Diagnostics::ET_RegistryNotBindable:
     return "Matcher does not support binding.";
   case Diagnostics::ET_RegistryAmbiguousOverload:
     // TODO: Add type info about the overload error.
     return "Ambiguous matcher overload.";
   case Diagnostics::ET_RegistryValueNotFound:
     return "Value not found: $0";
+  case Diagnostics::ET_RegistryUnknownEnumWithReplace:
+    return "Unknown value '$1' for arg $0; did you mean '$2'";
 
   case Diagnostics::ET_ParserStringError:
     return "Error parsing string token: <$0>";
   case Diagnostics::ET_ParserNoOpenParen:
     return "Error parsing matcher. Found token <$0> while looking for '('.";
   case Diagnostics::ET_ParserNoCloseParen:
     return "Error parsing matcher. Found end-of-code while looking for ')'.";
   case Diagnostics::ET_ParserNoComma:

clang/lib/ASTMatchers/Dynamic/Marshallers.h

 #include "clang/ASTMatchers/ASTMatchersInternal.h"
 #include "clang/ASTMatchers/Dynamic/Diagnostics.h"
 #include "clang/ASTMatchers/Dynamic/VariantValue.h"
 #include "clang/Basic/AttrKinds.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/OpenMPKinds.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/Twine.h"
 #include <cassert>
 #include <cstddef>
 #include <iterator>
 #include <limits>
template <> struct ArgTypeTraits<std::string> {
 
   static const std::string &get(const VariantValue &Value) {
     return Value.getString();
   }
 
   static ArgKind getKind() {
     return ArgKind(ArgKind::AK_String);
   }
+
+  static llvm::Optional<std::string> getBestGuess(const VariantValue &) {
+    return llvm::None;
+  }
 };
 
 template <>
 struct ArgTypeTraits<StringRef> : public ArgTypeTraits<std::string> {
 };
 
 template <class T> struct ArgTypeTraits<ast_matchers::internal::Matcher<T>> {
   static bool is(const VariantValue &Value) {
     return Value.isMatcher() && Value.getMatcher().hasTypedMatcher<T>();
   }
 
   static ast_matchers::internal::Matcher<T> get(const VariantValue &Value) {
     return Value.getMatcher().getTypedMatcher<T>();
   }
 
   static ArgKind getKind() {
     return ArgKind(ASTNodeKind::getFromNodeKind<T>());
   }
+
+  static llvm::Optional<std::string> getBestGuess(const VariantValue &) {
+    return llvm::None;
+  }
 };
 
 template <> struct ArgTypeTraits<bool> {
   static bool is(const VariantValue &Value) { return Value.isBoolean(); }
 
   static bool get(const VariantValue &Value) {
     return Value.getBoolean();
   }
 
   static ArgKind getKind() {
     return ArgKind(ArgKind::AK_Boolean);
   }
+
+  static llvm::Optional<std::string> getBestGuess(const VariantValue &) {
+    return llvm::None;
+  }
 };
 
 template <> struct ArgTypeTraits<double> {
   static bool is(const VariantValue &Value) { return Value.isDouble(); }
 
   static double get(const VariantValue &Value) {
     return Value.getDouble();
   }
 
   static ArgKind getKind() {
     return ArgKind(ArgKind::AK_Double);
   }
+
+  static llvm::Optional<std::string> getBestGuess(const VariantValue &) {
+    return llvm::None;
+  }
 };
 
 template <> struct ArgTypeTraits<unsigned> {
   static bool is(const VariantValue &Value) { return Value.isUnsigned(); }
 
   static unsigned get(const VariantValue &Value) {
     return Value.getUnsigned();
   }
 
   static ArgKind getKind() {
     return ArgKind(ArgKind::AK_Unsigned);
   }
+
+  static llvm::Optional<std::string> getBestGuess(const VariantValue &) {
+    return llvm::None;
+  }
 };
 
 template <> struct ArgTypeTraits<attr::Kind> {
 private:
   static Optional<attr::Kind> getAttrKind(llvm::StringRef AttrKind) {
     return llvm::StringSwitch<Optional<attr::Kind>>(AttrKind)
 #define ATTR(X) .Case("attr::" #X, attr:: X)
 #include "clang/Basic/AttrList.inc"
         .Default(llvm::None);
   }
 
 public:
   static bool is(const VariantValue &Value) {
     return Value.isString() && getAttrKind(Value.getString());
   }
 
   static attr::Kind get(const VariantValue &Value) {
     return *getAttrKind(Value.getString());
   }
 
   static ArgKind getKind() {
     return ArgKind(ArgKind::AK_String);
   }
+
+  static llvm::Optional<std::string> getBestGuess(const VariantValue &Value);
 };
 
 template <> struct ArgTypeTraits<CastKind> {
 private:
   static Optional<CastKind> getCastKind(llvm::StringRef AttrKind) {
     return llvm::StringSwitch<Optional<CastKind>>(AttrKind)
 #define CAST_OPERATION(Name) .Case( #Name, CK_##Name)
 #include "clang/AST/OperationKinds.def"
         .Default(llvm::None);
   }
 
 public:
   static bool is(const VariantValue &Value) {
     return Value.isString() && getCastKind(Value.getString());
   }
 
   static CastKind get(const VariantValue &Value) {
     return *getCastKind(Value.getString());
   }
 
   static ArgKind getKind() {
     return ArgKind(ArgKind::AK_String);
   }
+
+  static llvm::Optional<std::string> getBestGuess(const VariantValue &Value);
 };
 
 template <> struct ArgTypeTraits<OpenMPClauseKind> {
 private:
   static Optional<OpenMPClauseKind> getClauseKind(llvm::StringRef ClauseKind) {
     return llvm::StringSwitch<Optional<OpenMPClauseKind>>(ClauseKind)
 #define OMP_CLAUSE_CLASS(Enum, Str, Class) .Case(#Enum, llvm::omp::Clause::Enum)
 #include "llvm/Frontend/OpenMP/OMPKinds.def"
         .Default(llvm::None);
   }
 
 public:
   static bool is(const VariantValue &Value) {
     return Value.isString() && getClauseKind(Value.getString());
   }
 
   static OpenMPClauseKind get(const VariantValue &Value) {
     return *getClauseKind(Value.getString());
   }
 
   static ArgKind getKind() { return ArgKind(ArgKind::AK_String); }
+
+  static llvm::Optional<std::string> getBestGuess(const VariantValue &Value);
 };
 
 /// Matcher descriptor interface.
 ///
 /// Provides a \c create() method that constructs the matcher from the provided
 /// arguments, and various other methods for type introspection.
 class MatcherDescriptor {
 public:

 }
 
 /// Convert the return values of the functions into a VariantMatcher.
 ///
 /// There are 2 cases right now: The return value is a Matcher<T> or is a
 /// polymorphic matcher. For the former, we just construct the VariantMatcher.
 /// For the latter, we instantiate all the possible Matcher<T> of the poly
 /// matcher.
-static VariantMatcher outvalueToVariantMatcher(const DynTypedMatcher &Matcher) {
+inline VariantMatcher outvalueToVariantMatcher(const DynTypedMatcher &Matcher) {
   return VariantMatcher::SingleMatcher(Matcher);
 }
 
 template <typename T>
 static VariantMatcher outvalueToVariantMatcher(const T &PolyMatcher,
                                                typename T::ReturnTypes * =
                                                    nullptr) {
   std::vector<DynTypedMatcher> Matchers;
#define CHECK_ARG_COUNT(count)                                                 \
   if (Args.size() != count) {                                                  \
     Error->addError(NameRange, Error->ET_RegistryWrongArgCount)                \
         << count << Args.size();                                               \
     return VariantMatcher();                                                   \
   }
 
 #define CHECK_ARG_TYPE(index, type)                                            \
   if (!ArgTypeTraits<type>::is(Args[index].Value)) {                           \
-    Error->addError(Args[index].Range, Error->ET_RegistryWrongArgType)         \
-        << (index + 1) << ArgTypeTraits<type>::getKind().asString()            \
-        << Args[index].Value.getTypeAsString();                                \
+    if (llvm::Optional<std::string> BestGuess =                                \
+            ArgTypeTraits<type>::getBestGuess(Args[index].Value)) {            \
+      Error->addError(Args[index].Range,                                       \
+                      Error->ET_RegistryUnknownEnumWithReplace)                \
+          << index + 1 << Args[index].Value.getString() << *BestGuess;         \
+    } else {                                                                   \
+      Error->addError(Args[index].Range, Error->ET_RegistryWrongArgType)       \
+          << (index + 1) << ArgTypeTraits<type>::getKind().asString()          \
+          << Args[index].Value.getTypeAsString();                              \
+    }                                                                          \
     return VariantMatcher();                                                   \
   }
 
 /// 0-arg marshaller function.
 template <typename ReturnType>
 static VariantMatcher matcherMarshall0(void (*Func)(), StringRef MatcherName,
                                        SourceRange NameRange,
                                        ArrayRef<ParserValue> Args,

clang/lib/ASTMatchers/Dynamic/Marshallers.cpp

+#include "Marshallers.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
+#include <string>
+
+static llvm::Optional<std::string>
+getBestGuess(llvm::StringRef Search, llvm::ArrayRef<llvm::StringRef> Allowed,
+             llvm::StringRef DropPrefix = "", unsigned MaxEditDistance = 3) {
+  if (MaxEditDistance != ~0U)
+    ++MaxEditDistance;
+  llvm::StringRef Res;
+  for (const llvm::StringRef &Item : Allowed) {
+    if (Item.equals_lower(Search)) {
+      assert(!Item.equals(Search) && "This should be handled earlier on.");
+      MaxEditDistance = 1;
+      Res = Item;
+      continue;
+    }
+    unsigned Distance = Item.edit_distance(Search);
+    if (Distance < MaxEditDistance) {
+      MaxEditDistance = Distance;
+      Res = Item;
+    }
+  }
+  if (!Res.empty())
+    return Res.str();
+  if (!DropPrefix.empty()) {
+    --MaxEditDistance; // Treat dropping the prefix as 1 edit
+    for (const llvm::StringRef &Item : Allowed) {
+      auto NoPrefix = Item;
+      if (!NoPrefix.consume_front(DropPrefix))
+        continue;
+      if (NoPrefix.equals_lower(Search)) {
+        if (NoPrefix.equals(Search))
+          return Item.str();
+        MaxEditDistance = 1;
+        Res = Item;
+        continue;
+      }
+      unsigned Distance = NoPrefix.edit_distance(Search);
+      if (Distance < MaxEditDistance) {
+        MaxEditDistance = Distance;
+        Res = Item;
+      }
+    }
+    if (!Res.empty())
+      return Res.str();
+  }
+  return llvm::None;
+}
+
+llvm::Optional<std::string>
+clang::ast_matchers::dynamic::internal::ArgTypeTraits<
+    clang::attr::Kind>::getBestGuess(const VariantValue &Value) {
+  static constexpr llvm::StringRef Allowed[] = {
+#define ATTR(X) "attr::" #X,
+#include "clang/Basic/AttrList.inc"
+  };
+  if (Value.isString())
+    return ::getBestGuess(Value.getString(), llvm::makeArrayRef(Allowed),
+                          "attr::");
+  return llvm::None;
+}
+
+llvm::Optional<std::string>
+clang::ast_matchers::dynamic::internal::ArgTypeTraits<
+    clang::CastKind>::getBestGuess(const VariantValue &Value) {
+  static constexpr llvm::StringRef Allowed[] = {
+#define CAST_OPERATION(Name) #Name,
+#include "clang/AST/OperationKinds.def"
+  };
+  if (Value.isString())
+    return ::getBestGuess(Value.getString(), llvm::makeArrayRef(Allowed));
+  return llvm::None;
+}
+
+llvm::Optional<std::string>
+clang::ast_matchers::dynamic::internal::ArgTypeTraits<
+    clang::OpenMPClauseKind>::getBestGuess(const VariantValue &Value) {
+  static constexpr llvm::StringRef Allowed[] = {
+#define OPENMP_CLAUSE(TextualSpelling, Class) "OMPC_" #TextualSpelling,
+#include "clang/Basic/OpenMPKinds.def"
+  };
+  if (Value.isString())
+    return ::getBestGuess(Value.getString(), llvm::makeArrayRef(Allowed),
+                          "OMPC_");
+  return llvm::None;
+}

clang/unittests/ASTMatchers/Dynamic/ParserTest.cpp

TEST(ParserTest, Errors) {
   EXPECT_EQ("1:21: Malformed bind() expression.",
             ParseWithError("isArrow().bind(\"foo\""));
   EXPECT_EQ("1:1: Error building matcher isArrow.\n"
             "1:1: Matcher does not support binding.",
             ParseWithError("isArrow().bind(\"foo\")"));
   EXPECT_EQ("Input value has unresolved overloaded type: "
             "Matcher<DoStmt|ForStmt|WhileStmt|CXXForRangeStmt|FunctionDecl>",
             ParseMatcherWithError("hasBody(stmt())"));
+  EXPECT_EQ(
+      "1:1: Error parsing argument 1 for matcher decl.\n"
+      "1:6: Error building matcher hasAttr.\n"
+      "1:14: Unknown value 'attr::Fnal' for arg 1; did you mean 'attr::Final'",
+      ParseMatcherWithError(R"query(decl(hasAttr("attr::Fnal")))query"));
+  EXPECT_EQ("1:1: Error parsing argument 1 for matcher decl.\n"
+            "1:6: Error building matcher hasAttr.\n"
+            "1:14: Unknown value 'Final' for arg 1; did you mean 'attr::Final'",
+            ParseMatcherWithError(R"query(decl(hasAttr("Final")))query"));
+  EXPECT_EQ("1:1: Error parsing argument 1 for matcher decl.\n"
+            "1:6: Error building matcher hasAttr.\n"
+            "1:14: Incorrect type for arg 1. (Expected = string) != (Actual = "
+            "String)",
+            ParseMatcherWithError(R"query(decl(hasAttr("unrelated")))query"));
 }
 
 TEST(ParserTest, OverloadErrors) {
   EXPECT_EQ("1:1: Error building matcher callee.\n"
             "1:8: Candidate 1: Incorrect type for arg 1. "
             "(Expected = Matcher<Stmt>) != (Actual = String)\n"
             "1:8: Candidate 2: Incorrect type for arg 1. "
             "(Expected = Matcher<Decl>) != (Actual = String)",