[clangd] Migrate to proto2 syntax

[kbobyrev]

Summary

This allows us to check whether enum field is actually sent over the wire or missing.

[sammccall]

clang-tools-extra/clangd/index/remote/Index.proto

15

can we just switch to proto2 instead?

apart from reserving the zero value, proto3 also requires you to explicitly handle zero in your code (vs being able to set a default in the proto definition)

Offsetting enums by one isn't a big deal, but it can be in other cases (consider a number like num_refs where 0 is a meaningful value and offsetting by 1 is really confusing)

clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp

118–123

this -1 and +1 everywhere makes me a bit leery.
Seems a bit easy to reverse or forget it.

What about

template <typename T> std::underlying_type_t<T> toProtobuf(T Value) {
  return static_cast<std::underlying_type_t<T>>(Value) + 1;
}

and the inverse?

291

?!

this is a bitfield

[kbobyrev]

Migrate to proto2 syntax.

[kbobyrev]

Remove outdated comment.

[kbobyrev]

Remove a newline.

[sammccall]

clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp

118–123

I don't think this comment is necessary. The code reads clearly and is more obvious/natural than the alternative you're arguing against here.

137

nit: RelationsRequest

[kbobyrev]

Remove unnecessary comment and fix a typo.

clang-tools-extra/clangd/index/remote/Index.proto

 //===--- Index.proto - Remote index Protocol Buffers definition -----------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
-syntax = "proto3";
+syntax = "proto2";
 
 package clang.clangd.remote.v1;
 
 // Semantics of SymbolIndex match clangd::SymbolIndex with all required
 // structures corresponding to their clangd::* counterparts.
+// NOTE: Enum values are offset by one to detect missing values.
 service SymbolIndex {
   rpc Lookup(LookupRequest) returns (stream LookupReply) {}
 
   rpc FuzzyFind(FuzzyFindRequest) returns (stream FuzzyFindReply) {}
 
   rpc Refs(RefsRequest) returns (stream RefsReply) {}
 
   rpc Relations(RelationsRequest) returns (stream RelationsReply) {}
 }
 
 message LookupRequest { repeated string ids = 1; }
 
 // The response is a stream of symbol messages and the terminating message
 // indicating the end of stream.
 message LookupReply {
   oneof kind {
     Symbol stream_result = 1;
     bool final_result = 2;
   }
 }
 
 message FuzzyFindRequest {
-  string query = 1;
+  optional string query = 1;
   repeated string scopes = 2;
-  bool any_scope = 3;
-  uint32 limit = 4;
-  bool restricted_for_code_completion = 5;
+  optional bool any_scope = 3;
+  optional uint32 limit = 4;
+  optional bool restricted_for_code_completion = 5;
   repeated string proximity_paths = 6;
   repeated string preferred_types = 7;
 }
 
 // The response is a stream of symbol messages, and one terminating has_more
 // message.
 message FuzzyFindReply {
   oneof kind {
     Symbol stream_result = 1;
     bool final_result = 2; // HasMore
   }
 }
 
 message RefsRequest {
   repeated string ids = 1;
-  uint32 filter = 2;
-  uint32 limit = 3;
+  optional uint32 filter = 2;
+  optional uint32 limit = 3;
 }
 
 // The response is a stream of reference messages, and one terminating has_more
 // message.
 message RefsReply {
   oneof kind {
     Ref stream_result = 1;
     bool final_result = 2; // HasMore
   }
 }
 
 message Symbol {
-  string id = 1;
-  SymbolInfo info = 2;
-  string name = 3;
-  SymbolLocation definition = 4;
-  string scope = 5;
-  SymbolLocation canonical_declaration = 6;
-  int32 references = 7;
-  uint32 origin = 8;
-  string signature = 9;
-  string template_specialization_args = 10;
-  string completion_snippet_suffix = 11;
-  string documentation = 12;
-  string return_type = 13;
-  string type = 14;
+  optional string id = 1;
+  optional SymbolInfo info = 2;
+  optional string name = 3;
+  optional SymbolLocation definition = 4;
+  optional string scope = 5;
+  optional SymbolLocation canonical_declaration = 6;
+  optional int32 references = 7;
+  optional uint32 origin = 8;
+  optional string signature = 9;
+  optional string template_specialization_args = 10;
+  optional string completion_snippet_suffix = 11;
+  optional string documentation = 12;
+  optional string return_type = 13;
+  optional string type = 14;
   repeated HeaderWithReferences headers = 15;
-  uint32 flags = 16;
+  optional uint32 flags = 16;
 }
 
 message Ref {
-  SymbolLocation location = 1;
-  uint32 kind = 2;
+  optional SymbolLocation location = 1;
+  optional uint32 kind = 2;
 }
 
 message SymbolInfo {
-  uint32 kind = 1;
-  uint32 subkind = 2;
-  uint32 language = 3;
-  uint32 properties = 4;
+  optional uint32 kind = 1;
+  optional uint32 subkind = 2;
+  optional uint32 language = 3;
+  optional uint32 properties = 4;
 }
 
 message SymbolLocation {
-  Position start = 1;
-  Position end = 2;
+  optional Position start = 1;
+  optional Position end = 2;
   // clangd::SymbolLocation stores FileURI, but the protocol transmits a the
   // relative path. Because paths are different on the remote and local machines
   // they will be translated in the marshalling layer.
-  string file_path = 3;
+  optional string file_path = 3;
 }
 
 message Position {
-  uint32 line = 1;
-  uint32 column = 2;
+  optional uint32 line = 1;
+  optional uint32 column = 2;
 }
 
 message HeaderWithReferences {
-  string header = 1;
-  uint32 references = 2;
+  optional string header = 1;
+  optional uint32 references = 2;
 }
 
 message RelationsRequest {
   repeated string subjects = 1;
-  uint32 predicate = 2;
-  uint32 limit = 3;
+  optional uint32 predicate = 2;
+  optional uint32 limit = 3;
 }
 
 // The response is a stream of reference messages, and one terminating has_more
 // message.
 message RelationsReply {
   oneof kind {
     Relation stream_result = 1;
     bool final_result = 2; // HasMore
   }
 }
 
 // This struct does not mirror clangd::Relation but rather the arguments of
 // SymbolIndex::relations callback.
 message Relation {
-  string subject_id = 1;
-  Symbol object = 2;
+  optional string subject_id = 1;
+  optional Symbol object = 2;
 }

clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp

 //===--- Marshalling.cpp -----------------------------------------*- C++-*-===//
 //
 // 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 "Marshalling.h"
 #include "Headers.h"
 #include "Index.pb.h"
 #include "Protocol.h"
 #include "index/Index.h"
+#include "index/Ref.h"
 #include "index/Serialization.h"
 #include "index/Symbol.h"
 #include "index/SymbolID.h"
 #include "index/SymbolLocation.h"
 #include "index/SymbolOrigin.h"
 #include "support/Logger.h"
 #include "clang/Index/IndexSymbol.h"
 #include "llvm/ADT/DenseSet.h"

 
 llvm::Expected<clangd::RefsRequest>
 Marshaller::fromProtobuf(const v1::RefsRequest *Message) {
   clangd::RefsRequest Req;
   auto IDs = getIDs(Message->ids());
   if (!IDs)
     return IDs.takeError();
   Req.IDs = std::move(*IDs);
-  Req.Filter = static_cast<RefKind>(Message->filter());
+  // This sets a default for RefKind. Doing it explicitly in proto definition
+  // is error-prone because clangd::RefKind::All can change unexpectedly.
+  if (Message->has_filter())
+    Req.Filter = static_cast<clangd::RefKind>(Message->filter());
+  else
+    Req.Filter = clangd::RefKind::All;
   if (Message->limit())
     Req.Limit = Message->limit();
   return Req;
 }
 
 llvm::Expected<clangd::RelationsRequest>
 Marshaller::fromProtobuf(const v1::RelationsRequest *Message) {
   clangd::RelationsRequest Req;
   auto IDs = getIDs(Message->subjects());
   if (!IDs)
     return IDs.takeError();
   Req.Subjects = std::move(*IDs);
+  if (!Message->has_predicate())
+    return error("RelationnsRequest requires RelationKind predicate.");
   Req.Predicate = static_cast<RelationKind>(Message->predicate());
+
   if (Message->limit())
     Req.Limit = Message->limit();
   return Req;
 }
 
 llvm::Expected<clangd::Symbol>
 Marshaller::fromProtobuf(const v1::Symbol &Message) {
   if (!Message.has_info() || !Message.has_canonical_declaration())

 llvm::Expected<clangd::Ref> Marshaller::fromProtobuf(const v1::Ref &Message) {
   if (!Message.has_location())
     return error("Missing location.");
   clangd::Ref Result;
   auto Location = fromProtobuf(Message.location());
   if (!Location)
     return Location.takeError();
   Result.Location = *Location;
-  Result.Kind = static_cast<clangd::RefKind>(Message.kind());
+  Result.Kind = static_cast<RefKind>(Message.kind());
   return Result;
 }
 
 llvm::Expected<std::pair<clangd::SymbolID, clangd::Symbol>>
 Marshaller::fromProtobuf(const v1::Relation &Message) {
   auto SubjectID = SymbolID::fromStr(Message.subject_id());
   if (!SubjectID)
     return SubjectID.takeError();
llvm::Expected<v1::Symbol> Marshaller::toProtobuf(const clangd::Symbol &From) {
   Result.set_type(From.Type.str());
   for (const auto &Header : From.IncludeHeaders) {
     auto Serialized = toProtobuf(Header);
     if (!Serialized)
       return Serialized.takeError();
     auto *NextHeader = Result.add_headers();
     *NextHeader = *Serialized;
   }
   Result.set_flags(static_cast<uint32_t>(From.Flags));
   return Result;
 }
 
 llvm::Expected<v1::Ref> Marshaller::toProtobuf(const clangd::Ref &From) {
   v1::Ref Result;
   Result.set_kind(static_cast<uint32_t>(From.Kind));
   auto Location = toProtobuf(From.Location);
   if (!Location)