[lldb] Also apply Fix-Its in "note:" diagnostics that belong to an error diagnostic

[teemperor]

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.

[teemperor]

• Rebase and now only considering error diagnostics.

[JDevlieghere]

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

242

This loop and the one 20 lines down seem very similar. Maybe extract a function and share the comments?

[teemperor]

• Move Fix-It loops into single function.

[JDevlieghere]

LGTM

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

for (ClangModulesDeclVendor::ModuleID module : exported_modules)
       m_persistent_vars.AddHandLoadedClangModule(module);
   }
 
   bool hasErrors() { return m_has_errors; }
 
   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) {
     DiagnosticOptions *m_options = new DiagnosticOptions(opts);
     m_options->ShowPresumedLoc = true;
     m_options->ShowLevel = false;
     m_os.reset(new llvm::raw_string_ostream(m_output));
     m_passthrough.reset(
         new clang::TextDiagnosticPrinter(*m_os, m_options, false));
   }
 
   void ResetManager(DiagnosticManager *manager = nullptr) {
     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) {
       // We have no DiagnosticManager before/after parsing but we still could
       // receive diagnostics (e.g., by the ASTImporter failing to copy decls
       // when we move the expression result ot the ScratchASTContext). Let's at
       // least log these diagnostics until we find a way to properly render
       // them and display them to the user.
case DiagnosticsEngine::Level::Warning:
       break;
     case DiagnosticsEngine::Level::Remark:
     case DiagnosticsEngine::Level::Ignored:
       severity = eDiagnosticSeverityRemark;
       break;
     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
       // around them.
       std::string stripped_output =
           std::string(llvm::StringRef(m_output).trim());
 
       auto new_diagnostic = std::make_unique<ClangDiagnostic>(
           stripped_output, severity, Info.getID());
 
       // Don't store away warning fixits, since the compiler doesn't have
       // 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));
     }
   }
 
   clang::TextDiagnosticPrinter *GetPassthrough() { return m_passthrough.get(); }
 
 private:

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

def test_with_target(self):
 
         # Try with two errors:
         two_error_expression = "my_pointer.second->a"
         value = frame.EvaluateExpression(two_error_expression, options)
         self.assertTrue(value.IsValid())
         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)
         self.assertTrue(value.IsValid())
         self.assertTrue(value.GetError().Fail())
         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")