[lldb] Add option to retry Fix-Its multiple times to failed expressions

Summary:
Usually when Clang emits an error Fix-It it does two things. It emits the diagnostic and then it fixes the
currently generated AST to reflect the applied Fix-It. While emitting the diagnostic is easy to implement,
fixing the currently generated AST is often tricky. That causes that some Fix-Its just keep the AST as-is or
abort the parsing process entirely. Once the parser stopped, any Fix-Its for the rest of the expression are
not detected and when the user manually applies the Fix-It, the next expression will just produce a new
Fix-It.

This is often occurring with quickly made Fix-Its that are just used to bridge temporary API changes
and that often are not worth implementing a proper API fixup in addition to the diagnostic. To still
give some kind of reasonable user-experience for users that have these Fix-Its and rely on them to
fix their expressions, this patch adds the ability to retry parsing with applied Fix-Its multiple time to
give the normal Fix-It experience where things Clang knows how to fix are not causing actual expression
error (at least when automatically applying Fix-Its is activated).

The way this is implemented is just by having another setting in the expression options that specify how
often we should try applying Fix-Its and then reparse the expression. The default setting is still 1 for everyone
so this should not affect the speed in which we fail to parse expressions.

Reviewers: jingham, JDevlieghere, friss, shafik

Reviewed By: shafik

Subscribers: shafik, abidh

Differential Revision: https://reviews.llvm.org/D77214

lldb/bindings/interface/SBExpressionOptions.i

@@ -126,6 +126,14 @@ public:
     bool
     GetAutoApplyFixIts();
 
+    %feature("docstring", "Sets how often LLDB should retry applying fix-its to an expression.") SetRetriesWithFixIts;
+    void
+    SetRetriesWithFixIts(uint64_t retries);
+
+    %feature("docstring", "Gets how often LLDB will retry applying fix-its to an expression.") GetRetriesWithFixIts;
+    uint64_t
+    GetRetriesWithFixIts();
+
     bool
     GetTopLevel();
 

lldb/include/lldb/API/SBExpressionOptions.h

@@ -86,6 +86,10 @@ public:
 
   bool GetAutoApplyFixIts();
 
+  void SetRetriesWithFixIts(uint64_t retries);
+
+  uint64_t GetRetriesWithFixIts();
+
   bool GetTopLevel();
 
   void SetTopLevel(bool b = true);

lldb/include/lldb/Target/Target.h

@@ -135,6 +135,8 @@ public:
 
   bool GetEnableAutoApplyFixIts() const;
 
+  uint64_t GetNumberOfRetriesWithFixits() const;
+
   bool GetEnableNotifyAboutFixIts() const;
 
   bool GetEnableSaveObjects() const;
@@ -385,6 +387,12 @@ public:
 
   bool GetAutoApplyFixIts() const { return m_auto_apply_fixits; }
 
+  void SetRetriesWithFixIts(uint64_t number_of_retries) {
+    m_retries_with_fixits = number_of_retries;
+  }
+
+  uint64_t GetRetriesWithFixIts() const { return m_retries_with_fixits; }
+
   bool IsForUtilityExpr() const { return m_running_utility_expression; }
 
   void SetIsForUtilityExpr(bool b) { m_running_utility_expression = b; }
@@ -406,6 +414,7 @@ private:
   bool m_ansi_color_errors = false;
   bool m_result_is_internal = false;
   bool m_auto_apply_fixits = true;
+  uint64_t m_retries_with_fixits = 1;
   /// True if the executed code should be treated as utility code that is only
   /// used by LLDB internally.
   bool m_running_utility_expression = false;

lldb/source/API/SBExpressionOptions.cpp

@@ -237,6 +237,20 @@ void SBExpressionOptions::SetAutoApplyFixIts(bool b) {
   return m_opaque_up->SetAutoApplyFixIts(b);
 }
 
