Support inline diagnostics in CommandReturnObject

adrian-prantl · adrian-prantl · commit 5eff438dfba2 · 2024-10-04T09:35:54.000-07:00
and implement them for dwim-print (a.k.a. `p`) as an example.

The next step will be to expose them as structured data in
SBCommandReturnObject.
diff --git a/lldb/include/lldb/Expression/DiagnosticManager.h b/lldb/include/lldb/Expression/DiagnosticManager.h
@@ -23,35 +23,6 @@
 
 namespace lldb_private {
 
-/// A compiler-independent representation of a Diagnostic. Expression
-/// evaluation failures often have more than one diagnostic that a UI
-/// layer might want to render differently, for example to colorize
-/// it.
-///
-/// Running example:
-///   (lldb) expr 1+foo
-///   error: <user expression 0>:1:3: use of undeclared identifier 'foo'
-///   1+foo
-///     ^
-struct DiagnosticDetail {
-  struct SourceLocation {
-    FileSpec file;
-    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.
-  std::optional<SourceLocation> source_location;
-  /// Contains eSeverityError in the example above.
-  lldb::Severity severity = lldb::eSeverityInfo;
-  /// Contains "use of undeclared identifier 'x'" in the example above.
-  std::string message;
-  /// Contains the fully rendered error message.
-  std::string rendered;
-};
-
 /// An llvm::Error used to communicate diagnostics in Status. Multiple
 /// diagnostics may be chained in an llvm::ErrorList.
 class ExpressionError
@@ -65,7 +36,9 @@ class ExpressionError
   ExpressionError(lldb::ExpressionResults result, std::string msg,
                   std::vector<DiagnosticDetail> details = {});
   std::string message() const override;
-  llvm::ArrayRef<DiagnosticDetail> GetDetails() const { return m_details; }
+  llvm::ArrayRef<DiagnosticDetail> GetDetails() const override {
+    return m_details;
+  }
   std::error_code convertToErrorCode() const override;
   void log(llvm::raw_ostream &OS) const override;
   std::unique_ptr<CloneableError> Clone() const override;
diff --git a/lldb/include/lldb/Interpreter/CommandReturnObject.h b/lldb/include/lldb/Interpreter/CommandReturnObject.h
@@ -29,19 +29,17 @@ class CommandReturnObject {
 
   ~CommandReturnObject() = default;
 
+  llvm::StringRef GetInlineDiagnosticsData(unsigned indent,
+                                           llvm::StringRef command);
+
   llvm::StringRef GetOutputData() {
     lldb::StreamSP stream_sp(m_out_stream.GetStreamAtIndex(eStreamStringIndex));
     if (stream_sp)
       return std::static_pointer_cast<StreamString>(stream_sp)->GetString();
     return llvm::StringRef();
   }
 
-  llvm::StringRef GetErrorData() {
-    lldb::StreamSP stream_sp(m_err_stream.GetStreamAtIndex(eStreamStringIndex));
-    if (stream_sp)
-      return std::static_pointer_cast<StreamString>(stream_sp)->GetString();
-    return llvm::StringRef();
-  }
+  llvm::StringRef GetErrorData();
 
   Stream &GetOutputStream() {
     // Make sure we at least have our normal string stream output stream
@@ -135,6 +133,8 @@ class CommandReturnObject {
 
   void SetError(llvm::Error error);
 
+  void SetUserInput(llvm::StringRef user_input) { m_user_input = user_input; }
+
   lldb::ReturnStatus GetStatus() const;
 
   void SetStatus(lldb::ReturnStatus status);
@@ -160,6 +160,9 @@ class CommandReturnObject {
 
   StreamTee m_out_stream;
   StreamTee m_err_stream;
+  std::vector<DiagnosticDetail> m_diagnostics;
+  StreamString m_diag_stream;
+  std::string m_user_input;
 
   lldb::ReturnStatus m_status = lldb::eReturnStatusStarted;
 
diff --git a/lldb/include/lldb/Utility/DiagnosticsRendering.h b/lldb/include/lldb/Utility/DiagnosticsRendering.h
@@ -0,0 +1,25 @@
+//===-- DiagnosticsRendering.h ----------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_UTILITY_DIAGNOSTICSRENDERING_H
+#define LLDB_UTILITY_DIAGNOSTICSRENDERING_H
+
+#include "lldb/Utility/Status.h"
+#include "lldb/Utility/Stream.h"
+#include "llvm/Support/WithColor.h"
+
+namespace lldb_private {
+
+llvm::raw_ostream &PrintSeverity(Stream &stream, lldb::Severity severity);
+
+void RenderDiagnosticDetails(Stream &stream,
+                             std::optional<uint16_t> offset_in_command,
+                             bool show_inline,
+                             llvm::ArrayRef<DiagnosticDetail> details);
+} // namespace lldb_private
+#endif
diff --git a/lldb/include/lldb/Utility/Status.h b/lldb/include/lldb/Utility/Status.h
@@ -9,6 +9,7 @@
 #ifndef LLDB_UTILITY_STATUS_H
 #define LLDB_UTILITY_STATUS_H
 
+#include "lldb/Utility/FileSpec.h"
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-enumerations.h"
 #include "llvm/ADT/StringRef.h"
@@ -26,6 +27,48 @@ class raw_ostream;
 
 namespace lldb_private {
 
+/// A compiler-independent representation of an \c
+/// lldb_private::Diagnostic. Expression evaluation failures often
+/// have more than one diagnostic that a UI layer might want to render
+/// differently, for example to colorize it.
+///
+/// Running example:
+///   (lldb) expr 1 + foo
+///   error: <user expression 0>:1:3: use of undeclared identifier 'foo'
+///   1 + foo
+///       ^~~
+struct DiagnosticDetail {
+  /// A source location consisting of a file name and position.
+  struct SourceLocation {
+    /// \c "<user expression 0>" in the example above.
+    FileSpec file;
+    /// \c 1 in the example above.
+    unsigned line = 0;
+    /// \c 5 in the example above.
+    uint16_t column = 0;
+    /// \c 3 in the example above.
+    uint16_t length = 0;
+    /// Whether this source location should be surfaced to the
+    /// user. For example, syntax errors diagnosed in LLDB's
+    /// expression wrapper code have this set to true.
+    bool hidden = false;
+    /// Whether this source location refers to something the user
+    /// typed as part of the command, i.e., if this qualifies for
+    /// inline display, or if the source line would need to be echoed
+    /// again for the message to make sense.
+    bool in_user_input = false;
+  };
+  /// Contains this diagnostic's source location, if applicable.
+  std::optional<SourceLocation> source_location;
+  /// Contains \c eSeverityError in the example above.
+  lldb::Severity severity = lldb::eSeverityInfo;
+  /// Contains "use of undeclared identifier 'foo'" in the example above.
+  std::string message;
+  /// Contains the fully rendered error message, without "error: ",
+  /// but including the source context.
+  std::string rendered;
+};
+
 const char *ExpressionResultAsCString(lldb::ExpressionResults result);
 
 /// Going a bit against the spirit of llvm::Error,
@@ -86,6 +129,7 @@ class ExpressionErrorBase
   ExpressionErrorBase(std::error_code ec, std::string msg = {})
       : ErrorInfo(ec) {}
   lldb::ErrorType GetErrorType() const override;
+  virtual llvm::ArrayRef<DiagnosticDetail> GetDetails() const;
   static char ID;
 };
 
diff --git a/lldb/source/Commands/CommandObjectDWIMPrint.cpp b/lldb/source/Commands/CommandObjectDWIMPrint.cpp
@@ -188,12 +188,14 @@ void CommandObjectDWIMPrint::DoExecute(StringRef command,
     ValueObjectSP valobj_sp;
     std::string fixed_expression;
 
+    result.SetUserInput(expr);
     ExpressionResults expr_result = target.EvaluateExpression(
         expr, exe_scope, valobj_sp, eval_options, &fixed_expression);
 
     // Only mention Fix-Its if the expression evaluator applied them.
     // Compiler errors refer to the final expression after applying Fix-It(s).
     if (!fixed_expression.empty() && target.GetEnableNotifyAboutFixIts()) {
+      result.SetUserInput(fixed_expression);
       Stream &error_stream = result.GetErrorStream();
       error_stream << "  Evaluated this expression after applying Fix-It(s):\n";
       error_stream << "    " << fixed_expression << "\n";
diff --git a/lldb/source/Commands/CommandObjectExpression.cpp b/lldb/source/Commands/CommandObjectExpression.cpp
@@ -7,9 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "CommandObjectExpression.h"
-#include "DiagnosticRendering.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"
@@ -22,6 +20,7 @@
 #include "lldb/Target/Process.h"
 #include "lldb/Target/StackFrame.h"
 #include "lldb/Target/Target.h"
+#include "lldb/Utility/DiagnosticsRendering.h"
 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-private-enumerations.h"
 
@@ -490,7 +489,7 @@ bool CommandObjectExpression::EvaluateExpression(llvm::StringRef expr,
         std::vector<DiagnosticDetail> details;
         llvm::consumeError(llvm::handleErrors(
             result_valobj_sp->GetError().ToError(),
-            [&](ExpressionError &error) { details = error.GetDetails(); }));
+            [&](ExpressionErrorBase &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);
diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp
@@ -2069,7 +2069,7 @@ bool CommandInterpreter::HandleCommand(const char *command_line,
         remainder.c_str());
 
     // To test whether or not transcript should be saved, `transcript_item` is
-    // used instead of `GetSaveTrasncript()`. This is because the latter will
+    // used instead of `GetSaveTranscript()`. This is because the latter will
     // fail when the command is "settings set interpreter.save-transcript true".
     if (transcript_item) {
       transcript_item->AddStringItem("commandName", cmd_obj->GetCommandName());
@@ -3180,15 +3180,24 @@ void CommandInterpreter::IOHandlerInputComplete(IOHandler &io_handler,
   if ((result.Succeeded() &&
        io_handler.GetFlags().Test(eHandleCommandFlagPrintResult)) ||
       io_handler.GetFlags().Test(eHandleCommandFlagPrintErrors)) {
-    // Display any STDOUT/STDERR _prior_ to emitting the command result text
     GetProcessOutput();
 
+    // Display any inline diagnostics first.
+    if (!result.GetImmediateErrorStream() &&
+        GetDebugger().GetShowInlineDiagnostics() &&
+        line.find('\n') == std::string::npos) {
+      unsigned indent = GetDebugger().GetPrompt().size();
+      llvm::StringRef diags = result.GetInlineDiagnosticsData(indent, line);
+      PrintCommandOutput(io_handler, diags, true);
+    }
+
+    // Display any STDOUT/STDERR _prior_ to emitting the command result text.
     if (!result.GetImmediateOutputStream()) {
       llvm::StringRef output = result.GetOutputData();
       PrintCommandOutput(io_handler, output, true);
     }
 
-    // Now emit the command error text from the command we just executed
+    // Now emit the command error text from the command we just executed.
     if (!result.GetImmediateErrorStream()) {
       llvm::StringRef error = result.GetErrorData();
       PrintCommandOutput(io_handler, error, false);
diff --git a/lldb/source/Interpreter/CommandReturnObject.cpp b/lldb/source/Interpreter/CommandReturnObject.cpp
@@ -8,6 +8,7 @@
 
 #include "lldb/Interpreter/CommandReturnObject.h"
 
+#include "lldb/Utility/DiagnosticsRendering.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/StreamString.h"
 
@@ -112,8 +113,46 @@ void CommandReturnObject::SetError(Status error) {
 }
 
 void CommandReturnObject::SetError(llvm::Error error) {
-  if (error)
+  // Retrieve any diagnostics.
+  error = llvm::handleErrors(std::move(error), [&](ExpressionErrorBase &error) {
+    SetStatus(eReturnStatusFailed);
+    m_diagnostics = error.GetDetails();
+  });
+  if (error) {
     AppendError(llvm::toString(std::move(error)));
+  }
+}
+
+llvm::StringRef
+CommandReturnObject::GetInlineDiagnosticsData(unsigned indent,
+                                              llvm::StringRef command) {
+  size_t nchar = command.find(m_user_input);
+  if (nchar == llvm::StringRef::npos)
+    return {};
+
+  indent += nchar;
+  RenderDiagnosticDetails(m_diag_stream, indent, true, m_diagnostics);
+  // Duplex the diagnostics to the secondary stream (but not inlined).
+  if (auto stream_sp = m_err_stream.GetStreamAtIndex(eStreamStringIndex))
+    RenderDiagnosticDetails(*stream_sp, std::nullopt, false, m_diagnostics);
+
+  // Clear them so GetErrorData() doesn't render them again.
+  m_diagnostics.clear();
+  return m_diag_stream.GetString();
+}
+
+llvm::StringRef CommandReturnObject::GetErrorData() {
+  // Diagnostics haven't been fetched; render them now (not inlined).
+  if (!m_diagnostics.empty()) {
+    RenderDiagnosticDetails(GetErrorStream(), std::nullopt, false,
+                            m_diagnostics);
+    m_diagnostics.clear();
+  }
+
+  lldb::StreamSP stream_sp(m_err_stream.GetStreamAtIndex(eStreamStringIndex));
+  if (stream_sp)
+    return std::static_pointer_cast<StreamString>(stream_sp)->GetString();
+  return llvm::StringRef();
 }
 
 // Similar to AppendError, but do not prepend 'Status: ' to message, and don't
diff --git a/lldb/source/Utility/CMakeLists.txt b/lldb/source/Utility/CMakeLists.txt
@@ -38,6 +38,7 @@ add_lldb_library(lldbUtility NO_INTERNAL_DEPENDENCIES
   DataEncoder.cpp
   DataExtractor.cpp
   Diagnostics.cpp
+  DiagnosticsRendering.cpp
   Environment.cpp
   ErrorMessages.cpp
   Event.cpp
diff --git a/lldb/source/Utility/DiagnosticsRendering.cpp b/lldb/source/Utility/DiagnosticsRendering.cpp
@@ -1,22 +1,19 @@
-//===-- DiagnosticRendering.h -----------------------------------*- C++ -*-===//
+//===-- DiagnosticsRendering.cpp ------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
-#ifndef LLDB_SOURCE_COMMANDS_DIAGNOSTICRENDERING_H
-#define LLDB_SOURCE_COMMANDS_DIAGNOSTICRENDERING_H
+#include "lldb/Utility/DiagnosticsRendering.h"
 
-#include "lldb/Expression/DiagnosticManager.h"
-#include "lldb/Utility/Stream.h"
-#include "llvm/Support/WithColor.h"
+using namespace lldb_private;
+using namespace lldb;
 
 namespace lldb_private {
 
-static llvm::raw_ostream &PrintSeverity(Stream &stream,
-                                        lldb::Severity severity) {
+llvm::raw_ostream &PrintSeverity(Stream &stream, lldb::Severity severity) {
   llvm::HighlightColor color;
   llvm::StringRef text;
   switch (severity) {
@@ -36,12 +33,11 @@ static llvm::raw_ostream &PrintSeverity(Stream &stream,
   return llvm::WithColor(stream.AsRawOstream(), color, llvm::ColorMode::Enable)
          << text;
 }
-  
-// Public for unittesting.
-static void RenderDiagnosticDetails(Stream &stream,
-                                    std::optional<uint16_t> offset_in_command,
-                                    bool show_inline,
-                                    llvm::ArrayRef<DiagnosticDetail> details) {
+
+void RenderDiagnosticDetails(Stream &stream,
+                             std::optional<uint16_t> offset_in_command,
+                             bool show_inline,
+                             llvm::ArrayRef<DiagnosticDetail> details) {
   if (details.empty())
     return;
 
@@ -130,4 +126,3 @@ static void RenderDiagnosticDetails(Stream &stream,
 }
 
 } // namespace lldb_private
-#endif
diff --git a/lldb/test/Shell/Commands/command-dwim-print.test b/lldb/test/Shell/Commands/command-dwim-print.test
@@ -0,0 +1,16 @@
+# RUN: echo quit | %lldb -o "dwim-print a" \
+# RUN:   | FileCheck %s --strict-whitespace --check-prefix=CHECK1
+#            (lldb) dwim-print a 
+# CHECK1:{{^                  \^}}
+# CHECK1: {{^                  ╰─ error: use of undeclared identifier 'a'}}
+# RUN: echo quit | %lldb -o "p a" \
+# RUN:   | FileCheck %s --strict-whitespace --check-prefix=CHECK2
+#            (lldb) p a 
+# CHECK2:{{^         \^}}
+# RUN: echo quit | %lldb -o "dwim-print -- a" \
+# RUN:   | FileCheck %s --strict-whitespace --check-prefix=CHECK3
+#            (lldb) dwim-print -- a 
+# CHECK3:{{^                     \^}}
+# RUN: echo quit | %lldb -o "settings set show-inline-diagnostics false" \
+# RUN:   -o "dwim-print a" 2>&1 | FileCheck %s --check-prefix=CHECK4
+# CHECK4: error: <user expression 0>:1:1: use of undeclared identifier
diff --git a/lldb/unittests/Interpreter/CMakeLists.txt b/lldb/unittests/Interpreter/CMakeLists.txt
@@ -1,6 +1,5 @@
 add_lldb_unittest(InterpreterTests
   TestCommandPaths.cpp
-  TestCommandObjectExpression.cpp
   TestCompletion.cpp
   TestOptionArgParser.cpp
   TestOptions.cpp
diff --git a/lldb/unittests/Utility/CMakeLists.txt b/lldb/unittests/Utility/CMakeLists.txt
diff --git a/lldb/unittests/Utility/DiagnosticsRenderingTest.cpp b/lldb/unittests/Utility/DiagnosticsRenderingTest.cpp
