[Lldb-commits] [lldb] [lldb][lldb-dap] use the new protocol for setVariable requests. (PR #137803)

[John Harrison via lldb-commits]

================
@@ -13,168 +13,66 @@
 
 namespace lldb_dap {
 
-// "SetVariableRequest": {
-//   "allOf": [ { "$ref": "#/definitions/Request" }, {
-//     "type": "object",
-//     "description": "setVariable request; value of command field is
-//     'setVariable'. Set the variable with the given name in the variable
-//     container to a new value.", "properties": {
-//       "command": {
-//         "type": "string",
-//         "enum": [ "setVariable" ]
-//       },
-//       "arguments": {
-//         "$ref": "#/definitions/SetVariableArguments"
-//       }
-//     },
-//     "required": [ "command", "arguments"  ]
-//   }]
-// },
-// "SetVariableArguments": {
-//   "type": "object",
-//   "description": "Arguments for 'setVariable' request.",
-//   "properties": {
-//     "variablesReference": {
-//       "type": "integer",
-//       "description": "The reference of the variable container."
-//     },
-//     "name": {
-//       "type": "string",
-//       "description": "The name of the variable."
-//     },
-//     "value": {
-//       "type": "string",
-//       "description": "The value of the variable."
-//     },
-//     "format": {
-//       "$ref": "#/definitions/ValueFormat",
-//       "description": "Specifies details on how to format the response value."
-//     }
-//   },
-//   "required": [ "variablesReference", "name", "value" ]
-// },
-// "SetVariableResponse": {
-//   "allOf": [ { "$ref": "#/definitions/Response" }, {
-//     "type": "object",
-//     "description": "Response to 'setVariable' request.",
-//     "properties": {
-//       "body": {
-//         "type": "object",
-//         "properties": {
-//           "value": {
-//             "type": "string",
-//             "description": "The new value of the variable."
-//           },
-//           "type": {
-//             "type": "string",
-//             "description": "The type of the new value. Typically shown in the
-//             UI when hovering over the value."
-//           },
-//           "variablesReference": {
-//             "type": "number",
-//             "description": "If variablesReference is > 0, the new value is
-//             structured and its children can be retrieved by passing
-//             variablesReference to the VariablesRequest."
-//           },
-//           "namedVariables": {
-//             "type": "number",
-//             "description": "The number of named child variables. The client
-//             can use this optional information to present the variables in a
-//             paged UI and fetch them in chunks."
-//           },
-//           "indexedVariables": {
-//             "type": "number",
-//             "description": "The number of indexed child variables. The client
-//             can use this optional information to present the variables in a
-//             paged UI and fetch them in chunks."
-//           },
-//           "valueLocationReference": {
-//             "type": "integer",
-//             "description": "A reference that allows the client to request the
-//             location where the new value is declared. For example, if the new
-//             value is function pointer, the adapter may be able to look up the
-//             function's location. This should be present only if the adapter
-//             is likely to be able to resolve the location.\n\nThis reference
-//             shares the same lifetime as the `variablesReference`. See
-//             'Lifetime of Object References' in the Overview section for
-//             details."
-//           }
-//         },
-//         "required": [ "value" ]
-//       }
-//     },
-//     "required": [ "body" ]
-//   }]
-// }
-void SetVariableRequestHandler::operator()(
-    const llvm::json::Object &request) const {
-  llvm::json::Object response;
-  FillResponse(request, response);
-  llvm::json::Array variables;
-  llvm::json::Object body;
-  const auto *arguments = request.getObject("arguments");
-  // This is a reference to the containing variable/scope
-  const auto variablesReference =
-      GetInteger<uint64_t>(arguments, "variablesReference").value_or(0);
-  llvm::StringRef name = GetString(arguments, "name").value_or("");
+/// Set the variable with the given name in the variable container to a new
+/// value. Clients should only call this request if the corresponding capability
+/// `supportsSetVariable` is true.
+///
+/// If a debug adapter implements both `setVariable` and `setExpression`,
+/// a client will only use `setExpression` if the variable has an evaluateName
+/// property.
+llvm::Expected<protocol::SetVariableResponseBody>
+SetVariableRequestHandler::Run(
+    const protocol::SetVariableArguments &args) const {
+  const auto args_name = llvm::StringRef(args.name);
 
-  const auto value = GetString(arguments, "value").value_or("");
-  // Set success to false just in case we don't find the variable by name
-  response.try_emplace("success", false);
+  constexpr llvm::StringRef return_value_name = "(Return Value)";
+  if (args_name == return_value_name)
+    return llvm::make_error<DAPError>(
+        "cannot change the value of the return value");
 
-  lldb::SBValue variable;
+  lldb::SBValue variable =
+      dap.variables.FindVariable(args.variablesReference, args_name);
 
-  // The "id" is the unique integer ID that is unique within the enclosing
-  // variablesReference. It is optionally added to any "interface Variable"
-  // objects to uniquely identify a variable within an enclosing
-  // variablesReference. It helps to disambiguate between two variables that
-  // have the same name within the same scope since the "setVariables" request
-  // only specifies the variable reference of the enclosing scope/variable, and
-  // the name of the variable. We could have two shadowed variables with the
-  // same name in "Locals" or "Globals". In our case the "id" absolute index
-  // of the variable within the dap.variables list.
-  const auto id_value =
-      GetInteger<uint64_t>(arguments, "id").value_or(UINT64_MAX);
-  if (id_value != UINT64_MAX) {
-    variable = dap.variables.GetVariable(id_value);
-  } else {
-    variable = dap.variables.FindVariable(variablesReference, name);
-  }
+  if (!variable.IsValid())
+    return llvm::make_error<DAPError>("could not find variable in scope");
+
+  lldb::SBError error;
+  const bool success = variable.SetValueFromCString(args.value.c_str(), error);
+  if (!success)
+    return llvm::make_error<DAPError>(error.GetCString());
+
+  VariableDescription desc(variable,
+                           dap.configuration.enableAutoVariableSummaries);
 
-  if (variable.IsValid()) {
-    lldb::SBError error;
-    bool success = variable.SetValueFromCString(value.data(), error);
-    if (success) {
-      VariableDescription desc(variable,
-                               dap.configuration.enableAutoVariableSummaries);
-      EmplaceSafeString(body, "value", desc.display_value);
-      EmplaceSafeString(body, "type", desc.display_type_name);
+  auto body = protocol::SetVariableResponseBody{};
----------------
ashgti wrote:

style nit: I think in lldb is more common to write this as `protocol::SetVariableResponseBody body;`, which is slightly shorter.

https://github.com/llvm/llvm-project/pull/137803