[lldb] Inline expression evaluator error visualization

This patch is a reworking of Pete Lawrence's (@PortalPete) proposal
for better expression evaluator error messages:
#80938

Before:

```
$ lldb -o "expr a+b"
(lldb) expr a+b
error: <user expression 0>:1:1: use of undeclared identifier 'a'
a+b
^
error: <user expression 0>:1:3: use of undeclared identifier 'b'
a+b
  ^
```

After:

```
(lldb) expr a+b
            ^ ^
            │ ╰─ error: use of undeclared identifier 'b'
            ╰─ error: use of undeclared identifier 'a'
```

This eliminates the confusing `<user expression 0>:1:3` source
location and avoids echoing the expression to the console again, which
results in a cleaner presentation that makes it easier to grasp what's
going on.

Author: adrian-prantl

lldb/include/lldb/Expression/DiagnosticManager.h

@@ -59,8 +59,9 @@ class DetailedExpressionError
 
 public:
   using llvm::ErrorInfo<DetailedExpressionError, llvm::ECError>::ErrorInfo;
-  DetailedExpressionError(DiagnosticDetail detail) {}
+  DetailedExpressionError(DiagnosticDetail detail) : m_detail(detail) {}
   std::string message() const override;
+  DiagnosticDetail GetDetail() const { return m_detail; }
   static char ID;
 };
 

lldb/include/lldb/Interpreter/CommandObject.h

@@ -340,6 +340,13 @@ class CommandObject : public std::enable_shared_from_this<CommandObject> {
       return false;
   }
 
+  /// Set the command input as it appeared in the terminal. This
+  /// is used to have errors refer directly to the original command.
+  void SetOriginalCommandString(std::string s) { m_original_command = s; }
+
+  /// \param offset_in_command is on what column \c args_string
+  /// appears, if applicable. This enables diagnostics that refer back
+  /// to the user input.
   virtual void Execute(const char *args_string,
                        CommandReturnObject &result) = 0;
 
