[clangd] Don't use the optional "severity" when comparing Diagnostic.

[hokein]

Summary

We use Diagnostic as a key to find the corresponding FixIt when we do
the "apply-fix", but the "severity" field could be omitted, in some cases,
the codeAction request sent from LSP clients (e.g. VScode) doesn't include the
severity field, which makes clangd fail to find the FixIt.

Test the following code in VScode, before the fix, no FixIt shown.

void main() {}
^~~~

[ilya-biryukov]

Maybe we could use a struct containing only range and message (e.g. pair<range, string>) as a key to search for fixits instead of Diagnostic?
Having an operator < that does not take all fields into account seems shaky.

[sammccall]

Can you give some more details about the problem (maybe offline)?
In my testing, I do see a FixIt in VSCode. It fails to apply, for reasons I don't yet understand.

clangd/Protocol.h

325–333

This definitely needs:

• us to work out for sure whose bug it is (clangd, vscode, or LSP), and file the bug
• a comment referencing the bug

326

why removing ==? Even if unused, I'd lean towards keeping it for consistency with <

[hokein]

Update based on the offline discussion.

[hokein]

In D41280#956597, @sammccall wrote:

Can you give some more details about the problem (maybe offline)?
In my testing, I do see a FixIt in VSCode. It fails to apply, for reasons I don't yet understand.

As discussed offline, this is a regression issue of vscode-languageclient (from v3.0.0), has filed a bug to vscode team https://git.io/vbr29.

The current vscode-clangd extension is stale, which is not compatible with the latest clangd. If you use the latest client code in the clangd/clients/clangd-vscode together with this patch, you can see a FixIt, and can apply it successfully.

[hokein]

ping, in case you miss this patch.

[sammccall]

Sorry, I did miss it!

clangd/Protocol.h

326

nit: diagnostic

326

it seems like this could move from Protocol to ClangdLSPServer, but up to you

[hokein]

Fix typos.

clangd/ClangdLSPServer.h

private:
   bool ShutdownRequestReceived = false;
 
   /// Used to indicate that the 'exit' notification was received from the
   /// Language Server client.
   /// It's used to break out of the LSP parsing loop.
   bool IsDone = false;
 
   std::mutex FixItsMutex;
-  typedef std::map<clangd::Diagnostic, std::vector<TextEdit>>
+  typedef std::map<clangd::Diagnostic, std::vector<TextEdit>,
+                   LSPDiagnosticCompare>
       DiagnosticToReplacementMap;
   /// Caches FixIts per file and diagnostics
   llvm::StringMap<DiagnosticToReplacementMap> FixItsMap;
 
   // Various ClangdServer parameters go here. It's important they're created
   // before ClangdServer.
   DirectoryBasedGlobalCompilationDatabase CDB;
   RealFileSystemProvider FSProvider;

clangd/Protocol.h

struct Diagnostic {
 
   /// A human-readable string describing the source of this
   /// diagnostic, e.g. 'typescript' or 'super lint'.
   /// Note: Not currently used by clangd
   // std::string source;
 
   /// The diagnostic's message.
   std::string message;
-
-  friend bool operator==(const Diagnostic &LHS, const Diagnostic &RHS) {
-    return std::tie(LHS.range, LHS.severity, LHS.message) ==
-           std::tie(RHS.range, RHS.severity, RHS.message);
-  }
-  friend bool operator<(const Diagnostic &LHS, const Diagnostic &RHS) {
-    return std::tie(LHS.range, LHS.severity, LHS.message) <
-           std::tie(RHS.range, RHS.severity, RHS.message);
+};
+/// A LSP-specific comparator used to find Disgnostic in a container like
+/// std:map.
+/// We only uses the required fields of Diagnostic to do the comparsion to avoid
+/// any regression issues from LSP clients (e.g. VScode), see
+/// https://git.io/vbr29
+struct LSPDiagnosticCompare {
+  bool operator()(const Diagnostic& LHS, const Diagnostic& RHS) const {
+    return std::tie(LHS.range, LHS.message) < std::tie(RHS.range, RHS.message);
   }
 };
 bool fromJSON(const json::Expr &, Diagnostic &);
 
 struct CodeActionContext {
   /// An array of diagnostics.
   std::vector<Diagnostic> diagnostics;
 };