[lldb] Show fix-it applied even if expression didn't evaluate succesfully

[augusto2112]

Summary

If we applied a fix-it before evaluating an expression and that
expression didn't evaluate correctly, we should still tell users about
the fix-it we applied since that may be the reason why it didn't work
correctly.

[teemperor]

Could you add a test for that? The usual test fixit in Clang is ./-> mixup, e.g.

(lldb) expr struct Foo { int i; }; Foo *f; f.i ; unknown_identifier_for_fatal_error
(int) $0 = 0
  Fix-it applied, fixed expression was: 
    struct Foo { int i; }; Foo *f; f->i ; unknown_identifier_for_fatal_error
...

I think this patch also needs to update UserExpression::Evaluate which, IIRC, is clearing the fixed expression when it fails to parse.

The direction of the patch is IMHO fine. One thing that would be nice is to see if we could maybe preserve the Fix-It diagnostics from the original expression. If we just return whatever the fixed expression fails with then it's not clear why the compiler actually generated a Fix-It. But that's more of a nice-to-have I think.

[augusto2112]

Add test

[augusto2112]

In D109908#3004789, @teemperor wrote:

Could you add a test for that? The usual test fixit in Clang is ./-> mixup, e.g.

(lldb) expr struct Foo { int i; }; Foo *f; f.i ; unknown_identifier_for_fatal_error
(int) $0 = 0
  Fix-it applied, fixed expression was: 
    struct Foo { int i; }; Foo *f; f->i ; unknown_identifier_for_fatal_error
...

Added!

I think this patch also needs to update UserExpression::Evaluate which, IIRC, is clearing the fixed expression when it fails to parse.

I think that makes sense... If we fail to parse the fix-it we silently ignore it and don't need to tell the user about it.

The direction of the patch is IMHO fine. One thing that would be nice is to see if we could maybe preserve the Fix-It diagnostics from the original expression.

What do you mean by this? If we could preserve the original expression.

If we just return whatever the fixed expression fails with then it's not clear why the compiler actually generated a Fix-It. But that's more of a nice-to-have I think.

You can see in the test I added, fixing foo.bar to foo->bar where foo won't execute correctly, but it's still nice for the user to see that the Fix-it was applied.

[teemperor]

My bad, I thought you wanted to print the fix-its if the 'fixed' expression failed to *parse* again, but this is about non-parsing errors. Ignore my other points, they were suggestions how to implemented what I thought this is about.

LGTM now. Just have some nits about the test but those don't need another round of review. Thanks for fixing this!

lldb/test/API/commands/expression/fixits/TestFixIts.py

86

Trailing white space here.

96

I think you meant ret_val.GetOutput() here as message gets printed if result != eReturnStatusFailed?

I anyway think you can just minimize everything from line 94 onwards with the normal expect routine:

self.expect("expression -- null_pointer.first", error=True, substrs=[
    "Fix-it applied, fixed expression was:", "null_pointer->first"])

lldb/source/Commands/CommandObjectExpression.cpp

bool CommandObjectExpression::EvaluateExpression(llvm::StringRef expr,
 
   const EvaluateExpressionOptions options = GetEvalOptions(target);
   ExpressionResults success = target.EvaluateExpression(
       expr, frame, result_valobj_sp, options, &m_fixed_expression);
 
   // We only tell you about the FixIt if we applied it.  The compiler errors
   // will suggest the FixIt if it parsed.
   if (!m_fixed_expression.empty() && target.GetEnableNotifyAboutFixIts()) {
-    if (success == eExpressionCompleted)
-      error_stream.Printf("  Fix-it applied, fixed expression was: \n    %s\n",
-                          m_fixed_expression.c_str());
   }
 
   if (result_valobj_sp) {
     Format format = m_format_options.GetFormat();
 
     if (result_valobj_sp->GetError().Success()) {
       if (format != eFormatVoid) {
         if (format != eFormatDefault)

lldb/test/API/commands/expression/fixits/TestFixIts.py

def test_with_target(self):
         error_string = value.GetError().GetCString()
         self.assertTrue(
             error_string.find("fixed expression suggested:") != -1,
             "Fix was suggested")
         self.assertTrue(
             error_string.find("my_pointer->second.a") != -1,
             "Fix was right")
 
+    def test_with_target_error_applies_fixit(self):
+        """ Check that applying a Fix-it which fails to execute correctly still 
+         prints that the Fix-it was applied. """
+        self.build()
+        (target, process, self.thread, bkpt) = lldbutil.run_to_source_breakpoint(self,
+                                        'Stop here to evaluate expressions',
+                                         lldb.SBFileSpec("main.cpp"))
+        # Enable fix-its as they were intentionally disabled by TestBase.setUp.
+        self.runCmd("settings set target.auto-apply-fixits true")
+        ret_val = lldb.SBCommandReturnObject()
+        result = self.dbg.GetCommandInterpreter().HandleCommand("expression null_pointer.first", ret_val)
+        self.assertEqual(result, lldb.eReturnStatusFailed, ret_val.GetError())
+
+        self.assertIn("Fix-it applied, fixed expression was:", ret_val.GetError())
+        self.assertIn("null_pointer->first", ret_val.GetError())
+
     # The final function call runs into SIGILL on aarch64-linux.
     @expectedFailureAll(archs=["aarch64"], oslist=["freebsd", "linux"],
                         bugnumber="llvm.org/pr49407")
     def test_with_multiple_retries(self):
         """Test calling expressions with errors that can be fixed by the FixIts."""
         self.build()
         (target, process, self.thread, bkpt) = lldbutil.run_to_source_breakpoint(self,
                                         'Stop here to evaluate expressions',

lldb/test/API/commands/expression/fixits/main.cpp

struct MyStruct
   struct SubStruct second;
 };
 
 int
 main()
 {
   struct MyStruct my_struct = {10, {20, 30}};
   struct MyStruct *my_pointer = &my_struct;
+  struct MyStruct *null_pointer = nullptr;
   printf ("Stop here to evaluate expressions: %d %d %p\n", my_pointer->first, my_pointer->second.a, my_pointer);
   return 0;
 }