[clangd] Retain main file fixes attached to diags from preamble

[kadircet]

Summary

Clangd ignores fixits if the diagnsotics is outside the main file (e.g.
a note on a declaration from a header), but the fix might still be inside the
main file (e.g. change the function call).

This patch changes the logic to retain fixes that touch main file, if the
diagnostic owning them is still inside main file, even if they are attached to a
note outside the main file.

[sammccall]

clang-tools-extra/clangd/Diagnostics.cpp

68

update comment: Fixes are only added if the fix or diagnostics is in the main file.

705–706

I think this can now be inlined to the one place it's used

724

Note that this may still miss some cases, e.g. if we had a diagnostic inside a template instantiation with a fix in the current file. tryMoveToMainFile has not been called yet, so LastDiag->InsideMainFile would be false.

I don't have a great idea about what to do about this, nor does it seem that important...

[kadircet]

• Get rid of redundant local variable
• Update comment around assumptions about main file-ness inside toLSPDiag.

clang-tools-extra/clangd/Diagnostics.cpp

68

as discussed offline, this is still the case and it isn't an or but an and, i.e. we preserve a fix if it's in the main file AND the primary diagnostic we'll attach it to is also inside the main file.

[sammccall]

LG, thanks!

clang-tools-extra/clangd/Diagnostics.cpp

 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
+#include <cassert>
 #include <cstddef>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 const char *getDiagnosticCode(unsigned ID) {
#undef DIAG
   default:
     return nullptr;
   }
 }
 
 bool mentionsMainFile(const Diag &D) {
   if (D.InsideMainFile)
     return true;
   // Fixes are always in the main file.
   if (!D.Fixes.empty())
     return true;
   for (auto &N : D.Notes) {
     if (N.InsideMainFile)
       return true;
   }
   return false;
 }

 void toLSPDiags(
     const Diag &D, const URIForFile &File, const ClangdDiagnosticOptions &Opts,
     llvm::function_ref<void(clangd::Diagnostic, llvm::ArrayRef<Fix>)> OutFn) {
   clangd::Diagnostic Main;
   Main.severity = getSeverity(D.Severity);
 
   // Main diagnostic should always refer to a range inside main file. If a
   // diagnostic made it so for, it means either itself or one of its notes is
-  // inside main file.
+  // inside main file. It's also possible that there's a fix in the main file,
+  // but we preserve fixes iff primary diagnostic is in the main file.
   if (D.InsideMainFile) {
     Main.range = D.Range;
   } else {
     auto It =
         llvm::find_if(D.Notes, [](const Note &N) { return N.InsideMainFile; });
     assert(It != D.Notes.end() &&
            "neither the main diagnostic nor notes are inside main file");
     Main.range = It->Range;
if (Info.getLocation().isInvalid()) {
     LastDiag->Range.end = Position{0, 0};
     return;
   }
 
   if (!LangOpts || !Info.hasSourceManager()) {
     IgnoreDiagnostics::log(DiagLevel, Info);
     return;
   }
 
-  bool InsideMainFile = isInsideMainFile(Info);
   SourceManager &SM = Info.getSourceManager();
 
   auto FillDiagBase = [&](DiagBase &D) {
     fillNonLocationData(DiagLevel, Info, D);
 
-    D.InsideMainFile = InsideMainFile;
+    D.InsideMainFile = isInsideMainFile(Info);
     D.Range = diagnosticRange(Info, *LangOpts);
     D.File = std::string(SM.getFilename(Info.getLocation()));
     D.AbsFile = getCanonicalPath(
         SM.getFileEntryForID(SM.getFileID(Info.getLocation())), SM);
     D.ID = Info.getID();
     return D;
   };
 
   auto AddFix = [&](bool SyntheticMessage) -> bool {
     assert(!Info.getFixItHints().empty() &&
            "diagnostic does not have attached fix-its");
-    if (!InsideMainFile)
+    // No point in generating fixes, if the diagnostic is for a different file.
+    if (!LastDiag->InsideMainFile)
       return false;
-
     // Copy as we may modify the ranges.
     auto FixIts = Info.getFixItHints().vec();
     llvm::SmallVector<TextEdit, 1> Edits;
     for (auto &FixIt : FixIts) {
       // Allow fixits within a single macro-arg expansion to be applied.
       // This can be incorrect if the argument is expanded multiple times in
       // different contexts. Hopefully this is rare!
       if (FixIt.RemoveRange.getBegin().isMacroID() &&

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp

 #include "TestIndex.h"
 #include "TestTU.h"
 #include "TidyProvider.h"
 #include "index/MemIndex.h"
 #include "support/Context.h"
 #include "support/Path.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticSema.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Support/TargetSelect.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <algorithm>
 #include <memory>
 
 namespace clang {
]]
   WithContextValue SuppressAllWithCfg(Config::Key, std::move(Cfg));
   EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
   Cfg.Diagnostics.SuppressAll = false;
   Cfg.Diagnostics.Suppress = {"unused-includes"};
   WithContextValue SuppressFilterWithCfg(Config::Key, std::move(Cfg));
   EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
 }
 
+TEST(DiagnosticsTest, FixItFromHeader) {
+  llvm::StringLiteral Header(R"cpp(
+    void foo(int *);
+    void foo(int *, int);)cpp");
+  Annotations Source(R"cpp(
+  /*error-ok*/
+    void bar() {
+      int x;
+      $diag[[foo]]($fix[[]]x, 1);
+    })cpp");
+  TestTU TU;
+  TU.Code = Source.code().str();
+  TU.HeaderCode = Header.str();
+  EXPECT_THAT(
+      *TU.build().getDiagnostics(),
+      UnorderedElementsAre(AllOf(
+          Diag(Source.range("diag"), "no matching function for call to 'foo'"),
+          withFix(Fix(Source.range("fix"), "&",
+                      "candidate function not viable: no known conversion from "
+                      "'int' to 'int *' for 1st argument; take the address of "
+                      "the argument with &")))));
+}
 } // namespace
 } // namespace clangd
 } // namespace clang