[lldb-dap] Migrating 'evaluate' to structured types.

Adding structured types for the evaluate request handler.

This should be mostly a non-functional change. I did catch some spelling mistakes in our tests ('variable' vs 'variables').

Author: ashgti

lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py

@@ -978,9 +978,10 @@ def request_evaluate(self, expression, frameIndex=0, threadId=None, context=None
             return []
         args_dict = {
             "expression": expression,
-            "context": context,
             "frameId": stackFrame["id"],
         }
+        if context:
+            args_dict["context"] = context
         command_dict = {
             "command": "evaluate",
             "type": "request",

lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py

@@ -1,5 +1,5 @@
 """
-Test lldb-dap completions request
+Test lldb-dap evaluate request
 """
 
 import re
@@ -245,4 +245,6 @@ def test_hover_evaluate_expressions(self):
     @skipIfWindows
     def test_variable_evaluate_expressions(self):
         # Tests expression evaluations that are triggered in the variable explorer
-        self.run_test_evaluate_expressions("variable", enableAutoVariableSummaries=True)
+        self.run_test_evaluate_expressions(
+            "variables", enableAutoVariableSummaries=True
+        )

lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp

@@ -10,148 +10,36 @@
 #include "EventHelper.h"
 #include "JSONUtils.h"
 #include "LLDBUtils.h"
+#include "Protocol/ProtocolRequests.h"
+#include "Protocol/ProtocolTypes.h"
 #include "RequestHandler.h"
+#include "lldb/API/SBCommandInterpreter.h"
+#include "lldb/API/SBCommandReturnObject.h"
+#include "lldb/lldb-enumerations.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
+#include <future>
+#include <optional>
+#include <unistd.h>
+
+using namespace llvm;
+using namespace lldb_dap;
+using namespace lldb_dap::protocol;
 
 namespace lldb_dap {
 
-//  "EvaluateRequest": {
-//    "allOf": [ { "$ref": "#/definitions/Request" }, {
-//      "type": "object",
-//      "description": "Evaluate request; value of command field is 'evaluate'.
-//                      Evaluates the given expression in the context of the
-//                      top most stack frame. The expression has access to any
-//                      variables and arguments that are in scope.",
-//      "properties": {
-//        "command": {
-//          "type": "string",
-//          "enum": [ "evaluate" ]
-//        },
-//        "arguments": {
-//          "$ref": "#/definitions/EvaluateArguments"
-//        }
-//      },
-//      "required": [ "command", "arguments"  ]
-//    }]
-//  },
-//  "EvaluateArguments": {
-//    "type": "object",
-//    "description": "Arguments for 'evaluate' request.",
-//    "properties": {
-//      "expression": {
-//        "type": "string",
-//        "description": "The expression to evaluate."
-//      },
-//      "frameId": {
-//        "type": "integer",
-//        "description": "Evaluate the expression in the scope of this stack
-//                        frame. If not specified, the expression is evaluated
-//                        in the global scope."
-//      },
-//      "context": {
-//        "type": "string",
-//        "_enum": [ "watch", "repl", "hover" ],
-//        "enumDescriptions": [
-//          "evaluate is run in a watch.",
-//          "evaluate is run from REPL console.",
-//          "evaluate is run from a data hover."
-//        ],
-//        "description": "The context in which the evaluate request is run."
-//      },
-//      "format": {
-//        "$ref": "#/definitions/ValueFormat",
-//        "description": "Specifies details on how to format the Evaluate
-//                        result."
-//      }
-//    },
-//    "required": [ "expression" ]
-//  },
-//  "EvaluateResponse": {
-//    "allOf": [ { "$ref": "#/definitions/Response" }, {
-//      "type": "object",
-//      "description": "Response to 'evaluate' request.",
-//      "properties": {
-//        "body": {
-//          "type": "object",
-//          "properties": {
-//            "result": {
-//              "type": "string",
-//              "description": "The result of the evaluate request."
-//            },
-//            "type": {
-//              "type": "string",
-//              "description": "The optional type of the evaluate result."
-//            },
-//            "presentationHint": {
-//              "$ref": "#/definitions/VariablePresentationHint",
-//              "description": "Properties of a evaluate result that can be
-//                              used to determine how to render the result in
-//                              the UI."
-//            },
-//            "variablesReference": {
-//              "type": "number",
-//              "description": "If variablesReference is > 0, the evaluate
-//                              result 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 returned value is
-//                              declared. For example, if a function pointer is
-//                              returned, 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."
-//            }
-//            "memoryReference": {
-//               "type": "string",
-//                "description": "A memory reference to a location appropriate
-//                                for this result. For pointer type eval
-//                                results, this is generally a reference to the
-//                                memory address contained in the pointer. This
-//                                attribute may be returned by a debug adapter
-//                                if corresponding capability
-//                                `supportsMemoryReferences` is true."
-//             },
-//          },
-//          "required": [ "result", "variablesReference" ]
-//        }
-//      },
-//      "required": [ "body" ]
-//    }]
-//  }
-void EvaluateRequestHandler::operator()(
-    const llvm::json::Object &request) const {
-  llvm::json::Object response;
-  FillResponse(request, response);
-  llvm::json::Object body;
-  const auto *arguments = request.getObject("arguments");
-  lldb::SBFrame frame = dap.GetLLDBFrame(*arguments);
-  std::string expression =
-      GetString(arguments, "expression").value_or("").str();
-  const llvm::StringRef context = GetString(arguments, "context").value_or("");
+/// Evaluates the given expression in the context of a stack frame.
+///
+/// The expression has access to any variables and arguments that are in scope.
+Expected<EvaluateResponseBody>
+EvaluateRequestHandler::Run(const EvaluateArguments &arguments) const {
+  EvaluateResponseBody body;
+  lldb::SBFrame frame = dap.GetLLDBFrame(arguments.frameId);
+  std::string expression = arguments.expression;
   bool repeat_last_command =
       expression.empty() && dap.last_nonempty_var_expression.empty();
 
-  if (context == "repl" &&
+  if (arguments.context == protocol::eEvaluateContextRepl &&
       (repeat_last_command ||
        (!expression.empty() &&
         dap.DetectReplMode(frame, expression, false) == ReplMode::Command))) {
@@ -169,66 +57,61 @@ void EvaluateRequestHandler::operator()(
         dap.debugger, llvm::StringRef(), {expression}, required_command_failed,
         /*parse_command_directives=*/false, /*echo_commands=*/false);
 
-    EmplaceSafeString(body, "result", result);
-    body.try_emplace("variablesReference", (int64_t)0);
-  } else {
-    if (context == "repl") {
-      // If the expression is empty and the last expression was for a
-      // variable, set the expression to the previous expression (repeat the
-      // evaluation); otherwise save the current non-empty expression for the
-      // next (possibly empty) variable expression.
-      if (expression.empty())
-        expression = dap.last_nonempty_var_expression;
-      else
-        dap.last_nonempty_var_expression = expression;
-    }
-    // Always try to get the answer from the local variables if possible. If
-    // this fails, then if the context is not "hover", actually evaluate an
-    // expression using the expression parser.
-    //
-    // "frame variable" is more reliable than the expression parser in
-    // many cases and it is faster.
-    lldb::SBValue value = frame.GetValueForVariablePath(
-        expression.data(), lldb::eDynamicDontRunTarget);
-
-    // Freeze dry the value in case users expand it later in the debug console
-    if (value.GetError().Success() && context == "repl")
-      value = value.Persist();
-
-    if (value.GetError().Fail() && context != "hover")
-      value = frame.EvaluateExpression(expression.data());
-
-    if (value.GetError().Fail()) {
-      response["success"] = llvm::json::Value(false);
-      // This error object must live until we're done with the pointer returned
-      // by GetCString().
-      lldb::SBError error = value.GetError();
-      const char *error_cstr = error.GetCString();
-      if (error_cstr && error_cstr[0])
-        EmplaceSafeString(response, "message", error_cstr);
-      else
-        EmplaceSafeString(response, "message", "evaluate failed");
-    } else {
-      VariableDescription desc(value,
-                               dap.configuration.enableAutoVariableSummaries);
-      EmplaceSafeString(body, "result", desc.GetResult(context));
-      EmplaceSafeString(body, "type", desc.display_type_name);
-      int64_t var_ref = 0;
-      if (value.MightHaveChildren() || ValuePointsToCode(value))
-        var_ref = dap.variables.InsertVariable(
-            value, /*is_permanent=*/context == "repl");
-      if (value.MightHaveChildren())
-        body.try_emplace("variablesReference", var_ref);
-      else
-        body.try_emplace("variablesReference", (int64_t)0);
-      if (lldb::addr_t addr = value.GetLoadAddress();
-          addr != LLDB_INVALID_ADDRESS)
-        body.try_emplace("memoryReference", EncodeMemoryReference(addr));
-      if (ValuePointsToCode(value))
-        body.try_emplace("valueLocationReference", var_ref);
-    }
+    body.result = result;
+    return body;
+  }
+
+  if (arguments.context == eEvaluateContextRepl) {
+    // If the expression is empty and the last expression was for a
+    // variable, set the expression to the previous expression (repeat the
+    // evaluation); otherwise save the current non-empty expression for the
+    // next (possibly empty) variable expression.
+    if (expression.empty())
+      expression = dap.last_nonempty_var_expression;
+    else
+      dap.last_nonempty_var_expression = expression;
+  }
+
+  // Always try to get the answer from the local variables if possible. If
+  // this fails, then if the context is not "hover", actually evaluate an
+  // expression using the expression parser.
+  //
+  // "frame variable" is more reliable than the expression parser in
+  // many cases and it is faster.
+  lldb::SBValue value = frame.GetValueForVariablePath(
+      expression.data(), lldb::eDynamicDontRunTarget);
+
+  // Freeze dry the value in case users expand it later in the debug console
+  if (value.GetError().Success() && arguments.context == eEvaluateContextRepl)
+    value = value.Persist();
+
+  if (value.GetError().Fail() && arguments.context != eEvaluateContextHover)
+    value = frame.EvaluateExpression(expression.data());
+
+  if (value.GetError().Fail()) {
+    // This error object must live until we're done with the pointer returned
+    // by GetCString().
+    lldb::SBError error = value.GetError();
+    const char *error_cstr = error.GetCString();
+    return make_error<DAPError>(
+        error_cstr && error_cstr[0] ? error_cstr : "evaluate failed");
   }
-  response.try_emplace("body", std::move(body));
-  dap.SendJSON(llvm::json::Value(std::move(response)));
+
+  VariableDescription desc(value,
+                           dap.configuration.enableAutoVariableSummaries);
+  body.result = desc.GetResult(arguments.context);
+  body.type = desc.display_type_name;
+  if (value.MightHaveChildren() || ValuePointsToCode(value))
+    body.variablesReference = dap.variables.InsertVariable(
+        value, /*is_permanent=*/arguments.context == eEvaluateContextRepl);
+  if (lldb::addr_t addr = value.GetLoadAddress(); addr != LLDB_INVALID_ADDRESS)
+    body.memoryReference = EncodeMemoryReference(addr);
+
+  if (ValuePointsToCode(value) &&
+      body.variablesReference != LLDB_DAP_INVALID_VARRERF)
+    body.valueLocationReference = body.variablesReference;
+
+  return body;
 }
+
 } // namespace lldb_dap

lldb/tools/lldb-dap/Handler/RequestHandler.h

@@ -292,11 +292,14 @@ class DisconnectRequestHandler
   Run(const std::optional<protocol::DisconnectArguments> &args) const override;
 };
 
-class EvaluateRequestHandler : public LegacyRequestHandler {
+class EvaluateRequestHandler
+    : public RequestHandler<protocol::EvaluateArguments,
+                            llvm::Expected<protocol::EvaluateResponseBody>> {
 public:
-  using LegacyRequestHandler::LegacyRequestHandler;
+  using RequestHandler::RequestHandler;
   static llvm::StringLiteral GetCommand() { return "evaluate"; }
-  void operator()(const llvm::json::Object &request) const override;
+  llvm::Expected<protocol::EvaluateResponseBody>
+  Run(const protocol::EvaluateArguments &) const override;
   FeatureSet GetSupportedFeatures() const override {
     return {protocol::eAdapterFeatureEvaluateForHovers};
   }

lldb/tools/lldb-dap/JSONUtils.cpp

@@ -11,6 +11,7 @@
 #include "ExceptionBreakpoint.h"
 #include "LLDBUtils.h"
 #include "Protocol/ProtocolBase.h"
+#include "Protocol/ProtocolRequests.h"
 #include "ProtocolUtils.h"
 #include "lldb/API/SBAddress.h"
 #include "lldb/API/SBCompileUnit.h"
@@ -817,10 +818,10 @@ VariableDescription::VariableDescription(lldb::SBValue v,
   evaluate_name = llvm::StringRef(evaluateStream.GetData()).str();
 }
 
-std::string VariableDescription::GetResult(llvm::StringRef context) {
+std::string VariableDescription::GetResult(protocol::EvaluateContext context) {
   // In repl context, the results can be displayed as multiple lines so more
   // detailed descriptions can be returned.
-  if (context != "repl")
+  if (context != protocol::eEvaluateContextRepl)
     return display_value;
 
   if (!v.IsValid())

lldb/tools/lldb-dap/JSONUtils.h

@@ -10,7 +10,7 @@
 #define LLDB_TOOLS_LLDB_DAP_JSONUTILS_H
 
 #include "DAPForward.h"
-#include "Protocol/ProtocolTypes.h"
+#include "Protocol/ProtocolRequests.h"
 #include "lldb/API/SBCompileUnit.h"
 #include "lldb/API/SBFormat.h"
 #include "lldb/API/SBType.h"
@@ -28,7 +28,7 @@
 
 namespace lldb_dap {
 
-/// Emplace a StringRef in a json::Object after enusring that the
+/// Emplace a StringRef in a json::Object after ensuring that the
 /// string is valid UTF8. If not, first call llvm::json::fixUTF8
 /// before emplacing.
 ///
@@ -351,7 +351,7 @@ struct VariableDescription {
                       std::optional<std::string> custom_name = {});
 
   /// Returns a description of the value appropriate for the specified context.
-  std::string GetResult(llvm::StringRef context);
+  std::string GetResult(protocol::EvaluateContext context);
 };
 
 /// Does the given variable have an associated value location?