From 5d8eb4a7582d88588944f414cadb690e342750fe Mon Sep 17 00:00:00 2001 From: Ben Clayton Date: Mon, 5 Jul 2021 21:48:37 +0000 Subject: [PATCH] reader/wgsl: Improve reserved keyword error messages And add `vec` and `mat` to the reserved keyword list (see https://github.com/gpuweb/gpuweb/pull/1896) Move these reserved keyword checks out of the lexer and into the parser. Generate a sensible error message. Add tests. Change-Id: I1876448201a2fd773f38f337b4e49bc61a7642f7 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/56545 Kokoro: Kokoro Reviewed-by: James Price Commit-Queue: Ben Clayton --- src/CMakeLists.txt | 1 + src/reader/wgsl/lexer.cc | 49 +------- src/reader/wgsl/lexer.h | 1 - src/reader/wgsl/lexer_test.cc | 32 ----- src/reader/wgsl/parser_impl.cc | 20 ++- .../wgsl/parser_impl_reserved_keyword_test.cc | 115 ++++++++++++++++++ src/reader/wgsl/token.cc | 2 - src/reader/wgsl/token.h | 3 - src/transform/inline_pointer_lets_test.cc | 8 +- src/transform/renamer_test.cc | 12 +- test/BUILD.gn | 1 + 11 files changed, 147 insertions(+), 97 deletions(-) create mode 100644 src/reader/wgsl/parser_impl_reserved_keyword_test.cc diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 5ef7d5c29b..7bde86ac41 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -762,6 +762,7 @@ if(${TINT_BUILD_TESTS}) reader/wgsl/parser_impl_pipeline_stage_test.cc 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_sampled_texture_type_test.cc reader/wgsl/parser_impl_sampler_type_test.cc reader/wgsl/parser_impl_shift_expression_test.cc diff --git a/src/reader/wgsl/lexer.cc b/src/reader/wgsl/lexer.cc index 67d88d92fd..8c92c14dcc 100644 --- a/src/reader/wgsl/lexer.cc +++ b/src/reader/wgsl/lexer.cc @@ -309,14 +309,9 @@ Token Lexer::try_ident() { } auto str = content_->data.substr(s, pos_ - s); - auto t = check_reserved(source, str); - if (!t.IsUninitialized()) { - return t; - } - end_source(source); - t = check_keyword(source, str); + auto t = check_keyword(source, str); if (!t.IsUninitialized()) { return t; } @@ -691,48 +686,6 @@ Token Lexer::check_keyword(const Source& source, const std::string& str) { return {}; } -Token Lexer::check_reserved(const Source& source, const std::string& str) { - if (str == "asm") - return {Token::Type::kReservedKeyword, source, "asm"}; - if (str == "bf16") - return {Token::Type::kReservedKeyword, source, "bf16"}; - if (str == "const") - return {Token::Type::kReservedKeyword, source, "const"}; - if (str == "do") - return {Token::Type::kReservedKeyword, source, "do"}; - if (str == "enum") - return {Token::Type::kReservedKeyword, source, "enum"}; - if (str == "f16") - return {Token::Type::kReservedKeyword, source, "f16"}; - if (str == "f64") - return {Token::Type::kReservedKeyword, source, "f64"}; - if (str == "handle") - return {Token::Type::kReservedKeyword, source, "handle"}; - if (str == "i8") - return {Token::Type::kReservedKeyword, source, "i8"}; - if (str == "i16") - return {Token::Type::kReservedKeyword, source, "i16"}; - if (str == "i64") - return {Token::Type::kReservedKeyword, source, "i64"}; - if (str == "premerge") - return {Token::Type::kReservedKeyword, source, "premerge"}; - if (str == "regardless") - return {Token::Type::kReservedKeyword, source, "regardless"}; - if (str == "typedef") - return {Token::Type::kReservedKeyword, source, "typedef"}; - if (str == "u8") - return {Token::Type::kReservedKeyword, source, "u8"}; - if (str == "u16") - return {Token::Type::kReservedKeyword, source, "u16"}; - if (str == "u64") - return {Token::Type::kReservedKeyword, source, "u64"}; - if (str == "unless") - return {Token::Type::kReservedKeyword, source, "unless"}; - if (str == "void") - return {Token::Type::kReservedKeyword, source, "void"}; - return {}; -} - } // namespace wgsl } // namespace reader } // namespace tint diff --git a/src/reader/wgsl/lexer.h b/src/reader/wgsl/lexer.h index cec632e5f4..b1774e9785 100644 --- a/src/reader/wgsl/lexer.h +++ b/src/reader/wgsl/lexer.h @@ -45,7 +45,6 @@ class Lexer { size_t end, int32_t base); Token check_keyword(const Source&, const std::string&); - Token check_reserved(const Source&, const std::string&); Token try_float(); Token try_hex_integer(); Token try_ident(); diff --git a/src/reader/wgsl/lexer_test.cc b/src/reader/wgsl/lexer_test.cc index 4134f1d6c4..bfcc1a7d1a 100644 --- a/src/reader/wgsl/lexer_test.cc +++ b/src/reader/wgsl/lexer_test.cc @@ -523,38 +523,6 @@ INSTANTIATE_TEST_SUITE_P( TokenData{"vec4", Token::Type::kVec4}, TokenData{"workgroup", Token::Type::kWorkgroup})); -using KeywordTest_Reserved = testing::TestWithParam; -TEST_P(KeywordTest_Reserved, Parses) { - auto* keyword = GetParam(); - Source::FileContent content(keyword); - Lexer l("test.wgsl", &content); - - auto t = l.next(); - EXPECT_TRUE(t.IsReservedKeyword()); - EXPECT_EQ(t.to_str(), keyword); -} -INSTANTIATE_TEST_SUITE_P(LexerTest, - KeywordTest_Reserved, - testing::Values("asm", - "bf16", - "const", - "do", - "enum", - "f16", - "f64", - "handle", - "i8", - "i16", - "i64", - "premerge", - "typedef", - "u8", - "u16", - "u64", - "unless", - "regardless", - "void")); - } // namespace } // namespace wgsl } // namespace reader diff --git a/src/reader/wgsl/parser_impl.cc b/src/reader/wgsl/parser_impl.cc index c8385e535e..7808f9cc23 100644 --- a/src/reader/wgsl/parser_impl.cc +++ b/src/reader/wgsl/parser_impl.cc @@ -121,8 +121,9 @@ const char kStrideDecoration[] = "stride"; const char kWorkgroupSizeDecoration[] = "workgroup_size"; bool is_decoration(Token t) { - if (!t.IsIdentifier()) + if (!t.IsIdentifier()) { return false; + } auto s = t.to_str(); return s == kAlignDecoration || s == kBindingDecoration || @@ -133,6 +134,17 @@ bool is_decoration(Token t) { s == kStrideDecoration || s == kWorkgroupSizeDecoration; } +// https://gpuweb.github.io/gpuweb/wgsl.html#reserved-keywords +bool is_reserved(Token t) { + auto s = t.to_str(); + return s == "asm" || s == "bf16" || s == "const" || s == "do" || + s == "enum" || s == "f16" || s == "f64" || s == "handle" || + s == "i8" || s == "i16" || s == "i64" || s == "mat" || + s == "premerge" || s == "regardless" || s == "typedef" || s == "u8" || + s == "u16" || s == "u64" || s == "unless" || s == "using" || + s == "vec" || s == "void" || s == "while"; +} + /// Enter-exit counters for block token types. /// Used by sync_to() to skip over closing block tokens that were opened during /// the forward scan. @@ -3261,6 +3273,12 @@ Expect ParserImpl::expect_ident(const std::string& use) { return Failure::kErrored; } next(); + + if (is_reserved(t)) { + return add_error(t.source(), + "'" + t.to_str() + "' is a reserved keyword"); + } + return {t.to_str(), t.source()}; } synchronized_ = false; diff --git a/src/reader/wgsl/parser_impl_reserved_keyword_test.cc b/src/reader/wgsl/parser_impl_reserved_keyword_test.cc new file mode 100644 index 0000000000..0b1ce39356 --- /dev/null +++ b/src/reader/wgsl/parser_impl_reserved_keyword_test.cc @@ -0,0 +1,115 @@ +// Copyright 2021 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/reader/wgsl/parser_impl_test_helper.h" + +namespace tint { +namespace reader { +namespace wgsl { +namespace { + +using ParserImplReservedKeywordTest = ParserImplTestWithParam; +TEST_P(ParserImplReservedKeywordTest, Function) { + auto name = GetParam(); + auto p = parser("fn " + name + "() {}"); + EXPECT_FALSE(p->Parse()); + EXPECT_TRUE(p->has_error()); + EXPECT_EQ(p->error(), "1:4: '" + name + "' is a reserved keyword"); +} +TEST_P(ParserImplReservedKeywordTest, ModuleLet) { + auto name = GetParam(); + auto p = parser("let " + name + " : i32 = 1;"); + EXPECT_FALSE(p->Parse()); + EXPECT_TRUE(p->has_error()); + EXPECT_EQ(p->error(), "1:5: '" + name + "' is a reserved keyword"); +} +TEST_P(ParserImplReservedKeywordTest, ModuleVar) { + auto name = GetParam(); + auto p = parser("var " + name + " : i32 = 1;"); + EXPECT_FALSE(p->Parse()); + EXPECT_TRUE(p->has_error()); + EXPECT_EQ(p->error(), "1:5: '" + name + "' is a reserved keyword"); +} +TEST_P(ParserImplReservedKeywordTest, FunctionLet) { + auto name = GetParam(); + auto p = parser("fn f() { let " + name + " : i32 = 1; }"); + EXPECT_FALSE(p->Parse()); + EXPECT_TRUE(p->has_error()); + EXPECT_EQ(p->error(), "1:14: '" + name + "' is a reserved keyword"); +} +TEST_P(ParserImplReservedKeywordTest, FunctionVar) { + auto name = GetParam(); + auto p = parser("fn f() { var " + name + " : i32 = 1; }"); + EXPECT_FALSE(p->Parse()); + EXPECT_TRUE(p->has_error()); + EXPECT_EQ(p->error(), "1:14: '" + name + "' is a reserved keyword"); +} +TEST_P(ParserImplReservedKeywordTest, FunctionParam) { + auto name = GetParam(); + auto p = parser("fn f(" + name + " : i32) {}"); + EXPECT_FALSE(p->Parse()); + EXPECT_TRUE(p->has_error()); + EXPECT_EQ(p->error(), "1:6: '" + name + "' is a reserved keyword"); +} +TEST_P(ParserImplReservedKeywordTest, Struct) { + auto name = GetParam(); + auto p = parser("struct " + name + " {};"); + EXPECT_FALSE(p->Parse()); + EXPECT_TRUE(p->has_error()); + EXPECT_EQ(p->error(), "1:8: '" + name + "' is a reserved keyword"); +} +TEST_P(ParserImplReservedKeywordTest, StructMember) { + auto name = GetParam(); + auto p = parser("struct S { " + name + " : i32; };"); + EXPECT_FALSE(p->Parse()); + EXPECT_TRUE(p->has_error()); + EXPECT_EQ(p->error(), "1:12: '" + name + "' is a reserved keyword"); +} +TEST_P(ParserImplReservedKeywordTest, Alias) { + auto name = GetParam(); + auto p = parser("type " + name + " = i32;"); + EXPECT_FALSE(p->Parse()); + EXPECT_TRUE(p->has_error()); + EXPECT_EQ(p->error(), "1:6: '" + name + "' is a reserved keyword"); +} +INSTANTIATE_TEST_SUITE_P(ParserImplReservedKeywordTest, + ParserImplReservedKeywordTest, + testing::Values("asm", + "bf16", + "const", + "do", + "enum", + "f16", + "f64", + "handle", + "i8", + "i16", + "i64", + "mat", + "premerge", + "regardless", + "typedef", + "u8", + "u16", + "u64", + "unless", + "using", + "vec", + "void", + "while")); + +} // namespace +} // namespace wgsl +} // namespace reader +} // namespace tint diff --git a/src/reader/wgsl/token.cc b/src/reader/wgsl/token.cc index acf70626da..7616a68e0d 100644 --- a/src/reader/wgsl/token.cc +++ b/src/reader/wgsl/token.cc @@ -23,8 +23,6 @@ std::string Token::TypeToName(Type type) { switch (type) { case Token::Type::kError: return "kError"; - case Token::Type::kReservedKeyword: - return "kReservedKeyword"; case Token::Type::kEOF: return "kEOF"; case Token::Type::kIdentifier: diff --git a/src/reader/wgsl/token.h b/src/reader/wgsl/token.h index 2b949f5656..893efaac1b 100644 --- a/src/reader/wgsl/token.h +++ b/src/reader/wgsl/token.h @@ -30,8 +30,6 @@ class Token { enum class Type { /// Error result kError = -2, - /// Reserved keyword - kReservedKeyword = -1, /// Uninitialized token kUninitialized = 0, /// End of input string reached @@ -367,7 +365,6 @@ class Token { /// @returns true if the token is uninitialized bool IsUninitialized() const { return type_ == Type::kUninitialized; } /// @returns true if the token is reserved - bool IsReservedKeyword() const { return type_ == Type::kReservedKeyword; } /// @returns true if the token is an error bool IsError() const { return type_ == Type::kError; } /// @returns true if the token is EOF diff --git a/src/transform/inline_pointer_lets_test.cc b/src/transform/inline_pointer_lets_test.cc index 51c1d89dbd..91818b8cc7 100644 --- a/src/transform/inline_pointer_lets_test.cc +++ b/src/transform/inline_pointer_lets_test.cc @@ -123,7 +123,7 @@ fn arr() { *p = 4; } -fn vec() { +fn vector() { var v : vec3; var i : i32 = 0; var j : i32 = 0; @@ -132,7 +132,7 @@ fn vec() { *p = 4.0; } -fn mat() { +fn matrix() { var m : mat3x3; var i : i32 = 0; var j : i32 = 0; @@ -156,7 +156,7 @@ fn arr() { *(&(a[p_save].i)) = 4; } -fn vec() { +fn vector() { var v : vec3; var i : i32 = 0; var j : i32 = 0; @@ -165,7 +165,7 @@ fn vec() { *(&(v[p_save_1])) = 4.0; } -fn mat() { +fn matrix() { var m : mat3x3; var i : i32 = 0; var j : i32 = 0; diff --git a/src/transform/renamer_test.cc b/src/transform/renamer_test.cc index 73467a08fb..1c57b4a25d 100644 --- a/src/transform/renamer_test.cc +++ b/src/transform/renamer_test.cc @@ -838,14 +838,14 @@ INSTANTIATE_TEST_SUITE_P(RenamerTestHlsl, "unorm", "unroll", "unsigned", - "using", + // "using", // WGSL reserved keyword "vector", "vertexfragment", "vertexshader", "virtual", // "void", // WGSL keyword - "volatile", - "while")); + "volatile")); +// "while" // WGSL reserved keyword INSTANTIATE_TEST_SUITE_P(RenamerTestMsl, RenamerTestMsl, @@ -928,12 +928,12 @@ INSTANTIATE_TEST_SUITE_P(RenamerTestMsl, "typename", "union", "unsigned", - "using", + // "using", // WGSL reserved keyword "virtual", // "void", // Also used in WGSL "volatile", "wchar_t", - "while", + // "while", // WGSL reserved keyword "xor", "xor_eq", @@ -1087,7 +1087,7 @@ INSTANTIATE_TEST_SUITE_P(RenamerTestMsl, "ushort2", "ushort3", "ushort4", - "vec", + // "vec", // WGSL reserved keyword "vertex")); } // namespace diff --git a/test/BUILD.gn b/test/BUILD.gn index 99f0e237d9..0431fa9366 100644 --- a/test/BUILD.gn +++ b/test/BUILD.gn @@ -450,6 +450,7 @@ tint_unittests_source_set("tint_unittests_wgsl_reader_src") { "../src/reader/wgsl/parser_impl_pipeline_stage_test.cc", "../src/reader/wgsl/parser_impl_primary_expression_test.cc", "../src/reader/wgsl/parser_impl_relational_expression_test.cc", + "../src/reader/wgsl/parser_impl_reserved_keyword_test.cc", "../src/reader/wgsl/parser_impl_sampled_texture_type_test.cc", "../src/reader/wgsl/parser_impl_sampler_type_test.cc", "../src/reader/wgsl/parser_impl_shift_expression_test.cc",