Improve dynamic AST matching diagnostics for conversion errors

[aaron.ballman]

Summary

Currently, when marshaling a dynamic AST matchers, we check for the type and value validity of matcher arguments at the same time for some matchers. For instance, when marshaling hasAttr("foo"), the argument is first type checked to ensure it's a string and then checked to see if that string can locate an attribute with that name. Similar happens for other enumeration conversions like cast kinds or unary operator kinds. If the type is correct but the value cannot be looked up, we make a best-effort attempt to find a nearby name that the user might have meant, but if one cannot be found, we throw our hands up and claim the types don't match.

This has an unfortunate behavior that when the user enters something of the correct type but a best guess cannot be located, you get confusing error messages like: Incorrect type for arg 1. (Expected = string) != (Actual = String).

This patch splits the argument check into two parts: if the types don't match, give a type diagnostic. If the type matches but the value cannot be converted, give a best guess diagnostic or a value could not be located diagnostic:

clang-query> m hasAttr("cool")
1:1: Error building matcher hasAttr.
1:9: Value not found: cool
clang-query> m hasAttr("attr::cool")
1:1: Error building matcher hasAttr.
1:9: Unknown value 'attr::cool' for arg 1; did you mean 'attr::Cold'

This addresses PR47057.

[aaron.ballman]

Ping

[aaron.ballman]

In D85611#2218144, @aaron.ballman wrote:

Ping

Ping x2

[aaron.ballman]

In D85611#2230481, @aaron.ballman wrote:

In D85611#2218144, @aaron.ballman wrote:

Ping

Ping x2

I realize it's performance review time for some folks and their schedules may be busier than usual, gently pinging this so it doesn't fall through the cracks.

[sammccall]

Sorry about the delay, good guess as to what was happening :-)

clang/lib/ASTMatchers/Dynamic/Marshallers.h

88

If the matcher type is incorrect e.g. hasValueType(cxxRecordDecl()) I think this report "value not found: cxxRecordDecl()" or something like that - really this is a type error.

This is minor and probably not worse than "matcher != matcher" today.

But for ideal error messages we might want to give the traits more responsibility for the structure of their error conditions (e.g. changing get() to return Expected<T>)

[aaron.ballman]

Thank you for the review. I've commit in 819ff6b945816dce144c8be577a3c245f702b59c

clang/lib/ASTMatchers/Dynamic/Marshallers.h

88

That's a good idea for a future change!

