[lldb-dap] Follow the spec more closely on 'initialize' arguments.

[ashgti]

Updates InitializeRequestArguments to correctly follow the spec, see https://microsoft.github.io/debug-adapter-protocol/specification#Requests_Initialize.

This should correct which fields are tracked as optional and simplifies some of the types to make sure they're meaningful (e.g. an optional<bool> isn't anymore helpful than a bool since undefined and false are basically equivalent).

[llvmbot]

@llvm/pr-subscribers-lldb

Author: John Harrison (ashgti)

Changes

Updates InitializeRequestArguments to correctly follow the spec, see https://microsoft.github.io/debug-adapter-protocol/specification#Requests_Initialize.

This should correct which fields are tracked as optional and simplifies some of the types to make sure they're meaningful (e.g. an optional<bool> isn't anymore helpful than a bool since undefined and false are basically equivalent).

Fixes #170171

---

Full diff: https://github.com/llvm/llvm-project/pull/170350.diff

4 Files Affected:

• (modified) lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp (+1-1)
• (modified) lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp (+6-5)
• (modified) lldb/tools/lldb-dap/Protocol/ProtocolRequests.h (+6-6)
• (modified) lldb/unittests/DAP/ProtocolRequestsTest.cpp (+65-1)

diff --git a/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp
index 53e1810a5b0e0..2d30e089447f1 100644
--- a/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp
@@ -23,7 +23,7 @@ llvm::Expected<InitializeResponse> InitializeRequestHandler::Run(
     const InitializeRequestArguments &arguments) const {
   // Store initialization arguments for later use in Launch/Attach.
   dap.clientFeatures = arguments.supportedFeatures;
-  dap.sourceInitFile = arguments.lldbExtSourceInitFile.value_or(true);
+  dap.sourceInitFile = arguments.lldbExtSourceInitFile;
 
   return dap.GetCapabilities();
 }
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp
index d53a520ade39b..0a1d580bffd68 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp
@@ -216,12 +216,13 @@ bool fromJSON(const json::Value &Params, InitializeRequestArguments &IRA,
   }
 
   return OM.map("adapterID", IRA.adapterID) &&
-         OM.map("clientID", IRA.clientID) &&
-         OM.map("clientName", IRA.clientName) && OM.map("locale", IRA.locale) &&
-         OM.map("linesStartAt1", IRA.linesStartAt1) &&
-         OM.map("columnsStartAt1", IRA.columnsStartAt1) &&
+         OM.mapOptional("clientID", IRA.clientID) &&
+         OM.mapOptional("clientName", IRA.clientName) &&
+         OM.mapOptional("locale", IRA.locale) &&
+         OM.mapOptional("linesStartAt1", IRA.linesStartAt1) &&
+         OM.mapOptional("columnsStartAt1", IRA.columnsStartAt1) &&
          OM.mapOptional("pathFormat", IRA.pathFormat) &&
