[clangd] Add language metrics for recovery AST usage.

[kadircet]

clang-tools-extra/clangd/Selection.cpp

41–44

nit: it might be nice to bail out early if no tracer is attached with trace::enabled

[njames93]

clang-tools-extra/clangd/Selection.cpp

40

Nit: any reason to use std::string here instead StringRef.

[hokein]

address comment.

[hokein]

clang-tools-extra/clangd/Selection.cpp

40

changed to const char *

41–44

good catch, thanks! (I didn't know there is such a thing).

[sammccall]

clang-tools-extra/clangd/Selection.cpp

41

Testing for specific C versions seems a bit weird to me - what if we're in C89?
I'm not sure what the intention is - if the idea is to exclude extensions like openmp I'm not sure this actually does that.
And by checking it before ObjC I think we're misclassifying clang -std=c99 foo.m

I'd suggest checking (optionally objc++), objc, c++, and calling everything else C. If you really want to avoid particular dialects, probably best to name them specifically.

53

I'm not clear what this is trying to measure - why isn't this the same metric as SelectionUsedRecovery (just adding a field to that one?)

[hokein]

address review comments.

clang-tools-extra/clangd/Selection.cpp

41

sounds good to me.

53

yeah, this is a good idea, didn't realize that we could use the label, thanks!

[sammccall]

clang-tools-extra/clangd/Selection.cpp

44

This classifies objc++ as c++.
Not necessarily a problem, up to you

[hokein]

clang-tools-extra/clangd/Selection.cpp

44

yeah, this is my intention as we don't really care about C++ or ObjC++.

clang-tools-extra/clangd/Selection.cpp

 #include <string>
 
 namespace clang {
 namespace clangd {
 namespace {
 using Node = SelectionTree::Node;
 using ast_type_traits::DynTypedNode;
 
 // Measure the fraction of selections that were enabled by recovery AST.
-void recordMetrics(const SelectionTree &S) {
+void recordMetrics(const SelectionTree &S, const LangOptions &Lang) {
+  if (!trace::enabled())
+    return;
+  const char *LanguageLabel = Lang.CPlusPlus ? "C++" : Lang.ObjC ? "ObjC" : "C";
   static constexpr trace::Metric SelectionUsedRecovery(
-      "selection_recovery", trace::Metric::Distribution);
-  static constexpr trace::Metric RecoveryType("selection_recovery_type",
-                                              trace::Metric::Distribution);
+      "selection_recovery", trace::Metric::Distribution, "language");
+  static constexpr trace::Metric RecoveryType(
+      "selection_recovery_type", trace::Metric::Distribution, "language");
   const auto *Common = S.commonAncestor();
   for (const auto *N = Common; N; N = N->Parent) {
     if (const auto *RE = N->ASTNode.get<RecoveryExpr>()) {
-      SelectionUsedRecovery.record(1); // used recovery ast.
-      RecoveryType.record(RE->isTypeDependent() ? 0 : 1);
+      SelectionUsedRecovery.record(1, LanguageLabel); // used recovery ast.
+      RecoveryType.record(RE->isTypeDependent() ? 0 : 1, LanguageLabel);
       return;
     }
   }
   if (Common)
-    SelectionUsedRecovery.record(0); // unused.
+    SelectionUsedRecovery.record(0, LanguageLabel); // unused.
 }
 
 // An IntervalSet maintains a set of disjoint subranges of an array.
 //
 // Initially, it contains the entire array.
 //           [-----------------------------------------------------------]
 //
 // When a range is erased(), it will typically split the array in two.
SelectionTree::SelectionTree(ASTContext &AST, const syntax::TokenBuffer &Tokens,
   PrintPolicy.TerseOutput = true;
   PrintPolicy.IncludeNewlines = false;
 
   dlog("Computing selection for {0}",
        SourceRange(SM.getComposedLoc(FID, Begin), SM.getComposedLoc(FID, End))
            .printToString(SM));
   Nodes = SelectionVisitor::collect(AST, Tokens, PrintPolicy, Begin, End, FID);
   Root = Nodes.empty() ? nullptr : &Nodes.front();
-  recordMetrics(*this);
+  recordMetrics(*this, AST.getLangOpts());
   dlog("Built selection tree\n{0}", *this);
 }
 
 const Node *SelectionTree::commonAncestor() const {
   const Node *Ancestor = Root;
   while (Ancestor->Children.size() == 1 && !Ancestor->Selected)
     Ancestor = Ancestor->Children.front();
   // Returning nullptr here is a bit unprincipled, but it makes the API safer:

clang-tools-extra/clangd/unittests/SelectionTests.cpp

 #include "clang/AST/Decl.h"
 #include "llvm/Support/Casting.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 namespace {
+using ::testing::ElementsAreArray;
 using ::testing::UnorderedElementsAreArray;
 
 // Create a selection tree corresponding to a point or pair of points.
 // This uses the precisely-defined createRight semantics. The fuzzier
 // createEach is tested separately.
 SelectionTree makeSelectionTree(const StringRef MarkedCode, ParsedAST &AST) {
   Annotations Test(MarkedCode);
   switch (Test.points().size()) {
for (const Case &C : Cases) {
 
     auto AST = TU.build();
     auto T = makeSelectionTree(C.Code, AST);
     EXPECT_EQ("TranslationUnitDecl", nodeKind(&T.root())) << C.Code;
 
     if (Test.ranges().empty()) {
       // If no [[range]] is marked in the example, there should be no selection.
       EXPECT_FALSE(T.commonAncestor()) << C.Code << "\n" << T;
-      EXPECT_THAT(Tracer.takeMetric("selection_recovery"), testing::IsEmpty());
+      EXPECT_THAT(Tracer.takeMetric("selection_recovery", "C++"),
+                  testing::IsEmpty());
     } else {
       // If there is an expected selection, common ancestor should exist
       // with the appropriate node type.
       EXPECT_EQ(C.CommonAncestorKind, nodeKind(T.commonAncestor()))
           << C.Code << "\n"
           << T;
       // Convert the reported common ancestor to a range and verify it.
       EXPECT_EQ(nodeRange(T.commonAncestor(), AST), Test.range())
           << C.Code << "\n"
           << T;
 
       // Check that common ancestor is reachable on exactly one path from root,
       // and no nodes outside it are selected.
       EXPECT_TRUE(verifyCommonAncestor(T.root(), T.commonAncestor(), C.Code))
           << C.Code;
-      EXPECT_THAT(Tracer.takeMetric("selection_recovery"),
-                  testing::ElementsAreArray({0}));
+      EXPECT_THAT(Tracer.takeMetric("selection_recovery", "C++"),
+                  ElementsAreArray({0}));
     }
   }
 }
 
 // Regression test: this used to match the injected X, not the outer X.
 TEST(SelectionTest, InjectedClassName) {
   const char *Code = "struct ^X { int x; };";
   auto AST = TestTU::withCode(Annotations(Code).code()).build();
   auto T = makeSelectionTree(Code, AST);
   ASSERT_EQ("CXXRecordDecl", nodeKind(T.commonAncestor())) << T;
   auto *D = dyn_cast<CXXRecordDecl>(T.commonAncestor()->ASTNode.get<Decl>());
   EXPECT_FALSE(D->isInjectedClassName());
 }
 
 TEST(SelectionTree, Metrics) {
   const char *Code = R"cpp(
     // error-ok: testing behavior on recovery expression
     int foo();
     int foo(int, int);
     int x = fo^o(42);
   )cpp";
   auto AST = TestTU::withCode(Annotations(Code).code()).build();
   trace::TestTracer Tracer;
   auto T = makeSelectionTree(Code, AST);
-  EXPECT_THAT(Tracer.takeMetric("selection_recovery"),
-              testing::ElementsAreArray({1}));
-  EXPECT_THAT(Tracer.takeMetric("selection_recovery_type"),
-              testing::ElementsAreArray({1}));
+  EXPECT_THAT(Tracer.takeMetric("selection_recovery", "C++"),
+              ElementsAreArray({1}));
+  EXPECT_THAT(Tracer.takeMetric("selection_recovery_type", "C++"),
+              ElementsAreArray({1}));
 }
 
 // FIXME: Doesn't select the binary operator node in
 //          #define FOO(X) X + 1
 //          int a, b = [[FOO(a)]];
 TEST(SelectionTest, Selected) {
   // Selection with ^marks^.
   // Partially selected nodes marked with a [[range]].