[lldb] Inline expression evaluator error visualization (#106470)

This patch is a reworking of Pete Lawrence's (@PortalPete) proposal
for better expression evaluator error messages:
https://github.com/llvm/llvm-project/pull/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. You can't see it here, bug the word "error" is now also in
color, if so desired.

Depends on https://github.com/llvm/llvm-project/pull/106442.

8 files changed, 184 insertions, 28 deletions

diff --git a/lldb/source/API/SBDebugger.cpp b/lldb/source/API/SBDebugger.cpp
index 6b72994..47931f1 100644
--- a/lldb/source/API/SBDebugger.cpp
+++ b/lldb/source/API/SBDebugger.cpp
@@ -1483,6 +1483,12 @@ bool SBDebugger::GetUseColor() const {
   return (m_opaque_sp ? m_opaque_sp->GetUseColor() : false);
 }
 
+bool SBDebugger::SetShowInlineDiagnostics(bool value) {
+  LLDB_INSTRUMENT_VA(this, value);
+
+  return (m_opaque_sp ? m_opaque_sp->SetShowInlineDiagnostics(value) : false);
+}
+
 bool SBDebugger::SetUseSourceCache(bool value) {
   LLDB_INSTRUMENT_VA(this, value);
 
diff --git a/lldb/source/Commands/CommandObjectExpression.cpp b/lldb/source/Commands/CommandObjectExpression.cpp
index 7711946..49f4421 100644
--- a/lldb/source/Commands/CommandObjectExpression.cpp
+++ b/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,122 @@ 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.
+void RenderDiagnosticDetails(Stream &stream,
+                             std::optional<uint16_t> offset_in_command,
+                             bool show_inline,
+                             llvm::ArrayRef<DiagnosticDetail> details) {
+  if (details.empty())
+    return;
+
+  if (!offset_in_command) {
+    for (const DiagnosticDetail &detail : details) {
+      PrintSeverity(stream, detail.severity);
+      stream << detail.rendered << '\n';
+    }
+    return;
+  }
+
+  // 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,
+      hidden_details;
+  for (const DiagnosticDetail &detail : details) {
+    if (!show_inline || !detail.source_location) {
+      other_details.push_back(detail);
+      continue;
+    }
+    if (detail.source_location->hidden) {
+      hidden_details.push_back(detail);
+      continue;
+    }
+    if (!detail.source_location->in_user_input) {
+      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.
+  bool did_print = false;
+  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';
+    did_print = true;
+  }
+
+  // Print the non-located details.
+  for (const DiagnosticDetail &detail : other_details) {
+    PrintSeverity(stream, detail.severity);
+    stream << detail.rendered << '\n';
+    did_print = true;
+  }
+
+  // Print the hidden details as a last resort.
+  if (!did_print)
+    for (const DiagnosticDetail &detail : hidden_details) {
+      PrintSeverity(stream, detail.severity);
+      stream << detail.rendered << '\n';
+    }
+}
+} // namespace lldb_private
+
 bool CommandObjectExpression::EvaluateExpression(llvm::StringRef expr,
                                                  Stream &output_stream,
                                                  Stream &error_stream,
@@ -486,19 +603,34 @@ 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();
+        // Retrieve the diagnostics.
+        std::vector<DiagnosticDetail> details;
+        llvm::consumeError(llvm::handleErrors(
+            result_valobj_sp->GetError().ToError(),
+            [&](ExpressionError &error) { details = error.GetDetails(); }));
+        // 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();
+
+        if (!details.empty()) {
+          bool show_inline =
+              GetDebugger().GetShowInlineDiagnostics() && !expr.contains('\n');
+          RenderDiagnosticDetails(error_stream, expr_pos, show_inline, details);
         } else {
-          error_stream.PutCString("error: unknown error\n");
+          const char *error_cstr = result_valobj_sp->GetError().AsCString();
+          llvm::StringRef error(error_cstr);
+          if (!error.empty()) {
+            if (!error.starts_with("error:"))
+              error_stream << "error: ";
+            error_stream << error;
+            if (!error.ends_with('\n'))
+              error_stream.EOL();
+          } else {
+            error_stream << "error: unknown error\n";
+          }
         }
-
         result.SetStatus(eReturnStatusFailed);
       }
     }
diff --git a/lldb/source/Core/CoreProperties.td b/lldb/source/Core/CoreProperties.td
index a6cb951..e11aad2 100644
--- a/lldb/source/Core/CoreProperties.td
+++ b/lldb/source/Core/CoreProperties.td
@@ -225,4 +225,8 @@ let Definition = "debugger" in {
     DefaultEnumValue<"eDWIMPrintVerbosityNone">,
     EnumValues<"OptionEnumValues(g_dwim_print_verbosities)">,
     Desc<"The verbosity level used by dwim-print.">;
+  def ShowInlineDiagnostics: Property<"show-inline-diagnostics", "Boolean">,
+    Global,
+    DefaultFalse,
+    Desc<"Controls whether diagnostics can refer directly to the command input, drawing arrows to it. If false, diagnostics will echo the input.">;
 }
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index 9bdc5a3..e6b9eed 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -592,7 +592,18 @@ lldb::DWIMPrintVerbosity Debugger::GetDWIMPrintVerbosity() const {
   const uint32_t idx = ePropertyDWIMPrintVerbosity;
   return GetPropertyAtIndexAs<lldb::DWIMPrintVerbosity>(
       idx, static_cast<lldb::DWIMPrintVerbosity>(
-               g_debugger_properties[idx].default_uint_value));
+               g_debugger_properties[idx].default_uint_value != 0));
+}
+
+bool Debugger::GetShowInlineDiagnostics() const {
+  const uint32_t idx = ePropertyShowInlineDiagnostics;
+  return GetPropertyAtIndexAs<bool>(
+      idx, g_debugger_properties[idx].default_uint_value);
+}
+
+bool Debugger::SetShowInlineDiagnostics(bool b) {
+  const uint32_t idx = ePropertyShowInlineDiagnostics;
+  return SetPropertyAtIndex(idx, b);
 }
 
 #pragma mark Debugger
