Page MenuHomePhabricator

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

Authored by hokein on Dec 15 2017, 3:54 AM.

Details

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() {}
^~~~

Diff Detail

Event Timeline

hokein created this revision.Dec 15 2017, 3:54 AM

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.

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 updated this revision to Diff 127129.Dec 15 2017, 7:19 AM
hokein marked 2 inline comments as done.

Update based on the offline discussion.

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.

ping, in case you miss this patch.

sammccall accepted this revision.Dec 19 2017, 7:49 AM

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

This revision is now accepted and ready to land.Dec 19 2017, 7:49 AM
hokein updated this revision to Diff 127586.Dec 19 2017, 12:33 PM
hokein marked an inline comment as done.

Fix typos.

This revision was automatically updated to reflect the committed changes.