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

This patch is a reworking of Pete Lawrence's (@PortalPete) proposal
for better expression evaluator error messages:
llvm/llvm-project#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 llvm/llvm-project#106442.

Author: adrian-prantl

lldb/include/lldb/API/SBDebugger.h

@@ -304,6 +304,8 @@ class LLDB_API SBDebugger {
 
   bool GetUseColor() const;
 
+  bool SetShowInlineDiagnostics(bool);
+
   bool SetUseSourceCache(bool use_source_cache);
 
   bool GetUseSourceCache() const;

lldb/include/lldb/Core/Debugger.h

@@ -364,6 +364,10 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
 
   const std::string &GetInstanceName() { return m_instance_name; }
 
+  bool GetShowInlineDiagnostics() const;
+
+  bool SetShowInlineDiagnostics(bool);
+
   bool LoadPlugin(const FileSpec &spec, Status &error);
 
   void RunIOHandlers();

lldb/include/lldb/Expression/DiagnosticManager.h

@@ -39,6 +39,7 @@ struct DiagnosticDetail {
     unsigned line = 0;
     uint16_t column = 0;
     uint16_t length = 0;
+    bool hidden = false;
     bool in_user_input = false;
   };
   /// Contains {{}, 1, 3, 3, true} in the example above.

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/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);
 

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);
       }
     }

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.">;
 }

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

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:

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/ExpressionParser/Clang/ClangExpressionParser.cpp

@@ -176,15 +176,22 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer {
     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 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer {
     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 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer {
       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 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer {
       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 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer {
           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 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer {
         AddAllFixIts(new_diagnostic.get(), Info);
 
       m_manager->AddDiagnostic(std::move(new_diagnostic));
-    }
   }
 
   void BeginSourceFile(const LangOptions &LO, const Preprocessor *PP) override {

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;
 };