[lldb-dap] Adjusting how repl-mode auto determines commands vs variable expressions.

lldb/test/API/tools/lldb-dap/completions/TestDAP_completions.py

@@ -41,13 +41,13 @@ def test_completions(self):
                 {
                     "text": "var",
                     "label": "var -- vector<baz> &",
-                }
-            ],
-            [
+                },
                 {
                     "text": "var",
                     "label": "var -- Show variables for the current stack frame. Defaults to all arguments and local variables in scope. Names of argument, local, file static and file global variables can be specified.",
                 },
+            ],
+            [
                 {"text": "var1", "label": "var1 -- int &"},
             ],
         )

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

@@ -87,7 +87,13 @@ def run_test_evaluate_expressions(
             )
             self.assertEvaluate("struct3", "0x.*0")
 
-        self.assertEvaluateFailure("var")  # local variable of a_function
+        if context == "repl":
+            # In the repl context expressions may be interpreted as lldb
+            # commands since no variables have the same name as the command.
+            self.assertEvaluate("var", r"\(lldb\) var\n.*")
+        else:
+            self.assertEvaluateFailure("var")  # local variable of a_function
+
         self.assertEvaluateFailure("my_struct")  # type name
         self.assertEvaluateFailure("int")  # type name
         self.assertEvaluateFailure("foo")  # member of my_struct

lldb/tools/lldb-dap/DAP.cpp

@@ -47,8 +47,7 @@ DAP::DAP()
       configuration_done_sent(false), waiting_for_run_in_terminal(false),
       progress_event_reporter(
           [&](const ProgressEvent &event) { SendJSON(event.ToJSON()); }),
