[clangd] Revamp handling of diagnostics.

[ilya-biryukov]

Summary

The new implementation attaches notes to diagnostic message and shows
the original diagnostics in the message of the note.

[ilya-biryukov]

This revision is still missing tests, but I wanted to get feedback for the overall design and the new diagnostic messages. So sending out for review now.

[ilya-biryukov]

clangd/Diagnostics.h

29

This could probably use a better name

[sammccall]

Behavior looks good. I think we can do a bit better with (most) fixits - your call on whether it makes sense to do here.

As discussed i'd simplify the diagnostic containers until we know there's a strong need.

clangd/ClangdLSPServer.cpp

329

nit: while here, can we add a colon after FixIt (or if appropriate in practice, just drop the prefix altogether?). My recollection is this is a bit hard to parse.

clangd/Diagnostics.cpp

86

inside

126

mind pasting a small example as a function comment?
Logic looks good, but I had to reconstruct the message in my head.

148

or just mainMessage (this is another verbs-for-pure-functions case where the style recommendation seems to hurt readability to me, but up to you)

193

PreBuild could be FillBasicFields or something? found this name confusing.

201

(nit: if the goal with the callback function is to avoid allocations, shouldn't we reuse the DiagWithFixIts?)

323

do you need else log here? (and just log the main diag)

clangd/Diagnostics.h

29

I found it a bit confusing that this represents both "top-level" diagnostics and notes. It obscures the nature of the hierarchy a bit: there *are* a fixed number of levels with different "kinds", but the kinds are similar enough to share a type.

I'd actually consider using inheritance here to model the relationships more clearly. (Yes, gross, I know)
Like:

struct DiagBase { Message, File, Range, InMainFile }
struct Diag : DiagBase { FixIts, Notes, Severity }
struct Note : DiagBase {} // or leave this one out until needed

(I think we came to the conclusion that different severity of notes isn't interesting and potentially just confusing)

36

As discussed offline - I think fixits should probably be a different struct hanging off the main diagnostic, with a name. Following clang's example seems... less than clear here.

They could also be modeled as notes with an optional TextEdit - this seems sligthly less clear but more faithful to clang internals*.
Either way, it should be clear that they're only allowed in *one* of these locations - having notes and main diags be distinct types would help.

I think this probably affects how we should expose them through LSP - they should be named code actions attached to the original diagnostic.
Maybe they should also be separate diagnostics, but only if they contribute a unique opportunity to the user e.g. they have a non-empty range that isn't contained within the diagnostic's range.
This patch seems like a reasonable place to do that, but also OK if you want to defer.

36

Nit: can we call these "Fix"es, which is a noun (at least in common usage?)

Clang calls them FixItHint, but I think Fix is no less clear (and shorter)

59

This is cool :) but really looks like premature optimization.
Particularly, if the diagnostics were a self-contained value type, this could just be vector<diag> I think. And if we care about copying strings (I'm not sure we do), being lifetime-scoped to the ASTcontext is probably OK.

(rawDiags() has potential uses but having callers explicitly traverse is going to be clearer as your comment suggests)

114

Can we try removing this struct in favor of two callback params? It seems like a small source of confusion, considering how important the type used to be.

124

I don't think this overload pays for itself - making callers write the outer loop would be a little clearer, I think.

(I'd consider having that fn return a smallvector instead of taking a callback, but don't feel strongly)

129

brief doc.
In particular: what filtering is applied

129

Maybe just StoreDiags?

131

If there's no strong reason otherwise, DiagList take() is slightly easier/clearer for tests. compare

DiagList Diags;
StoreDiagsConsumer Cons(Diags);
...
EXPECT_THAT(Diags, ...)

vs

StoreDiagsConsumer Diags;
...
EXPECT_THAT(Diags.take(), ...)

clangd/SourceCode.cpp

53

isn't this sourceLocToPosition?

test/clangd/diagnostics.test

23

I think ideally this wouldn't show up at all, "change return type to int" would be the name of the code action returned for the first diag. Note the range is the same, so we're not giving the user an extra place to apply the fix.

[ilya-biryukov]

This is not final, there are still unadressed comments.

• Significantly simplified the implementation by removing the DiagList, etc.
• Addressed some of the rest of the comments.

clangd/ClangdLSPServer.cpp

329

I added a colon for now, but happy to look into removing the prefix. The use-case that's slightly confusing is:

use of undeclared identifier 'foo'. did you mean 'bar'?

Since the actual fix-it is at the end, it's might be a bit hard to understand.

clangd/Diagnostics.cpp

201

Can we actually avoid allocating some of the strings there?
We're std::moveing the result into the callback, so we don't seem to waste memory too.

clangd/Diagnostics.h

59

Using a straight-forward representation for now.

124

Still using a callback, but happy to return a SmallVector instead. It seems the pattern for using SmallVector in llvm is passing it in as an output parameter (so that the users might decide themselves what's the count of items on the stack), which is slightly ugly.
It might be fine to return SmallVector here though, this function can probably make assumptions about the number of notes in a diagnostic.

[ilya-biryukov]

Addressed review comments.

The only thing that's left on my todo-list is a test for toLSPDiags().
Everything else should be ready.

[sammccall]

Not sure if you're waiting on comments from me: the changes look good.
As discussed I still think Fix should be a separate thing with a name because that's how editors treat it, and this is the right layer to decide how to do that mapping.

clangd/ClangdLSPServer.cpp

329

Yeah. I'd still be inclined to drop the prefix:

• this case isn't ideal, but is also not terrible
• this is a "no separate note for fixit" case, where we can consider synthesizing the message from the text edit rather than reusing the diagnostic. That would work well at least in this case.

329

nit: fixit --> fix in prefix and in lambda
(And drop the caps in the prefix I think, as this is now a regular english noun rather than a pseudo-product-name)

clangd/Diagnostics.h

40

I don't understand what "persistent" means in this context.
This does seem to be an is-a relationship rather than has-a, so I'd find DiagBase/Diag + inheritance clearer.

But with composition, this one seems rike it should be "Diag" and the type of Main could be DiagDetails or so?

[sammccall]

Sorry, last comments were concurrent with the latest snapshot.

Except where noted, the old comments still apply I think.

clangd/ClangdLSPServer.cpp

324

inline this into the for loop?

clangd/ClangdLSPServer.h

77

getFixes?

clangd/Diagnostics.h

36

This is done, and looks good to me.

40

(this happened already, yay!)

48

nit: severity of notes can...

52

Seems a bit confusing to use "diagnostic" to mean something importantly different than "diag", and to define top-level diagnostic in terms of notes.

Maybe just "A top-level diagnostic that may have Notes and Fixes"?

55

I think these deserve some comments:

• fixes are *alternative* fixes for this diagnostic, one should be chosen
• notes elaborate on the problem, usually pointing to a related piece of code

58

I'm not sure what "conventional" means here?

unittests/clangd/ClangdUnitTests.cpp

24

can you make these operator<< and move to the main code? This seems a fine general-purpose debug representation (could be tweaked, but also later).

That avoids the ODR issue, and makes casual logging easier.

43

Ah, and now that you've written this... ;-)

For the specific case of "field X matches matcher M" you can use ::testing::Field (you'd keep WithNote, but make it call Field)

Messages are good as long as you pass the optional field_name param.

81

this "same range and message as the diagnostic itself" is an important property of the code being tested, and it's hidden here in a comment of a helper function.
I'd probably prefer avoiding this helper altogether (spelling the string twice in the test) so the test behavior/change is obvious when changing the code.
Failing that, DiagWithEqualFix or something?

[ilya-biryukov]

• Added unit test for toLSPDiags

[ilya-biryukov]

• Addressed review comments

clangd/Diagnostics.h

36

We now provide a separate Fix class, but we still don't return it to LSP, keeping the behaviour as is (i.e. mapping LSP ranges to code actions on the server side).

The fixes are not reported as separate diagnostics, they are only shown as code actions when users hovers over the diagnostic.

58

Removed conventional from the comment. Other parts should make sense.

unittests/clangd/ClangdUnitTests.cpp

43

Was a useful exercise anyway.

Couldn't find the optional 'field_name' param, though, am I missing something?

81

I agree that, but spelling this in the code is so much noise. Renaming to DiagWithEqualFix makes total sense, did that.

[ilya-biryukov]

clangd/ClangdLSPServer.cpp

329

I still find a prefix useful in some cases, e.g. the unresolved identifier foo. did you mean bar?.
Mostly for cases where a fix note uses the error message of the main diagnostic or when it's badly phrased (haven't really seen those, but I would be surprised if there are none).

[sammccall]

Ship it! (but let's drop the extra operator== if possible?)

clangd/ClangdLSPServer.cpp

329

that's... exactly the case I was replying to :)
I don't think it's common or confusing enough, and there are other fixes when using the main diagnostic. But this isn't a big deal; let's leave it for now.

clangd/Protocol.h

166 ↗

(On Diff #137987)

The TextEdit/Diagnostic == operators seem odd - what do we need these for?

unittests/clangd/ClangdUnitTests.cpp

43

You're not, sorry - it was added recently to google's copy and hasn't been pushed to upstream gmock yet. So for now we have slightly bad error messages (or have the full MatcherInterface implementation)

[ilya-biryukov]

• Replace equality comparison ops with matchers, they were only used in tests.

clangd/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   )
 
 add_clang_library(clangDaemon
   ClangdLSPServer.cpp
   ClangdServer.cpp
   ClangdUnit.cpp
   CodeComplete.cpp
   CodeCompletionStrings.cpp
   CompileArgsCache.cpp
   Compiler.cpp
   Context.cpp
+  Diagnostics.cpp
   DraftStore.cpp
   FuzzyMatch.cpp
   GlobalCompilationDatabase.cpp
   Headers.cpp
   JSONExpr.cpp
   JSONRPCDispatcher.cpp
   Logger.cpp
   Protocol.cpp

clangd/ClangdLSPServer.h

public:
   /// each instance of ClangdLSPServer.
   ///
   /// \return Wether we received a 'shutdown' request before an 'exit' request
   bool run(std::istream &In,
            JSONStreamStyle InputStyle = JSONStreamStyle::Standard);
 
 private:
   // Implement DiagnosticsConsumer.
-  virtual void
+  void
   onDiagnosticsReady(PathRef File,
-                     Tagged<std::vector<DiagWithFixIts>> Diagnostics) override;
+                     Tagged<std::vector<PersistentDiag>> Diagnostics) override;
 
   // Implement ProtocolCallbacks.
   void onInitialize(InitializeParams &Params) override;
   void onShutdown(ShutdownParams &Params) override;
   void onExit(ExitParams &Params) override;
   void onDocumentDidOpen(DidOpenTextDocumentParams &Params) override;
   void onDocumentDidChange(DidChangeTextDocumentParams &Params) override;
   void onDocumentDidClose(DidCloseTextDocumentParams &Params) override;
private:
   void onSwitchSourceHeader(TextDocumentIdentifier &Params) override;
   void onDocumentHighlight(TextDocumentPositionParams &Params) override;
   void onFileEvent(DidChangeWatchedFilesParams &Params) override;
   void onCommand(ExecuteCommandParams &Params) override;
   void onRename(RenameParams &Parames) override;
   void onHover(TextDocumentPositionParams &Params) override;
   void onChangeConfiguration(DidChangeConfigurationParams &Params) override;
 
   std::vector<TextEdit> getFixIts(StringRef File, const clangd::Diagnostic &D);
 
   JSONOutput &Out;
   /// Used to indicate that the 'shutdown' request was received from the
   /// Language Server client.
   bool ShutdownRequestReceived = false;
 
   /// Used to indicate that the 'exit' notification was received from the
   /// Language Server client.

clangd/ClangdLSPServer.cpp

 //===--- ClangdLSPServer.cpp - LSP server ------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===---------------------------------------------------------------------===//
 
 #include "ClangdLSPServer.h"
+#include "Diagnostics.h"
 #include "JSONRPCDispatcher.h"
 #include "SourceCode.h"
 #include "URI.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 
 using namespace clang::clangd;
 using namespace clang;
void ClangdLSPServer::onDocumentFormatting(DocumentFormattingParams &Params) {
   if (ReplacementsOrError)
     reply(json::ary(replacementsToEdits(*Code, ReplacementsOrError.get())));
   else
     replyError(ErrorCode::UnknownErrorCode,
                llvm::toString(ReplacementsOrError.takeError()));
 }
 
 void ClangdLSPServer::onCodeAction(CodeActionParams &Params) {
+  auto GetFixitMessage = [](llvm::StringRef Message) -> llvm::StringRef {
+    // VSCode does not really print messages with newlines nicely.
+    // And we put notes into diagnostic messages, which are not useful for FixIt
+    // messages shown to the users.
+    return Message.take_until([](char C) { return C == '\n'; });
+  };
   // We provide a code action for each diagnostic at the requested location
   // which has FixIts available.
   auto Code = Server.getDocument(Params.textDocument.uri.file());
   if (!Code)
     return replyError(ErrorCode::InvalidParams,
                       "onCodeAction called for non-added file");
 
   json::ary Commands;
   for (Diagnostic &D : Params.context.diagnostics) {
     auto Edits = getFixIts(Params.textDocument.uri.file(), D);
     if (!Edits.empty()) {
       WorkspaceEdit WE;
       WE.changes = {{Params.textDocument.uri.uri(), std::move(Edits)}};
       Commands.push_back(json::obj{
-          {"title", llvm::formatv("Apply FixIt {0}", D.message)},
+          {"title",
+           llvm::formatv("Apply FixIt: {0}", GetFixitMessage(D.message))},
           {"command", ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND},
           {"arguments", {WE}},
       });
     }
   }
   reply(std::move(Commands));
 }
 
std::vector<TextEdit> ClangdLSPServer::getFixIts(StringRef File,
   auto FixItsIter = DiagToFixItsMap.find(D);
   if (FixItsIter == DiagToFixItsMap.end())
     return {};
 
   return FixItsIter->second;
 }
 
 void ClangdLSPServer::onDiagnosticsReady(
-    PathRef File, Tagged<std::vector<DiagWithFixIts>> Diagnostics) {
+    PathRef File, Tagged<std::vector<PersistentDiag>> Diagnostics) {
   json::ary DiagnosticsJSON;
 
   DiagnosticToReplacementMap LocalFixIts; // Temporary storage
-  for (auto &DiagWithFixes : Diagnostics.Value) {
-    auto Diag = DiagWithFixes.Diag;
+  for (auto &Diag : Diagnostics.Value) {
+    toLSPDiags(Diag, [&](DiagWithFixIts D) {
       DiagnosticsJSON.push_back(json::obj{
-        {"range", Diag.range},
-        {"severity", Diag.severity},
-        {"message", Diag.message},
+          {"range", D.Diag.range},
+          {"severity", D.Diag.severity},
+          {"message", D.Diag.message},
       });
-    // We convert to Replacements to become independent of the SourceManager.
-    auto &FixItsForDiagnostic = LocalFixIts[Diag];
-    std::copy(DiagWithFixes.FixIts.begin(), DiagWithFixes.FixIts.end(),
+
+      auto &FixItsForDiagnostic = LocalFixIts[D.Diag];
+      std::copy(D.FixIts.begin(), D.FixIts.end(),
                 std::back_inserter(FixItsForDiagnostic));
+    });
   }
 
   // Cache FixIts
   {
     // FIXME(ibiryukov): should be deleted when documents are removed
     std::lock_guard<std::mutex> Lock(FixItsMutex);
     FixItsMap[File] = LocalFixIts;
   }

clangd/ClangdServer.h

 
 class DiagnosticsConsumer {
 public:
   virtual ~DiagnosticsConsumer() = default;
 
   /// Called by ClangdServer when \p Diagnostics for \p File are ready.
   virtual void
   onDiagnosticsReady(PathRef File,
-                     Tagged<std::vector<DiagWithFixIts>> Diagnostics) = 0;
+                     Tagged<std::vector<PersistentDiag>> Diagnostics) = 0;
 };
 
 class FileSystemProvider {
 public:
   virtual ~FileSystemProvider() = default;
   /// Called by ClangdServer to obtain a vfs::FileSystem to be used for parsing.
   /// Name of the file that will be parsed is passed in \p File.
   ///

clangd/ClangdServer.cpp

void ClangdServer::scheduleReparseAndDiags(
     Tagged<IntrusiveRefCntPtr<vfs::FileSystem>> TaggedFS) {
   tooling::CompileCommand Command = CompileArgs.getCompileCommand(File);
 
   DocVersion Version = Contents.Version;
   Path FileStr = File.str();
   VFSTag Tag = std::move(TaggedFS.Tag);
 
   auto Callback = [this, Version, FileStr,
-                   Tag](std::vector<DiagWithFixIts> Diags) {
+                   Tag](std::vector<PersistentDiag> Diags) {
     // We need to serialize access to resulting diagnostics to avoid calling
     // `onDiagnosticsReady` in the wrong order.
     std::lock_guard<std::mutex> DiagsLock(DiagnosticsMutex);
     DocVersion &LastReportedDiagsVersion = ReportedDiagnosticVersions[FileStr];
     // FIXME(ibiryukov): get rid of '<' comparison here. In the current
     // implementation diagnostics will not be reported after version counters'
     // overflow. This should not happen in practice, since DocVersion is a
     // 64-bit unsigned integer.

clangd/ClangdUnit.h

 //===--- ClangdUnit.h -------------------------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===---------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_CLANGDUNIT_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CLANGDUNIT_H
 
+#include "Diagnostics.h"
 #include "Function.h"
 #include "Path.h"
 #include "Protocol.h"
 #include "clang/Frontend/FrontendAction.h"
 #include "clang/Frontend/PrecompiledPreamble.h"
 #include "clang/Serialization/ASTBitCodes.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "clang/Tooling/Core/Replacement.h"

 }
 
 namespace tooling {
 struct CompileCommand;
 }
 
 namespace clangd {
 
-/// A diagnostic with its FixIts.
-struct DiagWithFixIts {
-  clangd::Diagnostic Diag;
-  llvm::SmallVector<TextEdit, 1> FixIts;
-};
-
 using InclusionLocations = std::vector<std::pair<Range, Path>>;
 
 // Stores Preamble and associated data.
 struct PreambleData {
   PreambleData(PrecompiledPreamble Preamble,
                std::vector<serialization::DeclID> TopLevelDeclIDs,
-               std::vector<DiagWithFixIts> Diags,
+               std::vector<PersistentDiag> Diags,
                InclusionLocations IncLocations);
 
   PrecompiledPreamble Preamble;
   std::vector<serialization::DeclID> TopLevelDeclIDs;
-  std::vector<DiagWithFixIts> Diags;
+  std::vector<PersistentDiag> Diags;
   InclusionLocations IncLocations;
 };
 
 /// Information required to run clang, e.g. to parse AST or do code completion.
 struct ParseInputs {
   tooling::CompileCommand CompileCommand;
   IntrusiveRefCntPtr<vfs::FileSystem> FS;
   std::string Contents;
public:
   std::shared_ptr<Preprocessor> getPreprocessorPtr();
   const Preprocessor &getPreprocessor() const;
 
   /// This function returns all top-level decls, including those that come
   /// from Preamble. Decls, coming from Preamble, have to be deserialized, so
   /// this call might be expensive.
   ArrayRef<const Decl *> getTopLevelDecls();
 
-  const std::vector<DiagWithFixIts> &getDiagnostics() const;
+  const std::vector<PersistentDiag> &getDiagnostics() const;
 
   /// Returns the esitmated size of the AST and the accessory structures, in
   /// bytes. Does not include the size of the preamble.
   std::size_t getUsedBytes() const;
   const InclusionLocations &getInclusionLocations() const;
 
 private:
   ParsedAST(std::shared_ptr<const PreambleData> Preamble,
             std::unique_ptr<CompilerInstance> Clang,
             std::unique_ptr<FrontendAction> Action,
             std::vector<const Decl *> TopLevelDecls,
-            std::vector<DiagWithFixIts> Diags, InclusionLocations IncLocations);
+            std::vector<PersistentDiag> Diags, InclusionLocations IncLocations);
 
 private:
   void ensurePreambleDeclsDeserialized();
 
   // In-memory preambles must outlive the AST, it is important that this member
   // goes before Clang and Action.
   std::shared_ptr<const PreambleData> Preamble;
   // We store an "incomplete" FrontendAction (i.e. no EndSourceFile was called
   // on it) and CompilerInstance used to run it. That way we don't have to do
   // complex memory management of all Clang structures on our own. (They are
   // stored in CompilerInstance and cleaned up by
   // FrontendAction.EndSourceFile).
   std::unique_ptr<CompilerInstance> Clang;
   std::unique_ptr<FrontendAction> Action;
 
   // Data, stored after parsing.
-  std::vector<DiagWithFixIts> Diags;
+  std::vector<PersistentDiag> Diags;
   std::vector<const Decl *> TopLevelDecls;
   bool PreambleDeclsDeserialized;
   InclusionLocations IncLocations;
 };
 
 using ASTParsedCallback = std::function<void(PathRef Path, ParsedAST *)>;
 
 /// Manages resources, required by clangd. Allows to rebuild file with new
 /// contents, and provides AST and Preamble for it.
 class CppFile {
 public:
   CppFile(PathRef FileName, bool StorePreamblesInMemory,
           std::shared_ptr<PCHContainerOperations> PCHs,
           ASTParsedCallback ASTCallback);
 
   /// Rebuild the AST and the preamble.
   /// Returns a list of diagnostics or llvm::None, if an error occured.
-  llvm::Optional<std::vector<DiagWithFixIts>> rebuild(ParseInputs &&Inputs);
+  llvm::Optional<std::vector<PersistentDiag>> rebuild(ParseInputs &&Inputs);
   /// Returns the last built preamble.
   const std::shared_ptr<const PreambleData> &getPreamble() const;
   /// Returns the last built AST.
   ParsedAST *getAST() const;
   /// Returns an estimated size, in bytes, currently occupied by the AST and the
   /// Preamble.
   std::size_t getUsedBytes() const;
 

clangd/ClangdUnit.cpp

 //===--- ClangdUnit.cpp -----------------------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===---------------------------------------------------------------------===//
 
 #include "ClangdUnit.h"
 #include "Compiler.h"
+#include "Diagnostics.h"
 #include "Logger.h"
+#include "SourceCode.h"
 #include "Trace.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Frontend/Utils.h"
 #include "clang/Index/IndexDataConsumer.h"
 #include "clang/Index/IndexingAction.h"
 #include "clang/Lex/Lexer.h"
std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
                                                  StringRef InFile) override {
     return llvm::make_unique<DeclTrackingASTConsumer>(/*ref*/ TopLevelDecls);
   }
 
 private:
   std::vector<const Decl *> TopLevelDecls;
 };
 
-// Converts a half-open clang source range to an LSP range.
-// Note that clang also uses closed source ranges, which this can't handle!
-Range toRange(CharSourceRange R, const SourceManager &M) {
-  // Clang is 1-based, LSP uses 0-based indexes.
-  Position Begin;
-  Begin.line = static_cast<int>(M.getSpellingLineNumber(R.getBegin())) - 1;
-  Begin.character =
-      static_cast<int>(M.getSpellingColumnNumber(R.getBegin())) - 1;
-
-  Position End;
-  End.line = static_cast<int>(M.getSpellingLineNumber(R.getEnd())) - 1;
-  End.character = static_cast<int>(M.getSpellingColumnNumber(R.getEnd())) - 1;
-
-  return {Begin, End};
-}
-
 class InclusionLocationsCollector : public PPCallbacks {
 public:
   InclusionLocationsCollector(SourceManager &SourceMgr,
                               InclusionLocations &IncLocations)
       : SourceMgr(SourceMgr), IncLocations(IncLocations) {}
 
   void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
                           StringRef FileName, bool IsAngled,
                           CharSourceRange FilenameRange, const FileEntry *File,
                           StringRef SearchPath, StringRef RelativePath,
                           const Module *Imported) override {
     auto SR = FilenameRange.getAsRange();
     if (SR.isInvalid() || !File || File->tryGetRealPathName().empty())
       return;
 
     if (SourceMgr.isInMainFile(SR.getBegin())) {
       // Only inclusion directives in the main file make sense. The user cannot
       // select directives not in the main file.
-      IncLocations.emplace_back(toRange(FilenameRange, SourceMgr),
+      IncLocations.emplace_back(halfOpenToRange(SourceMgr, FilenameRange),
                                 File->tryGetRealPathName());
     }
   }
 
 private:
   SourceManager &SourceMgr;
   InclusionLocations &IncLocations;
 };

 
 private:
   std::vector<Decl *> TopLevelDecls;
   std::vector<serialization::DeclID> TopLevelDeclIDs;
   InclusionLocations IncLocations;
   SourceManager *SourceMgr = nullptr;
 };
 
-/// Convert from clang diagnostic level to LSP severity.
-static int getSeverity(DiagnosticsEngine::Level L) {
-  switch (L) {
-  case DiagnosticsEngine::Remark:
-    return 4;
-  case DiagnosticsEngine::Note:
-    return 3;
-  case DiagnosticsEngine::Warning:
-    return 2;
-  case DiagnosticsEngine::Fatal:
-  case DiagnosticsEngine::Error:
-    return 1;
-  case DiagnosticsEngine::Ignored:
-    return 0;
-  }
-  llvm_unreachable("Unknown diagnostic level!");
-}
-
-// Checks whether a location is within a half-open range.
-// Note that clang also uses closed source ranges, which this can't handle!
-bool locationInRange(SourceLocation L, CharSourceRange R,
-                     const SourceManager &M) {
-  assert(R.isCharRange());
-  if (!R.isValid() || M.getFileID(R.getBegin()) != M.getFileID(R.getEnd()) ||
-      M.getFileID(R.getBegin()) != M.getFileID(L))
-    return false;
-  return L != R.getEnd() && M.isPointWithin(L, R.getBegin(), R.getEnd());
-}
-
-// Clang diags have a location (shown as ^) and 0 or more ranges (~~~~).
-// LSP needs a single range.
-Range diagnosticRange(const clang::Diagnostic &D, const LangOptions &L) {
-  auto &M = D.getSourceManager();
-  auto Loc = M.getFileLoc(D.getLocation());
-  // Accept the first range that contains the location.
-  for (const auto &CR : D.getRanges()) {
-    auto R = Lexer::makeFileCharRange(CR, M, L);
-    if (locationInRange(Loc, R, M))
-      return toRange(R, M);
-  }
-  // The range may be given as a fixit hint instead.
-  for (const auto &F : D.getFixItHints()) {
-    auto R = Lexer::makeFileCharRange(F.RemoveRange, M, L);
-    if (locationInRange(Loc, R, M))
-      return toRange(R, M);
-  }
-  // If no suitable range is found, just use the token at the location.
-  auto R = Lexer::makeFileCharRange(CharSourceRange::getTokenRange(Loc), M, L);
-  if (!R.isValid()) // Fall back to location only, let the editor deal with it.
-    R = CharSourceRange::getCharRange(Loc);
-  return toRange(R, M);
-}
-
-TextEdit toTextEdit(const FixItHint &FixIt, const SourceManager &M,
-                    const LangOptions &L) {
-  TextEdit Result;
-  Result.range = toRange(Lexer::makeFileCharRange(FixIt.RemoveRange, M, L), M);
-  Result.newText = FixIt.CodeToInsert;
-  return Result;
-}
-
-llvm::Optional<DiagWithFixIts> toClangdDiag(const clang::Diagnostic &D,
-                                            DiagnosticsEngine::Level Level,
-                                            const LangOptions &LangOpts) {
-  if (!D.hasSourceManager() || !D.getLocation().isValid() ||
-      !D.getSourceManager().isInMainFile(D.getLocation())) {
-    IgnoreDiagnostics::log(Level, D);
-    return llvm::None;
-  }
-
-  SmallString<64> Message;
-  D.FormatDiagnostic(Message);
-
-  DiagWithFixIts Result;
-  Result.Diag.range = diagnosticRange(D, LangOpts);
-  Result.Diag.severity = getSeverity(Level);
-  Result.Diag.message = Message.str();
-  for (const FixItHint &Fix : D.getFixItHints())
-    Result.FixIts.push_back(toTextEdit(Fix, D.getSourceManager(), LangOpts));
-  return std::move(Result);
-}
-
-class StoreDiagsConsumer : public DiagnosticConsumer {
-public:
-  StoreDiagsConsumer(std::vector<DiagWithFixIts> &Output) : Output(Output) {}
-
-  // Track language options in case we need to expand token ranges.
-  void BeginSourceFile(const LangOptions &Opts, const Preprocessor *) override {
-    LangOpts = Opts;
-  }
-
-  void EndSourceFile() override { LangOpts = llvm::None; }
-
-  void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
-                        const clang::Diagnostic &Info) override {
-    DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info);
-
-    if (LangOpts)
-      if (auto D = toClangdDiag(Info, DiagLevel, *LangOpts))
-        Output.push_back(std::move(*D));
-  }
-
-private:
-  std::vector<DiagWithFixIts> &Output;
-  llvm::Optional<LangOptions> LangOpts;
-};
-
 } // namespace
 
 void clangd::dumpAST(ParsedAST &AST, llvm::raw_ostream &OS) {
   AST.getASTContext().getTranslationUnitDecl()->dump(OS, true);
 }
 
 llvm::Optional<ParsedAST>
 ParsedAST::Build(std::unique_ptr<clang::CompilerInvocation> CI,
                  std::shared_ptr<const PreambleData> Preamble,
                  std::unique_ptr<llvm::MemoryBuffer> Buffer,
                  std::shared_ptr<PCHContainerOperations> PCHs,
                  IntrusiveRefCntPtr<vfs::FileSystem> VFS) {
-  std::vector<DiagWithFixIts> ASTDiags;
-  StoreDiagsConsumer UnitDiagsConsumer(/*ref*/ ASTDiags);
+  std::vector<PersistentDiag> ASTDiags;
+  StoreDiagsConsumer UnitDiagsConsumer(ASTDiags);
 
   const PrecompiledPreamble *PreamblePCH =
       Preamble ? &Preamble->Preamble : nullptr;
   auto Clang = prepareCompilerInstance(
       std::move(CI), PreamblePCH, std::move(Buffer), std::move(PCHs),
       std::move(VFS), /*ref*/ UnitDiagsConsumer);
   if (!Clang)
     return llvm::None;
Clang->getPreprocessor().addPPCallbacks(
                                                      IncLocations));
 
   if (!Action->Execute())
     log("Execute() failed when building AST for " + MainInput.getFile());
 
   // UnitDiagsConsumer is local, we can not store it in CompilerInstance that
   // has a longer lifetime.
   Clang->getDiagnostics().setClient(new IgnoreDiagnostics);
+  // CompilerInstance won't run this callback, do it directly.
+  UnitDiagsConsumer.EndSourceFile();
 
   std::vector<const Decl *> ParsedDecls = Action->takeTopLevelDecls();
   return ParsedAST(std::move(Preamble), std::move(Clang), std::move(Action),
                    std::move(ParsedDecls), std::move(ASTDiags),
                    std::move(IncLocations));
 }
 
 namespace {
const Preprocessor &ParsedAST::getPreprocessor() const {
   return Clang->getPreprocessor();
 }
 
 ArrayRef<const Decl *> ParsedAST::getTopLevelDecls() {
   ensurePreambleDeclsDeserialized();
   return TopLevelDecls;
 }
 
-const std::vector<DiagWithFixIts> &ParsedAST::getDiagnostics() const {
+const std::vector<PersistentDiag> &ParsedAST::getDiagnostics() const {
   return Diags;
 }
 
 std::size_t ParsedAST::getUsedBytes() const {
   auto &AST = getASTContext();
   // FIXME(ibiryukov): we do not account for the dynamically allocated part of
   // SmallVector<FixIt> inside each Diag.
   return AST.getASTAllocatedMemory() + AST.getSideTableAllocatedMemory() +
          ::getUsedBytes(TopLevelDecls) + ::getUsedBytes(Diags);
 }
 
 const InclusionLocations &ParsedAST::getInclusionLocations() const {
   return IncLocations;
 }
 
 PreambleData::PreambleData(PrecompiledPreamble Preamble,
                            std::vector<serialization::DeclID> TopLevelDeclIDs,
-                           std::vector<DiagWithFixIts> Diags,
+                           std::vector<PersistentDiag> Diags,
                            InclusionLocations IncLocations)
     : Preamble(std::move(Preamble)),
       TopLevelDeclIDs(std::move(TopLevelDeclIDs)), Diags(std::move(Diags)),
       IncLocations(std::move(IncLocations)) {}
 
 ParsedAST::ParsedAST(std::shared_ptr<const PreambleData> Preamble,
                      std::unique_ptr<CompilerInstance> Clang,
                      std::unique_ptr<FrontendAction> Action,
                      std::vector<const Decl *> TopLevelDecls,
-                     std::vector<DiagWithFixIts> Diags,
+                     std::vector<PersistentDiag> Diags,
                      InclusionLocations IncLocations)
     : Preamble(std::move(Preamble)), Clang(std::move(Clang)),
       Action(std::move(Action)), Diags(std::move(Diags)),
       TopLevelDecls(std::move(TopLevelDecls)), PreambleDeclsDeserialized(false),
       IncLocations(std::move(IncLocations)) {
   assert(this->Clang);
   assert(this->Action);
 }
 
 CppFile::CppFile(PathRef FileName, bool StorePreamblesInMemory,
                  std::shared_ptr<PCHContainerOperations> PCHs,
                  ASTParsedCallback ASTCallback)
     : FileName(FileName), StorePreamblesInMemory(StorePreamblesInMemory),
       PCHs(std::move(PCHs)), ASTCallback(std::move(ASTCallback)) {
   log("Created CppFile for " + FileName);
 }
 
-llvm::Optional<std::vector<DiagWithFixIts>>
+llvm::Optional<std::vector<PersistentDiag>>
 CppFile::rebuild(ParseInputs &&Inputs) {
   log("Rebuilding file " + FileName + " with command [" +
       Inputs.CompileCommand.Directory + "] " +
       llvm::join(Inputs.CompileCommand.CommandLine, " "));
 
   std::vector<const char *> ArgStrs;
   for (const auto &S : Inputs.CompileCommand.CommandLine)
     ArgStrs.push_back(S.c_str());
CppFile::rebuild(ParseInputs &&Inputs) {
   llvm::Optional<ParsedAST> NewAST;
   {
     trace::Span Tracer("Build");
     SPAN_ATTACH(Tracer, "File", FileName);
     NewAST = ParsedAST::Build(std::move(CI), NewPreamble,
                               std::move(ContentsBuffer), PCHs, Inputs.FS);
   }
 
-  std::vector<DiagWithFixIts> Diagnostics;
+  std::vector<PersistentDiag> Diagnostics;
   if (NewAST) {
     // Collect diagnostics from both the preamble and the AST.
     if (NewPreamble)
-      Diagnostics.insert(Diagnostics.begin(), NewPreamble->Diags.begin(),
-                         NewPreamble->Diags.end());
+      Diagnostics = NewPreamble->Diags;
     Diagnostics.insert(Diagnostics.end(), NewAST->getDiagnostics().begin(),
                        NewAST->getDiagnostics().end());
   }
   if (ASTCallback && NewAST) {
     trace::Span Tracer("Running ASTCallback");
     ASTCallback(FileName, NewAST.getPointer());
   }
 
if (OldPreamble && compileCommandsAreEqual(this->Command, Command) &&
     log("Reusing preamble for file " + Twine(FileName));
     return OldPreamble;
   }
   log("Preamble for file " + Twine(FileName) +
       " cannot be reused. Attempting to rebuild it.");
 
   trace::Span Tracer("Preamble");
   SPAN_ATTACH(Tracer, "File", FileName);
-  std::vector<DiagWithFixIts> PreambleDiags;
+  std::vector<PersistentDiag> PreambleDiags;
   StoreDiagsConsumer PreambleDiagnosticsConsumer(/*ref*/ PreambleDiags);
   IntrusiveRefCntPtr<DiagnosticsEngine> PreambleDiagsEngine =
       CompilerInstance::createDiagnostics(&CI.getDiagnosticOpts(),
                                           &PreambleDiagnosticsConsumer, false);
 
   // Skip function bodies when building the preamble to speed up building
   // the preamble and make it smaller.
   assert(!CI.getFrontendOpts().SkipFunctionBodies);

clangd/Diagnostics.h

+//===--- Diagnostics.h ------------------------------------------*- C++-*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===---------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_DIAGNOSTICS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_DIAGNOSTICS_H
+
+#include "Path.h"
+#include "Protocol.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/LangOptions.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringSet.h"
+#include <cassert>
+#include <string>
+
+namespace clang {
+namespace clangd {
+
+struct Diag {
+  std::string Message;
+  // Intended to be used only in error messages.
+  // May be relative, absolute or even artifically constructed.
+  std::string File;
+  clangd::Range Range;
+  DiagnosticsEngine::Level Severity;
+  // Since File is only descriptive, we store a separate flag to distinguish
+  // diags from the main file.
+  bool InsideMainFile = false;
+  /// TextEdits from clang's fix-its.
+  llvm::SmallVector<TextEdit, 1> Fixes;
+};
+
+struct PersistentDiag {
+  /// Main diagnostic.
+  Diag Main;
+  /// Notes for Main.
+  std::vector<Diag> Notes;
+};
+
+/// An LSP diagnostic with FixIts.
+struct DiagWithFixIts {
+  clangd::Diagnostic Diag;
+  llvm::SmallVector<TextEdit, 1> FixIts;
+};
+
+/// Conventional conversion to LSP diagnostics. Formats the error message of
+/// each diagnostic to include all its notes. Notes inside main file are also
+/// provided as separate diagnostics with their corresponding fixits.
+/// Notes outside main file do not have a corresponding LSP diagnostic, but can
+/// still be included as part of their main diagnostic's message.
+void toLSPDiags(const PersistentDiag &D,
+                llvm::function_ref<void(DiagWithFixIts)> OutFn);
+
+/// StoreDiagsConsumer collects the diagnostics that can later be reported by
+/// clangd. It groups all notes for a diagnostic into a single PersistentDiag
+/// 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 StoreDiagsConsumer : public DiagnosticConsumer {
+public:
+  StoreDiagsConsumer(std::vector<PersistentDiag> &Output);
+
+  void BeginSourceFile(const LangOptions &Opts, const Preprocessor *) override;
+  void EndSourceFile() override;
+  void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
+                        const clang::Diagnostic &Info) override;
+
+private:
+  bool shouldIgnore(DiagnosticsEngine::Level DiagLevel,
+                    const clang::Diagnostic &Info);
+
+  void flushLastDiag();
+
+  std::vector<PersistentDiag> &Output;
+  llvm::Optional<LangOptions> LangOpts;
+  llvm::Optional<PersistentDiag> LastDiag;
+  /// Is any diag or note from LastDiag in the main file?
+  bool LastDiagMentionsMainFile = false;
+};
+
+} // namespace clangd
+} // namespace clang
+
+#endif

clangd/Diagnostics.cpp

+//===--- Diagnostics.cpp ----------------------------------------*- C++-*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===---------------------------------------------------------------------===//
+
+#include "Diagnostics.h"
+#include "Compiler.h"
+#include "Logger.h"
+#include "SourceCode.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Lex/Lexer.h"
+#include "llvm/Support/Capacity.h"
+#include "llvm/Support/Path.h"
+#include <algorithm>
+
+namespace clang {
+namespace clangd {
+
+namespace {
+
+bool mentionsMainFile(const PersistentDiag &D) {
+  if (D.Main.InsideMainFile)
+    return true;
+  for (auto &N : D.Notes) {
+    if (N.InsideMainFile)
+      return true;
+  }
+  return false;
+}
+
+/// Convert from clang diagnostic level to LSP severity.
+int getSeverity(DiagnosticsEngine::Level L) {
+  switch (L) {
+  case DiagnosticsEngine::Remark:
+    return 4;
+  case DiagnosticsEngine::Note:
+    return 3;
+  case DiagnosticsEngine::Warning:
+    return 2;
+  case DiagnosticsEngine::Fatal:
+  case DiagnosticsEngine::Error:
+    return 1;
+  case DiagnosticsEngine::Ignored:
+    return 0;
+  }
+  llvm_unreachable("Unknown diagnostic level!");
+}
+
+// Checks whether a location is within a half-open range.
+// Note that clang also uses closed source ranges, which this can't handle!
+bool locationInRange(SourceLocation L, CharSourceRange R,
+                     const SourceManager &M) {
+  assert(R.isCharRange());
+  if (!R.isValid() || M.getFileID(R.getBegin()) != M.getFileID(R.getEnd()) ||
+      M.getFileID(R.getBegin()) != M.getFileID(L))
+    return false;
+  return L != R.getEnd() && M.isPointWithin(L, R.getBegin(), R.getEnd());
+}
+
+// Clang diags have a location (shown as ^) and 0 or more ranges (~~~~).
+// LSP needs a single range.
+Range diagnosticRange(const clang::Diagnostic &D, const LangOptions &L) {
+  auto &M = D.getSourceManager();
+  auto Loc = M.getFileLoc(D.getLocation());
+  // Accept the first range that contains the location.
+  for (const auto &CR : D.getRanges()) {
+    auto R = Lexer::makeFileCharRange(CR, M, L);
+    if (locationInRange(Loc, R, M))
+      return halfOpenToRange(M, R);
+  }
+  // The range may be given as a fixit hint instead.
+  for (const auto &F : D.getFixItHints()) {
+    auto R = Lexer::makeFileCharRange(F.RemoveRange, M, L);
+    if (locationInRange(Loc, R, M))
+      return halfOpenToRange(M, R);
+  }
+  // If no suitable range is found, just use the token at the location.
+  auto R = Lexer::makeFileCharRange(CharSourceRange::getTokenRange(Loc), M, L);
+  if (!R.isValid()) // Fall back to location only, let the editor deal with it.
+    R = CharSourceRange::getCharRange(Loc);
+  return halfOpenToRange(M, R);
+}
+
+TextEdit toTextEdit(const FixItHint &FixIt, const SourceManager &M,
+                    const LangOptions &L) {
+  TextEdit Result;
+  Result.range =
+      halfOpenToRange(M, Lexer::makeFileCharRange(FixIt.RemoveRange, M, L));
+  Result.newText = FixIt.CodeToInsert;
+  return Result;
+}
+
+bool isInsideMainFile(const clang::Diagnostic &D) {
+  if (!D.hasSourceManager())
+    return false;
+
+  auto Loc = D.getLocation();
+  return Loc.isValid() && D.getSourceManager().isInMainFile(Loc);
+}
+
+bool isNote(DiagnosticsEngine::Level L) {
+  return L == DiagnosticsEngine::Note || L == DiagnosticsEngine::Remark;
+}
+
+llvm::StringRef diagLeveltoString(DiagnosticsEngine::Level Lvl) {
+  switch (Lvl) {
+  case DiagnosticsEngine::Ignored:
+    return "ignored";
+  case DiagnosticsEngine::Note:
+    return "note";
+  case DiagnosticsEngine::Remark:
+    return "remark";
+  case DiagnosticsEngine::Warning:
+    return "warning";
+  case DiagnosticsEngine::Error:
+    return "error";
+  case DiagnosticsEngine::Fatal:
+    return "fatal error";
+  }
+  llvm_unreachable("unhandled DiagnosticsEngine::Level");
+}
+
+/// Prints a single diagnostic in a clang-like manner, the output includes
+/// location, severity and error message. An example of the output message is:
+///
+///     main.cpp:12:23: error: undeclared identifier
+///
+/// For main file we only print the basename and for all other files we print
+/// the filename on a separate line to provide a slightly more readable output
+/// in the editors:
+///
+///     dir1/dir2/dir3/../../dir4/header.h:12:23
+///     error: undeclared identifier
+void printDiag(llvm::raw_string_ostream &OS, const Diag &D) {
+  if (D.InsideMainFile) {
+    // Paths to main files are often taken from compile_command.json, where they
+    // are typically absolute. To reduce noise we print only basename for them,
+    // it should not be confusing and saves space.
+    OS << llvm::sys::path::filename(D.File) << ":";
+  } else {
+    OS << D.File << ":";
+  }
+  // Note +1 to line and character. clangd::Range is zero-based, but when
+  // printing for users we want one-based indexes.
+  auto Pos = D.Range.start;
+  OS << (Pos.line + 1) << ":" << (Pos.character + 1) << ":";
+  // The non-main-file paths are often too long, putting them on a separate
+  // line improves readability.
+  if (D.InsideMainFile)
+    OS << " ";
+  else
+    OS << "\n";
+  OS << diagLeveltoString(D.Severity) << ": " << D.Message;
+}
+
+/// Returns a message sent to LSP for the main diagnostic in \p D.
+/// The message includes all the notes with their corresponding locations.
+/// However, notes with fix-its are excluded as those usually only contain a
+/// fix-it message and just add noise if included in the message for diagnostic.
+/// Example output:
+///
+///     no matching function for call to 'foo'
+///
+///     main.cpp:3:5: note: candidate function not viable: requires 2 arguments
+///
+///     dir1/dir2/dir3/../../dir4/header.h:12:23
+///     note: candidate function not viable: requires 3 arguments
+std::string mainMessage(const PersistentDiag &D) {
+  std::string Result;
+  llvm::raw_string_ostream OS(Result);
+  OS << D.Main.Message;
+  for (auto &Note : D.Notes) {
+    if (!Note.Fixes.empty()) {
+      // We don't add fix-it notes to main diag message, they add too much
+      // noise.
+      continue;
+    }
+    OS << "\n\n";
+    printDiag(OS, Note);
+  }
+  OS.flush();
+  return Result;
+}
+
+/// Returns a message sent to LSP for the note of the main diagnostic.
+/// The message includes the main diagnostic to provide the necessary context
+/// for the user to understand the note.
+std::string noteMessage(const Diag &Main, const Diag &Note) {
+  std::string Result;
+  llvm::raw_string_ostream OS(Result);
+  OS << Note.Message;
+  OS << "\n\n";
+  printDiag(OS, Main);
+  OS.flush();
+  return Result;
+}
+} // namespace
+
+void toLSPDiags(const PersistentDiag &D,
+                llvm::function_ref<void(DiagWithFixIts)> OutFn) {
+  auto FillBasicFields = [](const Diag &D) {
+    DiagWithFixIts Res;
+    Res.Diag.range = D.Range;
+    Res.Diag.severity = getSeverity(D.Severity);
+    Res.FixIts = D.Fixes;
+    return Res;
+  };
+
+  DiagWithFixIts Main = FillBasicFields(D.Main);
+  Main.Diag.message = mainMessage(D);
+  OutFn(std::move(Main));
+
+  for (auto &Note : D.Notes) {
+    DiagWithFixIts Res = FillBasicFields(Note);
+    Res.Diag.message = noteMessage(D.Main, Note);
+    OutFn(std::move(Res));
+  }
+}
+
+StoreDiagsConsumer::StoreDiagsConsumer(std::vector<PersistentDiag> &Output)
+    : Output(Output) {}
+
+void StoreDiagsConsumer::BeginSourceFile(const LangOptions &Opts,
+                                         const Preprocessor *) {
+  LangOpts = Opts;
+}
+
+void StoreDiagsConsumer::EndSourceFile() {
+  flushLastDiag();
+  LangOpts = llvm::None;
+}
+
+void StoreDiagsConsumer::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
+                                          const clang::Diagnostic &Info) {
+  DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info);
+
+  if (!LangOpts || !Info.hasSourceManager()) {
+    IgnoreDiagnostics::log(DiagLevel, Info);
+    return;
+  }
+
+  bool InsideMainFile = isInsideMainFile(Info);
+
+  auto BuildDiagData = [&]() -> Diag {
+    Diag D;
+    D.Range = diagnosticRange(Info, *LangOpts);
+    llvm::SmallString<64> Message;
+    Info.FormatDiagnostic(Message);
+    D.Message = Message.str();
+    D.InsideMainFile = InsideMainFile;
+    D.File = Info.getSourceManager().getFilename(Info.getLocation());
+    D.Severity = DiagLevel;
+
+    if (D.InsideMainFile) {
+      for (auto &FixIt : Info.getFixItHints())
+        D.Fixes.push_back(
+            toTextEdit(FixIt, Info.getSourceManager(), *LangOpts));
+    }
+    return D;
+  };
+
+  if (!isNote(DiagLevel)) {
+    // A new diagnostic.
+    flushLastDiag();
+
+    LastDiag = PersistentDiag();
+    LastDiag->Main = BuildDiagData();
+  } else {
+    // A clang note without fix-its, transformed to clangd::Note.
+    LastDiag->Notes.push_back(BuildDiagData());
+  }
+}
+
+void StoreDiagsConsumer::flushLastDiag() {
+  if (!LastDiag)
+    return;
+  if (mentionsMainFile(*LastDiag))
+    Output.push_back(std::move(*LastDiag));
+  else
+    log(Twine("Dropped diagnostic outside main file:") + LastDiag->Main.File +
+        ":" + LastDiag->Main.Message);
+  LastDiag.reset();
+}
+
+} // namespace clangd
+} // namespace clang

clangd/SourceCode.h

 size_t positionToOffset(llvm::StringRef Code, Position P);
 
 /// Turn an offset in Code into a [line, column] pair.
 Position offsetToPosition(llvm::StringRef Code, size_t Offset);
 
 /// Turn a SourceLocation into a [line, column] pair.
 Position sourceLocToPosition(const SourceManager &SM, SourceLocation Loc);
 
+// Converts a half-open clang source range to an LSP range.
+// Note that clang also uses closed source ranges, which this can't handle!
+Range halfOpenToRange(const SourceManager &SM, CharSourceRange R);
+
 } // namespace clangd
 } // namespace clang
 #endif

