[clangd] Extend YAML Serialization

[mnauw]

This was previously part of D77385. Rather than a separate file (which seems a bit tricky), the test YAML has simply been included in the unit test itself.

[sammccall]

Thanks for doing this!

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

1

Ack, sorry for not mentioning this in the previous review.
The YAML stuff is tested mixed together with the binary format stuff in SerializationTests.cpp.
Is it possible to extend the YAML literal in that file with a synthetic Source and Cmd entry, and test it in a similar way?

[mnauw]

As suggested, existing (YAML) Serialization test has been extended.

[sammccall]

Thank you!

Do you have commit access, or would you like me to land this and D78038 for you?

[mnauw]

I do not have commit access, so it would be helpful that you land this (and the other) ;-)

[sammccall]

Done, thanks for the contributions!
Feel free to apply for commit access at any point if you plan to work more on clangd or other LLVM stuff.

clang-tools-extra/clangd/index/YAMLSerialization.cpp

 namespace {
 using RefBundle =
     std::pair<clang::clangd::SymbolID, std::vector<clang::clangd::Ref>>;
 // This is a pale imitation of std::variant<Symbol, RefBundle, Relation>
 struct VariantEntry {
   llvm::Optional<clang::clangd::Symbol> Symbol;
   llvm::Optional<RefBundle> Refs;
   llvm::Optional<clang::clangd::Relation> Relation;
+  llvm::Optional<clang::clangd::IncludeGraphNode> Source;
+  llvm::Optional<clang::tooling::CompileCommand> Cmd;
 };
 // A class helps YAML to serialize the 32-bit encoded position (Line&Column),
 // as YAMLIO can't directly map bitfields.
 struct YPosition {
   uint32_t Line;
   uint32_t Column;
 };
 
+// avoid ODR violation of specialization for non-owned CompileCommand
+struct CompileCommandYAML : clang::tooling::CompileCommand {};
+
 } // namespace
 namespace llvm {
 namespace yaml {
 
+using clang::clangd::FileDigest;
+using clang::clangd::IncludeGraph;
+using clang::clangd::IncludeGraphNode;
 using clang::clangd::Ref;
 using clang::clangd::RefKind;
 using clang::clangd::Relation;
 using clang::clangd::RelationKind;
 using clang::clangd::Symbol;
 using clang::clangd::SymbolID;
 using clang::clangd::SymbolLocation;
 using clang::clangd::SymbolOrigin;
 using clang::index::SymbolInfo;
 using clang::index::SymbolKind;
 using clang::index::SymbolLanguage;
 using clang::index::SymbolRole;
+using clang::tooling::CompileCommand;
 
 // Helper to (de)serialize the SymbolID. We serialize it as a hex string.
 struct NormalizedSymbolID {
   NormalizedSymbolID(IO &) {}
   NormalizedSymbolID(IO &, const SymbolID &ID) {
     llvm::raw_string_ostream OS(HexString);
     OS << ID;
   }
static void mapping(IO &IO, Relation &Relation) {
     MappingNormalization<NormalizedSymbolRole, RelationKind> NRole(
         IO, Relation.Predicate);
     IO.mapRequired("Subject", Relation.Subject);
     IO.mapRequired("Predicate", NRole->Kind);
     IO.mapRequired("Object", Relation.Object);
   }
 };
 
+struct NormalizedSourceFlag {
+  NormalizedSourceFlag(IO &) {}
+  NormalizedSourceFlag(IO &, IncludeGraphNode::SourceFlag O) {
+    Flag = static_cast<uint8_t>(O);
+  }
+
+  IncludeGraphNode::SourceFlag denormalize(IO &) {
+    return static_cast<IncludeGraphNode::SourceFlag>(Flag);
+  }
+
+  uint8_t Flag = 0;
+};
+
+struct NormalizedFileDigest {
+  NormalizedFileDigest(IO &) {}
+  NormalizedFileDigest(IO &, const FileDigest &Digest) {
+    HexString = llvm::toHex(Digest);
+  }
+
+  FileDigest denormalize(IO &I) {
+    FileDigest Digest;
+    if (HexString.size() == Digest.size() * 2 &&
+        llvm::all_of(HexString, llvm::isHexDigit)) {
+      memcpy(Digest.data(), llvm::fromHex(HexString).data(), Digest.size());
+    } else {
+      I.setError(std::string("Bad hex file digest: ") + HexString);
+    }
+    return Digest;
+  }
+
+  std::string HexString;
+};
+
+template <> struct MappingTraits<IncludeGraphNode> {
+  static void mapping(IO &IO, IncludeGraphNode &Node) {
+    IO.mapRequired("URI", Node.URI);
+    MappingNormalization<NormalizedSourceFlag, IncludeGraphNode::SourceFlag>
+        NSourceFlag(IO, Node.Flags);
+    IO.mapRequired("Flags", NSourceFlag->Flag);
+    MappingNormalization<NormalizedFileDigest, FileDigest> NDigest(IO,
+                                                                   Node.Digest);
+    IO.mapRequired("Digest", NDigest->HexString);
+    IO.mapRequired("DirectIncludes", Node.DirectIncludes);
+  }
+};
+
+template <> struct MappingTraits<CompileCommandYAML> {
+  static void mapping(IO &IO, CompileCommandYAML &Cmd) {
+    IO.mapRequired("Directory", Cmd.Directory);
+    IO.mapRequired("CommandLine", Cmd.CommandLine);
+  }
+};
+
 template <> struct MappingTraits<VariantEntry> {
   static void mapping(IO &IO, VariantEntry &Variant) {
     if (IO.mapTag("!Symbol", Variant.Symbol.hasValue())) {
       if (!IO.outputting())
         Variant.Symbol.emplace();
       MappingTraits<Symbol>::mapping(IO, *Variant.Symbol);
     } else if (IO.mapTag("!Refs", Variant.Refs.hasValue())) {
       if (!IO.outputting())
         Variant.Refs.emplace();
       MappingTraits<RefBundle>::mapping(IO, *Variant.Refs);
     } else if (IO.mapTag("!Relations", Variant.Relation.hasValue())) {
       if (!IO.outputting())
         Variant.Relation.emplace();
       MappingTraits<Relation>::mapping(IO, *Variant.Relation);
+    } else if (IO.mapTag("!Source", Variant.Source.hasValue())) {
+      if (!IO.outputting())
+        Variant.Source.emplace();
+      MappingTraits<IncludeGraphNode>::mapping(IO, *Variant.Source);
+    } else if (IO.mapTag("!Cmd", Variant.Cmd.hasValue())) {
+      if (!IO.outputting())
+        Variant.Cmd.emplace();
+      MappingTraits<CompileCommandYAML>::mapping(
+          IO, static_cast<CompileCommandYAML &>(*Variant.Cmd));
     }
   }
 };
 
 } // namespace yaml
 } // namespace llvm
 
 namespace clang {
for (auto &Sym : *O.Refs) {
       Yout << Entry;
     }
   if (O.Relations)
     for (auto &R : *O.Relations) {
       VariantEntry Entry;
       Entry.Relation = R;
       Yout << Entry;
     }
+  if (O.Sources) {
+    for (const auto &Source : *O.Sources) {
+      VariantEntry Entry;
+      Entry.Source = Source.getValue();
+      Yout << Entry;
+    }
+  }
+  if (O.Cmd) {
+    VariantEntry Entry;
+    Entry.Cmd = *O.Cmd;
+    Yout << Entry;
+  }
 }
 
 llvm::Expected<IndexFileIn> readYAML(llvm::StringRef Data) {
   SymbolSlab::Builder Symbols;
   RefSlab::Builder Refs;
   RelationSlab::Builder Relations;
   llvm::BumpPtrAllocator
       Arena; // store the underlying data of Position::FileURI.
   llvm::UniqueStringSaver Strings(Arena);
   llvm::yaml::Input Yin(Data, &Strings);
+  IncludeGraph Sources;
+  llvm::Optional<tooling::CompileCommand> Cmd;
   while (Yin.setCurrentDocument()) {
     llvm::yaml::EmptyContext Ctx;
     VariantEntry Variant;
     yamlize(Yin, Variant, true, Ctx);
     if (Yin.error())
       return llvm::errorCodeToError(Yin.error());
 
     if (Variant.Symbol)
       Symbols.insert(*Variant.Symbol);
     if (Variant.Refs)
       for (const auto &Ref : Variant.Refs->second)
         Refs.insert(Variant.Refs->first, Ref);
     if (Variant.Relation)
       Relations.insert(*Variant.Relation);
+    if (Variant.Source) {
+      auto &IGN = Variant.Source.getValue();
+      auto Entry = Sources.try_emplace(IGN.URI).first;
+      Entry->getValue() = std::move(IGN);
+      // Fixup refs to refer to map keys which will live on
+      Entry->getValue().URI = Entry->getKey();
+      for (auto &Include : Entry->getValue().DirectIncludes)
+        Include = Sources.try_emplace(Include).first->getKey();
+    }
+    if (Variant.Cmd)
+      Cmd = *Variant.Cmd;
     Yin.nextDocument();
   }
 
   IndexFileIn Result;
   Result.Symbols.emplace(std::move(Symbols).build());
   Result.Refs.emplace(std::move(Refs).build());
   Result.Relations.emplace(std::move(Relations).build());
+  if (Sources.size())
+    Result.Sources = std::move(Sources);
+  Result.Cmd = std::move(Cmd);
   return std::move(Result);
 }
 
 std::string toYAML(const Symbol &S) {
   std::string Buf;
   {
     llvm::raw_string_ostream OS(Buf);
     llvm::yaml::Output Yout(OS);

clang-tools-extra/clangd/unittests/CMakeLists.txt

add_unittest(ClangdUnitTests ClangdTests
   TestTU.cpp
   ThreadingTests.cpp
   TraceTests.cpp
   TypeHierarchyTests.cpp
   TweakTests.cpp
   TweakTesting.cpp
   URITests.cpp
   XRefsTests.cpp
+  YAMLTests.cpp
 
   $<TARGET_OBJECTS:obj.clangDaemonTweaks>
   )
 
 clang_target_link_libraries(ClangdTests
   PRIVATE
   clangAST
   clangASTMatchers

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

+//===-- YAMLTests.cpp - YAML container unit tests -------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "index/Serialization.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+std::string YAMLData =
+    R"|(--- !Symbol
+ID:              13C6B898A37B5E70
+Name:            changedFilesTask
+Scope:           'clang::clangd::BackgroundIndex::'
+SymInfo:
+  Kind:            InstanceMethod
+  Lang:            Cpp
+CanonicalDeclaration:
+  FileURI:         'file:///llvm-project/clang-tools-extra/clangd/index/Background.h'
+  Start:
+    Line:            197
+    Column:          2
+  End:
+    Line:            197
+    Column:          18
+Definition:
+  FileURI:         'file:///llvm-project/clang-tools-extra/clangd/index/Background.cpp'
+  Start:
+    Line:            166
+    Column:          39
+  End:
+    Line:            166
+    Column:          55
+Origin:          4
+Flags:           8
+Signature:       ''
+TemplateSpecializationArgs: ''
+CompletionSnippetSuffix: ''
+Documentation:   ''
+ReturnType:      ''
+Type:            ''
+...
+--- !Refs
+ID:              FA89F28EEE2EDA4F
+References:
+  - Kind:            4
+    Location:
+      FileURI:         'file:///llvm-project/clang-tools-extra/clangd/index/Background.cpp'
+      Start:
+        Line:            408
+        Column:          40
+      End:
+        Line:            408
+        Column:          42
+  - Kind:            4
+    Location:
+      FileURI:         'file:///llvm-project/clang-tools-extra/clangd/index/Background.cpp'
+      Start:
+        Line:            409
+        Column:          13
+      End:
+        Line:            409
+        Column:          15
+  - Kind:            4
+    Location:
+      FileURI:         'file:///llvm-project/clang-tools-extra/clangd/index/Background.cpp'
+      Start:
+        Line:            412
+        Column:          30
+      End:
+        Line:            412
+        Column:          32
+  - Kind:            4
+    Location:
+      FileURI:         'file:///llvm-project/clang-tools-extra/clangd/index/Background.cpp'
+      Start:
+        Line:            415
+        Column:          8
+      End:
+        Line:            415
+        Column:          10
+  - Kind:            4
+    Location:
+      FileURI:         'file:///llvm-project/clang-tools-extra/clangd/index/Background.cpp'
+      Start:
+        Line:            428
+        Column:          24
+      End:
+        Line:            428
+        Column:          26
+  - Kind:            4
+    Location:
+      FileURI:         'file:///llvm-project/clang-tools-extra/clangd/index/Background.cpp'
+      Start:
+        Line:            429
+        Column:          24
+      End:
+        Line:            429
+        Column:          26
+  - Kind:            4
+    Location:
+      FileURI:         'file:///llvm-project/clang-tools-extra/clangd/index/Background.cpp'
+      Start:
+        Line:            460
+        Column:          18
+      End:
+        Line:            460
+        Column:          20
+  - Kind:            4
+    Location:
+      FileURI:         'file:///llvm-project/clang-tools-extra/clangd/index/Background.cpp'
+      Start:
+        Line:            461
+        Column:          64
+      End:
+        Line:            461
+        Column:          66
+  - Kind:            4
+    Location:
+      FileURI:         'file:///llvm-project/clang-tools-extra/clangd/index/Background.cpp'
+      Start:
+        Line:            463
+        Column:          24
+      End:
+        Line:            463
+        Column:          26
+  - Kind:            4
+    Location:
+      FileURI:         'file:///llvm-project/clang-tools-extra/clangd/index/Background.cpp'
+      Start:
+        Line:            464
+        Column:          67
+      End:
+        Line:            464
+        Column:          69
+  - Kind:            4
+    Location:
+      FileURI:         'file:///llvm-project/clang-tools-extra/clangd/index/Background.cpp'
+      Start:
+        Line:            467
+        Column:          18
+      End:
+        Line:            467
+        Column:          20
+  - Kind:            4
+    Location:
+      FileURI:         'file:///llvm-project/clang-tools-extra/clangd/index/Background.cpp'
+      Start:
+        Line:            468
+        Column:          66
+      End:
+        Line:            468
+        Column:          68
+...
+--- !Source
+URI:             'file:///llvm-project/clang-tools-extra/clangd/index/Background.cpp'
+Flags:           1
+Digest:          EED8F5EAF25C453C
+DirectIncludes:
+  - 'file:///llvm-project/clang-tools-extra/clangd/index/Background.h'
+...
+--- !Source
+URI:             'file:///llvm-project/clang-tools-extra/clangd/index/Background.h'
+Flags:           0
+Digest:          '0000000000000000'
+DirectIncludes:  []
+...
+--- !Cmd
+Directory:       '/llvm-project/clang-tools-extra/clangd/index/Background.cpp'
+CommandLine:
+  - '/usr/bin/c++'
+  - '--driver-mode=g++'
+  - '-Wall'
+  - '-Wextra'
+  - '-std=c++14'
+  - '-o CMakeFiles/obj.clangDaemon.dir/index/Background.cpp'
+  - '-c'
+  - '/home/mnauw/src/cppobject/tools/cppgir.cpp'
+...
+)|";
+
+llvm::Expected<std::string> readWriteYAML(const std::string &Input) {
+  auto IndexIn = clang::clangd::readIndexFile(Input);
+  if (!IndexIn)
+    return IndexIn.takeError();
+
+  clang::clangd::IndexFileOut IndexOut(IndexIn.get());
+  IndexOut.Format = clang::clangd::IndexFileFormat::YAML;
+
+  std::string Output;
+  llvm::raw_string_ostream OS(Output);
+  OS << IndexOut;
+  OS.flush();
+  return Output;
+}
+
+TEST(YAMLTest, File) {
+  auto IndexIn = clang::clangd::readIndexFile(YAMLData);
+  ASSERT_TRUE(bool(IndexIn)) << IndexIn.takeError();
+  ASSERT_TRUE(IndexIn->Symbols);
+  EXPECT_EQ(size_t(1), IndexIn->Symbols->size());
+  ASSERT_TRUE(IndexIn->Refs);
+  EXPECT_EQ(size_t(1), IndexIn->Refs->size());
+  ASSERT_TRUE(IndexIn->Cmd);
+  EXPECT_EQ(size_t(8), IndexIn->Cmd->CommandLine.size());
+  ASSERT_TRUE(IndexIn->Sources);
+  EXPECT_EQ(size_t(2), IndexIn->Sources->size());
+
+  // Moreover, as the input is 'normalized', it should match output again.
+  // Such a check definitely covers a lot, but might be overly sensitive?
+  auto Output1 = readWriteYAML(YAMLData);
+  ASSERT_TRUE(bool(Output1)) << Output1.takeError();
+  EXPECT_EQ(Output1.get(), YAMLData);
+
+  // Output should not have dropped bits
+  // That was checked above already but let's do it this way as well.
+  auto IndexOut = clang::clangd::readIndexFile(Output1.get());
+  ASSERT_TRUE(bool(IndexOut)) << IndexOut.takeError();
+  ASSERT_TRUE(IndexOut->Symbols);
+  EXPECT_EQ(IndexIn->Symbols->size(), IndexOut->Symbols->size());
+  ASSERT_TRUE(IndexOut->Refs);
+  EXPECT_EQ(IndexIn->Refs->size(), IndexOut->Refs->size());
+  ASSERT_TRUE(IndexOut->Cmd);
+  EXPECT_EQ(IndexIn->Cmd->CommandLine.size(),
+            IndexOut->Cmd->CommandLine.size());
+  ASSERT_TRUE(IndexOut->Sources);
+  EXPECT_EQ(IndexIn->Sources->size(), IndexOut->Sources->size());
+
+  // Another round-trip should also match
+  auto Output2 = readWriteYAML(Output1.get());
+  ASSERT_TRUE(bool(Output2)) << Output2.takeError();
+  EXPECT_EQ(Output1.get(), Output2.get());
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang