Add support for publishDiagnostics #4912
Conversation
| @@ -42,6 +42,10 @@ constexpr DiagnosticKind UntestedDiagnosticKinds[] = { | |||
|
|
|||
| // This is a little long but is tested in lex/numeric_literal_test.cpp. | |||
| DiagnosticKind::TooManyDigits, | |||
|
|
|||
| // TODO: The LSP doesn't support checking multiple files, figure something | |||
There was a problem hiding this comment.
I didn't understand exactly what this comment meant by checking multiple files -- is it saying that the LSP doesn't support producing diagnostics in files other than the current file?
There was a problem hiding this comment.
Yes, unsupported. If you look at LSP's Diagnostic, range is a text range, not including a filename. There is no filename on that field; it's implicitly the given file.
In theory, compiling a file could publish diagnostics for the other files it finds diagnostics for. However, that would create complexity trying to merge diagnostics if you had the other file open at the same time -- we wouldn't want to accidentally replace a file's diagnostics with some misc diagnostic produced by a different file.
I think there's a possibility we should just require that messages[0].loc.filename is always the same for a given file when we call CheckUnit, which would eliminate the need for this. In theory, "in import" messages might essentially get there, but I believe we have no enforcement. Any thoughts on making it a strict requirement?
In the meantime, rephrasing to try to make it clearer.
| context_->file_emitter().Emit( | ||
| params_.uri.file(), LanguageServerDiagnosticInWrongFile, | ||
| message.loc.filename.str(), stream.TakeStr()); |
There was a problem hiding this comment.
Is this recursing back into this same function to produce the error, or is this diagnostic going somewhere else? If this recurses back here, would it make sense to directly push a diagnostic instead?
There was a problem hiding this comment.
The emitters on context are hooked up to stderr, for diagnosing issues with the language server itself. I think you're mixing this up with the consumer declared inside SetText, which isn't added to context -- it's ephemeral.
| // CHECK:STDOUT: Content-Length: 144{{\r}} | ||
| // CHECK:STDOUT: {{\r}} | ||
| // CHECK:STDOUT: { | ||
| // CHECK:STDOUT: "jsonrpc": "2.0", | ||
| // CHECK:STDOUT: "method": "textDocument/publishDiagnostics", | ||
| // CHECK:STDOUT: "params": { | ||
| // CHECK:STDOUT: "diagnostics": [], | ||
| // CHECK:STDOUT: "uri": "file:///test.carbon" | ||
| // CHECK:STDOUT: } | ||
| // CHECK:STDOUT: } |
There was a problem hiding this comment.
Would it make sense to skip the publishDiagnostics step if there are none? Is it important that we do publish after a /didChange in the case where we used to have diagnostics and don't any more?
There was a problem hiding this comment.
Correct, need to publish to clear. Added a TODO to consider caching diagnostics and only publishing on change.
Requires the caching work in #4897