From 8dd35110c28830d2d6f81145b142860d57bfc845 Mon Sep 17 00:00:00 2001 From: James Price Date: Tue, 24 Jan 2023 21:01:36 +0000 Subject: [PATCH] wgsl/reader: Parse diagnostic directives Bug: tint:1809 Change-Id: I9f129685f22f51f577b67050edeef8aeea0c0bc5 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/117569 Reviewed-by: Ben Clayton Kokoro: Kokoro --- src/tint/BUILD.gn | 2 + src/tint/CMakeLists.txt | 2 + src/tint/reader/wgsl/parser_impl.cc | 80 +++++++++++- src/tint/reader/wgsl/parser_impl.h | 9 ++ .../parser_impl_diagnostic_control_test.cc | 119 ++++++++++++++++++ .../parser_impl_diagnostic_directive_test.cc | 71 +++++++++++ .../wgsl/parser_impl_enable_directive_test.cc | 4 +- 7 files changed, 280 insertions(+), 7 deletions(-) create mode 100644 src/tint/reader/wgsl/parser_impl_diagnostic_control_test.cc create mode 100644 src/tint/reader/wgsl/parser_impl_diagnostic_directive_test.cc diff --git a/src/tint/BUILD.gn b/src/tint/BUILD.gn index a0e1292e3c..92565b2500 100644 --- a/src/tint/BUILD.gn +++ b/src/tint/BUILD.gn @@ -1594,6 +1594,8 @@ if (tint_build_unittests) { "reader/wgsl/parser_impl_continuing_stmt_test.cc", "reader/wgsl/parser_impl_core_lhs_expression_test.cc", "reader/wgsl/parser_impl_depth_texture_test.cc", + "reader/wgsl/parser_impl_diagnostic_control_test.cc", + "reader/wgsl/parser_impl_diagnostic_directive_test.cc", "reader/wgsl/parser_impl_element_count_expression_test.cc", "reader/wgsl/parser_impl_enable_directive_test.cc", "reader/wgsl/parser_impl_error_msg_test.cc", diff --git a/src/tint/CMakeLists.txt b/src/tint/CMakeLists.txt index f25bb02396..6c2a8e48a6 100644 --- a/src/tint/CMakeLists.txt +++ b/src/tint/CMakeLists.txt @@ -1097,6 +1097,8 @@ if(TINT_BUILD_TESTS) reader/wgsl/parser_impl_continuing_stmt_test.cc reader/wgsl/parser_impl_core_lhs_expression_test.cc reader/wgsl/parser_impl_depth_texture_test.cc + reader/wgsl/parser_impl_diagnostic_control_test.cc + reader/wgsl/parser_impl_diagnostic_directive_test.cc reader/wgsl/parser_impl_element_count_expression_test.cc reader/wgsl/parser_impl_enable_directive_test.cc reader/wgsl/parser_impl_error_msg_test.cc diff --git a/src/tint/reader/wgsl/parser_impl.cc b/src/tint/reader/wgsl/parser_impl.cc index 1110f5cd33..e8653bbaa9 100644 --- a/src/tint/reader/wgsl/parser_impl.cc +++ b/src/tint/reader/wgsl/parser_impl.cc @@ -360,14 +360,47 @@ void ParserImpl::translation_unit() { } // global_directive -// : enable_directive +// : diagnostic_directive +// | enable_directive Maybe ParserImpl::global_directive(bool have_parsed_decl) { auto& p = peek(); - auto ed = enable_directive(); - if (ed.matched && have_parsed_decl) { - return add_error(p, "enable directives must come before all global declarations"); + Maybe result = diagnostic_directive(); + if (!result.errored && !result.matched) { + result = enable_directive(); } - return ed; + + if (result.matched && have_parsed_decl) { + return add_error(p, "directives must come before all global declarations"); + } + return result; +} + +// diagnostic_directive +// : diagnostic diagnostic_control SEMICOLON +Maybe ParserImpl::diagnostic_directive() { + auto decl = sync(Token::Type::kSemicolon, [&]() -> Maybe { + if (!match(Token::Type::kDiagnostic)) { + return Failure::kNoMatch; + } + + auto control = expect_diagnostic_control(); + if (control.errored) { + return Failure::kErrored; + } + + if (!expect("diagnostic directive", Token::Type::kSemicolon)) { + return Failure::kErrored; + } + + builder_.AST().AddDiagnosticControl(std::move(control.value)); + + return kSuccess; + }); + + if (decl.errored) { + return Failure::kErrored; + } + return decl; } // enable_directive @@ -3697,6 +3730,43 @@ bool ParserImpl::expect_attributes_consumed(utils::VectorRef ParserImpl::expect_severity_control_name() { + return expect_enum("severity control", ast::ParseDiagnosticSeverity, + ast::kDiagnosticSeverityStrings); +} + +// diagnostic_control +// : PAREN_LEFT severity_control_name COMMA ident_pattern_token COMMA ? PAREN_RIGHT +Expect ParserImpl::expect_diagnostic_control() { + auto source = last_source(); + return expect_paren_block("diagnostic control", [&]() -> Expect { + auto severity_control = expect_severity_control_name(); + if (severity_control.errored) { + return Failure::kErrored; + } + + if (!expect("diagnostic control", Token::Type::kComma)) { + return Failure::kErrored; + } + + auto rule_name = expect_ident("diagnostic control"); + if (rule_name.errored) { + return Failure::kErrored; + } + match(Token::Type::kComma); + + return create( + source, severity_control.value, + create(rule_name.source, + builder_.Symbols().Register(rule_name.value))); + }); +} + bool ParserImpl::match(Token::Type tok, Source* source /*= nullptr*/) { auto& t = peek(); diff --git a/src/tint/reader/wgsl/parser_impl.h b/src/tint/reader/wgsl/parser_impl.h index ed7c226ea6..8101261437 100644 --- a/src/tint/reader/wgsl/parser_impl.h +++ b/src/tint/reader/wgsl/parser_impl.h @@ -402,6 +402,9 @@ class ParserImpl { /// @param has_parsed_decl flag indicating if the parser has consumed a global declaration. /// @return true on parse success, otherwise an error or no-match. Maybe global_directive(bool has_parsed_decl); + /// Parses the `diagnostic_directive` grammar element, erroring on parse failure. + /// @return true on parse success, otherwise an error or no-match. + Maybe diagnostic_directive(); /// Parses the `enable_directive` grammar element, erroring on parse failure. /// @return true on parse success, otherwise an error or no-match. Maybe enable_directive(); @@ -702,6 +705,12 @@ class ParserImpl { /// @see #attribute for the full list of attributes this method parses. /// @return the parsed attribute, or nullptr on error. Expect expect_attribute(); + /// Parses a severity_control_name grammar element. + /// @return the parsed severity control name, or nullptr on error. + Expect expect_severity_control_name(); + /// Parses a diagnostic_control grammar element. + /// @return the parsed diagnostic control, or nullptr on error. + Expect expect_diagnostic_control(); /// Splits a peekable token into to parts filling in the peekable fields. /// @param lhs the token to set in the current position diff --git a/src/tint/reader/wgsl/parser_impl_diagnostic_control_test.cc b/src/tint/reader/wgsl/parser_impl_diagnostic_control_test.cc new file mode 100644 index 0000000000..36de427a15 --- /dev/null +++ b/src/tint/reader/wgsl/parser_impl_diagnostic_control_test.cc @@ -0,0 +1,119 @@ +// Copyright 2023 The Tint Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "src/tint/reader/wgsl/parser_impl_test_helper.h" + +#include "src/tint/ast/diagnostic_control.h" + +namespace tint::reader::wgsl { +namespace { + +using SeverityPair = std::pair; +class DiagnosticControlParserTest : public ParserImplTestWithParam {}; + +TEST_P(DiagnosticControlParserTest, DiagnosticControl_Valid) { + auto& params = GetParam(); + auto p = parser("(" + params.first + ", foo)"); + auto e = p->expect_diagnostic_control(); + EXPECT_FALSE(e.errored); + EXPECT_FALSE(p->has_error()) << p->error(); + ASSERT_NE(e.value, nullptr); + ASSERT_TRUE(e->Is()); + EXPECT_EQ(e->severity, params.second); + + auto* r = As(e->rule_name); + ASSERT_NE(r, nullptr); + EXPECT_EQ(p->builder().Symbols().NameFor(r->symbol), "foo"); +} +INSTANTIATE_TEST_SUITE_P(DiagnosticControlParserTest, + DiagnosticControlParserTest, + testing::Values(SeverityPair{"error", ast::DiagnosticSeverity::kError}, + SeverityPair{"warning", ast::DiagnosticSeverity::kWarning}, + SeverityPair{"info", ast::DiagnosticSeverity::kInfo}, + SeverityPair{"off", ast::DiagnosticSeverity::kOff})); + +TEST_F(ParserImplTest, DiagnosticControl_Valid_TrailingComma) { + auto p = parser("(error, foo,)"); + auto e = p->expect_diagnostic_control(); + EXPECT_FALSE(e.errored); + EXPECT_FALSE(p->has_error()) << p->error(); + ASSERT_NE(e.value, nullptr); + ASSERT_TRUE(e->Is()); + EXPECT_EQ(e->severity, ast::DiagnosticSeverity::kError); + + auto* r = As(e->rule_name); + ASSERT_NE(r, nullptr); + EXPECT_EQ(p->builder().Symbols().NameFor(r->symbol), "foo"); +} + +TEST_F(ParserImplTest, DiagnosticControl_MissingOpenParen) { + auto p = parser("off, foo)"); + auto e = p->expect_diagnostic_control(); + EXPECT_TRUE(e.errored); + EXPECT_TRUE(p->has_error()); + EXPECT_EQ(p->error(), R"(1:1: expected '(' for diagnostic control)"); +} + +TEST_F(ParserImplTest, DiagnosticControl_MissingCloseParen) { + auto p = parser("(off, foo"); + auto e = p->expect_diagnostic_control(); + EXPECT_TRUE(e.errored); + EXPECT_TRUE(p->has_error()); + EXPECT_EQ(p->error(), R"(1:10: expected ')' for diagnostic control)"); +} + +TEST_F(ParserImplTest, DiagnosticControl_MissingDiagnosticSeverity) { + auto p = parser("(, foo"); + auto e = p->expect_diagnostic_control(); + EXPECT_TRUE(e.errored); + EXPECT_TRUE(p->has_error()); + EXPECT_EQ(p->error(), R"(1:2: expected severity control +Possible values: 'error', 'info', 'off', 'warning')"); +} + +TEST_F(ParserImplTest, DiagnosticControl_InvalidDiagnosticSeverity) { + auto p = parser("(fatal, foo)"); + auto e = p->expect_diagnostic_control(); + EXPECT_TRUE(e.errored); + EXPECT_TRUE(p->has_error()); + EXPECT_EQ(p->error(), R"(1:2: expected severity control +Possible values: 'error', 'info', 'off', 'warning')"); +} + +TEST_F(ParserImplTest, DiagnosticControl_MissingComma) { + auto p = parser("(off foo"); + auto e = p->expect_diagnostic_control(); + EXPECT_TRUE(e.errored); + EXPECT_TRUE(p->has_error()); + EXPECT_EQ(p->error(), R"(1:6: expected ',' for diagnostic control)"); +} + +TEST_F(ParserImplTest, DiagnosticControl_MissingRuleName) { + auto p = parser("(off,)"); + auto e = p->expect_diagnostic_control(); + EXPECT_TRUE(e.errored); + EXPECT_TRUE(p->has_error()); + EXPECT_EQ(p->error(), R"(1:6: expected identifier for diagnostic control)"); +} + +TEST_F(ParserImplTest, DiagnosticControl_InvalidRuleName) { + auto p = parser("(off, foo$bar)"); + auto e = p->expect_diagnostic_control(); + EXPECT_TRUE(e.errored); + EXPECT_TRUE(p->has_error()); + EXPECT_EQ(p->error(), R"(1:10: invalid character found)"); +} + +} // namespace +} // namespace tint::reader::wgsl diff --git a/src/tint/reader/wgsl/parser_impl_diagnostic_directive_test.cc b/src/tint/reader/wgsl/parser_impl_diagnostic_directive_test.cc new file mode 100644 index 0000000000..c91b7d1787 --- /dev/null +++ b/src/tint/reader/wgsl/parser_impl_diagnostic_directive_test.cc @@ -0,0 +1,71 @@ +// Copyright 2023 The Tint Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "src/tint/reader/wgsl/parser_impl_test_helper.h" + +#include "src/tint/ast/diagnostic_control.h" + +namespace tint::reader::wgsl { +namespace { + +TEST_F(ParserImplTest, DiagnosticDirective_Valid) { + auto p = parser("diagnostic(off, foo);"); + p->diagnostic_directive(); + EXPECT_FALSE(p->has_error()) << p->error(); + auto& ast = p->builder().AST(); + ASSERT_EQ(ast.DiagnosticControls().Length(), 1u); + auto* control = ast.DiagnosticControls()[0]; + EXPECT_EQ(control->severity, ast::DiagnosticSeverity::kOff); + ASSERT_EQ(ast.GlobalDeclarations().Length(), 1u); + EXPECT_EQ(ast.GlobalDeclarations()[0], control); + + auto* r = As(control->rule_name); + ASSERT_NE(r, nullptr); + EXPECT_EQ(p->builder().Symbols().NameFor(r->symbol), "foo"); +} + +TEST_F(ParserImplTest, DiagnosticDirective_MissingSemicolon) { + auto p = parser("diagnostic(off, foo)"); + p->translation_unit(); + EXPECT_TRUE(p->has_error()); + EXPECT_EQ(p->error(), "1:21: expected ';' for diagnostic directive"); + auto program = p->program(); + auto& ast = program.AST(); + EXPECT_EQ(ast.DiagnosticControls().Length(), 0u); + EXPECT_EQ(ast.GlobalDeclarations().Length(), 0u); +} + +TEST_F(ParserImplTest, DiagnosticDirective_FollowingOtherGlobalDecl) { + auto p = parser(R"( +var t: f32 = 0f; +diagnostic(off, foo); +)"); + p->translation_unit(); + EXPECT_TRUE(p->has_error()); + EXPECT_EQ(p->error(), "3:1: directives must come before all global declarations"); +} + +TEST_F(ParserImplTest, DiagnosticDirective_FollowingEmptySemicolon) { + auto p = parser(R"( +; +diagnostic(off, foo); +)"); + p->translation_unit(); + // An empty semicolon is treated as a global declaration. + EXPECT_TRUE(p->has_error()); + EXPECT_EQ(p->error(), "3:1: directives must come before all global declarations"); +} + +} // namespace +} // namespace tint::reader::wgsl 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 392f0dd2a3..23a8cb2076 100644 --- a/src/tint/reader/wgsl/parser_impl_enable_directive_test.cc +++ b/src/tint/reader/wgsl/parser_impl_enable_directive_test.cc @@ -161,7 +161,7 @@ enable f16; )"); p->translation_unit(); EXPECT_TRUE(p->has_error()); - EXPECT_EQ(p->error(), "3:1: enable directives must come before all global declarations"); + EXPECT_EQ(p->error(), "3:1: directives must come before all global declarations"); auto program = p->program(); auto& ast = program.AST(); // Accept the enable directive although it caused an error @@ -181,7 +181,7 @@ enable f16; p->translation_unit(); // An empty semicolon is treated as a global declaration EXPECT_TRUE(p->has_error()); - EXPECT_EQ(p->error(), "3:1: enable directives must come before all global declarations"); + EXPECT_EQ(p->error(), "3:1: directives must come before all global declarations"); auto program = p->program(); auto& ast = program.AST(); // Accept the enable directive although it cause an error