Author: Raphael Isemann Date: 2020-04-06T11:25:36+02:00 New Revision: 203a8adb65429ec6b8989afdf5956564972b9703
URL: https://github.com/llvm/llvm-project/commit/203a8adb65429ec6b8989afdf5956564972b9703 DIFF: https://github.com/llvm/llvm-project/commit/203a8adb65429ec6b8989afdf5956564972b9703.diff LOG: [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 Added: Modified: lldb/bindings/interface/SBExpressionOptions.i lldb/include/lldb/API/SBExpressionOptions.h lldb/include/lldb/Target/Target.h lldb/source/API/SBExpressionOptions.cpp lldb/source/Commands/CommandObjectExpression.cpp lldb/source/Expression/UserExpression.cpp lldb/source/Target/Target.cpp lldb/source/Target/TargetProperties.td lldb/test/API/commands/expression/fixits/TestFixIts.py Removed: ################################################################################ diff --git a/lldb/bindings/interface/SBExpressionOptions.i b/lldb/bindings/interface/SBExpressionOptions.i index 5dbd7007c01c..33a6ed745a8d 100644 --- a/lldb/bindings/interface/SBExpressionOptions.i +++ b/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(); diff --git a/lldb/include/lldb/API/SBExpressionOptions.h b/lldb/include/lldb/API/SBExpressionOptions.h index 14b52684ddce..9fc6e9ea957e 100644 --- a/lldb/include/lldb/API/SBExpressionOptions.h +++ b/lldb/include/lldb/API/SBExpressionOptions.h @@ -86,6 +86,10 @@ class LLDB_API SBExpressionOptions { bool GetAutoApplyFixIts(); + void SetRetriesWithFixIts(uint64_t retries); + + uint64_t GetRetriesWithFixIts(); + bool GetTopLevel(); void SetTopLevel(bool b = true); diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h index f0a57b8f3827..27997e5ea76d 100644 --- a/lldb/include/lldb/Target/Target.h +++ b/lldb/include/lldb/Target/Target.h @@ -135,6 +135,8 @@ class TargetProperties : public Properties { bool GetEnableAutoApplyFixIts() const; + uint64_t GetNumberOfRetriesWithFixits() const; + bool GetEnableNotifyAboutFixIts() const; bool GetEnableSaveObjects() const; @@ -385,6 +387,12 @@ class EvaluateExpressionOptions { 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 @@ class EvaluateExpressionOptions { 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; diff --git a/lldb/source/API/SBExpressionOptions.cpp b/lldb/source/API/SBExpressionOptions.cpp index 2c92d090a40f..bbe7cba7012c 100644 --- a/lldb/source/API/SBExpressionOptions.cpp +++ b/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); diff --git a/lldb/source/Commands/CommandObjectExpression.cpp b/lldb/source/Commands/CommandObjectExpression.cpp index 9a314c251475..7cec714a0e17 100644 --- a/lldb/source/Commands/CommandObjectExpression.cpp +++ b/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); diff --git a/lldb/source/Expression/UserExpression.cpp b/lldb/source/Expression/UserExpression.cpp index 5bd2321e48dd..47d13f052bfb 100644 --- a/lldb/source/Expression/UserExpression.cpp +++ b/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; + } + } } } diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp index bdfd314695dc..9c61ddeadcc8 100644 --- a/lldb/source/Target/Target.cpp +++ b/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( diff --git a/lldb/source/Target/TargetProperties.td b/lldb/source/Target/TargetProperties.td index fa4f4bdd9bd6..a73f1a3b695c 100644 --- a/lldb/source/Target/TargetProperties.td +++ b/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.">; diff --git a/lldb/test/API/commands/expression/fixits/TestFixIts.py b/lldb/test/API/commands/expression/fixits/TestFixIts.py index 8ccb08ebbaa2..44d3ddd34068 100644 --- a/lldb/test/API/commands/expression/fixits/TestFixIts.py +++ b/lldb/test/API/commands/expression/fixits/TestFixIts.py @@ -81,3 +81,71 @@ def test_with_target(self): 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") _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