clangd/SourceCode.cpp

 
 Position sourceLocToPosition(const SourceManager &SM, SourceLocation Loc) {
   Position P;
   P.line = static_cast<int>(SM.getSpellingLineNumber(Loc)) - 1;
   P.character = static_cast<int>(SM.getSpellingColumnNumber(Loc)) - 1;
   return P;
 }
 
+Range halfOpenToRange(const SourceManager &SM, CharSourceRange R) {
+  // Clang is 1-based, LSP uses 0-based indexes.
+  Position Begin = sourceLocToPosition(SM, R.getBegin());
+  Position End = sourceLocToPosition(SM, R.getEnd());
+
+  return {Begin, End};
+}
+
 } // namespace clangd
 } // namespace clang

clangd/TUScheduler.h

public:
   /// Returns estimated memory usage for each of the currently open files.
   /// The order of results is unspecified.
   std::vector<std::pair<Path, std::size_t>> getUsedBytesPerFile() const;
 
   /// Schedule an update for \p File. Adds \p File to a list of tracked files if
   /// \p File was not part of it before.
   /// FIXME(ibiryukov): remove the callback from this function.
   void update(PathRef File, ParseInputs Inputs, WantDiagnostics WD,
-              UniqueFunction<void(std::vector<DiagWithFixIts>)> OnUpdated);
+              UniqueFunction<void(std::vector<PersistentDiag>)> OnUpdated);
 
   /// Remove \p File from the list of tracked files and schedule removal of its
   /// resources.
   void remove(PathRef File);
 
   /// Schedule an async read of the AST. \p Action will be called when AST is
   /// ready. The AST passed to \p Action refers to the version of \p File
   /// tracked at the time of the call, even if new updates are received before

