comments

carbon-language · Feb 7, 2025 · b3f2151 · b3f2151
1 parent 20b9611
commit b3f2151
Show file tree

Hide file tree

Showing 4 changed files with 27 additions and 11 deletions.
diff --git a/toolchain/diagnostics/coverage_test.cpp b/toolchain/diagnostics/coverage_test.cpp
@@ -43,8 +43,13 @@ 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
-    // out when it does (or, more strongly restrict diagnostic structure).
+    // TODO: This can only fire if the first message in a diagnostic is rooted
+    // in a file other than the file being compiled. The language server
+    // currently only supports compiling one file at a time. Do one of:
+    // - When imports are supported, find a diagnostic whose first message isn't
+    //   in the current file.
+    // - Require all diagnostics produced by compiling have their first location
+    //   be in the file being compiled, never an import.
     DiagnosticKind::LanguageServerDiagnosticInWrongFile,
 };
 

diff --git a/toolchain/language_server/context.cpp b/toolchain/language_server/context.cpp
@@ -19,15 +19,18 @@
 
 namespace Carbon::LanguageServer {
 
+// A consumer for turning diagnostics into a `textDocument/publishDiagnostics`
+// notification.
+// https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_publishDiagnostics
 class PublishDiagnosticConsumer : public DiagnosticConsumer {
  public:
+  // Initializes params with the target file information.
   explicit PublishDiagnosticConsumer(Context* context,
                                      const clang::clangd::URIForFile& uri,
                                      std::optional<int64_t> version)
       : context_(context), params_{.uri = uri, .version = version} {}
 
-  auto params() -> llvm::json::Value { return params_; }
-
+  // Turns a diagnostic into an LSP diagnostic.
   auto HandleDiagnostic(Diagnostic diagnostic) -> void override {
     const auto& message = diagnostic.messages[0];
     if (message.loc.filename != params_.uri.file()) {
@@ -57,6 +60,9 @@ class PublishDiagnosticConsumer : public DiagnosticConsumer {
     // TODO: Figure out constructing URIs for note locations.
   }
 
+  // Returns the constructed request.
+  auto params() -> llvm::json::Value { return params_; }
+
  private:
   // Returns the LSP range for a diagnostic. Note that Carbon uses 1-based
   // numbers while LSP uses 0-based.
@@ -100,6 +106,7 @@ auto Context::File::SetText(Context& context, std::optional<int64_t> version,
   value_stores_.reset();
   source_.reset();
 
+  // A consumer to gather diagnostics for the file.
   PublishDiagnosticConsumer consumer(&context, uri_, version);
 
   // TODO: Make the processing asynchronous, to better handle rapid text
@@ -141,6 +148,9 @@ auto Context::File::SetText(Context& context, std::optional<int64_t> version,
   Check::CheckParseTrees(units, /*prelude_import=*/false, fs,
                          context.vlog_stream(), /*fuzzing=*/false);
 
+  // Note we need to publish diagnostics even when empty.
+  // TODO: Consider caching previously published diagnostics and only publishing
+  // when they change.
   context.outgoing().notify("textDocument/publishDiagnostics",
                             consumer.params());
 }

diff --git a/toolchain/language_server/context.h b/toolchain/language_server/context.h
@@ -30,7 +30,8 @@ class Context {
     explicit File(clang::clangd::URIForFile uri) : uri_(std::move(uri)) {}
 
     // Changes the file's text, updating dependent state.
-    auto SetText(Context& context, std::optional<int64_t> version, llvm::StringRef text) -> void;
+    auto SetText(Context& context, std::optional<int64_t> version,
+                 llvm::StringRef text) -> void;
 
     auto tree_and_subtrees() const -> const Parse::TreeAndSubtrees& {
       return *tree_and_subtrees_;

diff --git a/toolchain/language_server/handle_text_document.cpp b/toolchain/language_server/handle_text_document.cpp
@@ -7,8 +7,8 @@
 namespace Carbon::LanguageServer {
 
 auto HandleDidOpenTextDocument(
-    Context& context,
-    const clang::clangd::DidOpenTextDocumentParams& params) -> void {
+    Context& context, const clang::clangd::DidOpenTextDocumentParams& params)
+    -> void {
   llvm::StringRef filename = params.textDocument.uri.file();
   if (!filename.ends_with(".carbon")) {
     // Ignore non-Carbon files.
@@ -27,8 +27,8 @@ auto HandleDidOpenTextDocument(
 }
 
 auto HandleDidChangeTextDocument(
-    Context& context,
-    const clang::clangd::DidChangeTextDocumentParams& params) -> void {
+    Context& context, const clang::clangd::DidChangeTextDocumentParams& params)
+    -> void {
   llvm::StringRef filename = params.textDocument.uri.file();
   if (!filename.ends_with(".carbon")) {
     // Ignore non-Carbon files.
@@ -50,8 +50,8 @@ auto HandleDidChangeTextDocument(
 }
 
 auto HandleDidCloseTextDocument(
-    Context& context,
-    const clang::clangd::DidCloseTextDocumentParams& params) -> void {
+    Context& context, const clang::clangd::DidCloseTextDocumentParams& params)
+    -> void {
   llvm::StringRef filename = params.textDocument.uri.file();
   if (!filename.ends_with(".carbon")) {
     // Ignore non-Carbon files.