-      reverse_request_seq(0), repl_mode(ReplMode::Auto),
-      auto_repl_mode_collision_warning(false) {
+      reverse_request_seq(0), repl_mode(ReplMode::Auto) {
   const char *log_file_path = getenv("LLDBDAP_LOG");
 #if defined(_WIN32)
   // Windows opens stdout and stdin in text mode which converts \n to 13,10
@@ -380,12 +379,12 @@ llvm::json::Value DAP::CreateTopLevelScopes() {
   return llvm::json::Value(std::move(scopes));
 }
 
-ExpressionContext DAP::DetectExpressionContext(lldb::SBFrame &frame,
-                                               std::string &text) {
+ExpressionContext DAP::DetectExpressionContext(lldb::SBFrame frame,
+                                               std::string &expression) {
   // Include the escape hatch prefix.
-  if (!text.empty() &&
-      llvm::StringRef(text).starts_with(g_dap.command_escape_prefix)) {
-    text = text.substr(g_dap.command_escape_prefix.size());
+  if (!expression.empty() &&
+      llvm::StringRef(expression).starts_with(g_dap.command_escape_prefix)) {
+    expression = expression.substr(g_dap.command_escape_prefix.size());
     return ExpressionContext::Command;
   }
 
@@ -395,43 +394,40 @@ ExpressionContext DAP::DetectExpressionContext(lldb::SBFrame &frame,
   case ReplMode::Command:
     return ExpressionContext::Command;
   case ReplMode::Auto:
-    // If the frame is invalid then there is no variables to complete, assume
-    // this is an lldb command instead.
-    if (!frame.IsValid()) {
-      return ExpressionContext::Command;
-    }
-
+    // To determine if the expression is a command or not, check if the first
+    // term is a variable or command. If it's a variable in scope we will prefer
+    // that behavior and give a warning to the user if they meant to invoke the
+    // operation as a command.
+    //
+    // Example use case:
+    //   int p and expression "p + 1" > variable
+    //   int i and expression "i" > variable
+    //   int var and expression "va" > command
+    std::pair<llvm::StringRef, llvm::StringRef> token =
+        llvm::getToken(expression);
+    std::string term = token.first.str();
     lldb::SBCommandReturnObject result;
-    debugger.GetCommandInterpreter().ResolveCommand(text.data(), result);
-
-    // If this command is a simple expression like `var + 1` check if there is
-    // a local variable name that is in the current expression. If so, ensure
-    // the expression runs in the variable context.
-    lldb::SBValueList variables = frame.GetVariables(true, true, true, true);
-    llvm::StringRef input = text;
-    for (uint32_t i = 0; i < variables.GetSize(); i++) {
-      llvm::StringRef name = variables.GetValueAtIndex(i).GetName();
-      // Check both directions in case the input is a partial of a variable
-      // (e.g. input = `va` and local variable = `var1`).
-      if (input.contains(name) || name.contains(input)) {
-        if (!auto_repl_mode_collision_warning) {
-          llvm::errs() << "Variable expression '" << text
-                       << "' is hiding an lldb command, prefix an expression "
-                          "with '"
-                       << g_dap.command_escape_prefix
-                       << "' to ensure it runs as a lldb command.\n";
-          auto_repl_mode_collision_warning = true;
-        }
-        return ExpressionContext::Variable;
-      }
+    debugger.GetCommandInterpreter().ResolveCommand(term.c_str(), result);
+    bool term_is_command = result.Succeeded();
+    bool term_is_variable = frame.FindVariable(term.c_str()).IsValid();
+
+    // If we have both a variable and command, warn the user about the conflict.
+    if (term_is_command && term_is_variable) {
+      llvm::errs()
+          << "Warning: Expression '" << term
+          << "' is both an LLDB command and variable. It will be evaluated as "
+             "a variable. To evaluate the expression as an LLDB command, use '"
+          << g_dap.command_escape_prefix << "' as a prefix.\n";
     }
 
-    if (result.Succeeded()) {
-      return ExpressionContext::Command;
-    }
+    // Variables take preference to commands in auto, since commands can always
+    // be called using the command_escape_prefix
+    return term_is_variable  ? ExpressionContext::Variable
+           : term_is_command ? ExpressionContext::Command
+                             : ExpressionContext::Variable;
   }
 
-  return ExpressionContext::Variable;
+  llvm_unreachable("enum cases exhausted.");
 }
 
 bool DAP::RunLLDBCommands(llvm::StringRef prefix,

lldb/tools/lldb-dap/DAP.h

@@ -189,7 +189,6 @@ struct DAP {
   StartDebuggingRequestHandler start_debugging_request_handler;
   ReplModeRequestHandler repl_mode_request_handler;
   ReplMode repl_mode;
-  bool auto_repl_mode_collision_warning;
   std::string command_escape_prefix = "`";
   lldb::SBFormat frame_format;
   lldb::SBFormat thread_format;
@@ -225,8 +224,12 @@ struct DAP {
 
   llvm::json::Value CreateTopLevelScopes();
 
-  ExpressionContext DetectExpressionContext(lldb::SBFrame &frame,
-                                            std::string &text);
+  /// \return
+  ///   Attempt to determine if an expression is a variable expression or
+  ///   lldb command using a hueristic based on the first term of the
+  ///   expression.
+  ExpressionContext DetectExpressionContext(lldb::SBFrame frame,
+                                            std::string &expression);
 
   /// \return
   ///   \b false if a fatal error was found while executing these commands,

lldb/tools/lldb-dap/lldb-dap.cpp

@@ -37,6 +37,7 @@
 #endif
 
 #include <algorithm>
+#include <array>
 #include <chrono>
 #include <fstream>
 #include <map>
@@ -1125,21 +1126,33 @@ void request_completions(const llvm::json::Object &request) {
   }
   llvm::json::Array targets;
 
-  if (g_dap.DetectExpressionContext(frame, text) ==
-      ExpressionContext::Variable) {
-    char command[] = "expression -- ";
-    text = command + text;
-    offset += strlen(command);
-  }
-  lldb::SBStringList matches;
-  lldb::SBStringList descriptions;
+  if (!text.empty() &&
+      llvm::StringRef(text).starts_with(g_dap.command_escape_prefix)) {
+    text = text.substr(g_dap.command_escape_prefix.size());
+  }
+
+  // While the user is typing then we likely have an incomplete input and cannot
+  // reliably determine the precise intent (command vs variable), try completing
+  // the text as both a command and variable expression, if applicable.
+  const std::string expr_prefix = "expression -- ";
+  std::array<std::tuple<ReplMode, std::string, uint64_t>, 2> exprs = {
+      {std::make_tuple(ReplMode::Command, text, offset),
+       std::make_tuple(ReplMode::Variable, expr_prefix + text,
+                       offset + expr_prefix.size())}};
+  for (const auto &[mode, line, cursor] : exprs) {
+    if (g_dap.repl_mode != ReplMode::Auto && g_dap.repl_mode != mode)
+      continue;
+
+    lldb::SBStringList matches;
+    lldb::SBStringList descriptions;
+    if (!g_dap.debugger.GetCommandInterpreter()
+             .HandleCompletionWithDescriptions(line.c_str(), cursor, 0, 100,
+                                               matches, descriptions))
+      continue;
 
-  if (g_dap.debugger.GetCommandInterpreter().HandleCompletionWithDescriptions(
-          text.c_str(), offset, 0, 100, matches, descriptions)) {
     // The first element is the common substring after the cursor position for
     // all the matches. The rest of the elements are the matches so ignore the
     // first result.
-    targets.reserve(matches.GetSize() - 1);
     for (size_t i = 1; i < matches.GetSize(); i++) {
       std::string match = matches.GetStringAtIndex(i);
       std::string description = descriptions.GetStringAtIndex(i);