[clangd] Improve const-correctness of Symbol->Detail. NFC

[sammccall]

Summary

This would have caught a bug I wrote in an early version of D42049, where
an index user could overwrite data internal to the index because the Symbol is
not deep-const.

The YAML traits are now a bit more verbose, but separate concerns a bit more
nicely: ArenaPtr can be reused for other similarly-allocated objects, including
scalars etc.

[hokein]

LTGM.

clangd/index/SymbolYAML.cpp

84 ↗

(On Diff #129830)

woo, I learnt new thing here. I'd expect a comment, using placement new with the custom allocator :)

88 ↗

(On Diff #129830)

The template type should be T, I think.

clang-tools-extra/trunk/clangd/index/Index.h

struct Details {
     // Documentation including comment for the symbol declaration.
     llvm::StringRef Documentation;
     // This is what goes into the LSP detail field in a completion item. For
     // example, the result type of a function.
     llvm::StringRef CompletionDetail;
   };
 
   // Optional details of the symbol.
-  Details *Detail = nullptr; // FIXME: should be const
+  const Details *Detail = nullptr;
 
   // FIXME: add definition location of the symbol.
   // FIXME: add all occurrences support.
   // FIXME: add extra fields for index scoring signals.
 };
 
 // An immutable symbol container that stores a set of symbols.
 // The container will maintain the lifetime of the symbols.

clang-tools-extra/trunk/clangd/index/Index.cpp

static void own(Symbol &S, DenseSet<StringRef> &Strings,
   Intern(S.CompletionLabel);
   Intern(S.CompletionFilterText);
   Intern(S.CompletionPlainInsertText);
   Intern(S.CompletionSnippetInsertText);
 
   if (S.Detail) {
     // Copy values of StringRefs into arena.
     auto *Detail = Arena.Allocate<Symbol::Details>();
-    Detail->Documentation = S.Detail->Documentation;
-    Detail->CompletionDetail = S.Detail->CompletionDetail;
-    S.Detail = Detail;
-
+    *Detail = *S.Detail;
     // Intern the actual strings.
-    Intern(S.Detail->Documentation);
-    Intern(S.Detail->CompletionDetail);
+    Intern(Detail->Documentation);
+    Intern(Detail->CompletionDetail);
+    // Replace the detail pointer with our copy.
+    S.Detail = Detail;
   }
 }
 
 void SymbolSlab::Builder::insert(const Symbol &S) {
   auto R = SymbolIndex.try_emplace(S.ID, Symbols.size());
   if (R.second) {
     Symbols.push_back(S);
     own(Symbols.back(), Strings, Arena);

clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp

 template <> struct MappingTraits<SymbolInfo> {
   static void mapping(IO &io, SymbolInfo &SymInfo) {
     // FIXME: expose other fields?
     io.mapRequired("Kind", SymInfo.Kind);
     io.mapRequired("Lang", SymInfo.Lang);
   }
 };
 
-template <>
-struct MappingTraits<Symbol::Details *> {
-  static void mapping(IO &io, Symbol::Details *&Detail) {
-    if (!io.outputting()) {
-      assert(io.getContext() && "Expecting an arena (as context) to allocate "
-                                "data for new symbols.");
-      Detail = static_cast<llvm::BumpPtrAllocator *>(io.getContext())
-                   ->Allocate<Symbol::Details>();
-    } else if (!Detail) {
-      // Detail is optional in outputting.
-      return;
-    }
-    assert(Detail);
-    io.mapOptional("Documentation", Detail->Documentation);
-    io.mapOptional("CompletionDetail", Detail->CompletionDetail);
+template <> struct MappingTraits<Symbol::Details> {
+  static void mapping(IO &io, Symbol::Details &Detail) {
+    io.mapOptional("Documentation", Detail.Documentation);
+    io.mapOptional("CompletionDetail", Detail.CompletionDetail);
   }
 };
 
+// A YamlIO normalizer for fields of type "const T*" allocated on an arena.
+// Normalizes to Optional<T>, so traits should be provided for T.
+template <typename T> struct ArenaPtr {
+  ArenaPtr(IO &) {}
+  ArenaPtr(IO &, const T *D) {
+    if (D)
+      Opt = *D;
+  }
+
+  const T *denormalize(IO &IO) {
+    assert(IO.getContext() && "Expecting an arena (as context) to allocate "
+                              "data for read symbols.");
+    if (!Opt)
+      return nullptr;
+    return new (*static_cast<llvm::BumpPtrAllocator *>(IO.getContext()))
+        T(std::move(*Opt)); // Allocate a copy of Opt on the arena.
+  }
+
+  llvm::Optional<T> Opt;
+};
+
 template <> struct MappingTraits<Symbol> {
   static void mapping(IO &IO, Symbol &Sym) {
     MappingNormalization<NormalizedSymbolID, SymbolID> NSymbolID(IO, Sym.ID);
+    MappingNormalization<ArenaPtr<Symbol::Details>, const Symbol::Details *>
+        NDetail(IO, Sym.Detail);
     IO.mapRequired("ID", NSymbolID->HexString);
     IO.mapRequired("Name", Sym.Name);
     IO.mapRequired("Scope", Sym.Scope);
     IO.mapRequired("SymInfo", Sym.SymInfo);
     IO.mapRequired("CanonicalDeclaration", Sym.CanonicalDeclaration);
     IO.mapRequired("CompletionLabel", Sym.CompletionLabel);
     IO.mapRequired("CompletionFilterText", Sym.CompletionFilterText);
     IO.mapRequired("CompletionPlainInsertText", Sym.CompletionPlainInsertText);
 
     IO.mapOptional("CompletionSnippetInsertText",
                    Sym.CompletionSnippetInsertText);
-    if (!IO.outputting() || Sym.Detail)
-      IO.mapOptional("Detail", Sym.Detail);
+    IO.mapOptional("Detail", NDetail->Opt);
   }
 };
 
 template <> struct ScalarEnumerationTraits<SymbolLanguage> {
   static void enumeration(IO &IO, SymbolLanguage &Value) {
     IO.enumCase(Value, "C", SymbolLanguage::C);
     IO.enumCase(Value, "Cpp", SymbolLanguage::CXX);
     IO.enumCase(Value, "ObjC", SymbolLanguage::ObjC);