From 16b2cafae1e04a7783d0e9de4361c4ab5c49dd45 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Thu, 30 Jan 2025 10:34:03 -0800 Subject: [PATCH] Clean up language server's output handling (#4855) Handles verbose logging and printing errors as diagnostics to stderr when there's no way to communicate them back on a request. --- toolchain/diagnostics/diagnostic_kind.def | 2 + .../driver/language_server_subcommand.cpp | 6 +-- toolchain/language_server/BUILD | 3 ++ toolchain/language_server/context.h | 9 +++++ .../language_server/incoming_messages.cpp | 28 ++++++++++++- toolchain/language_server/incoming_messages.h | 10 ++--- toolchain/language_server/language_server.cpp | 39 ++++++++++++++++--- toolchain/language_server/language_server.h | 5 ++- .../testdata/parse_error.carbon | 23 +++++++++++ .../testdata/unexpected_reply.carbon | 23 +++++++++++ .../testdata/unsupported_call.carbon | 27 +++++++++++++ .../testdata/unsupported_notification.carbon | 20 ++++++++++ .../language_server/testdata/verbose.carbon | 23 +++++++++++ 13 files changed, 200 insertions(+), 18 deletions(-) create mode 100644 toolchain/language_server/testdata/parse_error.carbon create mode 100644 toolchain/language_server/testdata/unexpected_reply.carbon create mode 100644 toolchain/language_server/testdata/unsupported_call.carbon create mode 100644 toolchain/language_server/testdata/unsupported_notification.carbon create mode 100644 toolchain/language_server/testdata/verbose.carbon diff --git a/toolchain/diagnostics/diagnostic_kind.def b/toolchain/diagnostics/diagnostic_kind.def index 74bc7fd8b741..d245d29eb810 100644 --- a/toolchain/diagnostics/diagnostic_kind.def +++ b/toolchain/diagnostics/diagnostic_kind.def @@ -30,6 +30,8 @@ CARBON_DIAGNOSTIC_KIND(CompileOutputFileOpenError) CARBON_DIAGNOSTIC_KIND(FormatMultipleFilesToOneOutput) CARBON_DIAGNOSTIC_KIND(LanguageServerMissingInputStream) CARBON_DIAGNOSTIC_KIND(LanguageServerTransportError) +CARBON_DIAGNOSTIC_KIND(LanguageServerUnexpectedReply) +CARBON_DIAGNOSTIC_KIND(LanguageServerUnsupportedNotification) // ============================================================================ // SourceBuffer diagnostics diff --git a/toolchain/driver/language_server_subcommand.cpp b/toolchain/driver/language_server_subcommand.cpp index ef88aad89ad9..0a0319cbf4cb 100644 --- a/toolchain/driver/language_server_subcommand.cpp +++ b/toolchain/driver/language_server_subcommand.cpp @@ -27,9 +27,9 @@ auto LanguageServerSubcommand::Run(DriverEnv& driver_env) -> DriverResult { return {.success = false}; } - bool success = - LanguageServer::Run(driver_env.input_stream, *driver_env.output_stream, - *driver_env.error_stream, driver_env.consumer); + bool success = LanguageServer::Run( + driver_env.input_stream, *driver_env.output_stream, + *driver_env.error_stream, driver_env.vlog_stream, driver_env.consumer); return {.success = success}; } diff --git a/toolchain/language_server/BUILD b/toolchain/language_server/BUILD index 0da239e43395..3becd4c83a11 100644 --- a/toolchain/language_server/BUILD +++ b/toolchain/language_server/BUILD @@ -31,6 +31,7 @@ cc_library( hdrs = ["context.h"], deps = [ "//common:map", + "//toolchain/diagnostics:diagnostic_emitter", ], ) @@ -60,6 +61,8 @@ cc_library( ":handle", "//common:check", "//common:map", + "//common:ostream", + "//common:raw_string_ostream", "@llvm-project//clang-tools-extra/clangd:ClangDaemon", ], ) diff --git a/toolchain/language_server/context.h b/toolchain/language_server/context.h index 04f51b6cd97f..ab544e17a3d4 100644 --- a/toolchain/language_server/context.h +++ b/toolchain/language_server/context.h @@ -8,15 +8,24 @@ #include #include "common/map.h" +#include "toolchain/diagnostics/diagnostic_consumer.h" +#include "toolchain/diagnostics/diagnostic_emitter.h" namespace Carbon::LanguageServer { // Context for LSP call handling. class Context { public: + // `consumer` is required. + explicit Context(DiagnosticConsumer* consumer) : no_loc_emitter_(consumer) {} + + auto no_loc_emitter() -> NoLocDiagnosticEmitter& { return no_loc_emitter_; } + auto files() -> Map& { return files_; } private: + NoLocDiagnosticEmitter no_loc_emitter_; + // Content of files managed by the language client. Map files_; }; diff --git a/toolchain/language_server/incoming_messages.cpp b/toolchain/language_server/incoming_messages.cpp index f628a43c9957..a355bbb9ec65 100644 --- a/toolchain/language_server/incoming_messages.cpp +++ b/toolchain/language_server/incoming_messages.cpp @@ -4,6 +4,8 @@ #include "toolchain/language_server/incoming_messages.h" +#include "common/ostream.h" +#include "common/raw_string_ostream.h" #include "toolchain/language_server/handle.h" namespace Carbon::LanguageServer { @@ -83,7 +85,7 @@ auto IncomingMessages::onCall(llvm::StringRef name, llvm::json::Value params, }); } else { transport_->reply(id, llvm::make_error( - llvm::formatv("call `{0}` not found", name), + llvm::formatv("unsupported call `{0}`", name), clang::clangd::ErrorCode::MethodNotFound)); } @@ -98,10 +100,32 @@ auto IncomingMessages::onNotify(llvm::StringRef name, llvm::json::Value value) if (auto result = notification_handlers_.Lookup(name)) { (result.value())(*context_, std::move(value)); } else { - clang::clangd::log("notification `{0}` not found", name); + CARBON_DIAGNOSTIC(LanguageServerUnsupportedNotification, Warning, + "unsupported notification `{0}`", std::string); + context_->no_loc_emitter().Emit(LanguageServerUnsupportedNotification, + name.str()); } return true; } +auto IncomingMessages::onReply(llvm::json::Value id, + llvm::Expected result) + -> bool { + RawStringOstream id_str; + id_str << id; + RawStringOstream result_str; + if (result) { + result_str << result.get(); + } else { + result_str << result.takeError(); + } + CARBON_DIAGNOSTIC(LanguageServerUnexpectedReply, Warning, + "unexpected reply to request ID {0}: {1}", std::string, + std::string); + context_->no_loc_emitter().Emit(LanguageServerUnexpectedReply, + id_str.TakeStr(), result_str.TakeStr()); + return true; +} + } // namespace Carbon::LanguageServer diff --git a/toolchain/language_server/incoming_messages.h b/toolchain/language_server/incoming_messages.h index fcf90a71109a..407dc98a7f94 100644 --- a/toolchain/language_server/incoming_messages.h +++ b/toolchain/language_server/incoming_messages.h @@ -25,7 +25,8 @@ class IncomingMessages : public clang::clangd::Transport::MessageHandler { explicit IncomingMessages(clang::clangd::Transport* transport, Context* context); - // Dispatches calls to the appropriate entry in `call_handlers_`. + // Dispatches calls to the appropriate entry in `call_handlers_`. Always + // returns true. auto onCall(llvm::StringRef name, llvm::json::Value params, llvm::json::Value id) -> bool override; @@ -33,12 +34,9 @@ class IncomingMessages : public clang::clangd::Transport::MessageHandler { // `notification_handlers_`, except for `exit` which directly returns false. auto onNotify(llvm::StringRef name, llvm::json::Value value) -> bool override; - // Handles replies. - // TODO: Implement when needed. + // Handles replies. Always returns true. auto onReply(llvm::json::Value /*id*/, - llvm::Expected /*result*/) -> bool override { - return true; - } + llvm::Expected /*result*/) -> bool override; private: // These are the signatures expected for handlers. diff --git a/toolchain/language_server/language_server.cpp b/toolchain/language_server/language_server.cpp index 02e02f6dfd07..e55182e98dd1 100644 --- a/toolchain/language_server/language_server.cpp +++ b/toolchain/language_server/language_server.cpp @@ -15,12 +15,39 @@ namespace Carbon::LanguageServer { +// An adapter for clangd's logging that sends most messages to `error_stream`. +// Verbose logging is only printed if `vlog_stream` is provided, and will be +// sent there. +class Logger : public clang::clangd::Logger { + public: + explicit Logger(llvm::raw_ostream* error_stream, + llvm::raw_ostream* vlog_stream) + : error_logger_(*error_stream, clang::clangd::Logger::Info), + vlog_logger_(vlog_stream + ? std::make_unique( + *vlog_stream, clang::clangd::Logger::Verbose) + : nullptr) {} + + auto log(Level level, const char* format, + const llvm::formatv_object_base& message) -> void override { + if (level != clang::clangd::Logger::Verbose) { + error_logger_.log(level, format, message); + } else if (vlog_logger_) { + vlog_logger_->log(level, format, message); + } + } + + private: + clang::clangd::StreamLogger error_logger_; + std::unique_ptr vlog_logger_; +}; + auto Run(FILE* input_stream, llvm::raw_ostream& output_stream, - llvm::raw_ostream& error_stream, DiagnosticConsumer& consumer) - -> bool { - // TODO: Consider implementing a custom logger that splits vlog to - // vlog_stream when provided. For now, this disables verbose logging. - clang::clangd::StreamLogger logger(error_stream, clang::clangd::Logger::Info); + llvm::raw_ostream& error_stream, llvm::raw_ostream* vlog_stream, + DiagnosticConsumer& consumer) -> bool { + // The language server internally uses diagnostics for logging issues, but the + // clangd parts have their own logging system. We intercept that here. + Logger logger(&error_stream, vlog_stream); clang::clangd::LoggingSession logging_session(logger); // Set up the connection. @@ -28,7 +55,7 @@ auto Run(FILE* input_stream, llvm::raw_ostream& output_stream, clang::clangd::newJSONTransport(input_stream, output_stream, /*InMirror=*/nullptr, /*Pretty=*/true)); - Context context; + Context context(&consumer); // TODO: Use error_stream in IncomingMessages to report dropped errors. IncomingMessages incoming(transport.get(), &context); OutgoingMessages outgoing(transport.get()); diff --git a/toolchain/language_server/language_server.h b/toolchain/language_server/language_server.h index 1836fdb660f3..5f233997e967 100644 --- a/toolchain/language_server/language_server.h +++ b/toolchain/language_server/language_server.h @@ -13,8 +13,11 @@ namespace Carbon::LanguageServer { // Start the language server. input_stream and output_stream are used by LSP; // error_stream is primarily for errors that don't fit into LSP. Returns true if // the server cleanly exits. +// +// This is thread-hostile because `clangd::LoggingSession` relies on a global. auto Run(FILE* input_stream, llvm::raw_ostream& output_stream, - llvm::raw_ostream& error_stream, DiagnosticConsumer& consumer) -> bool; + llvm::raw_ostream& error_stream, llvm::raw_ostream* vlog_stream, + DiagnosticConsumer& consumer) -> bool; } // namespace Carbon::LanguageServer diff --git a/toolchain/language_server/testdata/parse_error.carbon b/toolchain/language_server/testdata/parse_error.carbon new file mode 100644 index 000000000000..0952137eb05e --- /dev/null +++ b/toolchain/language_server/testdata/parse_error.carbon @@ -0,0 +1,23 @@ +// Part of the Carbon Language project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +// AUTOUPDATE +// TIP: To test this file alone, run: +// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/language_server/testdata/parse_error.carbon +// TIP: To dump output, run: +// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/language_server/testdata/parse_error.carbon + + +// --- STDIN +[[@LSP-REPLY:1]] +[[@LSP-REPLY:2:"result": 3]] +[[@LSP-NOTIFY:exit]] + +// --- AUTOUPDATE-SPLIT + +// CHECK:STDERR: warning: unexpected reply to request ID "1": null [LanguageServerUnexpectedReply] +// CHECK:STDERR: +// CHECK:STDERR: warning: unexpected reply to request ID "2": 3 [LanguageServerUnexpectedReply] +// CHECK:STDERR: +// CHECK:STDOUT: diff --git a/toolchain/language_server/testdata/unexpected_reply.carbon b/toolchain/language_server/testdata/unexpected_reply.carbon new file mode 100644 index 000000000000..e03550ae69f4 --- /dev/null +++ b/toolchain/language_server/testdata/unexpected_reply.carbon @@ -0,0 +1,23 @@ +// Part of the Carbon Language project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +// AUTOUPDATE +// TIP: To test this file alone, run: +// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/language_server/testdata/unexpected_reply.carbon +// TIP: To dump output, run: +// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/language_server/testdata/unexpected_reply.carbon + + +// --- STDIN +[[@LSP-REPLY:1]] +[[@LSP-REPLY:2:"value": 3]] +[[@LSP-NOTIFY:exit]] + +// --- AUTOUPDATE-SPLIT + +// CHECK:STDERR: warning: unexpected reply to request ID "1": null [LanguageServerUnexpectedReply] +// CHECK:STDERR: +// CHECK:STDERR: warning: unexpected reply to request ID "2": null [LanguageServerUnexpectedReply] +// CHECK:STDERR: +// CHECK:STDOUT: diff --git a/toolchain/language_server/testdata/unsupported_call.carbon b/toolchain/language_server/testdata/unsupported_call.carbon new file mode 100644 index 000000000000..d7417f246a5c --- /dev/null +++ b/toolchain/language_server/testdata/unsupported_call.carbon @@ -0,0 +1,27 @@ +// Part of the Carbon Language project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +// AUTOUPDATE +// TIP: To test this file alone, run: +// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/language_server/testdata/unsupported_call.carbon +// TIP: To dump output, run: +// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/language_server/testdata/unsupported_call.carbon + + +// --- STDIN +[[@LSP:unknown-call]] +[[@LSP-NOTIFY:exit]] + +// --- AUTOUPDATE-SPLIT + +// CHECK:STDOUT: Content-Length: 122{{\r}} +// CHECK:STDOUT: {{\r}} +// CHECK:STDOUT: { +// CHECK:STDOUT: "error": { +// CHECK:STDOUT: "code": -32601, +// CHECK:STDOUT: "message": "unsupported call `unknown-call`" +// CHECK:STDOUT: }, +// CHECK:STDOUT: "id": "1", +// CHECK:STDOUT: "jsonrpc": "2.0" +// CHECK:STDOUT: } diff --git a/toolchain/language_server/testdata/unsupported_notification.carbon b/toolchain/language_server/testdata/unsupported_notification.carbon new file mode 100644 index 000000000000..7d48987348f0 --- /dev/null +++ b/toolchain/language_server/testdata/unsupported_notification.carbon @@ -0,0 +1,20 @@ +// Part of the Carbon Language project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +// AUTOUPDATE +// TIP: To test this file alone, run: +// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/language_server/testdata/unsupported_notification.carbon +// TIP: To dump output, run: +// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/language_server/testdata/unsupported_notification.carbon + + +// --- STDIN +[[@LSP-NOTIFY:unknown-notify]] +[[@LSP-NOTIFY:exit]] + +// --- AUTOUPDATE-SPLIT + +// CHECK:STDERR: warning: unsupported notification `unknown-notify` [LanguageServerUnsupportedNotification] +// CHECK:STDERR: +// CHECK:STDOUT: diff --git a/toolchain/language_server/testdata/verbose.carbon b/toolchain/language_server/testdata/verbose.carbon new file mode 100644 index 000000000000..4ec934cbc7e7 --- /dev/null +++ b/toolchain/language_server/testdata/verbose.carbon @@ -0,0 +1,23 @@ +// Part of the Carbon Language project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +// ARGS: -v language-server +// +// Verbose output includes a timestamp. +// NOAUTOUPDATE +// TIP: To test this file alone, run: +// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/language_server/testdata/verbose.carbon +// TIP: To dump output, run: +// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/language_server/testdata/verbose.carbon + +// CHECK:STDERR: V[{{.+}}] <<< { +// CHECK:STDERR: "jsonrpc": "2.0", +// CHECK:STDERR: "method": "exit" +// CHECK:STDERR: } +// CHECK:STDERR: +// CHECK:STDERR: +// CHECK:STDOUT: + +// --- STDIN +[[@LSP-NOTIFY:exit:]]