[clangd] Discard diagnostics from another SourceManager.

[adamcz]

Summary

This can happen when building implicit modules, as demonstrated in test.
The CompilerInstance uses the same StoredDiags, but different
SourceManager. This used to crash clangd when it tried to relocate the
diagnostic to the main file, which, according to SourceManager from the
diagnostic, is a fake <module-includes> file.

[adamcz]

log dropped dianostic

[adamcz]

Hey Sam. What's your opinion on this?

The options we have are:

1. Drop the diagnostics, like this change is doing
2. Relocate the diagnostic to the beginning of the "real" main file, like we do with SourceManager-less diagnostics
3. Try to find the place where we import the module and relocate the diagnostic there

Right now this change is just an example of approach 1. Not necessarily meant to be submitted as-is, since it silently drops possibly valuable diagnostics, although it does prevent a crash.

Option 3 sounds best, but it's very tricky. At the point HandleDiagnostic() is called, the module is not imported yet, so we would have to either delay the relocation until later or do some tricks with Preprocessor object (not sure how that would work).

Let me know what you think about this.

[kadircet]

clang-tools-extra/clangd/Diagnostics.cpp

209

can't we rather record the mainfilename once and make use of it here?
i believe the rest of the logic operates on the local sourcelocations/files associated with the sourcemanager inside the diagnostic in process.

[adamcz]

clang-tools-extra/clangd/Diagnostics.cpp

209

getMainFileRange() doesn't.

We can easily get main file from the original source manager or record it in BeginSourceFile. The thing is that it's not easy to find a place in the main file to put this diagnostic at. We can, of course, put it at the top of the file, but normally we try to find the #include location in the main file. That's done in the getMainFileRange() above and it mixes the location of the diagnostic (which is for the new SourceManager) and the main file buffer (which is for the OrigSrcMgr).

I'm a bit worried that if we put a diagnostic relating to #include at the top of the file, where another #include may be, it will become very confusing.

[kadircet]

clang-tools-extra/clangd/Diagnostics.cpp

209

getMainFileRange() doesn't.

ah right i forgot about that bit :/

I'm a bit worried that if we put a diagnostic relating to #include at the top of the file, where another #include may be, it will become very confusing.

I totally agree.

A crazy idea, we can register a callback to PP on BeginSourceFile to track file changes, than whenever we see a diagnostic from a different sourcemanager, we can map it back to last position a filechange was triggered. Ofc, this goes with the assumption that we see a filechanged event on #include "modular.h" line, but I guess we should at least get an includedirective callback or something.

[adamcz]

clang-tools-extra/clangd/Diagnostics.cpp

209

Right, that's a good idea, thanks. Sam suggested something like that in parallel, during in-person discussion too. I'll look into that in the near future.

There's the other issue of only seeing this diagnostics when a module is built, but not when a module was loaded from cache, so we will not be consistent unless we serialize them somehow.

[sammccall]

Yeah I think this is the right approach for now, because the cached modules don't include diagnostics and ensuring that they're persisted and available is a larger project.

clang-tools-extra/clangd/Diagnostics.cpp

568

maybe a FIXME: errors from implicitly built modules should be surfaced somehow (but then must also be surfaced when the module was cached)

568

Can we be a bit more specific: this happens when an #include causes a module to be implicitly built, using a separate SourceManager.

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

44

nit: strict

[adamcz]

addressed review comments

clang-tools-extra/clangd/Diagnostics.cpp

568

It's not just #include, could be "import". But yes, we can be more specific. Done.

clang-tools-extra/clangd/Diagnostics.h

 /// clangd. It groups all notes for a diagnostic into a single Diag
 /// and filters out diagnostics that don't mention the main file (i.e. neither
 /// the diag itself nor its notes are in the main file).
 class StoreDiags : public DiagnosticConsumer {
 public:
   // The ClangTidyContext populates Source and Name for clang-tidy diagnostics.
   std::vector<Diag> take(const clang::tidy::ClangTidyContext *Tidy = nullptr);
 
-  void BeginSourceFile(const LangOptions &Opts, const Preprocessor *) override;
+  void BeginSourceFile(const LangOptions &Opts,
+                       const Preprocessor *PP) override;
   void EndSourceFile() override;
   void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
                         const clang::Diagnostic &Info) override;
 
   using DiagFixer = std::function<std::vector<Fix>(DiagnosticsEngine::Level,
                                                    const clang::Diagnostic &)>;
   using LevelAdjuster = std::function<DiagnosticsEngine::Level(
       DiagnosticsEngine::Level, const clang::Diagnostic &)>;
private:
 
   DiagFixer Fixer = nullptr;
   LevelAdjuster Adjuster = nullptr;
   std::vector<Diag> Output;
   llvm::Optional<LangOptions> LangOpts;
   llvm::Optional<Diag> LastDiag;
   llvm::Optional<FullSourceLoc> LastDiagLoc; // Valid only when LastDiag is set.
   bool LastDiagOriginallyError = false;      // Valid only when LastDiag is set.
