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

@@ -3,6 +3,7 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
 #include <tuple>
+#include <typeinfo>
 #include <utility>
 
 #include "common/error.h"
@@ -144,7 +145,7 @@ struct Extractable<NodeIdOneOf<T, U>> {
     }
     if (trace) {
       *trace << "NodeIdOneOf " << T::Kind << " or " << U::Kind << ": "
-             << tree->node_kind(*it) << " consumed";
+             << tree->node_kind(*it) << " consumed\n";
     }
     return NodeIdOneOf<T, U>(*it++);
   }
@@ -209,18 +210,18 @@ struct Extractable<std::optional<T>> {
                       Tree::SiblingIterator end, ErrorBuilder* trace)
       -> std::optional<std::optional<T>> {
     if (trace) {
-      *trace << "Optional" << typeid(T).name() << ": begin\n";
+      *trace << "Optional " << typeid(T).name() << ": begin\n";
     }
     auto old_it = it;
     std::optional<T> value = Extractable<T>::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::tuple<T...>> {
       -> std::optional<std::tuple<T...>> {
     std::tuple<std::optional<T>...> 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<std::tuple<T...>> {
 
     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<T...>{std::move(std::get<Index>(fields).value())...};
   }
@@ -286,13 +287,13 @@ struct Extractable {
     auto tuple = Extractable<TupleType>::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(

toolchain/parse/tree.cpp

@@ -248,8 +248,8 @@ auto Tree::Verify() const -> ErrorOr<Success> {
     }
     // 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<Success> {
     prev_index = n.index;
   }
 
+  // Validate the roots, ensures Tree::ExtractFile() doesn't CHECK-fail.
+  if (!TryExtractNodeFromChildren<File>(roots(), nullptr)) {
+    ErrorBuilder trace;
+    trace << "Roots of tree couldn't be extracted as a `File`. Trace:\n";
+    TryExtractNodeFromChildren<File>(roots(), &trace);
+    return trace;
+  }
+
   if (!has_errors_ && static_cast<int32_t>(node_impls_.size()) !=
                           tokens_->expected_parse_tree_size()) {
     return Error(

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<PackageNameId> name;
@@ -176,7 +177,8 @@ struct PackageDirective {
 // `import TheirPackage library "TheirLibrary";`
 using ImportIntroducer = LeafNode<NodeKind::ImportIntroducer>;
 struct ImportDirective {
-  static constexpr auto Kind = NodeKind::ImportDirective.Define();
+  static constexpr auto Kind =
+      NodeKind::ImportDirective.Define(NodeCategory::Decl);
 
   ImportIntroducerId introducer;
   std::optional<PackageNameId> name;
@@ -186,7 +188,8 @@ struct ImportDirective {
 // `library` as directive.
 using LibraryIntroducer = LeafNode<NodeKind::LibraryIntroducer>;
 struct LibraryDirective {
-  static constexpr auto Kind = NodeKind::LibraryDirective.Define();
+  static constexpr auto Kind =
+      NodeKind::LibraryDirective.Define(NodeCategory::Decl);
 
   LibraryIntroducerId introducer;
   NodeIdOneOf<LibraryName, DefaultLibrary> library_name;

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<LibraryDirective>(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<VariableDecl>(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<VariableDecl>(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<MemberAccessExpr>(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<ClassDecl>(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;