encounter
/
dawn-cmake
mirror of https://github.com/encounter/dawn-cmake.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

tint/reader/wgsl: Better diagnostics for missing parentheses 

This change required removing the `&&` splitting for `a & b && c` which never valid WGSL (right now).

Fixed: tint:1658
Change-Id: Ideb9f1aa9cf9b9b1054a6fc65860106dc072a9dc
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/105820
Commit-Queue: Ben Clayton <bclayton@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: David Neto <dneto@google.com>
Reviewed-by: Dan Sinclair <dsinclair@chromium.org>
This commit is contained in:
Ben Clayton 2022-10-18 23:57:25 +00:00 committed by Dawn LUCI CQ
parent b6e1bc7d5d
commit 00aa7ef462
6 changed files with 317 additions and 202 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

48
src/tint/ast/binary_expression.h
View File

@ -248,7 +248,53 @@ constexpr const char* FriendlyName(BinaryOp op) {
case BinaryOp::kModulo:
return "modulo";
}
return "INVALID";
return "<invalid>";
}
/// @returns the WGSL operator of the BinaryOp
/// @param op the BinaryOp
constexpr const char* Operator(BinaryOp op) {
switch (op) {
case BinaryOp::kAnd:
return "&";
case BinaryOp::kOr:
return "|";
case BinaryOp::kXor:
return "^";
case BinaryOp::kLogicalAnd:
return "&&";
case BinaryOp::kLogicalOr:
return "||";
case BinaryOp::kEqual:
return "==";
case BinaryOp::kNotEqual:
return "!=";
case BinaryOp::kLessThan:
return "<";
case BinaryOp::kGreaterThan:
return ">";
case BinaryOp::kLessThanEqual:
return "<=";
case BinaryOp::kGreaterThanEqual:
return ">=";
case BinaryOp::kShiftLeft:
return "<<";
case BinaryOp::kShiftRight:
return ">>";
case BinaryOp::kAdd:
return "+";
case BinaryOp::kSubtract:
return "-";
case BinaryOp::kMultiply:
return "*";
case BinaryOp::kDivide:
return "/";
case BinaryOp::kModulo:
return "%";
default:
break;
}
return "<invalid>";
}
/// @param out the std::ostream to write to

108
src/tint/reader/wgsl/parser_impl.cc
View File