diff --git a/lldb/source/Expression/DiagnosticManager.cpp b/lldb/source/Expression/DiagnosticManager.cpp
index 3731a20..7c67a0c 100644
--- a/lldb/source/Expression/DiagnosticManager.cpp
+++ b/lldb/source/Expression/DiagnosticManager.cpp
@@ -37,7 +37,7 @@ ExpressionError::ExpressionError(lldb::ExpressionResults result,
     : ErrorInfo(std::error_code(result, expression_category())), m_message(msg),
       m_details(details) {}
 
-static const char *StringForSeverity(lldb::Severity severity) {
+static llvm::StringRef StringForSeverity(lldb::Severity severity) {
   switch (severity) {
   // this should be exhaustive
   case lldb::eSeverityError:
diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp
index acd592c..d17aa6f 100644
--- a/lldb/source/Interpreter/CommandInterpreter.cpp
+++ b/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);
   }
 
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
index 687962c..9b056ea 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -176,15 +176,22 @@ public:
     m_manager = manager;
   }
 
-  /// Returns the last ClangDiagnostic message that the DiagnosticManager
-  /// received or a nullptr if the DiagnosticMangager hasn't seen any
-  /// Clang diagnostics yet.
+  /// Returns the last error ClangDiagnostic message that the
+  /// DiagnosticManager received or a nullptr.
   ClangDiagnostic *MaybeGetLastClangDiag() const {
     if (m_manager->Diagnostics().empty())
       return nullptr;
-    lldb_private::Diagnostic *diag = m_manager->Diagnostics().back().get();
-    ClangDiagnostic *clang_diag = dyn_cast<ClangDiagnostic>(diag);
-    return clang_diag;
+    auto &diags = m_manager->Diagnostics();
+    for (auto it = diags.rbegin(); it != diags.rend(); it++) {
+      lldb_private::Diagnostic *diag = it->get();
+      if (ClangDiagnostic *clang_diag = dyn_cast<ClangDiagnostic>(diag)) {
+        if (clang_diag->GetSeverity() == lldb::eSeverityWarning)
+          return nullptr;
+        if (clang_diag->GetSeverity() == lldb::eSeverityError)
+          return clang_diag;
+      }
+    }
+    return nullptr;
   }
 
   void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
@@ -214,8 +221,6 @@ public:
     m_passthrough->HandleDiagnostic(DiagLevel, Info);
 
     DiagnosticDetail detail;
-    bool make_new_diagnostic = true;
-
     switch (DiagLevel) {
     case DiagnosticsEngine::Level::Fatal:
     case DiagnosticsEngine::Level::Error:
@@ -229,9 +234,6 @@ public:
       detail.severity = lldb::eSeverityInfo;
       break;
     case DiagnosticsEngine::Level::Note:
-      m_manager->AppendMessageToDiagnostic(m_output);
-      make_new_diagnostic = false;
-
       // 'note:' diagnostics for errors and warnings can also contain Fix-Its.
       // We add these Fix-Its to the last error diagnostic to make sure
       // that we later have all Fix-Its related to an 'error' diagnostic when
@@ -249,7 +251,6 @@ public:
       AddAllFixIts(clang_diag, Info);
       break;
     }
-    if (make_new_diagnostic) {
       // ClangDiagnostic messages are expected to have no whitespace/newlines
       // around them.
       std::string stripped_output =
@@ -269,6 +270,7 @@ public:
           loc.line = fsloc.getSpellingLineNumber();
           loc.column = fsloc.getSpellingColumnNumber();
           loc.in_user_input = filename == m_filename;
+          loc.hidden = filename.starts_with("<lldb wrapper ");
 
           // Find the range of the primary location.
           for (const auto &range : Info.getRanges()) {
@@ -298,7 +300,6 @@ public:
         AddAllFixIts(new_diagnostic.get(), Info);
 
       m_manager->AddDiagnostic(std::move(new_diagnostic));
-    }
   }
 
   void BeginSourceFile(const LangOptions &LO, const Preprocessor *PP) override {
diff --git a/lldb/source/Plugins/TraceExporter/ctf/CommandObjectThreadTraceExportCTF.h b/lldb/source/Plugins/TraceExporter/ctf/CommandObjectThreadTraceExportCTF.h
index 1a034e8..06834edf 100644
--- a/lldb/source/Plugins/TraceExporter/ctf/CommandObjectThreadTraceExportCTF.h
+++ b/lldb/source/Plugins/TraceExporter/ctf/CommandObjectThreadTraceExportCTF.h
@@ -48,7 +48,7 @@ public:
   Options *GetOptions() override { return &m_options; }
 
 protected:
-  void DoExecute(Args &command, CommandReturnObject &result) override;
+  void DoExecute(Args &args, CommandReturnObject &result) override;
 
   CommandOptions m_options;
 };