[PATCH] D85611: Improve dynamic AST matching diagnostics for conversion errors

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

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





Comment at: clang/lib/ASTMatchers/Dynamic/Marshallers.h:88
+  static bool hasCorrectValue(const VariantValue ) {
+return Value.getMatcher().hasTypedMatcher();
   }

sammccall wrote:
> 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)
That's a good idea for a future change!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85611/new/

https://reviews.llvm.org/D85611

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85611: Improve dynamic AST matching diagnostics for conversion errors

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

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




Comment at: clang/lib/ASTMatchers/Dynamic/Marshallers.h:88
+  static bool hasCorrectValue(const VariantValue ) {
+return Value.getMatcher().hasTypedMatcher();
   }

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)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85611/new/

https://reviews.llvm.org/D85611

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85611: Improve dynamic AST matching diagnostics for conversion errors

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85611/new/

https://reviews.llvm.org/D85611

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85611: Improve dynamic AST matching diagnostics for conversion errors

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D85611#2218144 , @aaron.ballman 
wrote:

> Ping

Ping x2


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85611/new/

https://reviews.llvm.org/D85611

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85611: Improve dynamic AST matching diagnostics for conversion errors

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Ping


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85611/new/

https://reviews.llvm.org/D85611

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85611: Improve dynamic AST matching diagnostics for conversion errors

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman created this revision.
aaron.ballman added reviewers: steveire, sbenza, klimek, sammccall.
aaron.ballman requested review of this revision.

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.


https://reviews.llvm.org/D85611

Files:
  clang/lib/ASTMatchers/Dynamic/Marshallers.h
  clang/unittests/ASTMatchers/Dynamic/ParserTest.cpp

Index: clang/unittests/ASTMatchers/Dynamic/ParserTest.cpp
===
--- clang/unittests/ASTMatchers/Dynamic/ParserTest.cpp
+++ clang/unittests/ASTMatchers/Dynamic/ParserTest.cpp
@@ -354,8 +354,7 @@
 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"
@@ -366,8 +365,7 @@
   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(
Index: clang/lib/ASTMatchers/Dynamic/Marshallers.h
===
--- clang/lib/ASTMatchers/Dynamic/Marshallers.h
+++ clang/lib/ASTMatchers/Dynamic/Marshallers.h
@@ -58,7 +58,10 @@
 };
 
 template <> struct ArgTypeTraits {
-  static bool is(const VariantValue ) { return Value.isString(); }
+  static bool hasCorrectType(const VariantValue ) {
+return Value.isString();
+  }
+  static bool hasCorrectValue(const VariantValue ) { return true; }
 
   static const std::string (const VariantValue ) {
 return Value.getString();
@@ -78,8 +81,11 @@
 };
 
 template  struct ArgTypeTraits> {
-  static bool is(const VariantValue ) {
-return Value.isMatcher() && Value.getMatcher().hasTypedMatcher();
+  static bool hasCorrectType(const VariantValue& Value) {
+return Value.isMatcher();
+  }
+  static bool hasCorrectValue(const VariantValue ) {
+return Value.getMatcher().hasTypedMatcher();
   }
 
   static ast_matchers::internal::Matcher get(const VariantValue ) {
@@ -96,7 +102,10 @@
 };
 
 template <> struct ArgTypeTraits {
-  static bool is(const VariantValue ) { return Value.isBoolean(); }
+  static bool hasCorrectType(const VariantValue ) {
+return Value.isBoolean();
+  }
+  static bool hasCorrectValue(const VariantValue ) { return true; }
 
   static bool get(const VariantValue ) {
 return Value.getBoolean();
@@ -112,7 +121,10 @@
 };
 
 template <> struct ArgTypeTraits {
-  static bool is(const VariantValue ) { return Value.isDouble(); }
+  static bool hasCorrectType(const VariantValue ) {
+return Value.isDouble();
+  }
+  static bool hasCorrectValue(const VariantValue ) { return true; }
 
   static double get(const VariantValue ) {
 return Value.getDouble();
@@ -128,7 +140,10 @@
 };
 
 template <> struct ArgTypeTraits {
-  static bool is(const VariantValue ) { return Value.isUnsigned(); }
+  static bool hasCorrectType(const VariantValue ) {
+return Value.isUnsigned();
+  }
+  static bool hasCorrectValue(const VariantValue ) { return true; }
 
   static unsigned get(const