From 187e0d5fe78c5996e93d5bb2981e2b16a88291db Mon Sep 17 00:00:00 2001 From: dan sinclair Date: Wed, 22 Feb 2023 20:57:28 +0000 Subject: [PATCH] Add `requires` directive This Cl adds the requires directive into Tint. Using the directive is currently always an error as there is no valid value which can be used. Bug: tint:1843 Change-Id: Idf77ba4e95ff0c1e177d02d1ba9598edc89a9812 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/120740 Commit-Queue: Dan Sinclair Kokoro: Kokoro Reviewed-by: David Neto Reviewed-by: Ben Clayton --- src/tint/BUILD.gn | 1 + src/tint/CMakeLists.txt | 1 + src/tint/reader/wgsl/lexer.cc | 3 + src/tint/reader/wgsl/lexer_test.cc | 1 + src/tint/reader/wgsl/parser_impl.cc | 71 ++++++++++++++++++- src/tint/reader/wgsl/parser_impl.h | 3 + .../parser_impl_require_directive_test.cc | 71 +++++++++++++++++++ .../wgsl/parser_impl_reserved_keyword_test.cc | 1 - src/tint/reader/wgsl/token.cc | 2 + src/tint/reader/wgsl/token.h | 2 + 10 files changed, 152 insertions(+), 4 deletions(-) create mode 100644 src/tint/reader/wgsl/parser_impl_require_directive_test.cc diff --git a/src/tint/BUILD.gn b/src/tint/BUILD.gn index abedec504b..4bcfdfd185 100644 --- a/src/tint/BUILD.gn +++ b/src/tint/BUILD.gn @@ -1716,6 +1716,7 @@ if (tint_build_unittests) { "reader/wgsl/parser_impl_paren_expression_test.cc", "reader/wgsl/parser_impl_primary_expression_test.cc", "reader/wgsl/parser_impl_relational_expression_test.cc", + "reader/wgsl/parser_impl_require_directive_test.cc", "reader/wgsl/parser_impl_reserved_keyword_test.cc", "reader/wgsl/parser_impl_shift_expression_test.cc", "reader/wgsl/parser_impl_singular_expression_test.cc", diff --git a/src/tint/CMakeLists.txt b/src/tint/CMakeLists.txt index dfd9624a64..fff86d0d17 100644 --- a/src/tint/CMakeLists.txt +++ b/src/tint/CMakeLists.txt @@ -1099,6 +1099,7 @@ if(TINT_BUILD_TESTS) reader/wgsl/parser_impl_primary_expression_test.cc reader/wgsl/parser_impl_relational_expression_test.cc reader/wgsl/parser_impl_reserved_keyword_test.cc + reader/wgsl/parser_impl_require_directive_test.cc reader/wgsl/parser_impl_shift_expression_test.cc reader/wgsl/parser_impl_singular_expression_test.cc reader/wgsl/parser_impl_statement_test.cc diff --git a/src/tint/reader/wgsl/lexer.cc b/src/tint/reader/wgsl/lexer.cc index 74c1dba941..5171e4c297 100644 --- a/src/tint/reader/wgsl/lexer.cc +++ b/src/tint/reader/wgsl/lexer.cc @@ -1181,6 +1181,9 @@ Token Lexer::check_keyword(const Source& source, std::string_view str) { if (str == "return") { return {Token::Type::kReturn, source, "return"}; } + if (str == "requires") { + return {Token::Type::kRequires, source, "requires"}; + } if (str == "struct") { return {Token::Type::kStruct, source, "struct"}; } diff --git a/src/tint/reader/wgsl/lexer_test.cc b/src/tint/reader/wgsl/lexer_test.cc index 5a3fd6dc91..95b43a7da0 100644 --- a/src/tint/reader/wgsl/lexer_test.cc +++ b/src/tint/reader/wgsl/lexer_test.cc @@ -1078,6 +1078,7 @@ INSTANTIATE_TEST_SUITE_P(LexerTest, TokenData{"loop", Token::Type::kLoop}, TokenData{"override", Token::Type::kOverride}, TokenData{"return", Token::Type::kReturn}, + TokenData{"requires", Token::Type::kRequires}, TokenData{"struct", Token::Type::kStruct}, TokenData{"switch", Token::Type::kSwitch}, TokenData{"true", Token::Type::kTrue}, diff --git a/src/tint/reader/wgsl/parser_impl.cc b/src/tint/reader/wgsl/parser_impl.cc index 2d74b20363..703c1012d3 100644 --- a/src/tint/reader/wgsl/parser_impl.cc +++ b/src/tint/reader/wgsl/parser_impl.cc @@ -94,9 +94,9 @@ bool is_reserved(const Token& t) { t == "partition" || t == "pass" || t == "patch" || t == "pixelfragment" || t == "precise" || t == "precision" || t == "premerge" || t == "priv" || t == "protected" || t == "pub" || t == "public" || t == "readonly" || t == "ref" || - t == "regardless" || t == "register" || t == "reinterpret_cast" || t == "requires" || - t == "resource" || t == "restrict" || t == "self" || t == "set" || t == "shared" || - t == "signed" || t == "sizeof" || t == "smooth" || t == "snorm" || t == "static" || + t == "regardless" || t == "register" || t == "reinterpret_cast" || t == "resource" || + t == "restrict" || t == "self" || t == "set" || t == "shared" || t == "signed" || + t == "sizeof" || t == "smooth" || t == "snorm" || t == "static" || t == "static_assert" || t == "static_cast" || t == "std" || t == "subroutine" || t == "super" || t == "target" || t == "template" || t == "this" || t == "thread_local" || t == "throw" || t == "trait" || t == "try" || t == "type" || t == "typedef" || @@ -341,6 +341,7 @@ void ParserImpl::translation_unit() { // global_directive // : diagnostic_directive +// | requires_directive // | enable_directive Maybe ParserImpl::global_directive(bool have_parsed_decl) { auto& p = peek(); @@ -348,6 +349,9 @@ Maybe ParserImpl::global_directive(bool have_parsed_decl) { if (!result.errored && !result.matched) { result = enable_directive(); } + if (!result.errored && !result.matched) { + result = requires_directive(); + } if (result.matched && have_parsed_decl) { return add_error(p, "directives must come before all global declarations"); @@ -428,6 +432,67 @@ Maybe ParserImpl::enable_directive() { return Failure::kNoMatch; } +// requires_directive +// : require identifier (COMMA identifier)? SEMICLON +Maybe ParserImpl::requires_directive() { + auto decl = sync(Token::Type::kSemicolon, [&]() -> Maybe { + if (!match(Token::Type::kRequires)) { + return Failure::kNoMatch; + } + + // Match the require name. + auto& t = peek(); + 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 `require(foo);` instead of `require foo;`. + synchronized_ = false; + return add_error(t.source(), "requires directives don't take parenthesis"); + } + + while (continue_parsing()) { + auto& t2 = peek(); + + // Match the require name. + if (handle_error(t2)) { + // The token might itself be an error. + return Failure::kErrored; + } + + if (t2.IsIdentifier()) { + // TODO(dsinclair): When there are actual values for a requires directive they + // should be checked here. + + // Any identifer is a valid feature name, so we correctly handle new feature + // names getting added in the future, they just all get flagged as not supported. + return add_error(t2.source(), "feature '" + t2.to_str() + "' is not supported"); + } + if (t2.Is(Token::Type::kSemicolon)) { + break; + } + if (!match(Token::Type::kComma)) { + return add_error(t2.source(), "invalid feature name for requires"); + } + } + // TODO(dsinclair): When there are actual values for a requires directive then the + // `while` will need to keep track if any were seen, and this needs to become + // conditional. + return add_error(t.source(), "missing feature names in requires directive"); + }); + + if (decl.errored) { + return Failure::kErrored; + } + if (decl.matched) { + return kSuccess; + } + + return Failure::kNoMatch; +} + // global_decl // : SEMICOLON // | global_variable_decl SEMICOLON diff --git a/src/tint/reader/wgsl/parser_impl.h b/src/tint/reader/wgsl/parser_impl.h index 34395ed61b..e26ffbd80d 100644 --- a/src/tint/reader/wgsl/parser_impl.h +++ b/src/tint/reader/wgsl/parser_impl.h @@ -381,6 +381,9 @@ class ParserImpl { /// Parses the `enable_directive` grammar element, erroring on parse failure. /// @return true on parse success, otherwise an error or no-match. Maybe enable_directive(); + /// Parses the `requires_directive` grammar element, erroring on parse failure. + /// @return true on parse success, otherwise an error or no-match. + Maybe requires_directive(); /// Parses the `global_decl` grammar element, erroring on parse failure. /// @return true on parse success, otherwise an error or no-match. Maybe global_decl(); diff --git a/src/tint/reader/wgsl/parser_impl_require_directive_test.cc b/src/tint/reader/wgsl/parser_impl_require_directive_test.cc new file mode 100644 index 0000000000..d3cf17ca92 --- /dev/null +++ b/src/tint/reader/wgsl/parser_impl_require_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" + +namespace tint::reader::wgsl { +namespace { + +using RequiresDirectiveTest = ParserImplTest; + +// Test a valid require directive. +// There currently are no valid require directives +TEST_F(RequiresDirectiveTest, DISABLED_Valid) { + auto p = parser("requires ;"); + p->requires_directive(); + EXPECT_FALSE(p->has_error()) << p->error(); +} + +// Test an unknown require identifier. +TEST_F(RequiresDirectiveTest, InvalidIdentifier) { + auto p = parser("requires NotAValidRequireName;"); + p->requires_directive(); + // Error when unknown require found + EXPECT_TRUE(p->has_error()); + EXPECT_EQ(p->error(), R"(1:10: feature 'NotAValidRequireName' is not supported)"); +} + +// Test the special error message when require are used with parenthesis. +TEST_F(RequiresDirectiveTest, ParenthesisSpecialCase) { + auto p = parser("requires(Something);"); + p->translation_unit(); + EXPECT_TRUE(p->has_error()); + EXPECT_EQ(p->error(), "1:9: requires directives don't take parenthesis"); +} + +// Test using invalid tokens in an require directive. +TEST_F(RequiresDirectiveTest, InvalidTokens) { + { + auto p = parser("requires translation_unit(); + EXPECT_TRUE(p->has_error()); + EXPECT_EQ(p->error(), R"(1:10: invalid feature name for requires)"); + } + { + auto p = parser("requires =;"); + p->translation_unit(); + EXPECT_TRUE(p->has_error()); + EXPECT_EQ(p->error(), R"(1:10: invalid feature name for requires)"); + } + + { + auto p = parser("requires;"); + p->translation_unit(); + EXPECT_TRUE(p->has_error()); + EXPECT_EQ(p->error(), R"(1:9: missing feature names in requires directive)"); + } +} + +} // namespace +} // namespace tint::reader::wgsl diff --git a/src/tint/reader/wgsl/parser_impl_reserved_keyword_test.cc b/src/tint/reader/wgsl/parser_impl_reserved_keyword_test.cc index bf91eceaea..647bb440ae 100644 --- a/src/tint/reader/wgsl/parser_impl_reserved_keyword_test.cc +++ b/src/tint/reader/wgsl/parser_impl_reserved_keyword_test.cc @@ -193,7 +193,6 @@ INSTANTIATE_TEST_SUITE_P(ParserImplReservedKeywordTest, "regardless", "register", "reinterpret_cast", - "requires", "resource", "restrict", "self", diff --git a/src/tint/reader/wgsl/token.cc b/src/tint/reader/wgsl/token.cc index 822f83466a..e133dfccda 100644 --- a/src/tint/reader/wgsl/token.cc +++ b/src/tint/reader/wgsl/token.cc @@ -181,6 +181,8 @@ std::string_view Token::TypeToName(Type type) { return "override"; case Token::Type::kReturn: return "return"; + case Token::Type::kRequires: + return "requires"; case Token::Type::kStruct: return "struct"; case Token::Type::kSwitch: diff --git a/src/tint/reader/wgsl/token.h b/src/tint/reader/wgsl/token.h index 2e9998d768..222f28ef8b 100644 --- a/src/tint/reader/wgsl/token.h +++ b/src/tint/reader/wgsl/token.h @@ -193,6 +193,8 @@ class Token { kOverride, /// A 'return' kReturn, + /// A 'requires' + kRequires, /// A 'struct' kStruct, /// A 'switch'