tint/reader/wgsl: Improve errors when parsing builtin

If the builtin doesn't parse, then generate an error message that includes the list of possible values, and a suggestion if there was a close match.

Fixed: tint:1629
Change-Id: I8f575a2ffcef2af308b9566ae7832702e76085ef
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/105323
Reviewed-by: Dan Sinclair <dsinclair@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Ben Clayton <bclayton@chromium.org>

src/tint/reader/wgsl/parser_impl.cc

@@ -43,6 +43,7 @@
 #include "src/tint/sem/external_texture.h"
 #include "src/tint/sem/multisampled_texture.h"
 #include "src/tint/sem/sampled_texture.h"
+#include "src/tint/utils/string.h"
 
 namespace tint::reader::wgsl {
 namespace {
@@ -1194,6 +1195,66 @@ Maybe<const ast::Type*> ParserImpl::type_specifier() {
     return type_specifier_without_ident();
 }
 
+template <typename ENUM, size_t N>
+Expect<ENUM> ParserImpl::expect_enum(std::string_view name,
+                                     ENUM (*parse)(std::string_view str),
+                                     const char* const (&strings)[N],
+                                     std::string_view use) {
+    auto& t = peek();
+    if (t.IsIdentifier()) {
+        auto val = parse(t.to_str());
+        if (val != ENUM::kInvalid) {
+            synchronized_ = true;
+            next();
+            return {val, t.source()};
+        }
+    }
+
+    // Was the token itself an error?
+    if (handle_error(t)) {
+        return Failure::kErrored;
+    }
+
+    /// Create a sensible error message
+    std::stringstream err;
+    err << "expected " << name;
+
+    if (!use.empty()) {
+        err << " for " << use;
+    }
+
+    // If the string typed was within kSuggestionDistance of one of the possible enum values,
+    // suggest that. Don't bother with suggestions if the string was extremely long.
+    constexpr size_t kSuggestionDistance = 5;
+    constexpr size_t kSuggestionMaxLength = 64;
+    if (auto got = t.to_str(); !got.empty() && got.size() < kSuggestionMaxLength) {
+        size_t candidate_dist = kSuggestionDistance;
+        const char* candidate = nullptr;
+        for (auto* str : strings) {
+            auto dist = utils::Distance(str, got);
+            if (dist < candidate_dist) {
+                candidate = str;
+                candidate_dist = dist;
+            }
+        }
+        if (candidate) {
+            err << ". Did you mean '" << candidate << "'?";
+        }
+    }
+
+    // List all the possible enumerator values
+    err << "\nPossible values: ";
+    for (auto* str : strings) {
+        if (str != strings[0]) {
+            err << ", ";
+        }
+        err << "'" << str << "'";
+    }
+
+    synchronized_ = false;
+    return add_error(t.source(), err.str());
+}
+
 Expect<const ast::Type*> ParserImpl::expect_type(std::string_view use) {
     auto type = type_specifier();
     if (type.errored) {
@@ -1663,17 +1724,7 @@ Expect<ast::InterpolationType> ParserImpl::expect_interpolation_type_name() {
 //   | vertex_index
 //   | workgroup_id
 Expect<ast::BuiltinValue> ParserImpl::expect_builtin() {
-    auto ident = expect_ident("builtin");
+    return expect_enum("builtin", ast::ParseBuiltinValue, ast::kBuiltinValueStrings);
-    if (ident.errored) {
-        return Failure::kErrored;
-    }
-
-    ast::BuiltinValue builtin = ast::ParseBuiltinValue(ident.value);
-    if (builtin == ast::BuiltinValue::kInvalid) {
-        return add_error(ident.source, "invalid value for builtin attribute");
-    }
-
-    return {builtin, ident.source};
 }
 
 // compound_statement

src/tint/reader/wgsl/parser_impl.h

@@ -868,6 +868,18 @@ class ParserImpl {
     Expect<const ast::Type*> expect_type_specifier_matrix(const Source& s,
                                                           const MatrixDimensions& dims);
 
+    /// Parses the given enum, providing sensible error messages if the next token does not match
+    /// any of the enum values.
+    /// @param name the name of the enumerator
+    /// @param parse the optimized function used to parse the enum
+    /// @param strings the list of possible strings in the enum
+    /// @param use an optional description of what was being parsed if an error was raised.
+    template <typename ENUM, size_t N>
+    Expect<ENUM> expect_enum(std::string_view name,
+                             ENUM (*parse)(std::string_view str),
+                             const char* const (&strings)[N],
+                             std::string_view use = "");
+
     Expect<const ast::Type*> expect_type(std::string_view use);
 
     Maybe<const ast::Statement*> non_block_statement();

src/tint/reader/wgsl/parser_impl_error_msg_test.cc

@@ -765,7 +765,8 @@ type T = i32;
 }
 
 TEST_F(ParserImplErrorTest, GlobalDeclStaticAssertMissingLParen) {
-    EXPECT("static_assert true);", R"(test.wgsl:1:19 error: expected ';' for static assertion declaration
+    EXPECT("static_assert true);",
+           R"(test.wgsl:1:19 error: expected ';' for static assertion declaration
 static_assert true);
                   ^
 )");
@@ -989,17 +990,19 @@ TEST_F(ParserImplErrorTest, GlobalDeclVarAttrBuiltinMissingRParen) {
 
 TEST_F(ParserImplErrorTest, GlobalDeclVarAttrBuiltinInvalidIdentifer) {
     EXPECT("@builtin(1) var i : i32;",
-           R"(test.wgsl:1:10 error: expected identifier for builtin
+           R"(test.wgsl:1:10 error: expected builtin
+Possible values: 'frag_depth', 'front_facing', 'global_invocation_id', 'instance_index', 'local_invocation_id', 'local_invocation_index', 'num_workgroups', 'position', 'sample_index', 'sample_mask', 'vertex_index', 'workgroup_id'
 @builtin(1) var i : i32;
          ^
 )");
 }
 
 TEST_F(ParserImplErrorTest, GlobalDeclVarAttrBuiltinInvalidValue) {
-    EXPECT("@builtin(x) var i : i32;",
+    EXPECT("@builtin(frag_d3pth) var i : i32;",
-           R"(test.wgsl:1:10 error: invalid value for builtin attribute
+           R"(test.wgsl:1:10 error: expected builtin. Did you mean 'frag_depth'?
-@builtin(x) var i : i32;
+Possible values: 'frag_depth', 'front_facing', 'global_invocation_id', 'instance_index', 'local_invocation_id', 'local_invocation_index', 'num_workgroups', 'position', 'sample_index', 'sample_mask', 'vertex_index', 'workgroup_id'
-         ^
+@builtin(frag_d3pth) var i : i32;
+         ^^^^^^^^^^
 )");
 }
 

src/tint/reader/wgsl/parser_impl_variable_attribute_list_test.cc

@@ -58,8 +58,19 @@ TEST_F(ParserImplTest, AttributeList_InvalidValue) {
     EXPECT_TRUE(attrs.errored);
     EXPECT_FALSE(attrs.matched);
     EXPECT_TRUE(attrs.value.IsEmpty());
-    EXPECT_EQ(p->error(), "1:10: invalid value for builtin attribute");
+    EXPECT_EQ(p->error(), R"(1:10: expected builtin
+Possible values: 'frag_depth', 'front_facing', 'global_invocation_id', 'instance_index', 'local_invocation_id', 'local_invocation_index', 'num_workgroups', 'position', 'sample_index', 'sample_mask', 'vertex_index', 'workgroup_id')");
 }
 
+TEST_F(ParserImplTest, AttributeList_InvalidValueSuggest) {
+    auto p = parser("@builtin(instanceindex)");
+    auto attrs = p->attribute_list();
+    EXPECT_TRUE(p->has_error());
+    EXPECT_TRUE(attrs.errored);
+    EXPECT_FALSE(attrs.matched);
+    EXPECT_TRUE(attrs.value.IsEmpty());
+    EXPECT_EQ(p->error(), R"(1:10: expected builtin. Did you mean 'instance_index'?
+Possible values: 'frag_depth', 'front_facing', 'global_invocation_id', 'instance_index', 'local_invocation_id', 'local_invocation_index', 'num_workgroups', 'position', 'sample_index', 'sample_mask', 'vertex_index', 'workgroup_id')");
+}
 }  // namespace
 }  // namespace tint::reader::wgsl

src/tint/reader/wgsl/parser_impl_variable_attribute_test.cc

@@ -177,7 +177,8 @@ TEST_F(ParserImplTest, Attribute_Builtin_MissingValue) {
     EXPECT_TRUE(attr.errored);
     EXPECT_EQ(attr.value, nullptr);
     EXPECT_TRUE(p->has_error());
-    EXPECT_EQ(p->error(), "1:9: expected identifier for builtin");
+    EXPECT_EQ(p->error(), R"(1:9: expected builtin
+Possible values: 'frag_depth', 'front_facing', 'global_invocation_id', 'instance_index', 'local_invocation_id', 'local_invocation_index', 'num_workgroups', 'position', 'sample_index', 'sample_mask', 'vertex_index', 'workgroup_id')");
 }
 
 TEST_F(ParserImplTest, Attribute_Builtin_InvalidValue) {
@@ -187,7 +188,19 @@ TEST_F(ParserImplTest, Attribute_Builtin_InvalidValue) {
     EXPECT_TRUE(attr.errored);
     EXPECT_EQ(attr.value, nullptr);
     EXPECT_TRUE(p->has_error());
-    EXPECT_EQ(p->error(), "1:9: invalid value for builtin attribute");
+    EXPECT_EQ(p->error(), R"(1:9: expected builtin
+Possible values: 'frag_depth', 'front_facing', 'global_invocation_id', 'instance_index', 'local_invocation_id', 'local_invocation_index', 'num_workgroups', 'position', 'sample_index', 'sample_mask', 'vertex_index', 'workgroup_id')");
+}
+
+TEST_F(ParserImplTest, Attribute_Builtin_InvalidValueSuggest) {
+    auto p = parser("builtin(front_face)");
+    auto attr = p->attribute();
+    EXPECT_FALSE(attr.matched);
+    EXPECT_TRUE(attr.errored);
+    EXPECT_EQ(attr.value, nullptr);
+    EXPECT_TRUE(p->has_error());
+    EXPECT_EQ(p->error(), R"(1:9: expected builtin. Did you mean 'front_facing'?
+Possible values: 'frag_depth', 'front_facing', 'global_invocation_id', 'instance_index', 'local_invocation_id', 'local_invocation_index', 'num_workgroups', 'position', 'sample_index', 'sample_mask', 'vertex_index', 'workgroup_id')");
 }
 
 TEST_F(ParserImplTest, Attribute_Builtin_MissingInvalid) {
@@ -197,7 +210,8 @@ TEST_F(ParserImplTest, Attribute_Builtin_MissingInvalid) {
     EXPECT_TRUE(attr.errored);
     EXPECT_EQ(attr.value, nullptr);
     EXPECT_TRUE(p->has_error());
-    EXPECT_EQ(p->error(), "1:9: expected identifier for builtin");
+    EXPECT_EQ(p->error(), R"(1:9: expected builtin
+Possible values: 'frag_depth', 'front_facing', 'global_invocation_id', 'instance_index', 'local_invocation_id', 'local_invocation_index', 'num_workgroups', 'position', 'sample_index', 'sample_mask', 'vertex_index', 'workgroup_id')");
 }
 
 TEST_F(ParserImplTest, Attribute_Interpolate_Flat) {