clangd/TUScheduler.cpp

public:
   /// synchronously instead. \p Barrier is acquired when processing each
   /// request, it is be used to limit the number of actively running threads.
   static ASTWorkerHandle Create(llvm::StringRef File, AsyncTaskRunner *Tasks,
                                 Semaphore &Barrier, CppFile AST,
                                 steady_clock::duration UpdateDebounce);
   ~ASTWorker();
 
   void update(ParseInputs Inputs, WantDiagnostics,
-              UniqueFunction<void(std::vector<DiagWithFixIts>)> OnUpdated);
+              UniqueFunction<void(std::vector<PersistentDiag>)> OnUpdated);
   void runWithAST(llvm::StringRef Name,
                   UniqueFunction<void(llvm::Expected<InputsAndAST>)> Action);
   bool blockUntilIdle(Deadline Timeout) const;
 
   std::shared_ptr<const PreambleData> getPossiblyStalePreamble() const;
   std::size_t getUsedBytes() const;
 
 private:
private:
   // Inputs, corresponding to the current state of AST.
   ParseInputs FileInputs;
   // Guards members used by both TUScheduler and the worker thread.
   mutable std::mutex Mutex;
   std::shared_ptr<const PreambleData> LastBuiltPreamble; /* GUARDED_BY(Mutex) */
   // Result of getUsedBytes() after the last rebuild or read of AST.
   std::size_t LastASTSize; /* GUARDED_BY(Mutex) */
   // Set to true to signal run() to finish processing.