@ -2662,43 +2662,24 @@ Expect<ParserImpl::ExpressionList> ParserImpl::expect_argument_expression_list(
Maybe<const ast::Expression*> ParserImpl::bitwise_expression_post_unary_expression(
const ast::Expression* lhs) {
auto& t = peek();
if (!t.Is(Token::Type::kAnd) && !t.Is(Token::Type::kOr) && !t.Is(Token::Type::kXor)) {
return Failure::kNoMatch;
}
ast::BinaryOp op = ast::BinaryOp::kXor;
if (t.Is(Token::Type::kAnd)) {
switch (t.type()) {
case Token::Type::kAnd:
op = ast::BinaryOp::kAnd;
} else if (t.Is(Token::Type::kOr)) {
break;
case Token::Type::kOr:
op = ast::BinaryOp::kOr;
break;
case Token::Type::kXor:
op = ast::BinaryOp::kXor;
break;
default:
return Failure::kNoMatch;
}
next(); // Consume t
while (continue_parsing()) {
auto& n = peek();
// Handle the case of `a & b &&c` where `&c` is a unary_expression
bool split = false;
if (op == ast::BinaryOp::kAnd && n.Is(Token::Type::kAndAnd)) {
next();
split_token(Token::Type::kAnd, Token::Type::kAnd);
split = true;
}
if (!n.Is(t.type())) {
if (n.Is(Token::Type::kAnd) || n.Is(Token::Type::kOr) || n.Is(Token::Type::kXor)) {
return add_error(n.source(), std::string("mixing '") + std::string(t.to_name()) +
"' and '" + std::string(n.to_name()) +
"' requires parenthesis");
}
return lhs;
}
// If forced to split an `&&` then we've already done the `next` above which consumes
// the `&`. The type check above will always fail because we only split if already consuming
// a `&` operator.
if (!split) {
next();
}
auto rhs = unary_expression();
if (rhs.errored) {
return Failure::kErrored;
@ -2709,6 +2690,10 @@ Maybe<const ast::Expression*> ParserImpl::bitwise_expression_post_unary_expressi
}
lhs = create<ast::BinaryExpression>(t.source(), op, lhs, rhs.value);
if (!match(t.type())) {
return lhs;
}
}
return Failure::kErrored;
}
@ -2949,37 +2934,45 @@ Expect<const ast::Expression*> ParserImpl::expect_relational_expression_post_una
}
lhs = lhs_result.value;
auto& t = peek();
if (match(Token::Type::kEqualEqual) || match(Token::Type::kGreaterThan) ||
match(Token::Type::kGreaterThanEqual) || match(Token::Type::kLessThan) ||
match(Token::Type::kLessThanEqual) || match(Token::Type::kNotEqual)) {
auto& tok_op = peek();
ast::BinaryOp op = ast::BinaryOp::kNone;
if (t.Is(Token::Type::kLessThan)) {
switch (tok_op.type()) {
case Token::Type::kLessThan:
op = ast::BinaryOp::kLessThan;
} else if (t.Is(Token::Type::kGreaterThan)) {
break;
case Token::Type::kGreaterThan:
op = ast::BinaryOp::kGreaterThan;
} else if (t.Is(Token::Type::kLessThanEqual)) {
break;
case Token::Type::kLessThanEqual:
op = ast::BinaryOp::kLessThanEqual;
} else if (t.Is(Token::Type::kGreaterThanEqual)) {
break;
case Token::Type::kGreaterThanEqual:
op = ast::BinaryOp::kGreaterThanEqual;
} else if (t.Is(Token::Type::kEqualEqual)) {
break;
case Token::Type::kEqualEqual:
op = ast::BinaryOp::kEqual;
} else if (t.Is(Token::Type::kNotEqual)) {
break;
case Token::Type::kNotEqual:
op = ast::BinaryOp::kNotEqual;
break;
default:
return lhs;
}
auto& next = peek();
next(); // consume tok_op
auto& tok_rhs = peek();
auto rhs = shift_expression();
if (rhs.errored) {
return Failure::kErrored;
}
if (!rhs.matched) {
return add_error(next, std::string("unable to parse right side of ") +
std::string(t.to_name()) + " expression");
return add_error(tok_rhs, std::string("unable to parse right side of ") +
std::string(tok_op.to_name()) + " expression");
}
lhs = create<ast::BinaryExpression>(t.source(), op, lhs, rhs.value);
}
return lhs;
return create<ast::BinaryExpression>(tok_op.source(), op, lhs, rhs.value);
}
// expression
@ -2992,6 +2985,7 @@ Expect<const ast::Expression*> ParserImpl::expect_relational_expression_post_una
//
// Note, a `relational_expression` element was added to simplify many of the right sides
Maybe<const ast::Expression*> ParserImpl::expression() {
auto expr = [&]() -> Maybe<const ast::Expression*> {
auto lhs = unary_expression();
if (lhs.errored) {
return Failure::kErrored;
@ -3026,11 +3020,6 @@ Maybe<const ast::Expression*> ParserImpl::expression() {
while (continue_parsing()) {
auto& n = peek();
if (!n.Is(t.type())) {
if (n.Is(Token::Type::kAndAnd) || n.Is(Token::Type::kOrOr)) {
return add_error(
n.source(), std::string("mixing '") + std::string(t.to_name()) + "' and '" +
std::string(n.to_name()) + "' requires parenthesis");
}
break;
}
next();
@ -3048,6 +3037,23 @@ Maybe<const ast::Expression*> ParserImpl::expression() {
}
}
return ret;
}();
if (expr.matched) {
// Note, expression is greedy an will consume all the operators of the same type
// so, `a & a & a` would all be consumed above. If you see any binary operator
// after this then it _must_ be a different one, and hence an error.
if (auto* lhs = expr->As<ast::BinaryExpression>()) {
if (auto& n = peek(); n.IsBinaryOperator()) {
auto source = Source::Combine(expr->source, n.source());
add_error(source, std::string("mixing '") + ast::Operator(lhs->op) + "' and '" +
std::string(n.to_name()) + "' requires parenthesis");
return Failure::kErrored;
}
}
}
return expr;
}
// singular_expression

90
src/tint/reader/wgsl/parser_impl_bitwise_expression_test.cc
View File

@ -87,28 +87,6 @@ TEST_F(ParserImplTest, BitwiseExpr_Or_Parses_Multiple) {
ASSERT_TRUE(rel->rhs->As<ast::BoolLiteralExpression>()->value);
}
TEST_F(ParserImplTest, BitwiseExpr_Or_MixedWithAnd_Invalid) {
auto p = parser("a | b & c");
auto lhs = p->unary_expression();
auto e = p->bitwise_expression_post_unary_expression(lhs.value);
EXPECT_FALSE(e.matched);
EXPECT_TRUE(e.errored);
EXPECT_EQ(e.value, nullptr);
EXPECT_TRUE(p->has_error());
EXPECT_EQ(p->error(), "1:7: mixing '|' and '&' requires parenthesis");
}
TEST_F(ParserImplTest, BitwiseExpr_Or_MixedWithXor_Invalid) {
auto p = parser("a | b ^ c");
auto lhs = p->unary_expression();
auto e = p->bitwise_expression_post_unary_expression(lhs.value);
EXPECT_FALSE(e.matched);
EXPECT_TRUE(e.errored);
EXPECT_EQ(e.value, nullptr);
EXPECT_TRUE(p->has_error());
EXPECT_EQ(p->error(), "1:7: mixing '|' and '^' requires parenthesis");
}
TEST_F(ParserImplTest, BitwiseExpr_Or_InvalidRHS) {
auto p = parser("true | if (a) {}");
auto lhs = p->unary_expression();
@ -179,28 +157,6 @@ TEST_F(ParserImplTest, BitwiseExpr_Xor_Parses_Multiple) {
ASSERT_TRUE(rel->rhs->As<ast::BoolLiteralExpression>()->value);
}
TEST_F(ParserImplTest, BitwiseExpr_Xor_MixedWithOr_Invalid) {
auto p = parser("a ^ b | c");
auto lhs = p->unary_expression();
auto e = p->bitwise_expression_post_unary_expression(lhs.value);
EXPECT_FALSE(e.matched);
EXPECT_TRUE(e.errored);
EXPECT_EQ(e.value, nullptr);
EXPECT_TRUE(p->has_error());
EXPECT_EQ(p->error(), "1:7: mixing '^' and '|' requires parenthesis");
}
TEST_F(ParserImplTest, BitwiseExpr_Xor_MixedWithAnd_Invalid) {
auto p = parser("a ^ b & c");
auto lhs = p->unary_expression();
auto e = p->bitwise_expression_post_unary_expression(lhs.value);
EXPECT_FALSE(e.matched);
EXPECT_TRUE(e.errored);
EXPECT_EQ(e.value, nullptr);
EXPECT_TRUE(p->has_error());
EXPECT_EQ(p->error(), "1:7: mixing '^' and '&' requires parenthesis");
}
TEST_F(ParserImplTest, BitwiseExpr_Xor_InvalidRHS) {
auto p = parser("true ^ if (a) {}");
auto lhs = p->unary_expression();
@ -275,59 +231,21 @@ TEST_F(ParserImplTest, BitwiseExpr_And_Parses_AndAnd) {
auto p = parser("a & true &&b");
auto lhs = p->unary_expression();
auto e = p->bitwise_expression_post_unary_expression(lhs.value);
// bitwise_expression_post_unary_expression returns before parsing '&&'
EXPECT_TRUE(e.matched);
EXPECT_FALSE(e.errored);
EXPECT_FALSE(p->has_error()) << p->error();
ASSERT_NE(e.value, nullptr);
// lhs: (a & true)
// rhs: &b
// lhs: a
// rhs: true
ASSERT_TRUE(e->Is<ast::BinaryExpression>());
auto* rel = e->As<ast::BinaryExpression>();
EXPECT_EQ(ast::BinaryOp::kAnd, rel->op);
ASSERT_TRUE(rel->rhs->Is<ast::UnaryOpExpression>());
auto* unary = rel->rhs->As<ast::UnaryOpExpression>();
EXPECT_EQ(ast::UnaryOp::kAddressOf, unary->op);
ASSERT_TRUE(unary->expr->Is<ast::IdentifierExpression>());
auto* ident = unary->expr->As<ast::IdentifierExpression>();
EXPECT_EQ(ident->symbol, p->builder().Symbols().Register("b"));
ASSERT_TRUE(rel->lhs->Is<ast::BinaryExpression>());
// lhs: a
// rhs: true
rel = rel->lhs->As<ast::BinaryExpression>();
ASSERT_TRUE(rel->lhs->Is<ast::IdentifierExpression>());
ident = rel->lhs->As<ast::IdentifierExpression>();
EXPECT_EQ(ident->symbol, p->builder().Symbols().Register("a"));
ASSERT_TRUE(rel->rhs->Is<ast::BoolLiteralExpression>());
ASSERT_TRUE(rel->rhs->As<ast::BoolLiteralExpression>()->value);
}
TEST_F(ParserImplTest, BitwiseExpr_And_MixedWithOr_Invalid) {
auto p = parser("a & b | c");
auto lhs = p->unary_expression();
auto e = p->bitwise_expression_post_unary_expression(lhs.value);
EXPECT_FALSE(e.matched);
EXPECT_TRUE(e.errored);
EXPECT_EQ(e.value, nullptr);
EXPECT_TRUE(p->has_error());
EXPECT_EQ(p->error(), "1:7: mixing '&' and '|' requires parenthesis");
}
TEST_F(ParserImplTest, BitwiseExpr_And_MixedWithXor_Invalid) {
auto p = parser("a & b ^ c");
auto lhs = p->unary_expression();
auto e = p->bitwise_expression_post_unary_expression(lhs.value);
EXPECT_FALSE(e.matched);
EXPECT_TRUE(e.errored);
EXPECT_EQ(e.value, nullptr);
EXPECT_TRUE(p->has_error());
EXPECT_EQ(p->error(), "1:7: mixing '&' and '^' requires parenthesis");
}
TEST_F(ParserImplTest, BitwiseExpr_And_InvalidRHS) {

122
src/tint/reader/wgsl/parser_impl_expression_test.cc
View File

@ -165,7 +165,7 @@ TEST_F(ParserImplTest, Expression_Mixing_OrWithAnd) {
EXPECT_TRUE(e.errored);
EXPECT_EQ(e.value, nullptr);
EXPECT_TRUE(p->has_error());
EXPECT_EQ(p->error(), "1:11: mixing '&&' and '||' requires parenthesis");
EXPECT_EQ(p->error(), "1:3: mixing '&&' and '||' requires parenthesis");
}
TEST_F(ParserImplTest, Expression_Mixing_AndWithOr) {
@ -175,7 +175,7 @@ TEST_F(ParserImplTest, Expression_Mixing_AndWithOr) {
EXPECT_TRUE(e.errored);
EXPECT_EQ(e.value, nullptr);
EXPECT_TRUE(p->has_error());
EXPECT_EQ(p->error(), "1:11: mixing '||' and '&&' requires parenthesis");
EXPECT_EQ(p->error(), "1:3: mixing '||' and '&&' requires parenthesis");
}
TEST_F(ParserImplTest, Expression_Bitwise) {
@ -250,5 +250,123 @@ TEST_F(ParserImplTest, Expression_InvalidRelational) {
EXPECT_EQ(p->error(), "1:6: unable to parse right side of <= expression");
}
namespace mixing_binary_ops {
struct BinaryOperatorInfo {
// A uint64_t with a single bit assigned that uniquely identifies the binary-op.
uint64_t bit;
// The WGSL operator symbol. Example: '<='
const char* symbol;
// A bit mask of all operators that can immediately follow the RHS of this operator without
// requiring parentheses. In other words, `can_follow_without_paren` is the full set of
// operators that can substitute `<next-operator>` in the WGSL:
// `expr_a <this-operator> expr_b <next-operator> expr_c`
// without requiring additional parentheses.
uint64_t can_follow_without_paren;
};
// Each binary operator is given a unique bit in a uint64_t
static constexpr uint64_t kOpMul = static_cast<uint64_t>(1) << 0;
static constexpr uint64_t kOpDiv = static_cast<uint64_t>(1) << 1;
static constexpr uint64_t kOpMod = static_cast<uint64_t>(1) << 2;
static constexpr uint64_t kOpAdd = static_cast<uint64_t>(1) << 3;
static constexpr uint64_t kOpSub = static_cast<uint64_t>(1) << 4;
static constexpr uint64_t kOpBAnd = static_cast<uint64_t>(1) << 5;
static constexpr uint64_t kOpBOr = static_cast<uint64_t>(1) << 6;
static constexpr uint64_t kOpBXor = static_cast<uint64_t>(1) << 7;
static constexpr uint64_t kOpShl = static_cast<uint64_t>(1) << 8;
static constexpr uint64_t kOpShr = static_cast<uint64_t>(1) << 9;
static constexpr uint64_t kOpLt = static_cast<uint64_t>(1) << 10;
static constexpr uint64_t kOpGt = static_cast<uint64_t>(1) << 11;
static constexpr uint64_t kOpLe = static_cast<uint64_t>(1) << 12;
static constexpr uint64_t kOpGe = static_cast<uint64_t>(1) << 13;
static constexpr uint64_t kOpEq = static_cast<uint64_t>(1) << 14;
static constexpr uint64_t kOpNe = static_cast<uint64_t>(1) << 15;
static constexpr uint64_t kOpLAnd = static_cast<uint64_t>(1) << 16;
static constexpr uint64_t kOpLOr = static_cast<uint64_t>(1) << 17;
// Bit mask for the binary operator groups
static constexpr uint64_t kMultiplicative = kOpMul | kOpDiv | kOpMod;
static constexpr uint64_t kAdditive = kOpAdd | kOpSub;
static constexpr uint64_t kShift = kOpShl | kOpShr;
static constexpr uint64_t kRelational = kOpLt | kOpGt | kOpLe | kOpGe | kOpEq | kOpNe;
static constexpr uint64_t kLogical = kOpLAnd | kOpLOr;
// The binary operator table
static constexpr const BinaryOperatorInfo kBinaryOperators[] = {
// multiplicative
{kOpMul, "*", kLogical | kRelational | kAdditive | kMultiplicative},
{kOpDiv, "/", kLogical | kRelational | kAdditive | kMultiplicative},
{kOpMod, "%", kLogical | kRelational | kAdditive | kMultiplicative},
// additive
{kOpAdd, "+", kLogical | kRelational | kAdditive | kMultiplicative},
{kOpSub, "-", kLogical | kRelational | kAdditive | kMultiplicative},
// bitwise
{kOpBAnd, "&", kOpBAnd},
{kOpBOr, "|", kOpBOr},
{kOpBXor, "^", kOpBXor},
// shift
{kOpShl, "<<", kLogical | kRelational},
{kOpShr, ">>", kLogical | kRelational},
// relational
{kOpLt, "<", kLogical | kShift | kAdditive | kMultiplicative},
{kOpGt, ">", kLogical | kShift | kAdditive | kMultiplicative},
{kOpLe, "<=", kLogical | kShift | kAdditive | kMultiplicative},
{kOpGe, ">=", kLogical | kShift | kAdditive | kMultiplicative},
{kOpEq, "==", kLogical | kShift | kAdditive | kMultiplicative},
{kOpNe, "!=", kLogical | kShift | kAdditive | kMultiplicative},
// logical
{kOpLAnd, "&&", kOpLAnd | kRelational | kShift | kAdditive | kMultiplicative},
{kOpLOr, "||", kOpLOr | kRelational | kShift | kAdditive | kMultiplicative},
};
struct Case {
BinaryOperatorInfo lhs_op;
BinaryOperatorInfo rhs_op;
bool should_parse;
};
static std::ostream& operator<<(std::ostream& o, const Case& c) {
return o << "a " << c.lhs_op.symbol << " b " << c.rhs_op.symbol << " c ";
}
static std::vector<Case> Cases() {
std::vector<Case> out;
for (auto& lhs_op : kBinaryOperators) {
for (auto& rhs_op : kBinaryOperators) {
bool should_parse = lhs_op.can_follow_without_paren & rhs_op.bit;
out.push_back({lhs_op, rhs_op, should_parse});
}
}
return out;
}
using ParserImplMixedBinaryOpTest = ParserImplTestWithParam<Case>;
TEST_P(ParserImplMixedBinaryOpTest, Test) {
std::stringstream wgsl;
wgsl << GetParam();
auto p = parser(wgsl.str());
auto e = p->expression();
if (GetParam().should_parse) {
ASSERT_TRUE(e.matched) << e.errored;
EXPECT_TRUE(e->Is<ast::BinaryExpression>());
} else {
EXPECT_FALSE(e.matched);
EXPECT_TRUE(e.errored);
EXPECT_EQ(e.value, nullptr);
EXPECT_TRUE(p->has_error());
std::stringstream expected;
expected << "1:3: mixing '" << GetParam().lhs_op.symbol << "' and '"
<< GetParam().rhs_op.symbol << "' requires parenthesis";
EXPECT_EQ(p->error(), expected.str());
}
}
INSTANTIATE_TEST_SUITE_P(ParserImplMixedBinaryOpTest,
ParserImplMixedBinaryOpTest,
testing::ValuesIn(Cases()));
} // namespace mixing_binary_ops
} // namespace
} // namespace tint::reader::wgsl

2
src/tint/reader/wgsl/parser_impl_struct_member_attribute_test.cc
View File

@ -220,7 +220,7 @@ TEST_F(ParserImplTest, Attribute_Align_ExpressionInvalid) {
EXPECT_EQ(attr.value, nullptr);
EXPECT_TRUE(p->has_error());
EXPECT_EQ(p->error(), "1:13: expected ')' for align attribute");
EXPECT_EQ(p->error(), "1:9: mixing '+' and '<<' requires parenthesis");
}
} // namespace

31
src/tint/reader/wgsl/token.h
View File

@ -379,8 +379,35 @@ class Token {
/// @returns true if the token can be split during parse into component tokens
bool IsSplittable() const {
return Is(Token::Type::kShiftRight) || Is(Token::Type::kGreaterThanEqual) ||
Is(Token::Type::kAndAnd) || Is(Token::Type::kMinusMinus);
return Is(Type::kShiftRight) || Is(Type::kGreaterThanEqual) || Is(Type::kAndAnd) ||
Is(Type::kMinusMinus);
}
/// @returns true if the token is a binary operator
bool IsBinaryOperator() const {
switch (type_) {
case Type::kAnd:
case Type::kAndAnd:
case Type::kEqualEqual:
case Type::kForwardSlash:
case Type::kGreaterThan:
case Type::kGreaterThanEqual:
case Type::kLessThan:
case Type::kLessThanEqual:
case Type::kMinus:
case Type::kMod:
case Type::kNotEqual:
case Type::kOr:
case Type::kOrOr:
case Type::kPlus:
case Type::kShiftLeft:
case Type::kShiftRight:
case Type::kStar:
case Type::kXor:
return true;
default:
return false;
}
}
/// @returns the source information for this token