-         OM.map("$__lldb_sourceInitFile", IRA.lldbExtSourceInitFile);
+         OM.mapOptional("$__lldb_sourceInitFile", IRA.lldbExtSourceInitFile);
 }
 
 bool fromJSON(const json::Value &Params, Configuration &C, json::Path P) {
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
index 37fc2465f6a05..6a85033ae7ef2 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
@@ -108,23 +108,23 @@ struct InitializeRequestArguments {
   std::string adapterID;
 
   /// The ID of the client using this adapter.
-  std::optional<std::string> clientID;
+  std::string clientID;
 
   /// The human-readable name of the client using this adapter.
-  std::optional<std::string> clientName;
+  std::string clientName;
 
   /// The ISO-639 locale of the client using this adapter, e.g. en-US or de-CH.
-  std::optional<std::string> locale;
+  std::string locale;
 
   /// Determines in what format paths are specified. The default is `path`,
   /// which is the native format.
   PathFormat pathFormat = ePatFormatPath;
 
   /// If true all line numbers are 1-based (default).
-  std::optional<bool> linesStartAt1;
+  bool linesStartAt1 = true;
 
   /// If true all column numbers are 1-based (default).
-  std::optional<bool> columnsStartAt1;
+  bool columnsStartAt1 = true;
 
   /// The set of supported features reported by the client.
   llvm::DenseSet<ClientFeature> supportedFeatures;
@@ -133,7 +133,7 @@ struct InitializeRequestArguments {
   /// @{
 
   /// Source init files when initializing lldb::SBDebugger.
-  std::optional<bool> lldbExtSourceInitFile;
+  bool lldbExtSourceInitFile = true;
 
   /// @}
 };
diff --git a/lldb/unittests/DAP/ProtocolRequestsTest.cpp b/lldb/unittests/DAP/ProtocolRequestsTest.cpp
index ba9aef1e5fcc5..a74c369924b8e 100644
--- a/lldb/unittests/DAP/ProtocolRequestsTest.cpp
+++ b/lldb/unittests/DAP/ProtocolRequestsTest.cpp
@@ -77,7 +77,7 @@ TEST(ProtocolRequestsTest, EvaluateArguments) {
   EXPECT_EQ(expected->expression, "hello world");
   EXPECT_EQ(expected->context, eEvaluateContextRepl);
 
-  // Check required keys;
+  // Check required keys.
   EXPECT_THAT_EXPECTED(parse<EvaluateArguments>(R"({})"),
                        FailedWithMessage("missing value at (root).expression"));
 }
@@ -118,3 +118,67 @@ TEST(ProtocolRequestsTest, EvaluateResponseBody) {
   ASSERT_THAT_EXPECTED(expected_opt, llvm::Succeeded());
   EXPECT_EQ(PrettyPrint(*expected_opt), PrettyPrint(body));
 }
+
+TEST(ProtocolRequestsTest, InitializeRequestArguments) {
+  llvm::Expected<InitializeRequestArguments> expected =
+      parse<InitializeRequestArguments>(R"({"adapterID": "myid"})");
+  ASSERT_THAT_EXPECTED(expected, llvm::Succeeded());
+  EXPECT_EQ(expected->adapterID, "myid");
+
+  // Check optional keys.
+  expected = parse<InitializeRequestArguments>(R"({
+    "adapterID": "myid",
+    "clientID": "myclientid",
+    "clientName": "lldb-dap-unit-tests",
+    "locale": "en-US",
+    "linesStartAt1": true,
+    "columnsStartAt1": true,
+    "pathFormat": "uri",
+    "supportsVariableType": true,
+    "supportsVariablePaging": true,
+    "supportsRunInTerminalRequest": true,
+    "supportsMemoryReferences": true,
+    "supportsProgressReporting": true,
+    "supportsInvalidatedEvent": true,
+    "supportsMemoryEvent": true,
+    "supportsArgsCanBeInterpretedByShell": true,
+    "supportsStartDebuggingRequest": true,
+    "supportsANSIStyling": true
+  })");
+  ASSERT_THAT_EXPECTED(expected, llvm::Succeeded());
+  EXPECT_EQ(expected->adapterID, "myid");
+  EXPECT_EQ(expected->clientID, "myclientid");
+  EXPECT_EQ(expected->clientName, "lldb-dap-unit-tests");
+  EXPECT_EQ(expected->locale, "en-US");
+  EXPECT_EQ(expected->linesStartAt1, true);
+  EXPECT_EQ(expected->columnsStartAt1, true);
+  EXPECT_EQ(expected->pathFormat, ePathFormatURI);
+  EXPECT_EQ(expected->supportedFeatures.contains(eClientFeatureVariableType),
+            true);
+  EXPECT_EQ(
+      expected->supportedFeatures.contains(eClientFeatureRunInTerminalRequest),
+      true);
+  EXPECT_EQ(
+      expected->supportedFeatures.contains(eClientFeatureMemoryReferences),
+      true);
+  EXPECT_EQ(
+      expected->supportedFeatures.contains(eClientFeatureProgressReporting),
+      true);
+  EXPECT_EQ(
+      expected->supportedFeatures.contains(eClientFeatureInvalidatedEvent),
+      true);
+  EXPECT_EQ(expected->supportedFeatures.contains(eClientFeatureMemoryEvent),
+            true);
+  EXPECT_EQ(expected->supportedFeatures.contains(
+                eClientFeatureArgsCanBeInterpretedByShell),
+            true);
+  EXPECT_EQ(
+      expected->supportedFeatures.contains(eClientFeatureStartDebuggingRequest),
+      true);
+  EXPECT_EQ(expected->supportedFeatures.contains(eClientFeatureANSIStyling),
+            true);
+
+  // Check required keys.
+  EXPECT_THAT_EXPECTED(parse<InitializeRequestArguments>(R"({})"),
+                       FailedWithMessage("missing value at (root).adapterID"));
+}

[da-viper]

LGTM. but it does not fix the linked issue. as that requires back-porting the fix to 21.x branch.

[ashgti]

LGTM. but it does not fix the linked issue. as that requires back-porting the fix to 21.x branch.

Oops, your right, I'll remove the fixes remark.