resolver: Don't use ast to_str() methods
These return weird names that love the use of '__' and have little relation to WGSL.
Improve the duplicate case error message.
Clean up control_block_validation_test.cc by making used of the ProgramBuilder helpers.
Bug: tint:1225
Change-Id: I8c4cf3943145cf8372c00d33ae0166c0c0bcbb8b
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/66442
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
Reviewed-by: James Price <jrprice@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>
diff --git a/src/program_builder.h b/src/program_builder.h
index fc3b59e..1bfa21f 100644
--- a/src/program_builder.h
+++ b/src/program_builder.h
@@ -1105,18 +1105,46 @@
/// @return `list`
ast::ExpressionList ExprList(ast::ExpressionList list) { return list; }
+ /// @param source the source location for the literal
+ /// @param val the boolan value
+ /// @return a boolean literal with the given value
+ ast::BoolLiteral* Literal(const Source& source, bool val) {
+ return create<ast::BoolLiteral>(source, val);
+ }
+
/// @param val the boolan value
/// @return a boolean literal with the given value
ast::BoolLiteral* Literal(bool val) { return create<ast::BoolLiteral>(val); }
+ /// @param source the source location for the literal
+ /// @param val the float value
+ /// @return a float literal with the given value
+ ast::FloatLiteral* Literal(const Source& source, f32 val) {
+ return create<ast::FloatLiteral>(source, val);
+ }
+
/// @param val the float value
/// @return a float literal with the given value
ast::FloatLiteral* Literal(f32 val) { return create<ast::FloatLiteral>(val); }
+ /// @param source the source location for the literal
+ /// @param val the unsigned int value
+ /// @return a ast::UintLiteral with the given value
+ ast::UintLiteral* Literal(const Source& source, u32 val) {
+ return create<ast::UintLiteral>(source, val);
+ }
+
/// @param val the unsigned int value
/// @return a ast::UintLiteral with the given value
ast::UintLiteral* Literal(u32 val) { return create<ast::UintLiteral>(val); }
+ /// @param source the source location for the literal
+ /// @param val the integer value
+ /// @return the ast::SintLiteral with the given value
+ ast::SintLiteral* Literal(const Source& source, i32 val) {
+ return create<ast::SintLiteral>(source, val);
+ }
+
/// @param val the integer value
/// @return the ast::SintLiteral with the given value
ast::SintLiteral* Literal(i32 val) { return create<ast::SintLiteral>(val); }
@@ -2071,10 +2099,26 @@
}
/// Creates a ast::SwitchStatement with input expression and cases
+ /// @param source the source information
/// @param condition the condition expression initializer
/// @param cases case statements
/// @returns the switch statement pointer
template <typename ExpressionInit, typename... Cases>
+ ast::SwitchStatement* Switch(const Source& source,
+ ExpressionInit&& condition,
+ Cases&&... cases) {
+ return create<ast::SwitchStatement>(
+ source, Expr(std::forward<ExpressionInit>(condition)),
+ ast::CaseStatementList{std::forward<Cases>(cases)...});
+ }
+
+ /// Creates a ast::SwitchStatement with input expression and cases
+ /// @param condition the condition expression initializer
+ /// @param cases case statements
+ /// @returns the switch statement pointer
+ template <typename ExpressionInit,
+ typename... Cases,
+ typename = DisableIfSource<ExpressionInit>>
ast::SwitchStatement* Switch(ExpressionInit&& condition, Cases&&... cases) {
return create<ast::SwitchStatement>(
Expr(std::forward<ExpressionInit>(condition)),
@@ -2082,6 +2126,18 @@
}
/// Creates a ast::CaseStatement with input list of selectors, and body
+ /// @param source the source information
+ /// @param selectors list of selectors
+ /// @param body the case body
+ /// @returns the case statement pointer
+ ast::CaseStatement* Case(const Source& source,
+ ast::CaseSelectorList selectors,
+ ast::BlockStatement* body = nullptr) {
+ return create<ast::CaseStatement>(source, std::move(selectors),
+ body ? body : Block());
+ }
+
+ /// Creates a ast::CaseStatement with input list of selectors, and body
/// @param selectors list of selectors
/// @param body the case body
/// @returns the case statement pointer
@@ -2101,6 +2157,15 @@
}
/// Convenience function that creates a 'default' ast::CaseStatement
+ /// @param source the source information
+ /// @param body the case body
+ /// @returns the case statement pointer
+ ast::CaseStatement* DefaultCase(const Source& source,
+ ast::BlockStatement* body = nullptr) {
+ return Case(source, ast::CaseSelectorList{}, body);
+ }
+
+ /// Convenience function that creates a 'default' ast::CaseStatement
/// @param body the case body
/// @returns the case statement pointer
ast::CaseStatement* DefaultCase(ast::BlockStatement* body = nullptr) {
diff --git a/src/resolver/control_block_validation_test.cc b/src/resolver/control_block_validation_test.cc
index 6ee6c55..c317a42 100644
--- a/src/resolver/control_block_validation_test.cc
+++ b/src/resolver/control_block_validation_test.cc
@@ -32,14 +32,8 @@
// }
auto* var = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(3.14f));
- ast::CaseStatementList body;
- auto* block_default = Block();
- body.push_back(
- create<ast::CaseStatement>(ast::CaseSelectorList{}, block_default));
-
- auto* block =
- Block(Decl(var), create<ast::SwitchStatement>(
- Expr(Source{Source::Location{12, 34}}, "a"), body));
+ auto* block = Block(Decl(var), Switch(Expr(Source{{12, 34}}, "a"), //
+ DefaultCase()));
WrapInFunction(block);
@@ -56,15 +50,9 @@
// }
auto* var = Var("a", ty.i32(), ast::StorageClass::kNone, Expr(2));
- ast::CaseSelectorList csl;
- csl.push_back(Literal(1));
-
- ast::CaseStatementList body;
- body.push_back(create<ast::CaseStatement>(csl, Block()));
-
- auto* block =
- Block(Decl(var), create<ast::SwitchStatement>(
- Source{Source::Location{12, 34}}, Expr("a"), body));
+ auto* block = Block(Decl(var), //
+ Switch(Source{{12, 34}}, "a", //
+ Case(Literal(1))));
WrapInFunction(block);
@@ -82,24 +70,11 @@
// }
auto* var = Var("a", ty.i32(), ast::StorageClass::kNone, Expr(2));
- ast::CaseStatementList switch_body;
- ast::CaseSelectorList default_csl_1;
- auto* block_default_1 = Block();
- switch_body.push_back(
- create<ast::CaseStatement>(default_csl_1, block_default_1));
-
- ast::CaseSelectorList csl_case_1;
- csl_case_1.push_back(Literal(1));
- auto* block_case_1 = Block();
- switch_body.push_back(create<ast::CaseStatement>(csl_case_1, block_case_1));
-
- ast::CaseSelectorList default_csl_2;
- auto* block_default_2 = Block();
- switch_body.push_back(create<ast::CaseStatement>(
- Source{Source::Location{12, 34}}, default_csl_2, block_default_2));
-
- auto* block =
- Block(Decl(var), create<ast::SwitchStatement>(Expr("a"), switch_body));
+ auto* block = Block(Decl(var), //
+ Switch("a", //
+ DefaultCase(), //
+ Case(Literal(1)), //
+ DefaultCase(Source{{12, 34}})));
WrapInFunction(block);
@@ -173,34 +148,36 @@
}
TEST_F(ResolverControlBlockValidationTest, UnreachableCode_break) {
- // switch (a) {
+ // switch (1) {
// case 1: { break; var a : u32 = 2;}
// default: {}
// }
auto* decl = Decl(Source{{12, 34}},
Var("a", ty.i32(), ast::StorageClass::kNone, Expr(2)));
- WrapInFunction(Loop(Block(Switch(
- Expr(1), Case(Literal(1), Block(create<ast::BreakStatement>(), decl)),
- DefaultCase()))));
+ WrapInFunction( //
+ Loop(Block(Switch(1, //
+ Case(Literal(1), Block(Break(), decl)), //
+ DefaultCase()))));
EXPECT_FALSE(r()->Resolve());
EXPECT_EQ(r()->error(), "12:34 error: code is unreachable");
}
TEST_F(ResolverControlBlockValidationTest, UnreachableCode_break_InBlocks) {
- // switch (a) {
- // case 1: { {{{break;}}} var a : u32 = 2;}
- // default: {}
+ // loop {
+ // switch (1) {
+ // case 1: { {{{break;}}} var a : u32 = 2;}
+ // default: {}
+ // }
// }
auto* decl = Decl(Source{{12, 34}},
Var("a", ty.i32(), ast::StorageClass::kNone, Expr(2)));
- WrapInFunction(Loop(Block(Switch(
- Expr(1),
- Case(Literal(1),
- Block(Block(Block(Block(create<ast::BreakStatement>()))), decl)),
- DefaultCase()))));
+ WrapInFunction(Loop(
+ Block(Switch(1, //
+ Case(Literal(1), Block(Block(Block(Block(Break()))), decl)),
+ DefaultCase()))));
EXPECT_FALSE(r()->Resolve());
EXPECT_EQ(r()->error(), "12:34 error: code is unreachable");
@@ -215,18 +192,10 @@
// }
auto* var = Var("a", ty.i32(), ast::StorageClass::kNone, Expr(2));
- ast::CaseStatementList switch_body;
- ast::CaseSelectorList csl;
- csl.push_back(create<ast::UintLiteral>(1u));
- switch_body.push_back(create<ast::CaseStatement>(
- Source{Source::Location{12, 34}}, csl, Block()));
-
- ast::CaseSelectorList default_csl;
- auto* block_default = Block();
- switch_body.push_back(create<ast::CaseStatement>(default_csl, block_default));
-
auto* block =
- Block(Decl(var), create<ast::SwitchStatement>(Expr("a"), switch_body));
+ Block(Decl(var), Switch("a", //
+ Case(Source{{12, 34}}, {Literal(1u)}), //
+ DefaultCase()));
WrapInFunction(block);
EXPECT_FALSE(r()->Resolve());
@@ -244,18 +213,10 @@
// }
auto* var = Var("a", ty.u32(), ast::StorageClass::kNone, Expr(2u));
- ast::CaseStatementList switch_body;
- ast::CaseSelectorList csl;
- csl.push_back(Literal(-1));
- switch_body.push_back(create<ast::CaseStatement>(
- Source{Source::Location{12, 34}}, csl, Block()));
-
- ast::CaseSelectorList default_csl;
- auto* block_default = Block();
- switch_body.push_back(create<ast::CaseStatement>(default_csl, block_default));
-
- auto* block =
- Block(Decl(var), create<ast::SwitchStatement>(Expr("a"), switch_body));
+ auto* block = Block(Decl(var), //
+ Switch("a", //
+ Case(Source{{12, 34}}, {Literal(-1)}), //
+ DefaultCase()));
WrapInFunction(block);
EXPECT_FALSE(r()->Resolve());
@@ -268,72 +229,55 @@
NonUniqueCaseSelectorValueUint_Fail) {
// var a : u32 = 3;
// switch (a) {
- // case 0: {}
- // case 2, 2: {}
+ // case 0u: {}
+ // case 2u, 3u, 2u: {}
// default: {}
// }
auto* var = Var("a", ty.u32(), ast::StorageClass::kNone, Expr(3u));
- ast::CaseStatementList switch_body;
- ast::CaseSelectorList csl_1;
- csl_1.push_back(create<ast::UintLiteral>(0u));
- switch_body.push_back(create<ast::CaseStatement>(csl_1, Block()));
-
- ast::CaseSelectorList csl_2;
- csl_2.push_back(create<ast::UintLiteral>(2u));
- csl_2.push_back(create<ast::UintLiteral>(2u));
- switch_body.push_back(create<ast::CaseStatement>(
- Source{Source::Location{12, 34}}, csl_2, Block()));
-
- ast::CaseSelectorList default_csl;
- auto* block_default = Block();
- switch_body.push_back(create<ast::CaseStatement>(default_csl, block_default));
-
- auto* block =
- Block(Decl(var), create<ast::SwitchStatement>(Expr("a"), switch_body));
+ auto* block = Block(Decl(var), //
+ Switch("a", //
+ Case(Literal(0u)),
+ Case({
+ Literal(Source{{12, 34}}, 2u),
+ Literal(3u),
+ Literal(Source{{56, 78}}, 2u),
+ }),
+ DefaultCase()));
WrapInFunction(block);
EXPECT_FALSE(r()->Resolve());
EXPECT_EQ(r()->error(),
- "12:34 error: a literal value must not appear more than once in "
- "the case selectors for a switch statement: '2u'");
+ "56:78 error: duplicate switch case '2'\n"
+ "12:34 note: previous case declared here");
}
TEST_F(ResolverControlBlockValidationTest,
NonUniqueCaseSelectorValueSint_Fail) {
// var a : i32 = 2;
// switch (a) {
- // case 10: {}
- // case 0,1,2,10: {}
+ // case -10: {}
+ // case 0,1,2,-10: {}
// default: {}
// }
auto* var = Var("a", ty.i32(), ast::StorageClass::kNone, Expr(2));
- ast::CaseStatementList switch_body;
- ast::CaseSelectorList csl_1;
- csl_1.push_back(Literal(10));
- switch_body.push_back(create<ast::CaseStatement>(csl_1, Block()));
-
- ast::CaseSelectorList csl_2;
- csl_2.push_back(Literal(0));
- csl_2.push_back(Literal(1));
- csl_2.push_back(Literal(2));
- csl_2.push_back(Literal(10));
- switch_body.push_back(create<ast::CaseStatement>(
- Source{Source::Location{12, 34}}, csl_2, Block()));
-
- ast::CaseSelectorList default_csl;
- auto* block_default = Block();
- switch_body.push_back(create<ast::CaseStatement>(default_csl, block_default));
-
- auto* block =
- Block(Decl(var), create<ast::SwitchStatement>(Expr("a"), switch_body));
+ auto* block = Block(Decl(var), //
+ Switch("a", //
+ Case(Literal(Source{{12, 34}}, -10)),
+ Case({
+ Literal(0),
+ Literal(1),
+ Literal(2),
+ Literal(Source{{56, 78}}, -10),
+ }),
+ DefaultCase()));
WrapInFunction(block);
EXPECT_FALSE(r()->Resolve());
EXPECT_EQ(r()->error(),
- "12:34 error: a literal value must not appear more than once in "
- "the case selectors for a switch statement: '10'");
+ "56:78 error: duplicate switch case '-10'\n"
+ "12:34 note: previous case declared here");
}
TEST_F(ResolverControlBlockValidationTest,
@@ -343,14 +287,10 @@
// default: { fallthrough; }
// }
auto* var = Var("a", ty.i32(), ast::StorageClass::kNone, Expr(2));
-
- ast::CaseSelectorList default_csl;
- auto* block_default = Block(
- create<ast::FallthroughStatement>(Source{Source::Location{12, 34}}));
- ast::CaseStatementList body;
- body.push_back(create<ast::CaseStatement>(default_csl, block_default));
-
- auto* block = Block(Decl(var), create<ast::SwitchStatement>(Expr("a"), body));
+ auto* fallthrough = create<ast::FallthroughStatement>(Source{{12, 34}});
+ auto* block = Block(Decl(var), //
+ Switch("a", //
+ DefaultCase(Block(fallthrough))));
WrapInFunction(block);
EXPECT_FALSE(r()->Resolve());
@@ -367,17 +307,10 @@
// }
auto* var = Var("a", ty.i32(), ast::StorageClass::kNone, Expr(2));
- ast::CaseSelectorList default_csl;
- auto* block_default = Block();
- ast::CaseStatementList body;
- body.push_back(create<ast::CaseStatement>(Source{Source::Location{12, 34}},
- default_csl, block_default));
- ast::CaseSelectorList case_csl;
- case_csl.push_back(Literal(5));
- auto* block_case = Block();
- body.push_back(create<ast::CaseStatement>(case_csl, block_case));
-
- auto* block = Block(Decl(var), create<ast::SwitchStatement>(Expr("a"), body));
+ auto* block = Block(Decl(var), //
+ Switch("a", //
+ DefaultCase(Source{{12, 34}}), //
+ Case(Literal(5))));
WrapInFunction(block);
EXPECT_TRUE(r()->Resolve()) << r()->error();
@@ -392,14 +325,8 @@
auto* my_int = Alias("MyInt", ty.u32());
auto* var = Var("a", ty.Of(my_int), ast::StorageClass::kNone, Expr(2u));
-
- ast::CaseSelectorList default_csl;
- auto* block_default = Block();
- ast::CaseStatementList body;
- body.push_back(create<ast::CaseStatement>(Source{Source::Location{12, 34}},
- default_csl, block_default));
-
- auto* block = Block(Decl(var), create<ast::SwitchStatement>(Expr("a"), body));
+ auto* block = Block(Decl(var), //
+ Switch("a", DefaultCase(Source{{12, 34}})));
WrapInFunction(block);
diff --git a/src/resolver/resolver.cc b/src/resolver/resolver.cc
index a9ab165..cc64de0 100644
--- a/src/resolver/resolver.cc
+++ b/src/resolver/resolver.cc
@@ -273,12 +273,10 @@
bool result = true;
for (auto* node : builder_->ASTNodes().Objects()) {
if (marked_.count(node) == 0) {
- TINT_ICE(Resolver, diagnostics_)
- << "AST node '" << node->TypeInfo().name
- << "' was not reached by the resolver\n"
- << "At: " << node->source() << "\n"
- << "Content: " << builder_->str(node) << "\n"
- << "Pointer: " << node;
+ TINT_ICE(Resolver, diagnostics_) << "AST node '" << node->TypeInfo().name
+ << "' was not reached by the resolver\n"
+ << "At: " << node->source() << "\n"
+ << "Pointer: " << node;
result = false;
}
}
@@ -2123,9 +2121,9 @@
return VariableDeclStatement(v);
}
- AddError(
- "unknown statement type for type determination: " + builder_->str(stmt),
- stmt->source());
+ AddError("unknown statement type for type determination: " +
+ std::string(stmt->TypeInfo().name),
+ stmt->source());
return false;
}
@@ -4389,7 +4387,7 @@
}
bool has_default = false;
- std::unordered_set<uint32_t> selector_set;
+ std::unordered_map<uint32_t, Source> selectors;
for (auto* case_stmt : s->body()) {
if (case_stmt->IsDefault()) {
@@ -4412,15 +4410,16 @@
}
auto v = selector->value_as_u32();
- if (selector_set.find(v) != selector_set.end()) {
- AddError(
- "a literal value must not appear more than once in "
- "the case selectors for a switch statement: '" +
- builder_->str(selector) + "'",
- case_stmt->source());
+ auto it = selectors.find(v);
+ if (it != selectors.end()) {
+ auto val = selector->Is<ast::IntLiteral>()
+ ? std::to_string(selector->value_as_i32())
+ : std::to_string(selector->value_as_u32());
+ AddError("duplicate switch case '" + val + "'", selector->source());
+ AddNote("previous case declared here", it->second);
return false;
}
- selector_set.emplace(v);
+ selectors.emplace(v, selector->source());
}
}
@@ -4659,7 +4658,6 @@
<< "AST node '" << node->TypeInfo().name
<< "' was encountered twice in the same AST of a Program\n"
<< "At: " << node->source() << "\n"
- << "Content: " << builder_->str(node) << "\n"
<< "Pointer: " << node;
}
diff --git a/src/resolver/validation_test.cc b/src/resolver/validation_test.cc
index 1fe106a..1a95b03 100644
--- a/src/resolver/validation_test.cc
+++ b/src/resolver/validation_test.cc
@@ -47,10 +47,9 @@
using ResolverValidationTest = ResolverTest;
-class FakeStmt : public ast::Statement {
+class FakeStmt : public Castable<FakeStmt, ast::Statement> {
public:
- FakeStmt(ProgramID program_id, Source source)
- : ast::Statement(program_id, source) {}
+ FakeStmt(ProgramID program_id, Source source) : Base(program_id, source) {}
FakeStmt* Clone(CloneContext*) const override { return nullptr; }
void to_str(const sem::Info&, std::ostream& out, size_t) const override {
out << "Fake";
@@ -118,7 +117,8 @@
EXPECT_FALSE(r()->Resolve());
EXPECT_EQ(r()->error(),
- "error: unknown statement type for type determination: Fake");
+ "error: unknown statement type for type determination: "
+ "tint::resolver::FakeStmt");
}
TEST_F(ResolverValidationTest, Stmt_Error_Unknown) {
@@ -128,7 +128,8 @@
EXPECT_FALSE(r()->Resolve());
EXPECT_EQ(r()->error(),
- "2:30 error: unknown statement type for type determination: Fake");
+ "2:30 error: unknown statement type for type determination: "
+ "tint::resolver::FakeStmt");
}
TEST_F(ResolverValidationTest, Stmt_If_NonBool) {
@@ -1057,4 +1058,5 @@
} // namespace resolver
} // namespace tint
+TINT_INSTANTIATE_TYPEINFO(tint::resolver::FakeStmt);
TINT_INSTANTIATE_TYPEINFO(tint::resolver::FakeExpr);