-  bool Done;                           /* GUARDED_BY(Mutex) */
-  std::deque<Request> Requests;        /* GUARDED_BY(Mutex) */
+  bool Done;                    /* GUARDED_BY(Mutex) */
+  std::deque<Request> Requests; /* GUARDED_BY(Mutex) */
   mutable std::condition_variable RequestsCV;
 };
 
 /// A smart-pointer-like class that points to an active ASTWorker.
 /// In destructor, signals to the underlying ASTWorker that no new requests will
 /// be sent and the processing loop may exit (after running all pending
 /// requests).
 class ASTWorkerHandle {
#ifndef NDEBUG
   std::lock_guard<std::mutex> Lock(Mutex);
   assert(Done && "handle was not destroyed");
   assert(Requests.empty() && "unprocessed requests when destroying ASTWorker");
 #endif
 }
 
 void ASTWorker::update(
     ParseInputs Inputs, WantDiagnostics WantDiags,
-    UniqueFunction<void(std::vector<DiagWithFixIts>)> OnUpdated) {
+    UniqueFunction<void(std::vector<PersistentDiag>)> OnUpdated) {
   auto Task = [=](decltype(OnUpdated) OnUpdated) mutable {
     FileInputs = Inputs;
     auto Diags = AST.rebuild(std::move(Inputs));
 
     {
       std::lock_guard<std::mutex> Lock(Mutex);
       if (AST.getPreamble())
         LastBuiltPreamble = AST.getPreamble();
bool TUScheduler::blockUntilIdle(Deadline D) const {
   if (PreambleTasks)
     if (!PreambleTasks->wait(D))
       return false;
   return true;
 }
 
 void TUScheduler::update(
     PathRef File, ParseInputs Inputs, WantDiagnostics WantDiags,
-    UniqueFunction<void(std::vector<DiagWithFixIts>)> OnUpdated) {
+    UniqueFunction<void(std::vector<PersistentDiag>)> OnUpdated) {
   std::unique_ptr<FileData> &FD = Files[File];
   if (!FD) {
     // Create a new worker to process the AST-related tasks.
     ASTWorkerHandle Worker = ASTWorker::Create(
         File, WorkerThreads ? WorkerThreads.getPointer() : nullptr, Barrier,
         CppFile(File, StorePreamblesInMemory, PCHOps, ASTCallback),
         UpdateDebounce);
     FD = std::unique_ptr<FileData>(new FileData{Inputs, std::move(Worker)});

test/clangd/diagnostics.test

 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 0,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          }
 # CHECK-NEXT:        },
 # CHECK-NEXT:        "severity": 2
 # CHECK-NEXT:      },
 # CHECK-NEXT:      {
-# CHECK-NEXT:        "message": "change return type to 'int'",
+# CHECK-NEXT:        "message": "change return type to 'int'\n\nfoo.c:1:1: warning: return type of 'main' is not 'int'",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 4,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 0,
 # CHECK-NEXT:            "line": 0

test/clangd/execute-command.test

 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 32,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          }
 # CHECK-NEXT:        },
 # CHECK-NEXT:        "severity": 2
 # CHECK-NEXT:      },
 # CHECK-NEXT:      {
-# CHECK-NEXT:        "message": "place parentheses around the assignment to silence this warning",
+# CHECK-NEXT:        "message": "place parentheses around the assignment to silence this warning\n\nfoo.c:1:33: warning: using the result of an assignment as a condition without parentheses",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 35,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 34,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          }
 # CHECK-NEXT:        },
 # CHECK-NEXT:        "severity": 3
 # CHECK-NEXT:      },
 # CHECK-NEXT:      {
-# CHECK-NEXT:        "message": "use '==' to turn this assignment into an equality comparison",
+# CHECK-NEXT:        "message": "use '==' to turn this assignment into an equality comparison\n\nfoo.c:1:33: warning: using the result of an assignment as a condition without parentheses",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 35,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 34,
 # CHECK-NEXT:            "line": 0

test/clangd/extra-flags.test

 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 27,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          }
 # CHECK-NEXT:        },
 # CHECK-NEXT:        "severity": 2
 # CHECK-NEXT:      },
 # CHECK-NEXT:      {
-# CHECK-NEXT:        "message": "initialize the variable 'i' to silence this warning",
+# CHECK-NEXT:        "message": "initialize the variable 'i' to silence this warning\n\nfoo.c:1:28: warning: variable 'i' is uninitialized when used here",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 19,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 18,
 # CHECK-NEXT:            "line": 0

 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 27,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          }
 # CHECK-NEXT:        },
 # CHECK-NEXT:        "severity": 2
 # CHECK-NEXT:      },
 # CHECK-NEXT:      {
-# CHECK-NEXT:        "message": "initialize the variable 'i' to silence this warning",
+# CHECK-NEXT:        "message": "initialize the variable 'i' to silence this warning\n\nfoo.c:1:28: warning: variable 'i' is uninitialized when used here",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 19,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 18,
 # CHECK-NEXT:            "line": 0

test/clangd/fixits.test

 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 32,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          }
 # CHECK-NEXT:        },
 # CHECK-NEXT:        "severity": 2
 # CHECK-NEXT:      },
 # CHECK-NEXT:      {
