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

The previous logic for determining if an expression was a command or
variable expression in the repl would incorrectly identify the context
in many common cases where a local variable name partially overlaps with
the repl input.

For example:
```
int foo() {
  int var = 1; // break point, evaluating "p var", previously emitted a warning
}
```

Instead of checking potentially multiple conflicting values against the
expression input, I updated the heuristic to only consider the first
term. This is much more reliable at eliminating false positives when the
input does not actually hide a local variable.

Additionally, I updated the warning on conflicts to occur anytime the
conflict is detected since the specific conflict can change based on the
current input. This also includes additional details on how users can
change the behavior.

Example Debug Console output from
lldb/test/API/tools/lldb-dap/evaluate/main.cpp:11 breakpoint 3.

```
lldb-dap> var + 3
Warning: Expression 'var' is both an LLDB command and variable. It will be evaluated as a variable. To evaluate the expression as an LLDB command, use '`' as a prefix.
45

lldb-dap> var + 1
Warning: Expression 'var' is both an LLDB command and variable. It will be evaluated as a variable. To evaluate the expression as an LLDB command, use '`' as a prefix.
43
```

Author: ashgti

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>
@@ -1126,21 +1127,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);