From 73cf277bdf870b53386f28c774c5cfabcc8811f4 Mon Sep 17 00:00:00 2001 From: josh11b Date: Wed, 27 Dec 2023 16:27:53 -0800 Subject: [PATCH] Test trace output of `Tree::VerifyExtractAs`, fix found bugs (#3545) Tests previously uncovered code. Fix uncovered problems: * formatting of trace output * package & import directives need to be classified as declarations * the problem that meant the previous problem wasn't caught by existing tests (since `Tree::Verify` didn't check that top-level declarations match `AnyDeclId`, as required by `Tree::ExtractFile()`). --- toolchain/parse/extract.cpp | 19 ++-- toolchain/parse/tree.cpp | 12 ++- toolchain/parse/typed_nodes.h | 9 +- toolchain/parse/typed_nodes_test.cpp | 143 +++++++++++++++++++++++++++ 4 files changed, 169 insertions(+), 14 deletions(-) diff --git a/toolchain/parse/extract.cpp b/toolchain/parse/extract.cpp index 3b6fd5fd8084..ac1846124efe 100644 --- a/toolchain/parse/extract.cpp +++ b/toolchain/parse/extract.cpp @@ -3,6 +3,7 @@ // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception #include +#include #include #include "common/error.h" @@ -144,7 +145,7 @@ struct Extractable> { } if (trace) { *trace << "NodeIdOneOf " << T::Kind << " or " << U::Kind << ": " - << tree->node_kind(*it) << " consumed"; + << tree->node_kind(*it) << " consumed\n"; } return NodeIdOneOf(*it++); } @@ -209,18 +210,18 @@ struct Extractable> { Tree::SiblingIterator end, ErrorBuilder* trace) -> std::optional> { if (trace) { - *trace << "Optional" << typeid(T).name() << ": begin\n"; + *trace << "Optional " << typeid(T).name() << ": begin\n"; } auto old_it = it; std::optional value = Extractable::Extract(tree, it, end, trace); if (value) { if (trace) { - *trace << "Optional" << typeid(T).name() << ": found\n"; + *trace << "Optional " << typeid(T).name() << ": found\n"; } return value; } if (trace) { - *trace << "Optional" << typeid(T).name() << ": missing\n"; + *trace << "Optional " << typeid(T).name() << ": missing\n"; } it = old_it; return value; @@ -238,7 +239,7 @@ struct Extractable> { -> std::optional> { std::tuple...> fields; if (trace) { - *trace << "Tuple: begin\n"; + *trace << sizeof...(T) << "-tuple: begin\n"; } // Use a fold over the `=` operator to parse fields from right to left. @@ -252,13 +253,13 @@ struct Extractable> { if (!ok) { if (trace) { - *trace << "Tuple: error\n"; + *trace << sizeof...(T) << "-tuple: error\n"; } return std::nullopt; } if (trace) { - *trace << "Tuple: success\n"; + *trace << sizeof...(T) << "-tuple: success\n"; } return std::tuple{std::move(std::get(fields).value())...}; } @@ -286,13 +287,13 @@ struct Extractable { auto tuple = Extractable::Extract(tree, it, end, trace); if (!tuple.has_value()) { if (trace) { - *trace << "Aggregate" << typeid(T).name() << ": error\n"; + *trace << "Aggregate " << typeid(T).name() << ": error\n"; } return std::nullopt; } if (trace) { - *trace << "Aggregate" << typeid(T).name() << ": success\n"; + *trace << "Aggregate " << typeid(T).name() << ": success\n"; } // Convert the tuple to the struct type. return std::apply( diff --git a/toolchain/parse/tree.cpp b/toolchain/parse/tree.cpp index a5d0ee5733e8..2c3bb77f0eb6 100644 --- a/toolchain/parse/tree.cpp +++ b/toolchain/parse/tree.cpp @@ -248,8 +248,8 @@ auto Tree::Verify() const -> ErrorOr { } // Should extract successfully if node not marked as having an error. // Without this code, a 10 mloc test case of lex & parse takes - // 4.129 s ± 0.041 s. With this additional verification, it takes - // 5.768 s ± 0.036 s. + // 4.129 s ± 0.041 s. With this additional verification, it takes + // 5.768 s ± 0.036 s. if (!n_impl.has_error && !TestExtract(this, n, n_impl.kind, nullptr)) { ErrorBuilder trace; trace << llvm::formatv( @@ -311,6 +311,14 @@ auto Tree::Verify() const -> ErrorOr { prev_index = n.index; } + // Validate the roots, ensures Tree::ExtractFile() doesn't CHECK-fail. + if (!TryExtractNodeFromChildren(roots(), nullptr)) { + ErrorBuilder trace; + trace << "Roots of tree couldn't be extracted as a `File`. Trace:\n"; + TryExtractNodeFromChildren(roots(), &trace); + return trace; + } + if (!has_errors_ && static_cast(node_impls_.size()) != tokens_->expected_parse_tree_size()) { return Error( diff --git a/toolchain/parse/typed_nodes.h b/toolchain/parse/typed_nodes.h index 93afaff35bc3..d5972f78baa2 100644 --- a/toolchain/parse/typed_nodes.h +++ b/toolchain/parse/typed_nodes.h @@ -165,7 +165,8 @@ struct LibrarySpecifier { // First line of the file, such as: // `package MyPackage library "MyLibrary" impl;` struct PackageDirective { - static constexpr auto Kind = NodeKind::PackageDirective.Define(); + static constexpr auto Kind = + NodeKind::PackageDirective.Define(NodeCategory::Decl); PackageIntroducerId introducer; std::optional name; @@ -176,7 +177,8 @@ struct PackageDirective { // `import TheirPackage library "TheirLibrary";` using ImportIntroducer = LeafNode; struct ImportDirective { - static constexpr auto Kind = NodeKind::ImportDirective.Define(); + static constexpr auto Kind = + NodeKind::ImportDirective.Define(NodeCategory::Decl); ImportIntroducerId introducer; std::optional name; @@ -186,7 +188,8 @@ struct ImportDirective { // `library` as directive. using LibraryIntroducer = LeafNode; struct LibraryDirective { - static constexpr auto Kind = NodeKind::LibraryDirective.Define(); + static constexpr auto Kind = + NodeKind::LibraryDirective.Define(NodeCategory::Decl); LibraryIntroducerId introducer; NodeIdOneOf library_name; diff --git a/toolchain/parse/typed_nodes_test.cpp b/toolchain/parse/typed_nodes_test.cpp index 6094d0fb41c1..ec5e15bd9710 100644 --- a/toolchain/parse/typed_nodes_test.cpp +++ b/toolchain/parse/typed_nodes_test.cpp @@ -137,6 +137,149 @@ TEST_F(TypedNodeTest, For) { ASSERT_TRUE(for_var_name.has_value()); } +TEST_F(TypedNodeTest, VerifyExtractTraceLibrary) { + auto* tree = &GetTree(R"carbon( + library default impl; + )carbon"); + auto file = tree->ExtractFile(); + + ASSERT_EQ(file.decls.size(), 1); + ErrorBuilder trace; + auto library = tree->VerifyExtractAs(file.decls[0], &trace); + EXPECT_TRUE(library.has_value()); + Error err = trace; + // Use Regex matching to avoid hard-coding the result of `typeinfo(T).name()`. + EXPECT_THAT(err.message(), testing::MatchesRegex( + R"Trace(Aggregate [^:]*: begin +3-tuple: begin +NodeIdOneOf PackageApi or PackageImpl: PackageImpl consumed +NodeIdOneOf LibraryName or DefaultLibrary: DefaultLibrary consumed +NodeIdForKind: LibraryIntroducer consumed +3-tuple: success +Aggregate [^:]*: success +)Trace")); +} + +TEST_F(TypedNodeTest, VerifyExtractTraceVarNoInit) { + auto* tree = &GetTree(R"carbon( + var x: bool; + )carbon"); + auto file = tree->ExtractFile(); + + ASSERT_EQ(file.decls.size(), 1); + ErrorBuilder trace; + auto var = tree->VerifyExtractAs(file.decls[0], &trace); + ASSERT_TRUE(var.has_value()); + Error err = trace; + // Use Regex matching to avoid hard-coding the result of `typeinfo(T).name()`. + EXPECT_THAT(err.message(), testing::MatchesRegex( + R"Trace(Aggregate [^:]*: begin +5-tuple: begin +Optional [^:]*: begin +Aggregate [^:]*: begin +2-tuple: begin +NodeIdInCategory Expr error: kind BindingPattern doesn't match +2-tuple: error +Aggregate [^:]*: error +Optional [^:]*: missing +NodeIdInCategory Pattern: kind BindingPattern consumed +Optional [^:]*: begin +NodeIdForKind error: wrong kind VariableIntroducer, expected ReturnedModifier +Optional [^:]*: missing +Vector: begin +NodeIdInCategory Modifier error: kind VariableIntroducer doesn't match +Vector: end +NodeIdForKind: VariableIntroducer consumed +5-tuple: success +Aggregate [^:]*: success +)Trace")); +} + +TEST_F(TypedNodeTest, VerifyExtractTraceExpression) { + auto* tree = &GetTree(R"carbon( + var x: i32 = p->q.r; + )carbon"); + auto file = tree->ExtractFile(); + + ASSERT_EQ(file.decls.size(), 1); + ErrorBuilder trace1; + auto var = tree->VerifyExtractAs(file.decls[0], &trace1); + ASSERT_TRUE(var.has_value()); + Error err1 = trace1; + // Use Regex matching to avoid hard-coding the result of `typeinfo(T).name()`. + EXPECT_THAT(err1.message(), testing::MatchesRegex( + R"Trace(Aggregate [^:]*: begin +5-tuple: begin +Optional [^:]*leDecl11InitializerE: begin +Aggregate [^:]*: begin +2-tuple: begin +NodeIdInCategory Expr: kind MemberAccessExpr consumed +NodeIdForKind: VariableInitializer consumed +2-tuple: success +Aggregate [^:]*: success +Optional [^:]*: found +NodeIdInCategory Pattern: kind BindingPattern consumed +Optional [^:]*: begin +NodeIdForKind error: wrong kind VariableIntroducer, expected ReturnedModifier +Optional [^:]*: missing +Vector: begin +NodeIdInCategory Modifier error: kind VariableIntroducer doesn't match +Vector: end +NodeIdForKind: VariableIntroducer consumed +5-tuple: success +Aggregate [^:]*: success +)Trace")); + + ASSERT_TRUE(var->initializer.has_value()); + ErrorBuilder trace2; + auto value = + tree->VerifyExtractAs(var->initializer->value, &trace2); + ASSERT_TRUE(value.has_value()); + Error err2 = trace2; + // Use Regex matching to avoid hard-coding the result of `typeinfo(T).name()`. + EXPECT_THAT(err2.message(), testing::MatchesRegex( + R"Trace(Aggregate [^:]*: begin +2-tuple: begin +NodeId: IdentifierName consumed +NodeIdInCategory Expr: kind PointerMemberAccessExpr consumed +2-tuple: success +Aggregate [^:]*: success +)Trace")); +} + +TEST_F(TypedNodeTest, VerifyExtractTraceClassDecl) { + auto* tree = &GetTree(R"carbon( + private abstract class N.C(T:! type); + )carbon"); + auto file = tree->ExtractFile(); + + ASSERT_EQ(file.decls.size(), 1); + ErrorBuilder trace; + auto class_decl = tree->VerifyExtractAs(file.decls[0], &trace); + EXPECT_TRUE(class_decl.has_value()); + Error err = trace; + // Use Regex matching to avoid hard-coding the result of `typeinfo(T).name()`. + EXPECT_THAT(err.message(), testing::MatchesRegex( + R"Trace(Aggregate [^:]*: begin +5-tuple: begin +Optional [^:]*: begin +NodeIdForKind: TuplePattern consumed +Optional [^:]*: found +Optional [^:]*: begin +NodeIdForKind error: wrong kind QualifiedName, expected ImplicitParamList +Optional [^:]*: missing +NodeIdInCategory NameComponent: kind QualifiedName consumed +Vector: begin +NodeIdInCategory Modifier: kind AbstractModifier consumed +NodeIdInCategory Modifier: kind PrivateModifier consumed +NodeIdInCategory Modifier error: kind ClassIntroducer doesn't match +Vector: end +NodeIdForKind: ClassIntroducer consumed +5-tuple: success +Aggregate [^:]*: success +)Trace")); +} + auto CategoryMatches(const NodeKind::Definition& def, NodeKind kind, const char* name) { EXPECT_EQ(def.category(), kind.category()) << name;