+  SourceManager *OrigSrcMgr = nullptr;
 
   llvm::DenseSet<std::pair<unsigned, unsigned>> IncludedErrorLocations;
   bool LastPrimaryDiagnosticWasSuppressed = false;
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_DIAGNOSTICS_H

clang-tools-extra/clangd/Diagnostics.cpp

bool tryMoveToMainFile(Diag &D, FullSourceLoc DiagLoc) {
   D.Notes.emplace(D.Notes.begin());
   Note &N = D.Notes.front();
   N.AbsFile = std::string(FE->tryGetRealPathName());
   N.File = std::string(FE->getName());
   N.Message = "error occurred here";
   N.Range = D.Range;
 
   // Update diag to point at include inside main file.
   D.File = SM.getFileEntryForID(SM.getMainFileID())->getName().str();
   D.Range = std::move(R);
   D.InsideMainFile = true;
   // Update message to mention original file.
   D.Message = llvm::formatv("{0}: {1}", Prefix, D.Message);
   return true;
 }
 
 bool isInsideMainFile(const clang::Diagnostic &D) {
std::vector<Diag> StoreDiags::take(const clang::tidy::ClangTidyContext *Tidy) {
   std::set<std::pair<Range, std::string>> SeenDiags;
   llvm::erase_if(Output, [&](const Diag& D) {
     return !SeenDiags.emplace(D.Range, D.Message).second;
   });
   return std::move(Output);
 }
 
 void StoreDiags::BeginSourceFile(const LangOptions &Opts,
-                                 const Preprocessor *) {
+                                 const Preprocessor *PP) {
   LangOpts = Opts;
+  if (PP) {
+    OrigSrcMgr = &PP->getSourceManager();
+  }
 }
 
 void StoreDiags::EndSourceFile() {
   flushLastDiag();
   LangOpts = None;
+  OrigSrcMgr = nullptr;
 }
 
 /// Sanitizes a piece for presenting it in a synthesized fix message. Ensures
 /// the result is not too large and does not contain newlines.
 static void writeCodeToFixMessage(llvm::raw_ostream &OS, llvm::StringRef Code) {
   constexpr unsigned MaxLen = 50;
 
   // Only show the first line if there are many.
static void fillNonLocationData(DiagnosticsEngine::Level DiagLevel,
   D.Severity = DiagLevel;
   D.Category = DiagnosticIDs::getCategoryNameFromID(
                    DiagnosticIDs::getCategoryNumberForDiag(Info.getID()))
                    .str();
 }
 
 void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
                                   const clang::Diagnostic &Info) {
+  // If the diagnostic was generated for a different SourceManager, skip it.
+  // This happens when a module is imported and needs to be implicitly built.
+  // The compilation of that module will use the same StoreDiags, but different
+  // SourceManager.
+  if (OrigSrcMgr && Info.hasSourceManager() &&
+      OrigSrcMgr != &Info.getSourceManager()) {
+    IgnoreDiagnostics::log(DiagLevel, Info);
+    return;
+  }
+
   DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info);
   bool OriginallyError =
       Info.getDiags()->getDiagnosticIDs()->isDefaultMappingAsError(
           Info.getID());
 
   if (Info.getLocation().isInvalid()) {
     // Handle diagnostics coming from command-line arguments. The source manager
     // is *not* available at this point, so we cannot use it.

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

module M {
         textual header "Textual.h"
       }
     }
 )modulemap";
   // Test that we do not crash.
   TU.index();
 }
 
+TEST(Modules, Diagnostic) {
+  // Produce a diagnostic while building an implicit module. Use
+  // -fmodules-strict-decluse, but any non-silenced diagnostic will do.
+  TestTU TU = TestTU::withCode(R"cpp(
+    /*error-ok*/
+    #include "modular.h"
+
+    void bar() {}
+)cpp");
+  TU.OverlayRealFileSystemForModules = true;
+  TU.ExtraArgs.push_back("-fmodule-map-file=" + testPath("m.modulemap"));
+  TU.ExtraArgs.push_back("-fmodules");
+  TU.ExtraArgs.push_back("-fimplicit-modules");
+  TU.ExtraArgs.push_back("-fmodules-strict-decluse");
+  TU.AdditionalFiles["modular.h"] = R"cpp(
+    #include "non-modular.h"
+  )cpp";
+  TU.AdditionalFiles["non-modular.h"] = "";
+  TU.AdditionalFiles["m.modulemap"] = R"modulemap(
+    module M {
+      header "modular.h"
+    }
+)modulemap";
+
+  // Test that we do not crash.
+  TU.build();
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang