[lldb] Fix `po` alias by printing fix-its to the console. #68755

PortalPete · 2023-10-10T23:53:52Z

The po alias now matches the behavior of the expression command when the it can apply a Fix-It to an expression.
Modifications

Add has m_fixed_expression to the CommandObjectDWIMPrint class a protected member that stores the post Fix-It expression, just like the CommandObjectExpression class.
Converted messages to present tense.
Add test cases that confirms a Fix-It for a C++ expression for both po and expressions

rdar://115317419

llvmbot · 2023-10-10T23:54:54Z

@llvm/pr-subscribers-lldb

Author: Pete Lawrence (PortalPete)

Changes

The po alias now matches the behavior of the expression command when the it can apply a Fix-It to an expression.
Modifications

Add has m_fixed_expression to the CommandObjectDWIMPrint class a protected member that stores the post Fix-It expression, just like the CommandObjectExpression class.
Converted messages to present tense.
Add test cases that confirms a Fix-It for a C++ expression for both po and expressions

rdar://115317419

Full diff: https://github.com/llvm/llvm-project/pull/68755.diff

9 Files Affected:

(modified) lldb/source/Commands/CommandObjectDWIMPrint.cpp (+13-2)
(modified) lldb/source/Commands/CommandObjectExpression.cpp (+4-4)
(modified) lldb/test/API/commands/expression/fixits/TestFixIts.py (+2-2)
(added) lldb/test/API/lang/cpp/dwim-print-fixit/Makefile (+3)
(added) lldb/test/API/lang/cpp/dwim-print-fixit/TestCppDWIMPrintFixIt.py (+24)
(added) lldb/test/API/lang/cpp/dwim-print-fixit/main.cpp (+5)
(added) lldb/test/API/lang/cpp/expression-fixit/Makefile (+3)
(added) lldb/test/API/lang/cpp/expression-fixit/TestCppExpressionFixIt.py (+24)
(added) lldb/test/API/lang/cpp/expression-fixit/main.cpp (+5)

