fix: Use $ in version instead of Descriptor (#357)

When the descriptor contains an escaped identifier,
it needs to contain a backtick at the start. This means
that indiscriminately prefixing $ to a descriptor can create
an ill-formed symbol name. So instead, include the $ in
the version.

Author: varungandhi-src

indexer/SymbolFormatter.cc

@@ -100,22 +100,19 @@ void DescriptorBuilder::formatTo(llvm::raw_ostream &out) const {
 void SymbolBuilder::formatTo(std::string &buf) const {
   llvm::raw_string_ostream out(buf);
   out << "cxx . "; // scheme manager
-  for (auto text : {this->packageId.name, this->packageId.version}) {
-    if (text.empty()) {
-      out << ". ";
-      continue;
-    }
-    ::addSpaceEscaped(out, text);
+  if (this->packageId.name.empty()) {
+    out << ". ";
+  } else {
+    ::addSpaceEscaped(out, this->packageId.name);
     out << ' ';
   }
-  // NOTE(def: symbol-string-hack-for-forward-decls): Add a '$' prefix
-  // after the space after the version, but before any descriptors.
-  // When splitting the symbol, we check for the '$' preceded by a ' '.
-  // Strictly speaking, it is possible that the same pattern is present
-  // due to a funky filename which contains the pattern ' $', it's highly
-  // improbable to cause any incorrect lookup by not colliding with an
-  // actual symbol name.
-  out << '$';
+  if (!this->packageId.version.empty()) {
+    ::addSpaceEscaped(out, this->packageId.version);
+  }
+  out << "$ ";
+  // NOTE(def: symbol-string-hack-for-forward-decls): Add a '$' suffix
+  // after the version, but before the space for the symbol name.
+  // When splitting the symbol, we check for the '$' followed by a ' '.
   for (auto &descriptor : this->descriptors) {
     descriptor.formatTo(out);
   }

indexer/SymbolName.cc

@@ -25,16 +25,17 @@ std::optional<scip::SymbolSuffix>
 SymbolBuilder::getPackageAgnosticSuffix(scip::SymbolNameRef name) {
   // See NOTE(ref: symbol-string-hack-for-forward-decls)
   auto ix = name.value.find('$');
-  if (ix == std::string::npos || ix == 0 || name.value[ix - 1] != ' ') {
+  if (ix == std::string::npos || ix == name.value.size() - 1
+      || name.value[ix + 1] != ' ') {
     return std::nullopt;
   }
-  return scip::SymbolSuffix{name.value.substr(ix + 1)};
+  return scip::SymbolSuffix{name.value.substr(ix + 2)};
 }
 
 // static
 scip::SymbolName SymbolBuilder::addFakePrefix(scip::SymbolSuffix suffix) {
   std::string buf;
-#define FAKE_SYMBOL_PREFIX "cxx . . . $"
+#define FAKE_SYMBOL_PREFIX "cxx . . $ "
   buf.reserve(sizeof(FAKE_SYMBOL_PREFIX) + suffix.value.size());
   buf.append(FAKE_SYMBOL_PREFIX);
 #undef FAKE_SYMBOL_PREFIX

test/Snapshot.cc

@@ -93,7 +93,7 @@ static std::vector<RootRelativePath> listFilesRecursive(const RootPath &root) {
 static std::string formatSymbol(const std::string &symbol) {
   // Strip out repeating information for cleaner snapshots.
   return absl::StrReplaceAll(symbol, {
-                                         {"cxx . . . $", "[..] "},
+                                         {"cxx . . $ ", "[..] "},
                                          {"cxx . ", ""}, // indexer prefix
                                          {"todo-pkg todo-version", "[..]"},
                                      });