Author: Raphael Isemann Date: 2020-04-06T10:37:33+02:00 New Revision: 3c2dc28d812c917e01f46b3bcf5b5e0a2a803276
URL: https://github.com/llvm/llvm-project/commit/3c2dc28d812c917e01f46b3bcf5b5e0a2a803276 DIFF: https://github.com/llvm/llvm-project/commit/3c2dc28d812c917e01f46b3bcf5b5e0a2a803276.diff LOG: [lldb] Also apply Fix-Its in "note:" diagnostics that belong to an error diagnostic Summary: LLDB currently applies Fix-Its if they are attached to a Clang diagnostic that has the severity "error". Fix-Its connected to warnings and other severities are supposed to be ignored as LLDB doesn't seem to trust Clang Fix-Its in these situations. However, LLDB also ignores all Fix-Its coming from "note:" diagnostics. These diagnostics are usually emitted alongside other diagnostics (both warnings and errors), either to keep a single diagnostic message shorter or because the Fix-It is in a different source line. As they are technically their own (non-error) diagnostics, we currently are ignoring all Fix-Its associated with them. For example, this is a possible Clang diagnostic with a Fix-It that is currently ignored: ``` error: <user expression 1>:2:10: too many arguments provided to function-like macro invocation ToStr(0, {,}) ^ <user expression 1>:1:9: macro 'ToStr' defined here #define ToStr(x) #x ^ <user expression 1>:2:1: cannot use initializer list at the beginning of a macro argument ToStr(0, {,}) ^ ~~~~ ``` We also don't store "note:" diagnostics at all, as LLDB's abstraction around the whole diagnostic concept doesn't have such a concept. The text of "note:" diagnostics is instead appended to the last non-note diagnostic (which is causing that there is no "note:" text in the diagnostic above, as all the "note:" diagnostics have been appended to the first "error: ..." text). This patch fixes the ignored Fix-Its in note-diagnostics by appending them to the last non-note diagnostic, similar to the way we handle the text in these diagnostics. Reviewers: JDevlieghere, jingham Reviewed By: JDevlieghere Subscribers: abidh Differential Revision: https://reviews.llvm.org/D77055 Added: Modified: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp lldb/test/API/commands/expression/fixits/TestFixIts.py Removed: ################################################################################ diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp index e5de4b4651df..9996f2608e31 100644 --- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp +++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp @@ -147,6 +147,14 @@ class ClangExpressionParser::LLDBPreprocessorCallbacks : public PPCallbacks { llvm::StringRef getErrorString() { return m_error_stream.GetString(); } }; +static void AddAllFixIts(ClangDiagnostic *diag, const clang::Diagnostic &Info) { + for (auto &fix_it : Info.getFixItHints()) { + if (fix_it.isNull()) + continue; + diag->AddFixitHint(fix_it); + } +} + class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer { public: ClangDiagnosticManagerAdapter(DiagnosticOptions &opts) { @@ -162,6 +170,17 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer { m_manager = manager; } + /// Returns the last ClangDiagnostic message that the DiagnosticManager + /// received or a nullptr if the DiagnosticMangager hasn't seen any + /// Clang diagnostics yet. + ClangDiagnostic *MaybeGetLastClangDiag() const { + if (m_manager->Diagnostics().empty()) + return nullptr; + lldb_private::Diagnostic *diag = m_manager->Diagnostics().back().get(); + ClangDiagnostic *clang_diag = dyn_cast<ClangDiagnostic>(diag); + return clang_diag; + } + void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel, const clang::Diagnostic &Info) override { if (!m_manager) { @@ -204,6 +223,23 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer { case DiagnosticsEngine::Level::Note: m_manager->AppendMessageToDiagnostic(m_output); make_new_diagnostic = false; + + // 'note:' diagnostics for errors and warnings can also contain Fix-Its. + // We add these Fix-Its to the last error diagnostic to make sure + // that we later have all Fix-Its related to an 'error' diagnostic when + // we apply them to the user expression. + auto *clang_diag = MaybeGetLastClangDiag(); + // If we don't have a previous diagnostic there is nothing to do. + // If the previous diagnostic already has its own Fix-Its, assume that + // the 'note:' Fix-It is just an alternative way to solve the issue and + // ignore these Fix-Its. + if (!clang_diag || clang_diag->HasFixIts()) + break; + // Ignore all Fix-Its that are not associated with an error. + if (clang_diag->GetSeverity() != eDiagnosticSeverityError) + break; + AddAllFixIts(clang_diag, Info); + break; } if (make_new_diagnostic) { // ClangDiagnostic messages are expected to have no whitespace/newlines @@ -218,13 +254,8 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer { // enough context in an expression for the warning to be useful. // FIXME: Should we try to filter out FixIts that apply to our generated // code, and not the user's expression? - if (severity == eDiagnosticSeverityError) { - for (const clang::FixItHint &fixit : Info.getFixItHints()) { - if (fixit.isNull()) - continue; - new_diagnostic->AddFixitHint(fixit); - } - } + if (severity == eDiagnosticSeverityError) + AddAllFixIts(new_diagnostic.get(), Info); m_manager->AddDiagnostic(std::move(new_diagnostic)); } diff --git a/lldb/test/API/commands/expression/fixits/TestFixIts.py b/lldb/test/API/commands/expression/fixits/TestFixIts.py index eb1dd97aa9a9..8ccb08ebbaa2 100644 --- a/lldb/test/API/commands/expression/fixits/TestFixIts.py +++ b/lldb/test/API/commands/expression/fixits/TestFixIts.py @@ -61,6 +61,14 @@ def test_with_target(self): self.assertTrue(value.GetError().Success()) self.assertEquals(value.GetValueAsUnsigned(), 20) + # Try a Fix-It that is stored in the 'note:' diagnostic of an error. + # The Fix-It here is adding parantheses around the ToStr parameters. + fixit_in_note_expr ="#define ToStr(x) #x\nToStr(0 {, })" + value = frame.EvaluateExpression(fixit_in_note_expr, options) + self.assertTrue(value.IsValid()) + self.assertTrue(value.GetError().Success(), value.GetError()) + self.assertEquals(value.GetSummary(), '"(0 {, })"') + # Now turn off the fixits, and the expression should fail: options.SetAutoApplyFixIts(False) value = frame.EvaluateExpression(two_error_expression, options) _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