@@ -404,6 +411,7 @@ class CommandObject : public std::enable_shared_from_this<CommandObject> {
   std::string m_cmd_help_short;
   std::string m_cmd_help_long;
   std::string m_cmd_syntax;
+  std::string m_original_command;
   Flags m_flags;
   std::vector<CommandArgumentEntry> m_arguments;
   lldb::CommandOverrideCallback m_deprecated_command_override_callback;

lldb/source/Commands/CommandObjectExpression.cpp

@@ -10,6 +10,7 @@
 
 #include "CommandObjectExpression.h"
 #include "lldb/Core/Debugger.h"
+#include "lldb/Expression/DiagnosticManager.h"
 #include "lldb/Expression/ExpressionVariable.h"
 #include "lldb/Expression/REPL.h"
 #include "lldb/Expression/UserExpression.h"
@@ -398,6 +399,104 @@ CanBeUsedForElementCountPrinting(ValueObject &valobj) {
   return Status();
 }
 
+static llvm::raw_ostream &PrintSeverity(Stream &stream,
+                                        lldb::Severity severity) {
+  llvm::HighlightColor color;
+  llvm::StringRef text;
+  switch (severity) {
+  case eSeverityError:
+    color = llvm::HighlightColor::Error;
+    text = "error: ";
+    break;
+  case eSeverityWarning:
+    color = llvm::HighlightColor::Warning;
+    text = "warning: ";
+    break;
+  case eSeverityInfo:
+    color = llvm::HighlightColor::Remark;
+    text = "note: ";
+    break;
+  }
+  return llvm::WithColor(stream.AsRawOstream(), color, llvm::ColorMode::Enable)
+         << text;
+}
+
+namespace lldb_private {
+// Public for unittesting.
+std::string RenderDiagnosticDetails(std::optional<uint16_t> offset_in_command,
+                                    llvm::ArrayRef<DiagnosticDetail> details) {
+  if (details.empty())
+    return {};
+
+  StreamString stream(true);
+  if (!offset_in_command) {
+    for (const DiagnosticDetail &detail : details) {
+      PrintSeverity(stream, detail.severity);
+      stream << detail.rendered << '\n';
+    }
+    return stream.GetData();
+  }
+
+  // Print a line with caret indicator(s) below the lldb prompt + command.
+  const size_t padding = *offset_in_command;
+  stream << std::string(padding, ' ');
+
+  size_t offset = 1;
+  std::vector<DiagnosticDetail> remaining_details, other_details;
+  for (const DiagnosticDetail &detail : details) {
+    if (!detail.source_location) {
+      other_details.push_back(detail);
+      continue;
+    }
+
+    auto &loc = *detail.source_location;
+    remaining_details.push_back(detail);
+    if (offset > loc.column)
+      continue;
+    stream << std::string(loc.column - offset, ' ') << '^';
+    if (loc.length > 1)
+      stream << std::string(loc.length - 1, '~');
+    offset = loc.column + 1;
+  }
+  stream << '\n';
+
+  // Work through each detail in reverse order using the vector/stack.
+  for (auto detail = remaining_details.rbegin();
+       detail != remaining_details.rend();
+       ++detail, remaining_details.pop_back()) {
+    // Get the information to print this detail and remove it from the stack.
+    // Print all the lines for all the other messages first.
+    stream << std::string(padding, ' ');
+    size_t offset = 1;
+    for (auto &remaining_detail :
+         llvm::ArrayRef(remaining_details).drop_back(1)) {
+      uint16_t column = remaining_detail.source_location->column;
+      stream << std::string(column - offset, ' ') << "│";
+      offset = column + 1;
+    }
+
+    // Print the line connecting the ^ with the error message.
+    uint16_t column = detail->source_location->column;
+    if (offset <= column)
+      stream << std::string(column - offset, ' ') << "╰─ ";
+
+    // Print a colorized string based on the message's severity type.
+    PrintSeverity(stream, detail->severity);
+
+    // Finally, print the message and start a new line.
+    stream << detail->message << '\n';
+  }
+
+  // Print the non-located details.
+  for (const DiagnosticDetail &detail : other_details) {
+    PrintSeverity(stream, detail.severity);
+    stream << detail.message << '\n';
+  }
+
+  return stream.GetData();
+}
+} // namespace lldb_private
+
 bool CommandObjectExpression::EvaluateExpression(llvm::StringRef expr,
                                                  Stream &output_stream,
                                                  Stream &error_stream,
@@ -486,19 +585,37 @@ bool CommandObjectExpression::EvaluateExpression(llvm::StringRef expr,
 
         result.SetStatus(eReturnStatusSuccessFinishResult);
       } else {
-        const char *error_cstr = result_valobj_sp->GetError().AsCString();
-        if (error_cstr && error_cstr[0]) {
-          const size_t error_cstr_len = strlen(error_cstr);
-          const bool ends_with_newline = error_cstr[error_cstr_len - 1] == '\n';
-          if (strstr(error_cstr, "error:") != error_cstr)
-            error_stream.PutCString("error: ");
-          error_stream.Write(error_cstr, error_cstr_len);
-          if (!ends_with_newline)
-            error_stream.EOL();
-        } else {
-          error_stream.PutCString("error: unknown error\n");
+        // Retrieve the diagnostics.
+        std::vector<DiagnosticDetail> details;
+        llvm::consumeError(
+            llvm::handleErrors(result_valobj_sp->GetError().ToError(),
+                               [&](DetailedExpressionError &error) {
+                                 details.push_back(error.GetDetail());
+                               }));
+        // Find the position of the expression in the command.
+        std::optional<uint16_t> expr_pos;
+        size_t nchar = m_original_command.find(expr);
+        if (nchar != std::string::npos)
+          expr_pos = nchar + GetDebugger().GetPrompt().size();
+
+        std::string error = RenderDiagnosticDetails(expr_pos, details);
+        if (!error.empty())
+          error_stream.PutCString(error.c_str());
+        else {
+          const char *error_cstr = result_valobj_sp->GetError().AsCString();
+          if (error_cstr && error_cstr[0]) {
+            const size_t error_cstr_len = strlen(error_cstr);
+            const bool ends_with_newline =
+                error_cstr[error_cstr_len - 1] == '\n';
+            if (strstr(error_cstr, "error:") != error_cstr)
+              error_stream.PutCString("error: ");
+            error_stream.Write(error_cstr, error_cstr_len);
+            if (!ends_with_newline)
+              error_stream.EOL();
+          } else {
+            error_stream.PutCString("error: unknown error\n");
+          }
         }
-
         result.SetStatus(eReturnStatusFailed);
       }
     }

lldb/source/Expression/DiagnosticManager.cpp

@@ -15,10 +15,6 @@
 
 using namespace lldb_private;
 char DetailedExpressionError::ID;
-std::string DetailedExpressionError::message() const {
-  return m_detail.rendered;
-}
-
 
 static const char *StringForSeverity(lldb::Severity severity) {
   switch (severity) {
@@ -33,6 +29,9 @@ static const char *StringForSeverity(lldb::Severity severity) {
   llvm_unreachable("switch needs another case for lldb::Severity enum");
 }
 
+std::string DetailedExpressionError::message() const {
+  return std::string(StringForSeverity(m_detail.severity)) + m_detail.rendered;
+}
 
 void DiagnosticManager::Dump(Log *log) {
   if (!log)

lldb/source/Interpreter/CommandInterpreter.cpp

@@ -1887,7 +1887,8 @@ bool CommandInterpreter::HandleCommand(const char *command_line,
                                        CommandReturnObject &result,
                                        bool force_repeat_command) {
   std::string command_string(command_line);
-  std::string original_command_string(command_line);
+  std::string original_command_string(command_string);
+  std::string real_original_command_string(command_string);
 
   Log *log = GetLog(LLDBLog::Commands);
   llvm::PrettyStackTraceFormat stack_trace("HandleCommand(command = \"%s\")",
@@ -2076,6 +2077,7 @@ bool CommandInterpreter::HandleCommand(const char *command_line,
     }
 
     ElapsedTime elapsed(execute_time);
+    cmd_obj->SetOriginalCommandString(real_original_command_string);
     cmd_obj->Execute(remainder.c_str(), result);
   }
 

lldb/source/Plugins/TraceExporter/ctf/CommandObjectThreadTraceExportCTF.h

@@ -48,7 +48,7 @@ class CommandObjectThreadTraceExportCTF : public CommandObjectParsed {
   Options *GetOptions() override { return &m_options; }
 
 protected:
-  void DoExecute(Args &command, CommandReturnObject &result) override;
+  void DoExecute(Args &args, CommandReturnObject &result) override;
 
   CommandOptions m_options;
 };

lldb/source/Utility/Status.cpp

@@ -11,6 +11,7 @@
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/VASPrintf.h"
+#include "lldb/Expression/DiagnosticManager.h"
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-enumerations.h"
 #include "llvm/ADT/SmallString.h"
@@ -147,6 +148,9 @@ static llvm::Error cloneError(llvm::Error &error) {
     else if (error.isA<ExpressionError>())
       info.push_back(std::make_unique<ExpressionError>(
           error.convertToErrorCode(), error.message()));
+    else if (error.isA<DetailedExpressionError>())
+      info.push_back(std::make_unique<DetailedExpressionError>(
+          static_cast<const DetailedExpressionError *>(&error)->GetDetail()));
     else
       info.push_back(std::make_unique<llvm::StringError>(
           error.message(), error.convertToErrorCode(), true));
@@ -270,8 +274,6 @@ ErrorType Status::GetType() const {
     else if (error.convertToErrorCode().category() == generic_category() ||
              error.convertToErrorCode() == llvm::inconvertibleErrorCode())
       result = eErrorTypeGeneric;
-    else
-      result = eErrorTypeInvalid;
   });
   return result;
 }

lldb/test/API/commands/expression/persistent_variables/TestPersistentVariables.py

@@ -56,7 +56,7 @@ def test_persistent_variables(self):
         self.expect(
             "expr int $i = 123",
             error=True,
-            substrs=["error: redefinition of persistent variable '$i'"],
+            substrs=["redefinition of persistent variable '$i'"],
         )
         self.expect_expr("$i", result_type="int", result_value="5")
 
@@ -65,7 +65,7 @@ def test_persistent_variables(self):
         self.expect(
             "expr long $i = 123",
             error=True,
-            substrs=["error: redefinition of persistent variable '$i'"],
+            substrs=["redefinition of persistent variable '$i'"],
         )
         self.expect_expr("$i", result_type="int", result_value="5")
 

lldb/test/API/commands/expression/static-initializers/TestStaticInitializers.py

@@ -35,7 +35,7 @@ def test_failing_init(self):
         self.expect(
             "expr -p -- struct Foo2 { Foo2() { do_abort(); } }; Foo2 f;",
             error=True,
-            substrs=["error: couldn't run static initializer:"],
+            substrs=["couldn't run static initializer:"],
         )
 
     def test_without_process(self):

lldb/test/API/lang/cpp/template-function/TestTemplateFunctions.py

@@ -21,7 +21,7 @@ def do_test_template_function(self, add_cast):
                 "expr b1 <=> b2",
                 error=True,
                 substrs=[
-                    "warning: <user expression 0>:1:4: '<=>' is a single token in C++20; add a space to avoid a change in behavior"
+                    "warning:", "'<=>' is a single token in C++20; add a space to avoid a change in behavior"
                 ],
             )