-# CHECK-NEXT:        "message": "place parentheses around the assignment to silence this warning",
+# CHECK-NEXT:        "message": "place parentheses around the assignment to silence this warning\n\nfoo.c:1:33: warning: using the result of an assignment as a condition without parentheses",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 35,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 34,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          }
 # CHECK-NEXT:        },
 # CHECK-NEXT:        "severity": 3
 # CHECK-NEXT:      },
 # CHECK-NEXT:      {
-# CHECK-NEXT:        "message": "use '==' to turn this assignment into an equality comparison",
+# CHECK-NEXT:        "message": "use '==' to turn this assignment into an equality comparison\n\nfoo.c:1:33: warning: using the result of an assignment as a condition without parentheses",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 35,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 34,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          }
 # CHECK-NEXT:        },
 # CHECK-NEXT:        "severity": 3
 # CHECK-NEXT:      }
 # CHECK-NEXT:    ],
 # CHECK-NEXT:    "uri": "file://{{.*}}/foo.c"
 # CHECK-NEXT:  }
 ---
-{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}
+{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning\n\nfoo.c:1:33: warning: using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison\n\nfoo.c:1:33: warning: using the result of an assignment as a condition without parentheses"}]}}}
 #      CHECK:  "id": 2,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": [
 # CHECK-NEXT:    {
 # CHECK-NEXT:      "arguments": [
 # CHECK-NEXT:        {
 # CHECK-NEXT:          "changes": {
 # CHECK-NEXT:            "file://{{.*}}/foo.c": [

 # CHECK-NEXT:                  }
 # CHECK-NEXT:                }
 # CHECK-NEXT:              }
 # CHECK-NEXT:            ]
 # CHECK-NEXT:          }
 # CHECK-NEXT:        }
 # CHECK-NEXT:      ],
 # CHECK-NEXT:      "command": "clangd.applyFix",
-# CHECK-NEXT:      "title": "Apply FixIt place parentheses around the assignment to silence this warning"
+# CHECK-NEXT:      "title": "Apply FixIt: place parentheses around the assignment to silence this warning"
 # CHECK-NEXT:    },
 # CHECK-NEXT:    {
 # CHECK-NEXT:      "arguments": [
 # CHECK-NEXT:        {
 # CHECK-NEXT:          "changes": {
 # CHECK-NEXT:            "file://{{.*}}/foo.c": [
 # CHECK-NEXT:              {
 # CHECK-NEXT:                "newText": "==",
 # CHECK-NEXT:                "range": {
 # CHECK-NEXT:                  "end": {
 # CHECK-NEXT:                    "character": 35,
 # CHECK-NEXT:                    "line": 0
 # CHECK-NEXT:                  },
 # CHECK-NEXT:                  "start": {
 # CHECK-NEXT:                    "character": 34,
 # CHECK-NEXT:                    "line": 0
 # CHECK-NEXT:                  }
 # CHECK-NEXT:                }
 # CHECK-NEXT:              }
 # CHECK-NEXT:            ]
 # CHECK-NEXT:          }
 # CHECK-NEXT:        }
 # CHECK-NEXT:      ],
 # CHECK-NEXT:      "command": "clangd.applyFix",
-# CHECK-NEXT:      "title": "Apply FixIt use '==' to turn this assignment into an equality comparison"
+# CHECK-NEXT:      "title": "Apply FixIt: use '==' to turn this assignment into an equality comparison"
 # CHECK-NEXT:    }
 # CHECK-NEXT:  ]
 ---
-{"jsonrpc":"2.0","id":3,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"code":"1","source":"foo","message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}
+{"jsonrpc":"2.0","id":3,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning\n\nfoo.c:1:33: warning: using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison\n\nfoo.c:1:33: warning: using the result of an assignment as a condition without parentheses"}]}}}
 # Make sure unused "code" and "source" fields ignored gracefully
 #      CHECK:  "id": 3,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": [
 # CHECK-NEXT:    {
 # CHECK-NEXT:      "arguments": [
 # CHECK-NEXT:        {
 # CHECK-NEXT:          "changes": {

 # CHECK-NEXT:                  }
 # CHECK-NEXT:                }
 # CHECK-NEXT:              }
 # CHECK-NEXT:            ]
 # CHECK-NEXT:          }
 # CHECK-NEXT:        }
 # CHECK-NEXT:      ],
 # CHECK-NEXT:      "command": "clangd.applyFix",
-# CHECK-NEXT:      "title": "Apply FixIt place parentheses around the assignment to silence this warning"
+# CHECK-NEXT:      "title": "Apply FixIt: place parentheses around the assignment to silence this warning"
 # CHECK-NEXT:    },
 # CHECK-NEXT:    {
 # CHECK-NEXT:      "arguments": [
 # CHECK-NEXT:        {
 # CHECK-NEXT:          "changes": {
 # CHECK-NEXT:            "file://{{.*}}/foo.c": [
 # CHECK-NEXT:              {
 # CHECK-NEXT:                "newText": "==",
 # CHECK-NEXT:                "range": {
 # CHECK-NEXT:                  "end": {
 # CHECK-NEXT:                    "character": 35,
 # CHECK-NEXT:                    "line": 0
 # CHECK-NEXT:                  },
 # CHECK-NEXT:                  "start": {
 # CHECK-NEXT:                    "character": 34,
 # CHECK-NEXT:                    "line": 0
 # CHECK-NEXT:                  }
 # CHECK-NEXT:                }
 # CHECK-NEXT:              }
 # CHECK-NEXT:            ]
 # CHECK-NEXT:          }
 # CHECK-NEXT:        }
 # CHECK-NEXT:      ],
 # CHECK-NEXT:      "command": "clangd.applyFix",
-# CHECK-NEXT:      "title": "Apply FixIt use '==' to turn this assignment into an equality comparison"
+# CHECK-NEXT:      "title": "Apply FixIt: use '==' to turn this assignment into an equality comparison"
 # CHECK-NEXT:    }
 # CHECK-NEXT:  ]
 ---
 {"jsonrpc":"2.0","id":4,"method":"workspace/executeCommand","params":{"command":"clangd.applyFix","arguments":[{"changes":{"test:///foo.c":[{"range":{"start":{"line":0,"character":32},"end":{"line":0,"character":32}},"newText":"("},{"range":{"start":{"line":0,"character":37},"end":{"line":0,"character":37}},"newText":")"}]}}]}}
 #      CHECK:  "id": 4,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": "Fix applied."
 #

unittests/clangd/ClangdTests.cpp

 using ::testing::Eq;
 using ::testing::Gt;
 using ::testing::IsEmpty;
 using ::testing::Pair;
 using ::testing::UnorderedElementsAre;
 
 namespace {
 
-static bool diagsContainErrors(ArrayRef<DiagWithFixIts> Diagnostics) {
-  for (const auto &DiagAndFixIts : Diagnostics) {
-    // FIXME: severities returned by clangd should have a descriptive
-    // diagnostic severity enum
-    const int ErrorSeverity = 1;
-    if (DiagAndFixIts.Diag.severity == ErrorSeverity)
+static bool diagsContainErrors(const std::vector<PersistentDiag> &Diagnostics) {
+  for (auto D : Diagnostics) {
+    if (D.Main.Severity == DiagnosticsEngine::Error ||
+        D.Main.Severity == DiagnosticsEngine::Fatal)
       return true;
   }
   return false;
 }
 
 class ErrorCheckingDiagConsumer : public DiagnosticsConsumer {
 public:
   void
   onDiagnosticsReady(PathRef File,
-                     Tagged<std::vector<DiagWithFixIts>> Diagnostics) override {
+                     Tagged<std::vector<PersistentDiag>> Diagnostics) override {
     bool HadError = diagsContainErrors(Diagnostics.Value);
 
     std::lock_guard<std::mutex> Lock(Mutex);
     HadErrorInLastDiags = HadError;
     LastVFSTag = Diagnostics.Tag;
   }
 
   bool hadErrorInLastDiags() {

 };
 
 /// For each file, record whether the last published diagnostics contained at
 /// least one error.
 class MultipleErrorCheckingDiagConsumer : public DiagnosticsConsumer {
 public:
   void
   onDiagnosticsReady(PathRef File,
-                     Tagged<std::vector<DiagWithFixIts>> Diagnostics) override {
+                     Tagged<std::vector<PersistentDiag>> Diagnostics) override {
     bool HadError = diagsContainErrors(Diagnostics.Value);
 
     std::lock_guard<std::mutex> Lock(Mutex);
     LastDiagsHadError[File] = HadError;
   }
 
   /// Exposes all files consumed by onDiagnosticsReady in an unspecified order.
   /// For each file, a bool value indicates whether the last diagnostics
)cpp";
   };
 
   class TestDiagConsumer : public DiagnosticsConsumer {
   public:
     TestDiagConsumer() : Stats(FilesCount, FileStat()) {}
 
     void onDiagnosticsReady(
         PathRef File,
-        Tagged<std::vector<DiagWithFixIts>> Diagnostics) override {
+        Tagged<std::vector<PersistentDiag>> Diagnostics) override {
       StringRef FileIndexStr = llvm::sys::path::stem(File);
       ASSERT_TRUE(FileIndexStr.consume_front("Foo"));
 
       unsigned long FileIndex = std::stoul(FileIndexStr.str());
 
       bool HadError = diagsContainErrors(Diagnostics.Value);
 
       std::lock_guard<std::mutex> Lock(Mutex);
TEST_F(ClangdThreadingTest, NoConcurrentDiagnostics) {
   class NoConcurrentAccessDiagConsumer : public DiagnosticsConsumer {
   public:
     std::atomic<int> Count = {0};
 
     NoConcurrentAccessDiagConsumer(std::promise<void> StartSecondReparse)
         : StartSecondReparse(std::move(StartSecondReparse)) {}
 
     void onDiagnosticsReady(PathRef,
-                            Tagged<std::vector<DiagWithFixIts>>) override {
+                            Tagged<std::vector<PersistentDiag>>) override {
       ++Count;
       std::unique_lock<std::mutex> Lock(Mutex, std::try_to_lock_t());
       ASSERT_TRUE(Lock.owns_lock())
           << "Detected concurrent onDiagnosticsReady calls for the same file.";
 
       // If we started the second parse immediately, it might cancel the first.
       // So we don't allow it to start until the first has delivered diags...
       if (FirstRequest) {
TEST_F(ClangdVFSTest, InsertIncludes) {
   ErrorCheckingDiagConsumer DiagConsumer;
   MockCompilationDatabase CDB;
   std::string SearchDirArg = (llvm::Twine("-I") + testRoot()).str();
   CDB.ExtraClangFlags.insert(CDB.ExtraClangFlags.end(), {SearchDirArg.c_str()});
   ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
 
   auto FooCpp = testPath("foo.cpp");
   const auto Code = R"cpp(
-#include "z.h"
 #include "x.h"
+#include "z.h"
 
 void f() {}
 )cpp";
   FS.Files[FooCpp] = Code;
   runAddDocument(Server, FooCpp, Code);
 
   auto ChangedCode = [&](llvm::StringRef Original, llvm::StringRef Preferred) {
     auto Replaces = Server.insertInclude(
         FooCpp, Code, Original, Preferred.empty() ? Original : Preferred);
     EXPECT_TRUE(static_cast<bool>(Replaces));
     auto Changed = tooling::applyAllReplacements(Code, *Replaces);
     EXPECT_TRUE(static_cast<bool>(Changed));
     return *Changed;
   };
   auto Inserted = [&](llvm::StringRef Original, llvm::StringRef Preferred,
                       llvm::StringRef Expected) {
     return llvm::StringRef(ChangedCode(Original, Preferred))
         .contains((llvm::Twine("#include ") + Expected + "").str());
   };
 
-  EXPECT_TRUE(Inserted("\"y.h\"", /*Preferred=*/"","\"y.h\""));
+  EXPECT_TRUE(Inserted("\"y.h\"", /*Preferred=*/"", "\"y.h\""));
   EXPECT_TRUE(Inserted("\"y.h\"", /*Preferred=*/"\"Y.h\"", "\"Y.h\""));
   EXPECT_TRUE(Inserted("<string>", /*Preferred=*/"", "<string>"));
   EXPECT_TRUE(Inserted("<string>", /*Preferred=*/"", "<string>"));
 
   std::string OriginalHeader = URI::createFile(testPath("y.h")).toString();
   std::string PreferredHeader = URI::createFile(testPath("Y.h")).toString();
   EXPECT_TRUE(Inserted(OriginalHeader,
                        /*Preferred=*/"", "\"y.h\""));

 TEST_F(ClangdVFSTest, FormatCode) {
   MockFSProvider FS;
   ErrorCheckingDiagConsumer DiagConsumer;
   MockCompilationDatabase CDB;
   ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
 
   auto Path = testPath("foo.cpp");
   std::string Code = R"cpp(
-#include "y.h"
 #include "x.h"
+#include "y.h"
 
 void f(  )  {}
 )cpp";
   std::string Expected = R"cpp(
 #include "x.h"
 #include "y.h"
 
 void f() {}

unittests/clangd/ClangdUnitTests.cpp

 //===-- ClangdUnitTests.cpp - ClangdUnit tests ------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
-#include "ClangdUnit.h"
 #include "Annotations.h"
+#include "ClangdUnit.h"
 #include "TestFS.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PCHContainerOperations.h"
 #include "clang/Frontend/Utils.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 using namespace llvm;
-void PrintTo(const DiagWithFixIts &D, std::ostream *O) {
+
+void PrintTo(const Diag &D, std::ostream *O) {
   llvm::raw_os_ostream OS(*O);
-  OS << D.Diag;
-  if (!D.FixIts.empty()) {
+  if (!D.InsideMainFile)
+    OS << "[in " << D.File << "] ";
+  OS << D.Message;
+
+  if (D.Fixes.empty()) {
     OS << " {";
     const char *Sep = "";
-    for (const auto &F : D.FixIts) {
-      OS << Sep << F;
+    for (const auto &Edit : D.Fixes) {
+      OS << Sep << Edit;
       Sep = ", ";
     }
-    OS << "}";
   }
 }
 
 namespace {
 using testing::ElementsAre;
 
 // FIXME: this is duplicated with FileIndexTests. Share it.
-ParsedAST build(StringRef Code, std::vector<const char*> Flags = {}) {
-  std::vector<const char*> Cmd = {"clang", "main.cpp"};
+ParsedAST build(StringRef Code, std::vector<const char *> Flags = {}) {
+  std::vector<const char *> Cmd = {"clang", "main.cpp"};
   Cmd.insert(Cmd.begin() + 1, Flags.begin(), Flags.end());
   auto CI = createInvocationFromCommandLine(Cmd);
   auto Buf = MemoryBuffer::getMemBuffer(Code);
   auto AST = ParsedAST::Build(std::move(CI), nullptr, std::move(Buf),
                               std::make_shared<PCHContainerOperations>(),
                               vfs::getRealFileSystem());
   assert(AST.hasValue());
   return std::move(*AST);
 }
 
+std::vector<Diag> buildDiags(llvm::StringRef Code,
+                             std::vector<const char *> Flags = {}) {
+  auto Diags = build(Code, std::move(Flags)).getDiagnostics();
+  std::vector<Diag> Flattened;
+  for (auto &&D : Diags) {
+    Flattened.push_back(D.Main);
+    for (auto &&N : D.Notes)
+      Flattened.push_back(N);
+  }
+  return Flattened;
+}
+
 MATCHER_P2(Diag, Range, Message,
            "Diagnostic at " + llvm::to_string(Range) + " = [" + Message + "]") {
-  return arg.Diag.range == Range && arg.Diag.message == Message &&
-         arg.FixIts.empty();
+  return arg.Range == Range && arg.Message == Message && arg.Fixes.empty();
 }
 
 MATCHER_P3(Fix, Range, Replacement, Message,
            "Fix " + llvm::to_string(Range) + " => " +
                testing::PrintToString(Replacement) + " = [" + Message + "]") {
-  return arg.Diag.range == Range && arg.Diag.message == Message &&
-         arg.FixIts.size() == 1 && arg.FixIts[0].range == Range &&
-         arg.FixIts[0].newText == Replacement;
+  return arg.Range == Range && arg.Message == Message &&
+         arg.Fixes.size() == 1 && arg.Fixes[0].range == Range &&
+         arg.Fixes[0].newText == Replacement;
 }
 
 TEST(DiagnosticsTest, DiagnosticRanges) {
   // Check we report correct ranges, including various edge-cases.
   Annotations Test(R"cpp(
     void $decl[[foo]]();
     int main() {
       $typo[[go\
 o]]();
       foo()$semicolon[[]]
       $unk[[unknown]]();
     }
   )cpp");
-      llvm::errs() << Test.code();
-      EXPECT_THAT(
-          build(Test.code()).getDiagnostics(),
-          ElementsAre(
-              // This range spans lines.
-              Fix(Test.range("typo"), "foo",
-                  "use of undeclared identifier 'goo'; did you mean 'foo'?"),
-              // This is a pretty normal range.
-              Diag(Test.range("decl"), "'foo' declared here"),
-              // This range is zero-width, and at the end of a line.
-              Fix(Test.range("semicolon"), ";",
-                  "expected ';' after expression"),
-              // This range isn't provided by clang, we expand to the token.
-              Diag(Test.range("unk"),
-                   "use of undeclared identifier 'unknown'")));
+      buildDiags(Test.code()),
+          Fix(Test.range("semicolon"), ";", "expected ';' after expression"),
+          Diag(Test.range("unk"), "use of undeclared identifier 'unknown'")));
 }
 
 TEST(DiagnosticsTest, FlagsMatter) {
   Annotations Test("[[void]] main() {}");
   EXPECT_THAT(
-      build(Test.code()).getDiagnostics(),
+      buildDiags(Test.code()),
       ElementsAre(Fix(Test.range(), "int", "'main' must return 'int'")));
   // Same code built as C gets different diagnostics.
   EXPECT_THAT(
-      build(Test.code(), {"-x", "c"}).getDiagnostics(),
+      buildDiags(Test.code(), {"-x", "c"}),
       ElementsAre(
           // FIXME: ideally this would be one diagnostic with a named FixIt.
           Diag(Test.range(), "return type of 'main' is not 'int'"),
           Fix(Test.range(), "int", "change return type to 'int'")));
 }
 
 TEST(DiagnosticsTest, Preprocessor) {
   // This looks like a preamble, but there's an #else in the middle!
   // Check that:
   //  - the #else doesn't generate diagnostics (we had this bug)
   //  - we get diagnostics from the taken branch
   //  - we get no diagnostics from the not taken branch
   Annotations Test(R"cpp(
     #ifndef FOO
     #define FOO
       int a = [[b]];
     #else
       int x = y;
     #endif
     )cpp");
   EXPECT_THAT(
-      build(Test.code()).getDiagnostics(),
+      buildDiags(Test.code()),
       ElementsAre(Diag(Test.range(), "use of undeclared identifier 'b'")));
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang

unittests/clangd/CodeCompleteTests.cpp

 }
 
 namespace {
 using namespace llvm;
 using ::testing::AllOf;
 using ::testing::Contains;
 using ::testing::Each;
 using ::testing::ElementsAre;
+using ::testing::Field;
 using ::testing::Not;
 using ::testing::UnorderedElementsAre;
-using ::testing::Field;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
   void onDiagnosticsReady(
-      PathRef File, Tagged<std::vector<DiagWithFixIts>> Diagnostics) override {}
+      PathRef File, Tagged<std::vector<PersistentDiag>> Diagnostics) override {}
 };
 
 // GMock helpers for matching completion items.
 MATCHER_P(Named, Name, "") { return arg.insertText == Name; }
 MATCHER_P(Labeled, Label, "") { return arg.label == Label; }
 MATCHER_P(Kind, K, "") { return arg.kind == K; }
 MATCHER_P(Filter, F, "") { return arg.filterText == F; }
 MATCHER_P(Doc, D, "") { return arg.documentation == D; }

 
 TEST(CompletionTest, CompletionOptions) {
   auto Test = [&](const clangd::CodeCompleteOptions &Opts) {
     TestAfterDotCompletion(Opts);
     TestGlobalScopeCompletion(Opts);
   };
   // We used to test every combination of options, but that got too slow (2^N).
   auto Flags = {
       &clangd::CodeCompleteOptions::IncludeMacros,
       &clangd::CodeCompleteOptions::IncludeBriefComments,
       &clangd::CodeCompleteOptions::EnableSnippets,
       &clangd::CodeCompleteOptions::IncludeCodePatterns,
       &clangd::CodeCompleteOptions::IncludeIneligibleResults,
   };
   // Test default options.
   Test({});
   // Test with one flag flipped.
   for (auto &F : Flags) {
     clangd::CodeCompleteOptions O;
     O.*F ^= true;
     Test(O);
TEST(CompletionTest, IndexSuppressesPreambleCompletions) {
   auto ClassFromPreamble =
       runCodeComplete(Server, File, Test.point("2"), Opts).Value;
   EXPECT_THAT(ClassFromPreamble.items, Contains(Named("member")));
 
   Opts.Index = nullptr;
   auto WithoutIndex = runCodeComplete(Server, File, Test.point(), Opts).Value;
   EXPECT_THAT(WithoutIndex.items,
               UnorderedElementsAre(Named("local"), Named("preamble")));
-
 }
 
 TEST(CompletionTest, DynamicIndexMultiFile) {
   MockFSProvider FS;
   MockCompilationDatabase CDB;
   IgnoreDiagnostics DiagConsumer;
   auto Opts = ClangdServer::optsForTest();
   Opts.BuildDynamicSymbolIndex = true;

unittests/clangd/TUSchedulerTests.cpp

 #include <utility>
 
 namespace clang {
 namespace clangd {
 
 using ::testing::Pair;
 using ::testing::Pointee;
 
-void ignoreUpdate(llvm::Optional<std::vector<DiagWithFixIts>>) {}
+void ignoreUpdate(llvm::Optional<std::vector<PersistentDiag>>) {}
 void ignoreError(llvm::Error Err) {
   handleAllErrors(std::move(Err), [](const llvm::ErrorInfoBase &) {});
 }
 
 class TUSchedulerTests : public ::testing::Test {
 protected:
   ParseInputs getInputs(PathRef File, std::string Contents) {
     return ParseInputs{*CDB.getCompileCommand(File), buildTestFS(Files),
std::atomic<int> CallbackCount(0);
     Notification Ready;
     TUScheduler S(
         getDefaultAsyncThreadsCount(),
         /*StorePreamblesInMemory=*/true,
         /*ASTParsedCallback=*/nullptr,
         /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero());
     auto Path = testPath("foo.cpp");
     S.update(Path, getInputs(Path, ""), WantDiagnostics::Yes,
-             [&](std::vector<DiagWithFixIts>) { Ready.wait(); });
+             [&](std::vector<PersistentDiag>) { Ready.wait(); });
 
     S.update(Path, getInputs(Path, "request diags"), WantDiagnostics::Yes,
-             [&](std::vector<DiagWithFixIts> Diags) { ++CallbackCount; });
+             [&](std::vector<PersistentDiag> Diags) { ++CallbackCount; });
     S.update(Path, getInputs(Path, "auto (clobbered)"), WantDiagnostics::Auto,
-             [&](std::vector<DiagWithFixIts> Diags) {
+             [&](std::vector<PersistentDiag> Diags) {
                ADD_FAILURE() << "auto should have been cancelled by auto";
              });
     S.update(Path, getInputs(Path, "request no diags"), WantDiagnostics::No,
-             [&](std::vector<DiagWithFixIts> Diags) {
+             [&](std::vector<PersistentDiag> Diags) {
                ADD_FAILURE() << "no diags should not be called back";
              });
     S.update(Path, getInputs(Path, "auto (produces)"), WantDiagnostics::Auto,
-             [&](std::vector<DiagWithFixIts> Diags) { ++CallbackCount; });
+             [&](std::vector<PersistentDiag> Diags) { ++CallbackCount; });
     Ready.notify();
   }
   EXPECT_EQ(2, CallbackCount);
 }
 
 TEST_F(TUSchedulerTests, Debounce) {
   std::atomic<int> CallbackCount(0);
   {
     TUScheduler S(getDefaultAsyncThreadsCount(),
                   /*StorePreamblesInMemory=*/true,
                   /*ASTParsedCallback=*/nullptr,
                   /*UpdateDebounce=*/std::chrono::seconds(1));
     // FIXME: we could probably use timeouts lower than 1 second here.
     auto Path = testPath("foo.cpp");
     S.update(Path, getInputs(Path, "auto (debounced)"), WantDiagnostics::Auto,
-             [&](std::vector<DiagWithFixIts> Diags) {
+             [&](std::vector<PersistentDiag> Diags) {
                ADD_FAILURE() << "auto should have been debounced and canceled";
              });
     std::this_thread::sleep_for(std::chrono::milliseconds(200));
     S.update(Path, getInputs(Path, "auto (timed out)"), WantDiagnostics::Auto,
-             [&](std::vector<DiagWithFixIts> Diags) { ++CallbackCount; });
+             [&](std::vector<PersistentDiag> Diags) { ++CallbackCount; });
     std::this_thread::sleep_for(std::chrono::seconds(2));
     S.update(Path, getInputs(Path, "auto (shut down)"), WantDiagnostics::Auto,
-             [&](std::vector<DiagWithFixIts> Diags) { ++CallbackCount; });
+             [&](std::vector<PersistentDiag> Diags) { ++CallbackCount; });
   }
   EXPECT_EQ(2, CallbackCount);
 }
 
 TEST_F(TUSchedulerTests, ManyUpdates) {
   const int FilesCount = 3;
   const int UpdatesPerFile = 10;
 
for (int FileI = 0; FileI < FilesCount; ++FileI) {
 
         auto File = Files[FileI];
         auto Inputs = getInputs(File, Contents.str());
 
         {
           WithContextValue WithNonce(NonceKey, ++Nonce);
           S.update(File, Inputs, WantDiagnostics::Auto,
                    [Nonce, &Mut, &TotalUpdates](
-                       llvm::Optional<std::vector<DiagWithFixIts>> Diags) {
+                       llvm::Optional<std::vector<PersistentDiag>> Diags) {
                      EXPECT_THAT(Context::current().get(NonceKey),
                                  Pointee(Nonce));
 
                      std::lock_guard<std::mutex> Lock(Mut);
                      ++TotalUpdates;
                    });
         }
 

unittests/clangd/XRefsTests.cpp

 using testing::ElementsAre;
 using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
 using testing::UnorderedElementsAreArray;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
   void onDiagnosticsReady(
-      PathRef File, Tagged<std::vector<DiagWithFixIts>> Diagnostics) override {}
+      PathRef File, Tagged<std::vector<PersistentDiag>> Diagnostics) override {}
 };
 
 // FIXME: this is duplicated with FileIndexTests. Share it.
 ParsedAST build(StringRef Code) {
   auto CI = createInvocationFromCommandLine(
       {"clang", "-xc++", testPath("Foo.cpp").c_str()});
   auto Buf = MemoryBuffer::getMemBuffer(Code);
   auto AST = ParsedAST::Build(std::move(CI), nullptr, std::move(Buf),