From ecd7f7ee7fbbf438ff05a0b0ced26983e13fa6f6 Mon Sep 17 00:00:00 2001 From: Ben Clayton Date: Tue, 11 Oct 2022 19:34:19 +0000 Subject: [PATCH] tint/reader/wgsl: Improve errors when parsing extensions If the extension name doesn't parse, then generate an error message that includes the list of possible values, and a suggestion if there was a close match. Change-Id: I0eb2a682ca5a0717bb31d2716824663924ccd8f2 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/105324 Commit-Queue: Ben Clayton Kokoro: Kokoro Reviewed-by: Dan Sinclair --- src/tint/reader/wgsl/parser_impl.cc | 44 +++++++++---------- .../wgsl/parser_impl_enable_directive_test.cc | 25 +++++++++-- 2 files changed, 41 insertions(+), 28 deletions(-) diff --git a/src/tint/reader/wgsl/parser_impl.cc b/src/tint/reader/wgsl/parser_impl.cc index a03abf7e88..3097d50983 100644 --- a/src/tint/reader/wgsl/parser_impl.cc +++ b/src/tint/reader/wgsl/parser_impl.cc @@ -371,40 +371,36 @@ Maybe ParserImpl::enable_directive() { } // Match the extension name. - Expect name = {""}; auto& t = peek(); - if (t.IsIdentifier()) { - synchronized_ = true; - next(); - name = {t.to_str(), t.source()}; - } else if (t.Is(Token::Type::kF16)) { - // `f16` is a valid extension name and also a keyword - synchronized_ = true; - next(); - name = {"f16", t.source()}; - } else if (t.Is(Token::Type::kParenLeft)) { + if (handle_error(t)) { + // The token might itself be an error. + return Failure::kErrored; + } + + if (t.Is(Token::Type::kParenLeft)) { // A common error case is writing `enable(foo);` instead of `enable foo;`. synchronized_ = false; return add_error(t.source(), "enable directives don't take parenthesis"); - } else if (handle_error(t)) { - // The token might itself be an error. - return Failure::kErrored; + } + + auto extension = ast::Extension::kInvalid; + if (t.Is(Token::Type::kF16)) { + // `f16` is a valid extension name and also a keyword + synchronized_ = true; + next(); + extension = ast::Extension::kF16; } else { - // Failed to match an extension name. - synchronized_ = false; - return add_error(t.source(), "invalid extension name"); + auto ext = expect_enum("extension", ast::ParseExtension, ast::kExtensionStrings); + if (ext.errored) { + return Failure::kErrored; + } + extension = ext.value; } if (!expect("enable directive", Token::Type::kSemicolon)) { return Failure::kErrored; } - - auto extension = ast::ParseExtension(name.value); - if (extension == ast::Extension::kInvalid) { - return add_error(name.source, "unsupported extension: '" + name.value + "'"); - } - builder_.AST().AddEnable(create(name.source, extension)); - + builder_.AST().AddEnable(create(t.source(), extension)); return kSuccess; }); diff --git a/src/tint/reader/wgsl/parser_impl_enable_directive_test.cc b/src/tint/reader/wgsl/parser_impl_enable_directive_test.cc index ed57be8d28..aad8eb62b5 100644 --- a/src/tint/reader/wgsl/parser_impl_enable_directive_test.cc +++ b/src/tint/reader/wgsl/parser_impl_enable_directive_test.cc @@ -61,7 +61,21 @@ TEST_F(EnableDirectiveTest, InvalidIdentifier) { p->enable_directive(); // Error when unknown extension found EXPECT_TRUE(p->has_error()); - EXPECT_EQ(p->error(), "1:8: unsupported extension: 'NotAValidExtensionName'"); + EXPECT_EQ(p->error(), R"(1:8: expected extension +Possible values: 'chromium_disable_uniformity_analysis', 'chromium_experimental_dp4a', 'chromium_experimental_push_constant', 'f16')"); + auto program = p->program(); + auto& ast = program.AST(); + EXPECT_EQ(ast.Enables().Length(), 0u); + EXPECT_EQ(ast.GlobalDeclarations().Length(), 0u); +} + +TEST_F(EnableDirectiveTest, InvalidIdentifierSuggest) { + auto p = parser("enable f15;"); + p->enable_directive(); + // Error when unknown extension found + EXPECT_TRUE(p->has_error()); + EXPECT_EQ(p->error(), R"(1:8: expected extension. Did you mean 'f16'? +Possible values: 'chromium_disable_uniformity_analysis', 'chromium_experimental_dp4a', 'chromium_experimental_push_constant', 'f16')"); auto program = p->program(); auto& ast = program.AST(); EXPECT_EQ(ast.Enables().Length(), 0u); @@ -108,7 +122,8 @@ TEST_F(EnableDirectiveTest, InvalidTokens) { auto p = parser("enable translation_unit(); EXPECT_TRUE(p->has_error()); - EXPECT_EQ(p->error(), "1:8: invalid extension name"); + EXPECT_EQ(p->error(), R"(1:8: expected extension +Possible values: 'chromium_disable_uniformity_analysis', 'chromium_experimental_dp4a', 'chromium_experimental_push_constant', 'f16')"); auto program = p->program(); auto& ast = program.AST(); EXPECT_EQ(ast.Enables().Length(), 0u); @@ -118,7 +133,8 @@ TEST_F(EnableDirectiveTest, InvalidTokens) { auto p = parser("enable =;"); p->translation_unit(); EXPECT_TRUE(p->has_error()); - EXPECT_EQ(p->error(), "1:8: invalid extension name"); + EXPECT_EQ(p->error(), R"(1:8: expected extension +Possible values: 'chromium_disable_uniformity_analysis', 'chromium_experimental_dp4a', 'chromium_experimental_push_constant', 'f16')"); auto program = p->program(); auto& ast = program.AST(); EXPECT_EQ(ast.Enables().Length(), 0u); @@ -128,7 +144,8 @@ TEST_F(EnableDirectiveTest, InvalidTokens) { auto p = parser("enable vec4;"); p->translation_unit(); EXPECT_TRUE(p->has_error()); - EXPECT_EQ(p->error(), "1:8: invalid extension name"); + EXPECT_EQ(p->error(), R"(1:8: expected extension +Possible values: 'chromium_disable_uniformity_analysis', 'chromium_experimental_dp4a', 'chromium_experimental_push_constant', 'f16')"); auto program = p->program(); auto& ast = program.AST(); EXPECT_EQ(ast.Enables().Length(), 0u);