clang/lib/ASTMatchers/Dynamic/Marshallers.h

 /// Helper template class to just from argument type to the right is/get
 ///   functions in VariantValue.
 /// Used to verify and extract the matcher arguments below.
 template <class T> struct ArgTypeTraits;
 template <class T> struct ArgTypeTraits<const T &> : public ArgTypeTraits<T> {
 };
 
 template <> struct ArgTypeTraits<std::string> {
-  static bool is(const VariantValue &Value) { return Value.isString(); }
+  static bool hasCorrectType(const VariantValue &Value) {
+    return Value.isString();
+  }
+  static bool hasCorrectValue(const VariantValue &Value) { return true; }
 
   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 bool hasCorrectType(const VariantValue& Value) {
+    return Value.isMatcher();
+  }
+  static bool hasCorrectValue(const VariantValue &Value) {
+    return 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 hasCorrectType(const VariantValue &Value) {
+    return Value.isBoolean();
+  }
+  static bool hasCorrectValue(const VariantValue &Value) { return true; }
 
   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 bool hasCorrectType(const VariantValue &Value) {
+    return Value.isDouble();
+  }
+  static bool hasCorrectValue(const VariantValue &Value) { return true; }
 
   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 bool hasCorrectType(const VariantValue &Value) {
+    return Value.isUnsigned();
+  }
+  static bool hasCorrectValue(const VariantValue &Value) { return true; }
 
   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 bool hasCorrectType(const VariantValue &Value) {
+    return Value.isString();
+  }
+  static bool hasCorrectValue(const VariantValue& Value) {
+    return getAttrKind(Value.getString()).hasValue();
   }
 
   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("CK_" #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 bool hasCorrectType(const VariantValue &Value) {
+    return Value.isString();
+  }
+  static bool hasCorrectValue(const VariantValue& Value) {
+    return getCastKind(Value.getString()).hasValue();
   }
 
   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<llvm::Regex::RegexFlags> {
 private:
   static Optional<llvm::Regex::RegexFlags> getFlags(llvm::StringRef Flags);
 
 public:
-  static bool is(const VariantValue &Value) {
-    return Value.isString() && getFlags(Value.getString());
+  static bool hasCorrectType(const VariantValue &Value) {
+    return Value.isString();
+  }
+  static bool hasCorrectValue(const VariantValue& Value) {
+    return getFlags(Value.getString()).hasValue();
   }
 
   static llvm::Regex::RegexFlags get(const VariantValue &Value) {
     return *getFlags(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 bool hasCorrectType(const VariantValue &Value) {
+    return Value.isString();
+  }
+  static bool hasCorrectValue(const VariantValue& Value) {
+    return getClauseKind(Value.getString()).hasValue();
   }
 
   static OpenMPClauseKind get(const VariantValue &Value) {
     return *getClauseKind(Value.getString());
   }
 
   static ArgKind getKind() { return ArgKind(ArgKind::AK_String); }
 
#define UNARY_EXPR_OR_TYPE_TRAIT(Spelling, Name, Key)                          \
   .Case("UETT_" #Name, UETT_##Name)
 #define CXX11_UNARY_EXPR_OR_TYPE_TRAIT(Spelling, Name, Key)                    \
   .Case("UETT_" #Name, UETT_##Name)
 #include "clang/Basic/TokenKinds.def"
         .Default(llvm::None);
   }
 
 public:
-  static bool is(const VariantValue &Value) {
-    return Value.isString() && getUnaryOrTypeTraitKind(Value.getString());
+  static bool hasCorrectType(const VariantValue &Value) {
+    return Value.isString();
+  }
+  static bool hasCorrectValue(const VariantValue& Value) {
+    return getUnaryOrTypeTraitKind(Value.getString()).hasValue();
   }
 
   static UnaryExprOrTypeTrait get(const VariantValue &Value) {
     return *getUnaryOrTypeTraitKind(Value.getString());
   }
 
   static ArgKind getKind() { return ArgKind(ArgKind::AK_String); }
 
variadicMatcherDescriptor(StringRef MatcherName, SourceRange NameRange,
   ArgT **InnerArgs = new ArgT *[Args.size()]();
 
   bool HasError = false;
   for (size_t i = 0, e = Args.size(); i != e; ++i) {
     using ArgTraits = ArgTypeTraits<ArgT>;
 
     const ParserValue &Arg = Args[i];
     const VariantValue &Value = Arg.Value;
-    if (!ArgTraits::is(Value)) {
+    if (!ArgTraits::hasCorrectType(Value)) {
       Error->addError(Arg.Range, Error->ET_RegistryWrongArgType)
           << (i + 1) << ArgTraits::getKind().asString() << Value.getTypeAsString();
       HasError = true;
       break;
     }
+    if (!ArgTraits::hasCorrectValue(Value)) {
+      if (llvm::Optional<std::string> BestGuess =
+              ArgTraits::getBestGuess(Value)) {
+        Error->addError(Arg.Range, Error->ET_RegistryUnknownEnumWithReplace)
+            << i + 1 << Value.getString() << *BestGuess;
+      } else if (Value.isString()) {
+        Error->addError(Arg.Range, Error->ET_RegistryValueNotFound)
+            << Value.getString();
+      } else {
+        // This isn't ideal, but it's better than reporting an empty string as
+        // the error in this case.
+        Error->addError(Arg.Range, Error->ET_RegistryWrongArgType)
+            << (i + 1) << ArgTraits::getKind().asString()
+            << Value.getTypeAsString();
+      }
+      HasError = true;
+      break;
+    }
+
     InnerArgs[i] = new ArgT(ArgTraits::get(Value));
   }
 
   VariantMatcher Out;
   if (!HasError) {
     Out = outvalueToVariantMatcher(Func(llvm::makeArrayRef(InnerArgs,
                                                            Args.size())));
   }

 #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)) {                           \
+  if (!ArgTypeTraits<type>::hasCorrectType(Args[index].Value)) {               \
+    Error->addError(Args[index].Range, Error->ET_RegistryWrongArgType)         \
+        << (index + 1) << ArgTypeTraits<type>::getKind().asString()            \
+        << Args[index].Value.getTypeAsString();                                \
+    return VariantMatcher();                                                   \
+  }                                                                            \
+  if (!ArgTypeTraits<type>::hasCorrectValue(Args[index].Value)) {              \
     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();                              \
+    } else if (Args[index].Value.isString()) {                                 \
+      Error->addError(Args[index].Range, Error->ET_RegistryValueNotFound)      \
+          << Args[index].Value.getString();                                    \
     }                                                                          \
     return VariantMatcher();                                                   \
   }
 
 /// 0-arg marshaller function.
 template <typename ReturnType>
 static VariantMatcher matcherMarshall0(void (*Func)(), StringRef MatcherName,
                                        SourceRange NameRange,
public:
 
   VariantMatcher create(SourceRange NameRange, ArrayRef<ParserValue> Args,
                         Diagnostics *Error) const override {
     if (Args.size() < 1 || Args.size() > 2) {
       Error->addError(NameRange, Diagnostics::ET_RegistryWrongArgCount)
           << "1 or 2" << Args.size();
       return VariantMatcher();
     }
-    if (!ArgTypeTraits<StringRef>::is(Args[0].Value)) {
+    if (!ArgTypeTraits<StringRef>::hasCorrectType(Args[0].Value)) {
       Error->addError(Args[0].Range, Error->ET_RegistryWrongArgType)
           << 1 << ArgTypeTraits<StringRef>::getKind().asString()
           << Args[0].Value.getTypeAsString();
       return VariantMatcher();
     }
     if (Args.size() == 1) {
       return outvalueToVariantMatcher(
           NoFlags(ArgTypeTraits<StringRef>::get(Args[0].Value)));
     }
-    if (!ArgTypeTraits<llvm::Regex::RegexFlags>::is(Args[1].Value)) {
+    if (!ArgTypeTraits<llvm::Regex::RegexFlags>::hasCorrectType(
+            Args[1].Value)) {
+      Error->addError(Args[1].Range, Error->ET_RegistryWrongArgType)
+          << 2 << ArgTypeTraits<llvm::Regex::RegexFlags>::getKind().asString()
+          << Args[1].Value.getTypeAsString();
+      return VariantMatcher();
+    }
+    if (!ArgTypeTraits<llvm::Regex::RegexFlags>::hasCorrectValue(
+            Args[1].Value)) {
       if (llvm::Optional<std::string> BestGuess =
               ArgTypeTraits<llvm::Regex::RegexFlags>::getBestGuess(
                   Args[1].Value)) {
         Error->addError(Args[1].Range, Error->ET_RegistryUnknownEnumWithReplace)
             << 2 << Args[1].Value.getString() << *BestGuess;
       } else {
-        Error->addError(Args[1].Range, Error->ET_RegistryWrongArgType)
-            << 2 << ArgTypeTraits<llvm::Regex::RegexFlags>::getKind().asString()
-            << Args[1].Value.getTypeAsString();
+        Error->addError(Args[1].Range, Error->ET_RegistryValueNotFound)
+            << Args[1].Value.getString();
       }
       return VariantMatcher();
     }
     return outvalueToVariantMatcher(
         WithFlags(ArgTypeTraits<StringRef>::get(Args[0].Value),
                   ArgTypeTraits<llvm::Regex::RegexFlags>::get(Args[1].Value)));
   }
 

clang/unittests/ASTMatchers/Dynamic/ParserTest.cpp

EXPECT_EQ(
       "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)",
+            "1:14: Value not found: unrelated",
             ParseMatcherWithError(R"query(decl(hasAttr("unrelated")))query"));
   EXPECT_EQ(
       "1:1: Error parsing argument 1 for matcher namedDecl.\n"
       "1:11: Error building matcher matchesName.\n"
       "1:33: Unknown value 'Ignorecase' for arg 2; did you mean 'IgnoreCase'",
       ParseMatcherWithError(
           R"query(namedDecl(matchesName("[ABC]*", "Ignorecase")))query"));
   EXPECT_EQ(
       "1:1: Error parsing argument 1 for matcher namedDecl.\n"
       "1:11: Error building matcher matchesName.\n"
-      "1:33: Incorrect type for arg 2. (Expected = string) != (Actual = "
-      "String)",
+      "1:33: Value not found: IgnoreCase & BasicRegex",
       ParseMatcherWithError(
           R"query(namedDecl(matchesName("[ABC]*", "IgnoreCase & BasicRegex")))query"));
   EXPECT_EQ(
       "1:1: Error parsing argument 1 for matcher namedDecl.\n"
       "1:11: Error building matcher matchesName.\n"
       "1:33: Unknown value 'IgnoreCase | Basicregex' for arg 2; did you mean "
       "'IgnoreCase | BasicRegex'",
       ParseMatcherWithError(