[clangd] Include the diagnostics's code when comparing diagnostics

[nridge]

Summary

This fixes https://github.com/clangd/clangd/issues/60

[kadircet]

Could you also add a lit test for the case you mentioned in the github issue?

You can find pointers in clang-tools-extra/clangd/test/diagnostics.test and clang-tools-extra/clangd/test/fixits-codeaction.test

clang-tools-extra/clangd/Protocol.h

654 ↗

(On Diff #204702)

could you rather put code after range and message so that we somewhat preserve the ordering. I am not sure if it is used anywhere but looks like we are using an ordered container(std::map) to store these.

[nridge]

The fix actually broke another lit test, because in that test the client was not sending the diagnostic's code back in the codeAction request, resulting in a mismatch with the new comparison function.

I think a real client could potentially could this too (not send the code back), so I revised the fix to address this case, by only using the code in the comparison if it's present in both objects.

Also added the requested new lit test.

[nridge]

Revise fix and add test

[kadircet]

LGTM with one more test case request. Thanks!

clang-tools-extra/clangd/test/fixits-duplication.test

44 ↗

(On Diff #204939)

could you also add another codeAction request with one of the codes missing?

[nridge]

Add requested test case

clang-tools-extra/trunk/clangd/Protocol.h

 bool fromJSON(const llvm::json::Value &, DocumentFormattingParams &);
 
 struct DocumentSymbolParams {
   // The text document to find symbols in.
   TextDocumentIdentifier textDocument;
 };
 bool fromJSON(const llvm::json::Value &, DocumentSymbolParams &);
 
-
 /// Represents a related message and source code location for a diagnostic.
 /// This should be used to point to code locations that cause or related to a
 /// diagnostics, e.g when duplicating a symbol in a scope.
 struct DiagnosticRelatedInformation {
   /// The location of this related diagnostic information.
   Location location;
   /// The message of this related diagnostic information.
   std::string message;
struct Diagnostic {
   /// Only with capability textDocument.publishDiagnostics.codeActionsInline.
   /// (These actions can also be obtained using textDocument/codeAction).
   llvm::Optional<std::vector<CodeAction>> codeActions;
 };
 llvm::json::Value toJSON(const Diagnostic &);
 
 /// A LSP-specific comparator used to find diagnostic in a container like
 /// std:map.
-/// We only use 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
+/// We only use as many fields of Diagnostic as is needed to make distinct
+/// diagnostics unique in practice, to avoid  regression issues from LSP clients
+/// (e.g. VScode), see https://git.io/vbr29
 struct LSPDiagnosticCompare {
   bool operator()(const Diagnostic &LHS, const Diagnostic &RHS) const {
+    if (!LHS.code.empty() && !RHS.code.empty()) {
+      // If the code is known for both, use the code to diambiguate, as e.g.
+      // two checkers could produce diagnostics with the same range and message.
+      return std::tie(LHS.range, LHS.message, LHS.code) <
+             std::tie(RHS.range, RHS.message, RHS.code);
+    }
     return std::tie(LHS.range, LHS.message) < std::tie(RHS.range, RHS.message);
   }
 };
 bool fromJSON(const llvm::json::Value &, Diagnostic &);
 llvm::raw_ostream &operator<<(llvm::raw_ostream &, const Diagnostic &);
 
 struct CodeActionContext {
   /// An array of diagnostics.

clang-tools-extra/trunk/clangd/test/fixits-duplication.test

+# RUN: clangd -lit-test -clang-tidy-checks=modernize-use-nullptr,hicpp-use-nullptr < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument":{"codeAction":{"codeActionLiteralSupport":{}}}},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.cpp","languageId":"cpp","version":1,"text":"void foo() { char* p = 0; }"}}}
+#      CHECK:    "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:  "params": {
+# CHECK-NEXT:    "diagnostics": [
+# CHECK-NEXT:      {
+# CHECK-NEXT:        "code": "hicpp-use-nullptr",
+# CHECK-NEXT:        "message": "Use nullptr (fix available)",
+# CHECK-NEXT:        "range": {
+# CHECK-NEXT:          "end": {
+# CHECK-NEXT:            "character": 24,
+# CHECK-NEXT:            "line": 0
+# CHECK-NEXT:          },
+# CHECK-NEXT:          "start": {
+# CHECK-NEXT:            "character": 23,
+# CHECK-NEXT:            "line": 0
+# CHECK-NEXT:          }
+# CHECK-NEXT:        },
+# CHECK-NEXT:        "severity": 2,
+# CHECK-NEXT:        "source": "clang-tidy"
+# CHECK-NEXT:      },
+# CHECK-NEXT:      {
+# CHECK-NEXT:        "code": "modernize-use-nullptr",
+# CHECK-NEXT:        "message": "Use nullptr (fix available)",
+# CHECK-NEXT:        "range": {
+# CHECK-NEXT:          "end": {
+# CHECK-NEXT:            "character": 24,
+# CHECK-NEXT:            "line": 0
+# CHECK-NEXT:          },
+# CHECK-NEXT:          "start": {
+# CHECK-NEXT:            "character": 23,
+# CHECK-NEXT:            "line": 0
+# CHECK-NEXT:          }
+# CHECK-NEXT:        },
+# CHECK-NEXT:        "severity": 2,
+# CHECK-NEXT:        "source": "clang-tidy"
+# CHECK-NEXT:      }
+# CHECK-NEXT:    ],
+# CHECK-NEXT:    "uri": "file:///{{.*}}/foo.cpp"
+# CHECK-NEXT:  }
+---
+{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.cpp"},"range":{"start":{"line":0,"character":23},"end":{"line":0,"character":24}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 23}, "end": {"line": 0, "character": 24}},"severity":2,"message":"Use nullptr (fix available)", "code": "hicpp-use-nullptr", "source": "clang-tidy"},{"range":{"start": {"line": 0, "character": 23}, "end": {"line": 0, "character": 24}},"severity":2,"message":"Use nullptr (fix available)", "code": "modernize-use-nullptr", "source": "clang-tidy"}]}}}
+#      CHECK:  "id": 2,
+# CHECK-NEXT:  "jsonrpc": "2.0",
+# CHECK-NEXT:  "result": [
+# CHECK-NEXT:    {
+# CHECK-NEXT:      "diagnostics": [
+# CHECK-NEXT:        {
+# CHECK-NEXT:          "code": "hicpp-use-nullptr",
+# CHECK-NEXT:          "message": "Use nullptr (fix available)",
+# CHECK-NEXT:          "range": {
+# CHECK-NEXT:            "end": {
+# CHECK-NEXT:              "character": 24,
+# CHECK-NEXT:              "line": 0
+# CHECK-NEXT:            },
+# CHECK-NEXT:            "start": {
+# CHECK-NEXT:              "character": 23,
+# CHECK-NEXT:              "line": 0
+# CHECK-NEXT:            }
+# CHECK-NEXT:          },
+# CHECK-NEXT:          "severity": 2,
+# CHECK-NEXT:          "source": "clang-tidy"
+# CHECK-NEXT:        }
+# CHECK-NEXT:      ],
+# CHECK-NEXT:      "edit": {
+# CHECK-NEXT:        "changes": {
+# CHECK-NEXT:          "file://{{.*}}/foo.cpp": [
+# CHECK-NEXT:            {
+# CHECK-NEXT:              "newText": "nullptr",
+# CHECK-NEXT:              "range": {
+# CHECK-NEXT:                "end": {
+# CHECK-NEXT:                  "character": 24,
+# CHECK-NEXT:                  "line": 0
+# CHECK-NEXT:                },
+# CHECK-NEXT:                "start": {
+# CHECK-NEXT:                  "character": 23,
+# CHECK-NEXT:                  "line": 0
+# CHECK-NEXT:                }
+# CHECK-NEXT:              }
+# CHECK-NEXT:            }
+# CHECK-NEXT:          ]
+# CHECK-NEXT:        }
+# CHECK-NEXT:      },
+# CHECK-NEXT:      "kind": "quickfix",
+# CHECK-NEXT:      "title": "change '0' to 'nullptr'"
+# CHECK-NEXT:    },
+# CHECK-NEXT:    {
+# CHECK-NEXT:      "diagnostics": [
+# CHECK-NEXT:        {
+# CHECK-NEXT:          "code": "modernize-use-nullptr",
+# CHECK-NEXT:          "message": "Use nullptr (fix available)",
+# CHECK-NEXT:          "range": {
+# CHECK-NEXT:            "end": {
+# CHECK-NEXT:              "character": 24,
+# CHECK-NEXT:              "line": 0
+# CHECK-NEXT:            },
+# CHECK-NEXT:            "start": {
+# CHECK-NEXT:              "character": 23,
+# CHECK-NEXT:              "line": 0
+# CHECK-NEXT:            }
+# CHECK-NEXT:          },
+# CHECK-NEXT:          "severity": 2,
+# CHECK-NEXT:          "source": "clang-tidy"
+# CHECK-NEXT:        }
+# CHECK-NEXT:      ],
+# CHECK-NEXT:      "edit": {
+# CHECK-NEXT:        "changes": {
+# CHECK-NEXT:          "file://{{.*}}/foo.cpp": [
+# CHECK-NEXT:            {
+# CHECK-NEXT:              "newText": "nullptr",
+# CHECK-NEXT:              "range": {
+# CHECK-NEXT:                "end": {
+# CHECK-NEXT:                  "character": 24,
+# CHECK-NEXT:                  "line": 0
+# CHECK-NEXT:                },
+# CHECK-NEXT:                "start": {
+# CHECK-NEXT:                  "character": 23,
+# CHECK-NEXT:                  "line": 0
+# CHECK-NEXT:                }
+# CHECK-NEXT:              }
+# CHECK-NEXT:            }
+# CHECK-NEXT:          ]
+# CHECK-NEXT:        }
+# CHECK-NEXT:      },
+# CHECK-NEXT:      "kind": "quickfix",
+# CHECK-NEXT:      "title": "change '0' to 'nullptr'"
+# CHECK-NEXT:    }
+# CHECK-NEXT:  ]
+---
+{"jsonrpc":"2.0","id":3,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.cpp"},"range":{"start":{"line":0,"character":23},"end":{"line":0,"character":24}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 23}, "end": {"line": 0, "character": 24}},"severity":2,"message":"Use nullptr (fix available)", "source": "clang-tidy"},{"range":{"start": {"line": 0, "character": 23}, "end": {"line": 0, "character": 24}},"severity":2,"message":"Use nullptr (fix available)", "source": "clang-tidy"}]}}}
+#      CHECK:  "id": 3,
+# CHECK-NEXT:  "jsonrpc": "2.0",
+# CHECK-NEXT:  "result": [
+# CHECK-NEXT:    {
+# CHECK-NEXT:      "diagnostics": [
+# CHECK-NEXT:        {
+# CHECK-NEXT:          "message": "Use nullptr (fix available)",
+# CHECK-NEXT:          "range": {
+# CHECK-NEXT:            "end": {
+# CHECK-NEXT:              "character": 24,
+# CHECK-NEXT:              "line": 0
+# CHECK-NEXT:            },
+# CHECK-NEXT:            "start": {
+# CHECK-NEXT:              "character": 23,
+# CHECK-NEXT:              "line": 0
+# CHECK-NEXT:            }
+# CHECK-NEXT:          },
+# CHECK-NEXT:          "severity": 2,
+# CHECK-NEXT:          "source": "clang-tidy"
+# CHECK-NEXT:        }
+# CHECK-NEXT:      ],
+# CHECK-NEXT:      "edit": {
+# CHECK-NEXT:        "changes": {
+# CHECK-NEXT:          "file://{{.*}}/foo.cpp": [
+# CHECK-NEXT:            {
+# CHECK-NEXT:              "newText": "nullptr",
+# CHECK-NEXT:              "range": {
+# CHECK-NEXT:                "end": {
+# CHECK-NEXT:                  "character": 24,
+# CHECK-NEXT:                  "line": 0
+# CHECK-NEXT:                },
+# CHECK-NEXT:                "start": {
+# CHECK-NEXT:                  "character": 23,
+# CHECK-NEXT:                  "line": 0
+# CHECK-NEXT:                }
+# CHECK-NEXT:              }
+# CHECK-NEXT:            }
+# CHECK-NEXT:          ]
+# CHECK-NEXT:        }
+# CHECK-NEXT:      },
+# CHECK-NEXT:      "kind": "quickfix",
+# CHECK-NEXT:      "title": "change '0' to 'nullptr'"
+# CHECK-NEXT:    },
+# CHECK-NEXT:    {
+# CHECK-NEXT:      "diagnostics": [
+# CHECK-NEXT:        {
+# CHECK-NEXT:          "message": "Use nullptr (fix available)",
+# CHECK-NEXT:          "range": {
+# CHECK-NEXT:            "end": {
+# CHECK-NEXT:              "character": 24,
+# CHECK-NEXT:              "line": 0
+# CHECK-NEXT:            },
+# CHECK-NEXT:            "start": {
+# CHECK-NEXT:              "character": 23,
+# CHECK-NEXT:              "line": 0
+# CHECK-NEXT:            }
+# CHECK-NEXT:          },
+# CHECK-NEXT:          "severity": 2,
+# CHECK-NEXT:          "source": "clang-tidy"
+# CHECK-NEXT:        }
+# CHECK-NEXT:      ],
+# CHECK-NEXT:      "edit": {
+# CHECK-NEXT:        "changes": {
+# CHECK-NEXT:          "file://{{.*}}/foo.cpp": [
+# CHECK-NEXT:            {
+# CHECK-NEXT:              "newText": "nullptr",
+# CHECK-NEXT:              "range": {
+# CHECK-NEXT:                "end": {
+# CHECK-NEXT:                  "character": 24,
+# CHECK-NEXT:                  "line": 0
+# CHECK-NEXT:                },
+# CHECK-NEXT:                "start": {
+# CHECK-NEXT:                  "character": 23,
+# CHECK-NEXT:                  "line": 0
+# CHECK-NEXT:                }
+# CHECK-NEXT:              }
+# CHECK-NEXT:            }
+# CHECK-NEXT:          ]
+# CHECK-NEXT:        }
+# CHECK-NEXT:      },
+# CHECK-NEXT:      "kind": "quickfix",
+# CHECK-NEXT:      "title": "change '0' to 'nullptr'"
+# CHECK-NEXT:    }
+# CHECK-NEXT:  ]
+---
+{"jsonrpc":"2.0","id":4,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
+