[clangd] Explicitly initialize all primitive fields in Protocol.h

[ilya-biryukov]

Summary

Some of the existing structs had primimtive fields that were
not explicitly initialized on construction.
After this commit every struct consistently sets a defined value for
every field when default-initialized.

[ilya-biryukov]

This commit makes potential UB less likely. Logical errors (i.e. forgot to initialize some fields) will still be there.
Sending this for review to make sure everyone agrees this is a good thing.

[ilya-biryukov]

• Removed initializers for Optional<> fields, they are not problematic

[jkorous-apple]

clangd/Protocol.h

190

Sorry if I am asking stupid question but since the type is Optional<> isn't it's default-constructed value more appropriate here?

211

Does it still make more sense to use Optional<TraceLevel> than plain TraceLevel?

[ioeric]

I think it's useful to explicit initialize non-trivial types like FileChangeType. But I think it's safe and probably easier to rely on default values of primitive types like int, bool etc. Explicitly setting default values is probably fine (so this change LGTM), but do we really want to make this a requirement for future changes or even in our coding style?

[ilya-biryukov]

clangd/Protocol.h

190

Totally, my mistake. Fixed now.

211

Optional here makes sense (LSP defines the field as nullable), the intializer didn't. Fixed now.

[ilya-biryukov]

In D43230#1006104, @ioeric wrote:

But I think it's safe and probably easier to rely on default values of primitive types like int, bool etc

It's not always safe, as primitive types are sometimes left uninitialized (e.g. when constructed on the stack) and reading an uninitialized value is UB.

but do we really want to make this a requirement for future changes or even in our coding style?

I think we should, default values are less surprising than UB. Other people may disagree, though.
@sammccall , @hokein , WDYT? Should we always initialize primitive types in our code?

[ioeric]

In D43230#1006469, @ilya-biryukov wrote:

In D43230#1006104, @ioeric wrote:

But I think it's safe and probably easier to rely on default values of primitive types like int, bool etc

It's not always safe, as primitive types are sometimes left uninitialized (e.g. when constructed on the stack) and reading an uninitialized value is UB.

Oh, you are absolutely right! I I think l had protos get into my mind...

but do we really want to make this a requirement for future changes or even in our coding style?

I think we should, default values are less surprising than UB. Other people may disagree, though.
@sammccall , @hokein , WDYT? Should we always initialize primitive types in our code?

I think it would probably depend on whether we could find a sensible default value for a field (e.g. is 0 a better default value than UINT_MAX for line number?) and whether we expect users to always initialize a field (e.g. would it be better to pollute the field instead of providing a default value so that uninitialized values would be caught more easily).

[sammccall]

Yup, I got bitten recently from some of our plain-c-style structs with no default initializers (in Index).

Definitely a fan of this change. Main downside is you can't use aggregate initialization, but the field-by-field initialization is often more readable anyway.

And +1 to avoiding explicit = None for optionals, etc.

[sammccall]

clangd/Protocol.h

80

I'd lean to making callers initialize field-by-field here rather than provide constructors.
It's not terrible here (you can get it wrong, but only one way), but it's a pattern that doesn't scale so well I think.

[ilya-biryukov]

• Removed initializers for Optional<> fields, they are not problematic
• Remove ctor from Position, initialize on per-field basis instead.

[ilya-biryukov]

In D43230#1006498, @ioeric wrote:

I think it would probably depend on whether we could find a sensible default value for a field (e.g. is 0 a better default value than UINT_MAX for line number?) and whether we expect users to always initialize a field (e.g. would it be better to pollute the field instead of providing a default value so that uninitialized values would be caught more easily).

Yeah, it arguable whether 0 is the right default, but at least it's consistent with what compiler uses for zero-initialization, i.e. when you default-initialize builtin types explicitly (e.g. int b = int(); or int b = *(new int)).
For things like FileChangeType we choose 1, because it's the lowest acceptable value, but I'm not sure that's the right default value, merely a random valid enum value.

clangd/Protocol.h

80

As discussed offline, removed the ctor from Position and initialized everything on per-field basis.

It strikes me that this should be decided on per-struct basis. For things like position per-field init seems preferable to avoid accidentally confusing lines and chars.
On the other hand, for Range it seems totally fine to have a constructor, it's almost impossible to write Range{end, begin}, the ordering of ctor params is very natural.

