Add coverage testing for parse node kinds. (#4436)

This refactors the diagnostic kind coverage check into something that
also works for node kinds. Then, since this points out a few node kinds
that aren't having their parse verified, I'm adding minor tests for
those.

toolchain/diagnostics/BUILD

@@ -64,9 +64,9 @@ manifest(
 )
 
 cc_test(
-    name = "emitted_diagnostics_test",
+    name = "coverage_test",
     size = "small",
-    srcs = ["emitted_diagnostics_test.cpp"],
+    srcs = ["coverage_test.cpp"],
     args = ["--testdata_manifest=$(location :all_testdata.txt)"],
     data = [
         ":all_testdata.txt",
@@ -74,12 +74,10 @@ cc_test(
     ],
     deps = [
         ":diagnostic_kind",
-        "//common:set",
         "//testing/base:gtest_main",
+        "//toolchain/testing:coverage_helper",
         "@abseil-cpp//absl/flags:flag",
         "@googletest//:gtest",
-        "@llvm-project//llvm:Support",
-        "@re2",
     ],
 )
 

toolchain/diagnostics/coverage_test.cpp

@@ -0,0 +1,73 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#include <gtest/gtest.h>
+
+#include "absl/flags/flag.h"
+#include "toolchain/diagnostics/diagnostic_kind.h"
+#include "toolchain/testing/coverage_helper.h"
+
+ABSL_FLAG(std::string, testdata_manifest, "",
+          "A path to a file containing repo-relative names of test files.");
+
+namespace Carbon {
+namespace {
+
+constexpr DiagnosticKind DiagnosticKinds[] = {
+#define CARBON_DIAGNOSTIC_KIND(Name) DiagnosticKind::Name,
+#include "toolchain/diagnostics/diagnostic_kind.def"
+};
+
+constexpr DiagnosticKind UntestedDiagnosticKinds[] = {
+    // These exist only for unit tests.
+    DiagnosticKind::TestDiagnostic,
+    DiagnosticKind::TestDiagnosticNote,
+
+    // These diagnose filesystem issues that are hard to unit test.
+    DiagnosticKind::ErrorReadingFile,
+    DiagnosticKind::ErrorStattingFile,
+    DiagnosticKind::FileTooLarge,
+
+    // Int literals are currently limited to i32. Once that's fixed, this
+    // should be tested.
+    DiagnosticKind::ArrayBoundTooLarge,
+
+    // This isn't feasible to test with a normal testcase, but is tested in
+    // lex/tokenized_buffer_test.cpp.
+    DiagnosticKind::TooManyTokens,
+
+    // TODO: Should look closer at these, but adding tests is a high risk of
+    // loss in merge conflicts due to the amount of tests being changed right
+    // now.
+    DiagnosticKind::ExternLibraryInImporter,
+    DiagnosticKind::ExternLibraryOnDefinition,
+    DiagnosticKind::HexadecimalEscapeMissingDigits,
+    DiagnosticKind::ImplOfUndefinedInterface,
+    DiagnosticKind::IncompleteTypeInFunctionParam,
+    DiagnosticKind::InvalidDigit,
+    DiagnosticKind::InvalidDigitSeparator,
+    DiagnosticKind::InvalidHorizontalWhitespaceInString,
+    DiagnosticKind::MismatchedIndentInString,
+    DiagnosticKind::ModifierPrivateNotAllowed,
+    DiagnosticKind::MultiLineStringWithDoubleQuotes,
+    DiagnosticKind::NameAmbiguousDueToExtend,
+    DiagnosticKind::TooManyDigits,
+    DiagnosticKind::UnaryOperatorRequiresWhitespace,
+    DiagnosticKind::UnicodeEscapeSurrogate,
+    DiagnosticKind::UnicodeEscapeTooLarge,
+    DiagnosticKind::UnknownBaseSpecifier,
+    DiagnosticKind::UnsupportedCRLineEnding,
+    DiagnosticKind::UnsupportedLFCRLineEnding,
+};
+
+// Looks for diagnostic kinds that aren't covered by a file_test.
+TEST(Coverage, DiagnosticKind) {
+  Testing::TestKindCoverage(absl::GetFlag(FLAGS_testdata_manifest),
+                            R"(^ *// CHECK:STDERR: .*\.carbon:.* \[(\w+)\]$)",
+                            llvm::ArrayRef(DiagnosticKinds),
+                            llvm::ArrayRef(UntestedDiagnosticKinds));
+}
+
+}  // namespace
+}  // namespace Carbon

toolchain/diagnostics/emitted_diagnostics_test.cpp

@@ -1,130 +0,0 @@
-// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
-// Exceptions. See /LICENSE for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-
-#include <gtest/gtest.h>
-
-#include <fstream>
-#include <string>
-
-#include "absl/flags/flag.h"
-#include "common/set.h"
-#include "llvm/ADT/StringExtras.h"
-#include "re2/re2.h"
-#include "toolchain/diagnostics/diagnostic_kind.h"
-
-ABSL_FLAG(std::string, testdata_manifest, "",
-          "A path to a file containing repo-relative names of test files.");
-
-namespace Carbon {
-namespace {
-
-constexpr DiagnosticKind Diagnostics[] = {
-#define CARBON_DIAGNOSTIC_KIND(Name) DiagnosticKind::Name,
-#include "toolchain/diagnostics/diagnostic_kind.def"
-};
-
-// Returns true for diagnostics which have no tests. In general, diagnostics
-// should be tested.
-static auto IsUntestedDiagnostic(DiagnosticKind diagnostic_kind) -> bool {
-  switch (diagnostic_kind) {
-    case DiagnosticKind::TestDiagnostic:
-    case DiagnosticKind::TestDiagnosticNote:
-      // These exist only for unit tests.
-      return true;
-    case DiagnosticKind::ErrorReadingFile:
-    case DiagnosticKind::ErrorStattingFile:
-    case DiagnosticKind::FileTooLarge:
-      // These diagnose filesystem issues that are hard to unit test.
-      return true;
-    case DiagnosticKind::ArrayBoundTooLarge:
-      // Int literals are currently limited to i32. Once that's fixed, this
-      // should be tested.
-      return true;
-    case DiagnosticKind::ExternLibraryInImporter:
-    case DiagnosticKind::ExternLibraryOnDefinition:
-    case DiagnosticKind::HexadecimalEscapeMissingDigits:
-    case DiagnosticKind::ImplOfUndefinedInterface:
-    case DiagnosticKind::IncompleteTypeInFunctionParam:
-    case DiagnosticKind::InvalidDigit:
-    case DiagnosticKind::InvalidDigitSeparator:
-    case DiagnosticKind::InvalidHorizontalWhitespaceInString:
-    case DiagnosticKind::MismatchedIndentInString:
-    case DiagnosticKind::ModifierPrivateNotAllowed:
-    case DiagnosticKind::MultiLineStringWithDoubleQuotes:
-    case DiagnosticKind::NameAmbiguousDueToExtend:
-    case DiagnosticKind::TooManyDigits:
-    case DiagnosticKind::UnaryOperatorRequiresWhitespace:
-    case DiagnosticKind::UnicodeEscapeSurrogate:
-    case DiagnosticKind::UnicodeEscapeTooLarge:
-    case DiagnosticKind::UnknownBaseSpecifier:
-    case DiagnosticKind::UnsupportedCRLineEnding:
-    case DiagnosticKind::UnsupportedLFCRLineEnding:
-      // TODO: Should look closer at these, but adding tests is a high risk of
-      // loss in merge conflicts due to the amount of tests being changed right
-      // now.
-      return true;
-    case DiagnosticKind::TooManyTokens:
-      // This isn't feasible to test with a normal testcase, but is tested in
-      // lex/tokenized_buffer_test.cpp.
-      return true;
-    default:
-      return false;
-  }
-}
-
-TEST(EmittedDiagnostics, Verify) {
-  std::ifstream manifest_in(absl::GetFlag(FLAGS_testdata_manifest));
-  ASSERT_TRUE(manifest_in.good());
-
-  RE2 diagnostic_re(R"(^ *// CHECK:STDERR: .*\.carbon:.* \[(\w+)\]$)");
-  ASSERT_TRUE(diagnostic_re.ok()) << diagnostic_re.error();
-
-  Set<std::string> emitted_diagnostics;
-
-  std::string test_filename;
-  while (std::getline(manifest_in, test_filename)) {
-    std::ifstream test_in(test_filename);
-    ASSERT_TRUE(test_in.good());
-
-    std::string line;
-    while (std::getline(test_in, line)) {
-      std::string diagnostic;
-      if (RE2::PartialMatch(line, diagnostic_re, &diagnostic)) {
-        emitted_diagnostics.Insert(diagnostic);
-      }
-    }
-  }
-
-  llvm::SmallVector<llvm::StringRef> missing_diagnostics;
-  for (auto diagnostic_kind : Diagnostics) {
-    if (IsUntestedDiagnostic(diagnostic_kind)) {
-      EXPECT_FALSE(emitted_diagnostics.Erase(diagnostic_kind.name()))
-          << diagnostic_kind
-          << " was previously untested, and is now tested. That's good, but "
-             "please remove it from IsUntestedDiagnostic.";
-      continue;
-    }
-    if (!emitted_diagnostics.Erase(diagnostic_kind.name())) {
-      missing_diagnostics.push_back(diagnostic_kind.name());
-    }
-  }
-
-  constexpr llvm::StringLiteral Bullet = "\n  - ";
-
-  std::sort(missing_diagnostics.begin(), missing_diagnostics.end());
-  EXPECT_TRUE(missing_diagnostics.empty())
-      << "Some diagnostics have no tests:" << Bullet
-      << llvm::join(missing_diagnostics, Bullet);
-
-  llvm::SmallVector<std::string> unexpected_matches;
-  emitted_diagnostics.ForEach(
-      [&](const std::string& match) { unexpected_matches.push_back(match); });
-  std::sort(unexpected_matches.begin(), unexpected_matches.end());
-  EXPECT_TRUE(unexpected_matches.empty())
-      << "Matched things that don't appear to be diagnostics:" << Bullet
-      << llvm::join(unexpected_matches, Bullet);
-}
-
-}  // namespace
-}  // namespace Carbon

toolchain/parse/BUILD

@@ -3,6 +3,7 @@
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
 load("@rules_cc//cc:defs.bzl", "cc_library", "cc_test")
+load("//bazel/manifest:defs.bzl", "manifest")
 load("//testing/fuzzing:rules.bzl", "cc_fuzz_test")
 
 package(default_visibility = ["//visibility:public"])
@@ -12,6 +13,11 @@ filegroup(
     data = glob(["testdata/**/*.carbon"]),
 )
 
+manifest(
+    name = "testdata.txt",
+    srcs = [":testdata"],
+)
+
 cc_library(
     name = "node_kind",
     srcs = ["node_kind.cpp"],
@@ -198,3 +204,21 @@ cc_test(
         "@googletest//:gtest",
     ],
 )
+
+cc_test(
+    name = "coverage_test",
+    size = "small",
+    srcs = ["coverage_test.cpp"],
+    args = ["--testdata_manifest=$(location :testdata.txt)"],
+    data = [
+        ":testdata",
+        ":testdata.txt",
+    ],
+    deps = [
+        ":node_kind",
+        "//testing/base:gtest_main",
+        "//toolchain/testing:coverage_helper",
+        "@abseil-cpp//absl/flags:flag",
+        "@googletest//:gtest",
+    ],
+)

toolchain/parse/coverage_test.cpp

@@ -0,0 +1,32 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#include <gtest/gtest.h>
+
+#include "absl/flags/flag.h"
+#include "toolchain/parse/node_kind.h"
+#include "toolchain/testing/coverage_helper.h"
+
+ABSL_FLAG(std::string, testdata_manifest, "",
+          "A path to a file containing repo-relative names of test files.");
+
+namespace Carbon::Parse {
+namespace {
+
+constexpr NodeKind NodeKinds[] = {
+#define CARBON_PARSE_NODE_KIND(Name) NodeKind::Name,
+#include "toolchain/parse/node_kind.def"
+};
+
+constexpr NodeKind UntestedNodeKinds[] = {NodeKind::Placeholder};
+
+// Looks for node kinds that aren't covered by a file_test.
+TEST(Coverage, NodeKind) {
+  Testing::TestKindCoverage(absl::GetFlag(FLAGS_testdata_manifest),
+                            R"(kind: '(\w+)')", llvm::ArrayRef(NodeKinds),
+                            llvm::ArrayRef(UntestedNodeKinds));
+}
+
+}  // namespace
+}  // namespace Carbon::Parse

toolchain/parse/testdata/operators/infix.carbon

@@ -8,19 +8,42 @@
 // TIP: To dump output, run:
 // TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/parse/testdata/operators/infix.carbon
 
-var n: i8 = n * n;
+fn F() {
+  n * n;
+  n ^ n;
+  n >= n;
+  n >> n;
+  n / n;
+}
 
 // CHECK:STDOUT: - filename: infix.carbon
 // CHECK:STDOUT:   parse_tree: [
 // CHECK:STDOUT:     {kind: 'FileStart', text: ''},
-// CHECK:STDOUT:       {kind: 'VariableIntroducer', text: 'var'},
-// CHECK:STDOUT:         {kind: 'IdentifierName', text: 'n'},
-// CHECK:STDOUT:         {kind: 'IntTypeLiteral', text: 'i8'},
-// CHECK:STDOUT:       {kind: 'BindingPattern', text: ':', subtree_size: 3},
-// CHECK:STDOUT:       {kind: 'VariableInitializer', text: '='},
-// CHECK:STDOUT:         {kind: 'IdentifierNameExpr', text: 'n'},
-// CHECK:STDOUT:         {kind: 'IdentifierNameExpr', text: 'n'},
-// CHECK:STDOUT:       {kind: 'InfixOperatorStar', text: '*', subtree_size: 3},
-// CHECK:STDOUT:     {kind: 'VariableDecl', text: ';', subtree_size: 9},
+// CHECK:STDOUT:         {kind: 'FunctionIntroducer', text: 'fn'},
+// CHECK:STDOUT:         {kind: 'IdentifierName', text: 'F'},
+// CHECK:STDOUT:           {kind: 'TuplePatternStart', text: '('},
+// CHECK:STDOUT:         {kind: 'TuplePattern', text: ')', subtree_size: 2},
+// CHECK:STDOUT:       {kind: 'FunctionDefinitionStart', text: '{', subtree_size: 5},
+// CHECK:STDOUT:           {kind: 'IdentifierNameExpr', text: 'n'},
+// CHECK:STDOUT:           {kind: 'IdentifierNameExpr', text: 'n'},
+// CHECK:STDOUT:         {kind: 'InfixOperatorStar', text: '*', subtree_size: 3},
+// CHECK:STDOUT:       {kind: 'ExprStatement', text: ';', subtree_size: 4},
+// CHECK:STDOUT:           {kind: 'IdentifierNameExpr', text: 'n'},
+// CHECK:STDOUT:           {kind: 'IdentifierNameExpr', text: 'n'},
+// CHECK:STDOUT:         {kind: 'InfixOperatorCaret', text: '^', subtree_size: 3},
+// CHECK:STDOUT:       {kind: 'ExprStatement', text: ';', subtree_size: 4},
+// CHECK:STDOUT:           {kind: 'IdentifierNameExpr', text: 'n'},
+// CHECK:STDOUT:           {kind: 'IdentifierNameExpr', text: 'n'},
+// CHECK:STDOUT:         {kind: 'InfixOperatorGreaterEqual', text: '>=', subtree_size: 3},
+// CHECK:STDOUT:       {kind: 'ExprStatement', text: ';', subtree_size: 4},
+// CHECK:STDOUT:           {kind: 'IdentifierNameExpr', text: 'n'},
+// CHECK:STDOUT:           {kind: 'IdentifierNameExpr', text: 'n'},
+// CHECK:STDOUT:         {kind: 'InfixOperatorGreaterGreater', text: '>>', subtree_size: 3},
+// CHECK:STDOUT:       {kind: 'ExprStatement', text: ';', subtree_size: 4},
+// CHECK:STDOUT:           {kind: 'IdentifierNameExpr', text: 'n'},
+// CHECK:STDOUT:           {kind: 'IdentifierNameExpr', text: 'n'},
+// CHECK:STDOUT:         {kind: 'InfixOperatorSlash', text: '/', subtree_size: 3},
+// CHECK:STDOUT:       {kind: 'ExprStatement', text: ';', subtree_size: 4},
+// CHECK:STDOUT:     {kind: 'FunctionDefinition', text: '}', subtree_size: 26},
 // CHECK:STDOUT:     {kind: 'FileEnd', text: ''},
 // CHECK:STDOUT:   ]

toolchain/testing/BUILD

@@ -37,6 +37,18 @@ cc_library(
     ],
 )
 
+cc_library(
+    name = "coverage_helper",
+    testonly = 1,
+    hdrs = ["coverage_helper.h"],
+    deps = [
+        "//common:set",
+        "@googletest//:gtest",
+        "@llvm-project//llvm:Support",
+        "@re2",
+    ],
+)
+
 file_test(
     name = "file_test",
     size = "small",

toolchain/testing/coverage_helper.h

@@ -0,0 +1,82 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#ifndef CARBON_TOOLCHAIN_TESTING_COVERAGE_HELPER_H_
+#define CARBON_TOOLCHAIN_TESTING_COVERAGE_HELPER_H_
+
+#include <gtest/gtest.h>
+
+#include <fstream>
+#include <string>
+
+#include "common/set.h"
+#include "llvm/ADT/StringExtras.h"
+#include "re2/re2.h"
+
+namespace Carbon::Testing {
+
+// Looks for kinds that aren't covered by a file_test in the manifest path.
+// Kinds are identified by the provided regular expression kind_pattern.
+//
+// should_be_covered should return false when a kind is either untestable or not
+// yet tested.
+template <typename KindT>
+auto TestKindCoverage(const std::string& manifest_path,
+                      llvm::StringLiteral kind_pattern,
+                      llvm::ArrayRef<KindT> kinds,
+                      llvm::ArrayRef<KindT> untested_kinds) {
+  std::ifstream manifest_in(manifest_path.c_str());
+  ASSERT_TRUE(manifest_in.good());
+
+  RE2 kind_re(kind_pattern.data());
+  ASSERT_TRUE(kind_re.ok()) << kind_re.error();
+
+  Set<std::string> covered_kinds;
+
+  std::string test_filename;
+  while (std::getline(manifest_in, test_filename)) {
+    std::ifstream test_in(test_filename);
+    ASSERT_TRUE(test_in.good());
+
+    std::string line;
+    while (std::getline(test_in, line)) {
+      std::string kind;
+      if (RE2::PartialMatch(line, kind_re, &kind)) {
+        covered_kinds.Insert(kind);
+      }
+    }
+  }
+
+  llvm::SmallVector<llvm::StringRef> missing_kinds;
+  for (auto kind : kinds) {
+    if (llvm::find(untested_kinds, kind) != untested_kinds.end()) {
+      EXPECT_FALSE(covered_kinds.Erase(kind.name()))
+          << "Kind " << kind
+          << " has coverage even though none was expected. If this has "
+             "changed, update the coverage test.";
+      continue;
+    }
+    if (!covered_kinds.Erase(kind.name())) {
+      missing_kinds.push_back(kind.name());
+    }
+  }
+
+  constexpr llvm::StringLiteral Bullet = "\n  - ";
+
+  std::sort(missing_kinds.begin(), missing_kinds.end());
+  EXPECT_TRUE(missing_kinds.empty()) << "Some kinds have no tests:" << Bullet
+                                     << llvm::join(missing_kinds, Bullet);
+
+  llvm::SmallVector<std::string> unexpected_matches;
+  covered_kinds.ForEach(
+      [&](const std::string& match) { unexpected_matches.push_back(match); });
+  std::sort(unexpected_matches.begin(), unexpected_matches.end());
+  EXPECT_TRUE(unexpected_matches.empty())
+      << "Matched things that aren't in the kind list:" << Bullet
+      << llvm::join(unexpected_matches, Bullet);
+}
+
+}  // namespace Carbon::Testing
+
+#endif  // CARBON_TOOLCHAIN_TESTING_COVERAGE_HELPER_H_