[lldb] Print hint if object description is requested but not implemented

[augusto2112]

Summary

Lots of users use "po" as their default print command. If the type
doesn't implement the description function the output is often not what
the user wants. Print a hint telling the user that they might prefer
using "p" instead.

[augusto2112]

@kastiglione I wonder if we should hint in all 3 command objects (expr, frame var and DWIM) or if we should limit this to DWIM instead (since DWIM is the one aliased to p and po and I don't think vo or expr -O are often used unintentionally).

[augusto2112]

ping :)

[kastiglione]

lldb/source/Commands/CommandObjectDWIMPrint.cpp

81–83

I wonder if there are users who might ignore this, and then find it annoying to see it repeatedly. Should it be a once per session warning? Once per class? Or controlled by a setting.

157–166

I know it would be less "DRY", but it also feels weird to me to pass in a bool that controls whether the function does anything. At the callsite, I think the semantics would be more clear as:

if (is_po)
    MaybeAddPoHintAndDump(*valobj_sp.get(), dump_options, language, 
                          result.GetOutputStream());

186–196

lldb/source/Commands/CommandObjectDWIMPrint.h

46–51 ↗

(On Diff #533449)

Should this hint be part of dwim-print only? At this point I see expression as a command you use to do exactly what you ask it to do.

lldb/source/Commands/CommandObjectFrame.cpp

708 ↗

(On Diff #533449)

Why is this line added? If this getter has side effects, we should create an appropriately named function that performs the side effects.

[kastiglione]

I wonder if we should hint in all 3 command objects (expr, frame var and DWIM) or if we should limit this to DWIM instead (since DWIM is the one aliased to p and po and I don't think vo or expr -O are often used unintentionally).

I missed this comment. My instinct is to have only dwim-print produce the hint. It feels more in scope for it.

lldb/source/Commands/CommandObjectDWIMPrint.h

46–51 ↗

(On Diff #533449)

I see you asked this same question, and I had missed it at first.

[augusto2112]

Addressed comments

[augusto2112]

Ping

[kastiglione]

lldb/include/lldb/Target/Target.h

496 ↗

(On Diff #535561)

did you mean to use this, or should it be deleted?

lldb/source/Commands/CommandObjectDWIMPrint.cpp

129–130

question: would people want to be warned once per target, or once per type?

If I do like the warning, I might want to know every time I po a type that doesn't support it.

158–162

what do you think of passing in the result's stream into maybe_add_hint? Perhaps I am overlooking something, but I wonder if it would simplify the code to reuse the one stream, instead of separating and then combining two streams.

[augusto2112]

lldb/include/lldb/Target/Target.h

496 ↗

(On Diff #535561)

Leftover from a different implementation, I'll delete it.

lldb/source/Commands/CommandObjectDWIMPrint.cpp

129–130

In my opinion once per target should be enough, if we do it for every type that could potentially be too noisy, and hopefully after seeing the hint a few times users will start remembering to try p too.

158–162

I need the two streams to print it in the correct order (hint first, result later)

[kastiglione]

lldb/source/Commands/CommandObjectDWIMPrint.cpp

133–135

ok, I have a new suggestion. Since lldb will warn only once per target, and not per type, I think this note should be reworded to focus the guidance on the format of the output, not the type.

My concern is lldb emits basically "this type doesn't need a po", but then the diagnostic is printed for only one type, and never tells you about other types. How will people know that other types should use p not po?

If the message were on the format, and not the type, then I think it makes more sense as a once per target message.

A possible rewording:

note: this po used the default object description, which shows none of the objects properties. When you output like this, consider using p instead of po when you see such output.

158–162

do we have a precedent for before vs after? Maybe I need to see some examples, but I think it should be after. My logic is "here's the output you requested, and then here's a note about it". Also the note would be next to the next prompt, so maybe closer to the eyes? Just figured it was worth hashing out.

[augusto2112]

lldb/source/Commands/CommandObjectDWIMPrint.cpp

133–135

Personally I find the new message a bit more confusing for users to understand. Maybe:

note: object description requested, but type doesn't implement a custom object description. Consider using "p" instead of  "po" (this warning will only be displayed once per debug session).

What do you think?

158–162

DWIM print will add the note beforehand, I don't have strong feelings about this either way though. We'd probably still need 2 streams though, since we only want to match what's added by the value object's Dump, and nothing that may already be on the stream.

[augusto2112]

Addressed comments

lldb/include/lldb/Core/Debugger.h

public:
   llvm::StringRef GetShowProgressAnsiSuffix() const;
 
   bool GetUseAutosuggestion() const;
 
   llvm::StringRef GetAutosuggestionAnsiPrefix() const;
 
   llvm::StringRef GetAutosuggestionAnsiSuffix() const;
 
+  bool GetShowDontUsePoHint() const;
+
   bool GetUseSourceCache() const;
 
   bool SetUseSourceCache(bool use_source_cache);
 
   bool GetHighlightSource() const;
 
   lldb::StopShowColumn GetStopShowColumn() const;
 

lldb/source/Commands/CommandObjectDWIMPrint.cpp

 #include "lldb/Target/StackFrame.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-forward.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/FormatVariadic.h"
 
+#include <regex>
+
 using namespace llvm;
 using namespace lldb;
 using namespace lldb_private;
 
 CommandObjectDWIMPrint::CommandObjectDWIMPrint(CommandInterpreter &interpreter)
     : CommandObjectRaw(interpreter, "dwim-print",
                        "Print a variable or expression.",
                        "dwim-print [<variable-name> | <expression>]",
bool CommandObjectDWIMPrint::DoExecute(StringRef command,
   if (args.HasArgs()) {
     if (!ParseOptionsAndNotify(args.GetArgs(), result, m_option_group,
                                m_exe_ctx))
       return false;
   }
 
   // If the user has not specified, default to disabling persistent results.
   if (m_expr_options.suppress_persistent_result == eLazyBoolCalculate)
     m_expr_options.suppress_persistent_result = eLazyBoolYes;
   bool suppress_result = m_expr_options.ShouldSuppressResult(m_varobj_options);
 
   auto verbosity = GetDebugger().GetDWIMPrintVerbosity();
 
   Target *target_ptr = m_exe_ctx.GetTargetPtr();
   // Fallback to the dummy target, which can allow for expression evaluation.
   Target &target = target_ptr ? *target_ptr : GetDummyTarget();
 
   EvaluateExpressionOptions eval_options =
       m_expr_options.GetEvaluateExpressionOptions(target, m_varobj_options);
   // This command manually removes the result variable, make sure expression
   // evaluation doesn't do it first.
   eval_options.SetSuppressPersistentResult(false);
 
   DumpValueObjectOptions dump_options = m_varobj_options.GetAsDumpOptions(
       m_expr_options.m_verbosity, m_format_options.GetFormat());
   dump_options.SetHideRootName(suppress_result);
 
+  bool is_po = m_varobj_options.use_objc;
+
   StackFrame *frame = m_exe_ctx.GetFramePtr();
 
+  // Either Swift was explicitly specified, or the frame is Swift.
+  lldb::LanguageType language = m_expr_options.language;
+  if (language == lldb::eLanguageTypeUnknown && frame)
+    language = frame->GuessLanguage();
+
+  // Add a hint if object description was requested, but no description
+  // function was implemented.
+  auto maybe_add_hint = [&](llvm::StringRef output) {
+    // Identify the default output of object description for Swift and
+    // Objective-C
+    // "<Name: 0x...>. The regex is:
+    // - Start with "<".
+    // - Followed by 1 or more non-whitespace characters.
+    // - Followed by ": 0x".
+    // - Followed by 5 or more hex digits.
+    // - Followed by ">".
+    // - End with zero or more whitespace characters.
+    const std::regex swift_class_regex("^<\\S+: 0x[[:xdigit:]]{5,}>\\s*$");
+
+    if (GetDebugger().GetShowDontUsePoHint() && target_ptr &&
+        (language == lldb::eLanguageTypeSwift ||
+         language == lldb::eLanguageTypeObjC) &&
+        std::regex_match(output.data(), swift_class_regex)) {
+
+      static bool note_shown = false;
+      if (note_shown)
+        return;
+
+      result.GetOutputStream()
+          << "note: object description requested, but type doesn't implement "
+             "a custom object description. Consider using \"p\" instead of "
+             "\"po\" (this note will only be shown once per debug session).\n";
+      note_shown = true;
+    }
+  };
+
   // First, try `expr` as the name of a frame variable.
   if (frame) {
     auto valobj_sp = frame->FindVariable(ConstString(expr));
     if (valobj_sp && valobj_sp->GetError().Success()) {
       if (!suppress_result) {
         if (auto persisted_valobj = valobj_sp->Persist())
           valobj_sp = persisted_valobj;
       }
 
       if (verbosity == eDWIMPrintVerbosityFull) {
         StringRef flags;
         if (args.HasArgs())
           flags = args.GetArgString();
         result.AppendMessageWithFormatv("note: ran `frame variable {0}{1}`",
                                         flags, expr);
       }
 
+      if (is_po) {
+        StreamString temp_result_stream;
+        valobj_sp->Dump(temp_result_stream, dump_options);
+        llvm::StringRef output = temp_result_stream.GetString();
+        maybe_add_hint(output);
+        result.GetOutputStream() << output;
+      } else {
         valobj_sp->Dump(result.GetOutputStream(), dump_options);
+      }
       result.SetStatus(eReturnStatusSuccessFinishResult);
       return true;
     }
   }
 
   // Second, also lastly, try `expr` as a source expression to evaluate.
   {
     auto *exe_scope = m_exe_ctx.GetBestExecutionContextScope();
     ValueObjectSP valobj_sp;
     ExpressionResults expr_result =
         target.EvaluateExpression(expr, exe_scope, valobj_sp, eval_options);
     if (expr_result == eExpressionCompleted) {
       if (verbosity != eDWIMPrintVerbosityNone) {
         StringRef flags;
         if (args.HasArgs())
           flags = args.GetArgStringWithDelimiter();
         result.AppendMessageWithFormatv("note: ran `expression {0}{1}`", flags,
                                         expr);
       }
 
-      if (valobj_sp->GetError().GetError() != UserExpression::kNoResult)
+      if (valobj_sp->GetError().GetError() != UserExpression::kNoResult) {
+        if (is_po) {
+          StreamString temp_result_stream;
+          valobj_sp->Dump(temp_result_stream, dump_options);
+          llvm::StringRef output = temp_result_stream.GetString();
+          maybe_add_hint(output);
+          result.GetOutputStream() << output;
+        } else {
           valobj_sp->Dump(result.GetOutputStream(), dump_options);
+        }
+      }
 
       if (suppress_result)
         if (auto result_var_sp =
                 target.GetPersistentVariable(valobj_sp->GetName())) {
           auto language = valobj_sp->GetPreferredDisplayLanguage();
           if (auto *persistent_state =
                   target.GetPersistentExpressionStateForLanguage(language))
             persistent_state->RemovePersistentVariable(result_var_sp);

lldb/source/Core/CoreProperties.td

let Definition = "debugger" in {
   def ShowAutosuggestionAnsiPrefix: Property<"show-autosuggestion-ansi-prefix", "String">,
     Global,
     DefaultStringValue<"${ansi.faint}">,
     Desc<"When displaying suggestion in a color-enabled terminal, use the ANSI terminal code specified in this format immediately before the suggestion.">;
   def ShowAutosuggestionAnsiSuffix: Property<"show-autosuggestion-ansi-suffix", "String">,
     Global,
     DefaultStringValue<"${ansi.normal}">,
     Desc<"When displaying suggestion in a color-enabled terminal, use the ANSI terminal code specified in this format immediately after the suggestion.">;
+  def ShowDontUsePoHint: Property<"show-dont-use-po-hint", "Boolean">,
+    Global,
+    DefaultTrue,
+    Desc<"If true, and object description was requested for a type that does not implement it, LLDB will print a hint telling the user to consider using p instead.">;
   def DWIMPrintVerbosity: Property<"dwim-print-verbosity", "Enum">,
     Global,
     DefaultEnumValue<"eDWIMPrintVerbosityNone">,
     EnumValues<"OptionEnumValues(g_dwim_print_verbosities)">,
     Desc<"The verbosity level used by dwim-print.">;
 }

lldb/source/Core/Debugger.cpp

 }
 
 llvm::StringRef Debugger::GetAutosuggestionAnsiSuffix() const {
   const uint32_t idx = ePropertyShowAutosuggestionAnsiSuffix;
   return GetPropertyAtIndexAs<llvm::StringRef>(
       idx, g_debugger_properties[idx].default_cstr_value);
 }
 
+bool Debugger::GetShowDontUsePoHint() const {
+  const uint32_t idx = ePropertyShowDontUsePoHint;  
+  return GetPropertyAtIndexAs<bool>(
+      idx, g_debugger_properties[idx].default_uint_value != 0);
+}
+
 bool Debugger::GetUseSourceCache() const {
   const uint32_t idx = ePropertyUseSourceCache;
   return GetPropertyAtIndexAs<bool>(
       idx, g_debugger_properties[idx].default_uint_value != 0);
 }
 
 bool Debugger::SetUseSourceCache(bool b) {
   const uint32_t idx = ePropertyUseSourceCache;

lldb/test/API/lang/objc/objc-po-hint/Makefile

+OBJC_SOURCES := main.m
+LD_EXTRAS = -lobjc -framework Foundation
+
+include Makefile.rules

lldb/test/API/lang/objc/objc-po-hint/TestObjcPoHint.py

+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestObjcPoHint(TestBase):
+    NO_DEBUG_INFO_TESTCASE = True
+
+    def test_show_po_hint(self):
+        ### Test that the po hint is shown once with the DWIM print command
+        self.build()
+        _, _, _, _ = lldbutil.run_to_source_breakpoint(
+            self, "Set breakpoint here", lldb.SBFileSpec("main.m")
+        )
+        # Make sure the hint is printed the first time
+        self.expect(
+            "dwim-print -O -- foo",
+            substrs=[
+                "note: object description requested, but type doesn't implement "
+                'a custom object description. Consider using "p" instead of "po"',
+                "<Foo: 0x",
+            ],
+        )
+
+        # Make sure it's not printed again.
+        self.expect(
+            "dwim-print -O -- foo",
+            substrs=[
+                "note: object description"
+            ],
+            matching=False,
+        )
+
+    def test_show_po_hint_disabled(self):
+        ### Test that when the setting is disabled the hint is not printed
+        self.build()
+        _, _, _, _ = lldbutil.run_to_source_breakpoint(
+            self, "Set breakpoint here", lldb.SBFileSpec("main.m")
+        )
+        self.runCmd("setting set show-dont-use-po-hint false")
+        # Make sure the hint is printed the first time
+        self.expect(
+            "dwim-print -O -- foo",
+            substrs=[
+                "note: object description"
+            ],
+            matching=False,
+        )

lldb/test/API/lang/objc/objc-po-hint/main.m

+#import <Foundation/Foundation.h>
+
+@interface Foo : NSObject {}
+
+-(id) init;
+
+@end
+
+@implementation Foo
+
+-(id) init
+{
+    return self = [super init];
+}
+@end
+
+int main()
+{
+    Foo *foo = [Foo new];
+    NSLog(@"a"); // Set breakpoint here.
+    return 0;
+}