clangd/Protocol.h

 
 struct TextDocumentIdentifier {
   /// The text document's URI.
   URIForFile uri;
 };
 bool fromJSON(const json::Expr &, TextDocumentIdentifier &);
 
 struct Position {
+  Position() = default;
+  Position(int line, int character) : line(line), character(character) {}
+
   /// Line position in a document (zero-based).
-  int line;
+  int line = 0;
 
   /// Character offset on a line in a document (zero-based).
-  int character;
+  int character = 0;
 
   friend bool operator==(const Position &LHS, const Position &RHS) {
     return std::tie(LHS.line, LHS.character) ==
            std::tie(RHS.line, RHS.character);
   }
   friend bool operator<(const Position &LHS, const Position &RHS) {
     return std::tie(LHS.line, LHS.character) <
            std::tie(RHS.line, RHS.character);

 struct TextDocumentItem {
   /// The text document's URI.
   URIForFile uri;
 
   /// The text document's language identifier.
   std::string languageId;
 
   /// The version number of this document (it will strictly increase after each
-  int version;
+  int version = 0;
 
   /// The content of the opened text document.
   std::string text;
 };
 bool fromJSON(const json::Expr &, TextDocumentItem &);
 
 enum class TraceLevel {
   Off = 0,
   Messages = 1,
   Verbose = 2,
 };
 bool fromJSON(const json::Expr &E, TraceLevel &Out);
 
 struct NoParams {};
 inline bool fromJSON(const json::Expr &, NoParams &) { return true; }
 using ShutdownParams = NoParams;
 using ExitParams = NoParams;
 
 struct InitializeParams {
   /// The process Id of the parent process that started
   /// the server. Is null if the process has not been started by another
   /// process. If the parent process is not alive then the server should exit
   /// (see exit notification) its process.
-  llvm::Optional<int> processId;
+  llvm::Optional<int> processId = 0;
 
   /// The rootPath of the workspace. Is null
   /// if no folder is open.
   ///
   /// @deprecated in favour of rootUri.
   llvm::Optional<std::string> rootPath;
 
   /// The rootUri of the workspace. Is null if no
   /// folder is open. If both `rootPath` and `rootUri` are set
   /// `rootUri` wins.
   llvm::Optional<URIForFile> rootUri;
 
   // User provided initialization options.
   // initializationOptions?: any;
 
   /// The capabilities provided by the client (editor or tool)
   /// Note: Not currently used by clangd
   // ClientCapabilities capabilities;
 
   /// The initial trace setting. If omitted trace is disabled ('off').
-  llvm::Optional<TraceLevel> trace;
+  llvm::Optional<TraceLevel> trace = TraceLevel::Off;
 };
 bool fromJSON(const json::Expr &, InitializeParams &);
 
 struct DidOpenTextDocumentParams {
   /// The document that was opened.
   TextDocumentItem textDocument;
 
   /// Extension storing per-file metadata, such as compilation flags.
enum class FileChangeType {
   Deleted = 3
 };
 bool fromJSON(const json::Expr &E, FileChangeType &Out);
 
 struct FileEvent {
   /// The file's URI.
   URIForFile uri;
   /// The change type.
-  FileChangeType type;
+  FileChangeType type = FileChangeType::Created;
 };
 bool fromJSON(const json::Expr &, FileEvent &);
 
 struct DidChangeWatchedFilesParams {
   /// The actual file events.
   std::vector<FileEvent> changes;
 };
 bool fromJSON(const json::Expr &, DidChangeWatchedFilesParams &);
 
 struct FormattingOptions {
   /// Size of a tab in spaces.
-  int tabSize;
+  int tabSize = 0;
 
   /// Prefer spaces over tabs.
-  bool insertSpaces;
+  bool insertSpaces = false;
 };
 bool fromJSON(const json::Expr &, FormattingOptions &);
 json::Expr toJSON(const FormattingOptions &);
 
 struct DocumentRangeFormattingParams {
   /// The document to format.
   TextDocumentIdentifier textDocument;
 

 bool fromJSON(const json::Expr &, DocumentFormattingParams &);
 
 struct Diagnostic {
   /// The range at which the message applies.
   Range range;
 
   /// The diagnostic's severity. Can be omitted. If omitted it is up to the
   /// client to interpret diagnostics as error, warning, info or hint.
-  int severity;
+  int severity = 0;
 
   /// The diagnostic's code. Can be omitted.
   /// Note: Not currently used by clangd
   // std::string code;
 
   /// A human-readable string describing the source of this
   /// diagnostic, e.g. 'typescript' or 'super lint'.
   /// Note: Not currently used by clangd

 struct ExecuteCommandParams {
   // Command to apply fix-its. Uses WorkspaceEdit as argument.
   const static llvm::StringLiteral CLANGD_APPLY_FIX_COMMAND;
 
   /// The command identifier, e.g. CLANGD_APPLY_FIX_COMMAND
   std::string command;
 
   // Arguments
-
   llvm::Optional<WorkspaceEdit> workspaceEdit;
 };
 bool fromJSON(const json::Expr &, ExecuteCommandParams &);
 
 struct ApplyWorkspaceEditParams {
   WorkspaceEdit edit;
 };
 json::Expr toJSON(const ApplyWorkspaceEditParams &);
enum class InsertTextFormat {
   Snippet = 2,
 };
 
 /// Provides details for how a completion item was scored.
 /// This can be used for client-side filtering of completion items as the
 /// user keeps typing.
 /// This is a clangd extension.
 struct CompletionItemScores {
-  float finalScore;  /// The score that items are ranked by.
-                     /// This is filterScore * symbolScore.
-  float filterScore; /// How the partial identifier matched filterText. [0-1]
-  float symbolScore; /// How the symbol fits, ignoring the partial identifier.
+  /// The score that items are ranked by.
+  float finalScore = 0.f;
+  /// How the partial identifier matched filterText. [0-1]
+  float filterScore = 0.f;
+  /// How the symbol fits, ignoring the partial identifier.
+  float symbolScore = 0.f;
 };
 
 struct CompletionItem {
   /// The label of this completion item. By default also the text that is
   /// inserted when selecting this completion.
   std::string label;
 
   /// The kind of this completion item. Based of the kind an icon is chosen by

 
 enum class DocumentHighlightKind { Text = 1, Read = 2, Write = 3 };
 
 /// A document highlight is a range inside a text document which deserves
 /// special attention. Usually a document highlight is visualized by changing
 /// the background color of its range.
 
 struct DocumentHighlight {
-
   /// The range this highlight applies to.
-
   Range range;
 
   /// The highlight kind, default is DocumentHighlightKind.Text.
-
   DocumentHighlightKind kind = DocumentHighlightKind::Text;
 
   friend bool operator<(const DocumentHighlight &LHS,
                         const DocumentHighlight &RHS) {
     int LHSKind = static_cast<int>(LHS.kind);
     int RHSKind = static_cast<int>(RHS.kind);
     return std::tie(LHS.range, LHSKind) < std::tie(RHS.range, RHSKind);
   }