diff --git a/lldb/source/Commands/CommandObjectDWIMPrint.cpp b/lldb/source/Commands/CommandObjectDWIMPrint.cpp
index 7b168eab9e02d44..bdc17c9cffc779a 100644
--- a/lldb/source/Commands/CommandObjectDWIMPrint.cpp
+++ b/lldb/source/Commands/CommandObjectDWIMPrint.cpp
@@ -172,8 +172,19 @@ bool CommandObjectDWIMPrint::DoExecute(StringRef command,
   {
     auto *exe_scope = m_exe_ctx.GetBestExecutionContextScope();
     ValueObjectSP valobj_sp;
-    ExpressionResults expr_result =
-        target.EvaluateExpression(expr, exe_scope, valobj_sp, eval_options);
+    std::string fixed_expression;
+
+    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()) {
+      Stream &error_stream = result.GetErrorStream();
+      error_stream << "  Evaluated this expression after applying Fix-It(s):\n";
+      error_stream << "    " << fixed_expression << "\n";
+    }
+
     if (expr_result == eExpressionCompleted) {
       if (verbosity != eDWIMPrintVerbosityNone) {
         StringRef flags;
diff --git a/lldb/source/Commands/CommandObjectExpression.cpp b/lldb/source/Commands/CommandObjectExpression.cpp
index e7e6e3820b99133..2834be660abaf53 100644
--- a/lldb/source/Commands/CommandObjectExpression.cpp
+++ b/lldb/source/Commands/CommandObjectExpression.cpp
@@ -439,11 +439,11 @@ bool CommandObjectExpression::EvaluateExpression(llvm::StringRef expr,
   ExpressionResults success = target.EvaluateExpression(
       expr, frame, result_valobj_sp, eval_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.
+  // Only mention Fix-Its if the expression evaluator applied them.
+  // Compiler errors refer to the final expression after applying Fix-It(s).
   if (!m_fixed_expression.empty() && target.GetEnableNotifyAboutFixIts()) {
-    error_stream.Printf("  Fix-it applied, fixed expression was: \n    %s\n",
-                        m_fixed_expression.c_str());
+    error_stream << "  Evaluated this expression after applying Fix-It(s):\n";
+    error_stream << "    " << m_fixed_expression << "\n";
   }
 
   if (result_valobj_sp) {
diff --git a/lldb/test/API/commands/expression/fixits/TestFixIts.py b/lldb/test/API/commands/expression/fixits/TestFixIts.py
index 3bdeb84b4e79726..92a3ae8b943e318 100644
--- a/lldb/test/API/commands/expression/fixits/TestFixIts.py
+++ b/lldb/test/API/commands/expression/fixits/TestFixIts.py
@@ -22,7 +22,7 @@ def test_with_dummy_target(self):
         self.assertEqual(
             result, lldb.eReturnStatusSuccessFinishResult, ret_val.GetError()
         )
-        self.assertIn("Fix-it applied", ret_val.GetError())
+        self.assertIn("Evaluated this expression after applying Fix-It(s):", ret_val.GetError())
 
     def test_with_target(self):
         """Test calling expressions with errors that can be fixed by the FixIts."""
@@ -99,7 +99,7 @@ def test_with_target_error_applies_fixit(self):
         )
         self.assertEqual(result, lldb.eReturnStatusFailed, ret_val.GetError())
 
-        self.assertIn("Fix-it applied, fixed expression was:", ret_val.GetError())
+        self.assertIn("Evaluated this expression after applying Fix-It(s):", ret_val.GetError())
         self.assertIn("null_pointer->first", ret_val.GetError())
 
     # The final function call runs into SIGILL on aarch64-linux.
diff --git a/lldb/test/API/lang/cpp/dwim-print-fixit/Makefile b/lldb/test/API/lang/cpp/dwim-print-fixit/Makefile
new file mode 100644
index 000000000000000..99998b20bcb0502
--- /dev/null
+++ b/lldb/test/API/lang/cpp/dwim-print-fixit/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
diff --git a/lldb/test/API/lang/cpp/dwim-print-fixit/TestCppDWIMPrintFixIt.py b/lldb/test/API/lang/cpp/dwim-print-fixit/TestCppDWIMPrintFixIt.py
new file mode 100644
index 000000000000000..260903f30464caa
--- /dev/null
+++ b/lldb/test/API/lang/cpp/dwim-print-fixit/TestCppDWIMPrintFixIt.py
@@ -0,0 +1,24 @@
+"""
+Tests whether the do-what-I-mean (DWIM) print `po` alias applies FixIts like `expr` does
+"""
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestCase(TestBase):
+    def test_with_run_command(self):
+        """Confirms `po` shows an expression after applying Fix-It(s)."""
+
+        self.build()
+        lldbutil.run_to_source_breakpoint(
+            self, "// break here", lldb.SBFileSpec("main.cpp")
+        )
+
+        self.expect(
+            "dwim-print -O -- class C { int i; void f() { []() { ++i; }(); } }; 42",
+            error = True,
+            substrs=["Evaluated this expression after applying Fix-It(s)",
+                "class C { int i; void f() { [this]() { ++i; }(); } }"],
+        )
diff --git a/lldb/test/API/lang/cpp/dwim-print-fixit/main.cpp b/lldb/test/API/lang/cpp/dwim-print-fixit/main.cpp
new file mode 100644
index 000000000000000..e9cf11d18a6560d
--- /dev/null
+++ b/lldb/test/API/lang/cpp/dwim-print-fixit/main.cpp
@@ -0,0 +1,5 @@
+int main() {
+  long foo = 1234;
+
+  return 0; // break here
+}
diff --git a/lldb/test/API/lang/cpp/expression-fixit/Makefile b/lldb/test/API/lang/cpp/expression-fixit/Makefile
new file mode 100644
index 000000000000000..99998b20bcb0502
--- /dev/null
+++ b/lldb/test/API/lang/cpp/expression-fixit/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
diff --git a/lldb/test/API/lang/cpp/expression-fixit/TestCppExpressionFixIt.py b/lldb/test/API/lang/cpp/expression-fixit/TestCppExpressionFixIt.py
new file mode 100644
index 000000000000000..b6c255c5e004cfc
--- /dev/null
+++ b/lldb/test/API/lang/cpp/expression-fixit/TestCppExpressionFixIt.py
@@ -0,0 +1,24 @@
+"""
+Tests whether the expression command applies FixIts
+"""
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestCase(TestBase):
+    def test_with_run_command(self):
+        """Confirms `expression` shows an expression after applying Fix-It(s)."""
+
+        self.build()
+        lldbutil.run_to_source_breakpoint(
+            self, "// break here", lldb.SBFileSpec("main.cpp")
+        )
+
+        self.expect(
+            "expr class C { int i; void f() { []() { ++i; }(); } }; 42",
+            error = True,
+            substrs=["Evaluated this expression after applying Fix-It(s)",
+                "class C { int i; void f() { [this]() { ++i; }(); } }"],
+        )
diff --git a/lldb/test/API/lang/cpp/expression-fixit/main.cpp b/lldb/test/API/lang/cpp/expression-fixit/main.cpp
new file mode 100644
index 000000000000000..e9cf11d18a6560d
--- /dev/null
+++ b/lldb/test/API/lang/cpp/expression-fixit/main.cpp
@@ -0,0 +1,5 @@
+int main() {
+  long foo = 1234;
+
+  return 0; // break here
+}

github-actions · 2023-10-11T00:05:13Z

✅ With the latest revision this PR passed the Python code formatter.

medismailben · 2023-10-11T06:22:29Z

LGTM overall. I think you can make the test less redundant by using the same source file & Makefile for both tests.

The `po` alias now matches the behavior of the `expression` command when the it can apply a Fix-It to an expression. Modifications - Add has `m_fixed_expression` to the `CommandObjectDWIMPrint` class a `protected` member that stores the post Fix-It expression, just like the `CommandObjectExpression` class. - Converted messages to present tense. - Add test cases that confirms a Fix-It for a C++ expression for both `po` and `expressions` rdar://115317419

PortalPete · 2023-10-12T00:22:06Z

Thanks for this suggestion, @medismailben.

LGTM overall. I think you can make the test less redundant by using the same source file & Makefile for both tests.

I wasn't sure which is better:

Multiple files, each with a single test
Fewer files, each with multiple tests.

Ok, I put the 2 new tests into a single, new file, which makes the file naming a bit simpler and set up for future FixIt tests specific to C++.

medismailben · 2023-10-13T05:13:22Z

Ok, I put the 2 new tests into a single, new file, which makes the file naming a bit simpler and set up for future FixIt tests specific to C++.

This is exactly what I had in mind :) Thanks!

adrian-prantl · 2023-10-13T16:23:19Z

I'm testing this locally now and will then merge

The `po` alias now matches the behavior of the `expression` command when the it can apply a Fix-It to an expression. Modifications - Add has `m_fixed_expression` to the `CommandObjectDWIMPrint` class a `protected` member that stores the post Fix-It expression, just like the `CommandObjectExpression` class. - Converted messages to present tense. - Add test cases that confirms a Fix-It for a C++ expression for both `po` and `expressions` rdar://115317419 (cherry picked from commit 8e2bd05)

PortalPete requested a review from JDevlieghere as a code owner October 10, 2023 23:53

llvmbot added the lldb label Oct 10, 2023

PortalPete force-pushed the dwim-print-fixit branch from ee8e9b6 to 5d48fc3 Compare October 11, 2023 01:50

medismailben approved these changes Oct 11, 2023

View reviewed changes

PortalPete force-pushed the dwim-print-fixit branch from 5d48fc3 to b0dfcb5 Compare October 12, 2023 00:12

PortalPete force-pushed the dwim-print-fixit branch from b0dfcb5 to c32c877 Compare October 12, 2023 00:18

adrian-prantl merged commit 8e2bd05 into llvm:main Oct 13, 2023

madhur13490 mentioned this pull request Oct 20, 2023

Revert commit ba8565fbcb975e2d067ce3ae5a7dbaae4953edd3 madhur13490/llvm-project#3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[lldb] Fix `po` alias by printing fix-its to the console. #68755

[lldb] Fix `po` alias by printing fix-its to the console. #68755

Uh oh!

PortalPete commented Oct 10, 2023

Uh oh!

llvmbot commented Oct 10, 2023

Uh oh!

github-actions bot commented Oct 11, 2023 •

edited

Loading

Uh oh!

medismailben commented Oct 11, 2023

Uh oh!

PortalPete commented Oct 12, 2023 •

edited

Loading

Uh oh!

medismailben commented Oct 13, 2023 •

edited

Loading

Uh oh!

adrian-prantl commented Oct 13, 2023

Uh oh!

Uh oh!

[lldb] Fix po alias by printing fix-its to the console. #68755

[lldb] Fix po alias by printing fix-its to the console. #68755

Uh oh!

Conversation

PortalPete commented Oct 10, 2023

Uh oh!

llvmbot commented Oct 10, 2023

Uh oh!

github-actions bot commented Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

medismailben commented Oct 11, 2023

Uh oh!

PortalPete commented Oct 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

medismailben commented Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adrian-prantl commented Oct 13, 2023

Uh oh!

Uh oh!

[lldb] Fix `po` alias by printing fix-its to the console. #68755

[lldb] Fix `po` alias by printing fix-its to the console. #68755

github-actions bot commented Oct 11, 2023 •

edited

Loading

PortalPete commented Oct 12, 2023 •

edited

Loading

medismailben commented Oct 13, 2023 •

edited

Loading