+uint64_t SBExpressionOptions::GetRetriesWithFixIts() {
+  LLDB_RECORD_METHOD_NO_ARGS(uint64_t, SBExpressionOptions,
+                             GetRetriesWithFixIts);
+
+  return m_opaque_up->GetRetriesWithFixIts();
+}
+
+void SBExpressionOptions::SetRetriesWithFixIts(uint64_t retries) {
+  LLDB_RECORD_METHOD(void, SBExpressionOptions, SetRetriesWithFixIts,
+                     (uint64_t), retries);
+
+  return m_opaque_up->SetRetriesWithFixIts(retries);
+}
+
 bool SBExpressionOptions::GetTopLevel() {
   LLDB_RECORD_METHOD_NO_ARGS(bool, SBExpressionOptions, GetTopLevel);
 

lldb/source/Commands/CommandObjectExpression.cpp

@@ -385,6 +385,7 @@ CommandObjectExpression::GetEvalOptions(const Target &target) {
     auto_apply_fixits = m_command_options.auto_apply_fixits == eLazyBoolYes;
 
   options.SetAutoApplyFixIts(auto_apply_fixits);
+  options.SetRetriesWithFixIts(target.GetNumberOfRetriesWithFixits());
 
   if (m_command_options.top_level)
     options.SetExecutionPolicy(eExecutionPolicyTopLevel);

lldb/source/Expression/UserExpression.cpp

@@ -266,22 +266,33 @@ UserExpression::Evaluate(ExecutionContext &exe_ctx,
     execution_results = lldb::eExpressionParseError;
     if (fixed_expression && !fixed_expression->empty() &&
         options.GetAutoApplyFixIts()) {
-      lldb::UserExpressionSP fixed_expression_sp(
-          target->GetUserExpressionForLanguage(fixed_expression->c_str(),
-                                               full_prefix, language,
-                                               desired_type, options, ctx_obj,
-                                               error));
-      DiagnosticManager fixed_diagnostic_manager;
-      parse_success = fixed_expression_sp->Parse(
-          fixed_diagnostic_manager, exe_ctx, execution_policy,
-          keep_expression_in_memory, generate_debug_info);
-      if (parse_success) {
-        diagnostic_manager.Clear();
-        user_expression_sp = fixed_expression_sp;
-      } else {
-        // If the fixed expression failed to parse, don't tell the user about,
-        // that won't help.
-        fixed_expression->clear();
+      const uint64_t max_fix_retries = options.GetRetriesWithFixIts();
+      for (uint64_t i = 0; i < max_fix_retries; ++i) {
+        // Try parsing the fixed expression.
+        lldb::UserExpressionSP fixed_expression_sp(
+            target->GetUserExpressionForLanguage(
+                fixed_expression->c_str(), full_prefix, language, desired_type,
+                options, ctx_obj, error));
+        DiagnosticManager fixed_diagnostic_manager;
+        parse_success = fixed_expression_sp->Parse(
+            fixed_diagnostic_manager, exe_ctx, execution_policy,
+            keep_expression_in_memory, generate_debug_info);
+        if (parse_success) {
+          diagnostic_manager.Clear();
+          user_expression_sp = fixed_expression_sp;
+          break;
+        } else {
+          // The fixed expression also didn't parse. Let's check for any new
+          // Fix-Its we could try.
+          if (fixed_expression_sp->GetFixedText()) {
+            *fixed_expression = fixed_expression_sp->GetFixedText();
+          } else {
+            // Fixed expression didn't compile without a fixit, don't retry and
+            // don't tell the user about it.
+            fixed_expression->clear();
+            break;
+          }
+        }
       }
     }
 

lldb/source/Target/Target.cpp

@@ -3740,6 +3740,12 @@ bool TargetProperties::GetEnableAutoApplyFixIts() const {
       nullptr, idx, g_target_properties[idx].default_uint_value != 0);
 }
 
+uint64_t TargetProperties::GetNumberOfRetriesWithFixits() const {
+  const uint32_t idx = ePropertyRetriesWithFixIts;
+  return m_collection_sp->GetPropertyAtIndexAsUInt64(
+      nullptr, idx, g_target_properties[idx].default_uint_value);
+}
+
 bool TargetProperties::GetEnableNotifyAboutFixIts() const {
   const uint32_t idx = ePropertyNotifyAboutFixIts;
   return m_collection_sp->GetPropertyAtIndexAsBoolean(

lldb/source/Target/TargetProperties.td

@@ -55,6 +55,9 @@ let Definition = "target" in {
   def AutoApplyFixIts: Property<"auto-apply-fixits", "Boolean">,
     DefaultTrue,
     Desc<"Automatically apply fix-it hints to expressions.">;
+  def RetriesWithFixIts: Property<"retries-with-fixits", "UInt64">,
+    DefaultUnsignedValue<1>,
+    Desc<"Maximum number of attempts to fix an expression with Fix-Its">;
   def NotifyAboutFixIts: Property<"notify-about-fixits", "Boolean">,
     DefaultTrue,
     Desc<"Print the fixed expression text.">;

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

@@ -81,3 +81,71 @@ class ExprCommandWithFixits(TestBase):
         self.assertTrue(
             error_string.find("my_pointer->second.a") != -1,
             "Fix was right")
+
+
+        # Test repeatedly applying Fix-Its to expressions and reparsing them.
+        multiple_runs_options = lldb.SBExpressionOptions()
+        multiple_runs_options.SetAutoApplyFixIts(True)
+        multiple_runs_options.SetTopLevel(True)
+
+        # An expression that needs two parse attempts with one Fix-It each
+        # to be successfully parsed.
+        two_runs_expr = """
+        struct Data { int m; };
+
+        template<typename T>
+        struct S1 : public T {
+          using T::TypeDef;
+          int f() {
+            Data d;
+            d.m = 123;
+            // The first error as the using above requires a 'typename '.
+            // Will trigger a Fix-It that puts 'typename' in the right place.
+            typename S1<T>::TypeDef i = &d;
+            // i has the type "Data *", so this should be i.m.
+            // The second run will change the . to -> via the Fix-It.
+            return i.m;
+          }
+        };
+
+        struct ClassWithTypeDef {
+          typedef Data *TypeDef;
+        };
+
+        int test_X(int i) {
+          S1<ClassWithTypeDef> s1;
+          return s1.f();
+        }
+        """
+
+        # Disable retries which will fail.
+        multiple_runs_options.SetRetriesWithFixIts(0)
+        value = frame.EvaluateExpression(two_runs_expr, multiple_runs_options)
+        self.assertIn("expression failed to parse, fixed expression suggested:",
+                      value.GetError().GetCString())
+        self.assertIn("using typename T::TypeDef",
+                      value.GetError().GetCString())
+        # The second Fix-It shouldn't be suggested here as Clang should have
+        # aborted the parsing process.
+        self.assertNotIn("i->m",
+                      value.GetError().GetCString())
+
+        # Retry once, but the expression needs two retries.
+        multiple_runs_options.SetRetriesWithFixIts(1)
+        value = frame.EvaluateExpression(two_runs_expr, multiple_runs_options)
+        self.assertIn("expression failed to parse, fixed expression suggested:",
+                      value.GetError().GetCString())
+        # Both our fixed expressions should be in the suggested expression.
+        self.assertIn("using typename T::TypeDef",
+                      value.GetError().GetCString())
+        self.assertIn("i->m",
+                      value.GetError().GetCString())
+
+        # Retry twice, which will get the expression working.
+        multiple_runs_options.SetRetriesWithFixIts(2)
+        value = frame.EvaluateExpression(two_runs_expr, multiple_runs_options)
+        # This error signals success for top level expressions.
+        self.assertEquals(value.GetError().GetCString(), "error: No value")
+
+        # Test that the code above compiles to the right thing.
+        self.expect_expr("test_X(1)", result_type="int", result_value="123")