From 11ed0db30dba4759436d75f7e28bd0c09c2d0e7d Mon Sep 17 00:00:00 2001 From: Ben Clayton Date: Thu, 14 Oct 2021 20:30:29 +0000 Subject: [PATCH] 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 Reviewed-by: Antonio Maiorano Reviewed-by: James Price Commit-Queue: Ben Clayton --- src/program_builder.h | 65 ++++++ src/resolver/control_block_validation_test.cc | 209 ++++++------------ src/resolver/resolver.cc | 34 ++- src/resolver/validation_test.cc | 12 +- 4 files changed, 156 insertions(+), 164 deletions(-) diff --git a/src/program_builder.h b/src/program_builder.h index fc3b59e994..1bfa21f94f 100644 --- a/src/program_builder.h +++ b/src/program_builder.h @@ -1105,18 +1105,46 @@ class ProgramBuilder { /// @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(source, val); + } + /// @param val the boolan value /// @return a boolean literal with the given value ast::BoolLiteral* Literal(bool val) { return create(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(source, val); + } + /// @param val the float value /// @return a float literal with the given value ast::FloatLiteral* Literal(f32 val) { return create(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(source, val); + } + /// @param val the unsigned int value /// @return a ast::UintLiteral with the given value ast::UintLiteral* Literal(u32 val) { return create(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(source, val); + } + /// @param val the integer value /// @return the ast::SintLiteral with the given value ast::SintLiteral* Literal(i32 val) { return create(val); } @@ -2071,16 +2099,44 @@ class ProgramBuilder { } /// 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 + ast::SwitchStatement* Switch(const Source& source, + ExpressionInit&& condition, + Cases&&... cases) { + return create( + source, Expr(std::forward(condition)), + ast::CaseStatementList{std::forward(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 > ast::SwitchStatement* Switch(ExpressionInit&& condition, Cases&&... cases) { return create( Expr(std::forward(condition)), ast::CaseStatementList{std::forward(cases)...}); } + /// 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(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 @@ -2100,6 +2156,15 @@ class ProgramBuilder { return Case(ast::CaseSelectorList{selector}, body); } + /// 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 diff --git a/src/resolver/control_block_validation_test.cc b/src/resolver/control_block_validation_test.cc index 6ee6c55d46..c317a42b91 100644 --- a/src/resolver/control_block_validation_test.cc +++ b/src/resolver/control_block_validation_test.cc @@ -32,14 +32,8 @@ TEST_F(ResolverControlBlockValidationTest, // } auto* var = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(3.14f)); - ast::CaseStatementList body; - auto* block_default = Block(); - body.push_back( - create(ast::CaseSelectorList{}, block_default)); - - auto* block = - Block(Decl(var), create( - 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 @@ TEST_F(ResolverControlBlockValidationTest, SwitchWithoutDefault_Fail) { // } 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(csl, Block())); - - auto* block = - Block(Decl(var), create( - 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 @@ TEST_F(ResolverControlBlockValidationTest, SwitchWithTwoDefault_Fail) { // } 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(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(csl_case_1, block_case_1)); - - ast::CaseSelectorList default_csl_2; - auto* block_default_2 = Block(); - switch_body.push_back(create( - Source{Source::Location{12, 34}}, default_csl_2, block_default_2)); - - auto* block = - Block(Decl(var), create(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, } 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(), 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()))), 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 @@ TEST_F(ResolverControlBlockValidationTest, // } auto* var = Var("a", ty.i32(), ast::StorageClass::kNone, Expr(2)); - ast::CaseStatementList switch_body; - ast::CaseSelectorList csl; - csl.push_back(create(1u)); - switch_body.push_back(create( - Source{Source::Location{12, 34}}, csl, Block())); - - ast::CaseSelectorList default_csl; - auto* block_default = Block(); - switch_body.push_back(create(default_csl, block_default)); - auto* block = - Block(Decl(var), create(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 @@ TEST_F(ResolverControlBlockValidationTest, // } 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( - Source{Source::Location{12, 34}}, csl, Block())); - - ast::CaseSelectorList default_csl; - auto* block_default = Block(); - switch_body.push_back(create(default_csl, block_default)); - - auto* block = - Block(Decl(var), create(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 @@ TEST_F(ResolverControlBlockValidationTest, 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(0u)); - switch_body.push_back(create(csl_1, Block())); - - ast::CaseSelectorList csl_2; - csl_2.push_back(create(2u)); - csl_2.push_back(create(2u)); - switch_body.push_back(create( - Source{Source::Location{12, 34}}, csl_2, Block())); - - ast::CaseSelectorList default_csl; - auto* block_default = Block(); - switch_body.push_back(create(default_csl, block_default)); - - auto* block = - Block(Decl(var), create(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(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( - Source{Source::Location{12, 34}}, csl_2, Block())); - - ast::CaseSelectorList default_csl; - auto* block_default = Block(); - switch_body.push_back(create(default_csl, block_default)); - - auto* block = - Block(Decl(var), create(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 @@ TEST_F(ResolverControlBlockValidationTest, // default: { fallthrough; } // } auto* var = Var("a", ty.i32(), ast::StorageClass::kNone, Expr(2)); - - ast::CaseSelectorList default_csl; - auto* block_default = Block( - create(Source{Source::Location{12, 34}})); - ast::CaseStatementList body; - body.push_back(create(default_csl, block_default)); - - auto* block = Block(Decl(var), create(Expr("a"), body)); + auto* fallthrough = create(Source{{12, 34}}); + auto* block = Block(Decl(var), // + Switch("a", // + DefaultCase(Block(fallthrough)))); WrapInFunction(block); EXPECT_FALSE(r()->Resolve()); @@ -367,17 +307,10 @@ TEST_F(ResolverControlBlockValidationTest, SwitchCase_Pass) { // } 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(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(case_csl, block_case)); - - auto* block = Block(Decl(var), create(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 @@ TEST_F(ResolverControlBlockValidationTest, SwitchCaseAlias_Pass) { 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(Source{Source::Location{12, 34}}, - default_csl, block_default)); - - auto* block = Block(Decl(var), create(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 a9ab1653d3..cc64de02da 100644 --- a/src/resolver/resolver.cc +++ b/src/resolver/resolver.cc @@ -273,12 +273,10 @@ bool Resolver::ResolveInternal() { 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 @@ bool Resolver::Statement(ast::Statement* stmt) { 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 Resolver::ValidateSwitch(const ast::SwitchStatement* s) { } bool has_default = false; - std::unordered_set selector_set; + std::unordered_map selectors; for (auto* case_stmt : s->body()) { if (case_stmt->IsDefault()) { @@ -4412,15 +4410,16 @@ bool Resolver::ValidateSwitch(const ast::SwitchStatement* s) { } 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() + ? 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 @@ void Resolver::Mark(const ast::Node* node) { << "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 1fe106a84b..1a95b03b8b 100644 --- a/src/resolver/validation_test.cc +++ b/src/resolver/validation_test.cc @@ -47,10 +47,9 @@ namespace { using ResolverValidationTest = ResolverTest; -class FakeStmt : public ast::Statement { +class FakeStmt : public Castable { 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 @@ TEST_F(ResolverValidationTest, Error_WithEmptySource) { 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 @@ TEST_F(ResolverValidationTest, Stmt_Error_Unknown) { 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 @@ TEST_F(ResolverTest, Expr_Constructor_Cast_Pointer) { } // namespace resolver } // namespace tint +TINT_INSTANTIATE_TYPEINFO(tint::resolver::FakeStmt); TINT_INSTANTIATE_TYPEINFO(tint::resolver::FakeExpr);