From 20c20595bad32a63b419610af264f4a4ca2bd12d Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Tue, 3 Jun 2025 15:09:51 -0700 Subject: [PATCH] Fix language-server crash with cpp_ast (#5604) Removes the nullptr default for safety. Note, I think cpp support doesn't allow things like `` or inline code yet, and language-server support doesn't allow non-hermetic files, so the best I can test is an error. --- toolchain/check/check.h | 8 +- toolchain/diagnostics/coverage_test.cpp | 23 +++--- toolchain/language_server/context.cpp | 11 ++- .../open_with_cpp_nonexistent.carbon | 76 +++++++++++++++++++ 4 files changed, 102 insertions(+), 16 deletions(-) create mode 100644 toolchain/language_server/testdata/text_document/open_with_cpp_nonexistent.carbon diff --git a/toolchain/check/check.h b/toolchain/check/check.h index 3a64b74a2026..983e3da85dc6 100644 --- a/toolchain/check/check.h +++ b/toolchain/check/check.h @@ -15,7 +15,8 @@ namespace Carbon::Check { -// Checking information that's tracked per file. +// Checking information that's tracked per file. All members are caller-owned. +// Other than `timings`, members must be non-null. struct Unit { Diagnostics::Consumer* consumer; SharedValueStores* value_stores; @@ -25,8 +26,9 @@ struct Unit { // The unit's SemIR, provided as empty and filled in by CheckParseTrees. SemIR::File* sem_ir; - // The Clang AST owned by `CompileSubcommand`. - std::unique_ptr* cpp_ast = nullptr; + // Storage for the unit's Clang AST. The unique_ptr should start empty, and + // can be assigned as part of checking. + std::unique_ptr* cpp_ast; }; // Checks a group of parse trees. This will use imports to decide the order of diff --git a/toolchain/diagnostics/coverage_test.cpp b/toolchain/diagnostics/coverage_test.cpp index 6d261274816c..ff639ec79224 100644 --- a/toolchain/diagnostics/coverage_test.cpp +++ b/toolchain/diagnostics/coverage_test.cpp @@ -19,6 +19,20 @@ constexpr Kind Kinds[] = { #include "toolchain/diagnostics/diagnostic_kind.def" }; +// TODO: LanguageServerDiagnosticInWrongFile currently has coverage, but +// mainly due to incorrect behavior in C++ diagnostics. See +// language_server/testdata/text_document/open_with_cpp_nonexistent.carbon. +// Leaving this TODO here until it's more precisely tested, because the C++ +// diagnostics should be fixed (removing coverage); see below TODO. +// +// 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. +// Kind::LanguageServerDiagnosticInWrongFile, constexpr Kind UntestedKinds[] = { // These exist only for unit tests. Kind::TestDiagnostic, @@ -42,15 +56,6 @@ constexpr Kind UntestedKinds[] = { // This is a little long but is tested in lex/numeric_literal_test.cpp. Kind::TooManyDigits, - - // 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. - Kind::LanguageServerDiagnosticInWrongFile, }; // Looks for diagnostic kinds that aren't covered by a file_test. diff --git a/toolchain/language_server/context.cpp b/toolchain/language_server/context.cpp index 204a7fe5b28d..61fb295a8796 100644 --- a/toolchain/language_server/context.cpp +++ b/toolchain/language_server/context.cpp @@ -140,14 +140,17 @@ auto Context::File::SetText(Context& context, std::optional version, SemIR::File sem_ir(tree_.get(), SemIR::CheckIRId(0), tree_->packaging_decl(), *value_stores_, uri_.file().str()); - auto getter = [this]() -> const Parse::TreeAndSubtrees& { - return *tree_and_subtrees_; - }; + std::unique_ptr cpp_ast; // TODO: Support cross-file checking when multiple files have edits. llvm::SmallVector units = {{{.consumer = &consumer, .value_stores = value_stores_.get(), .timings = nullptr, - .sem_ir = &sem_ir}}}; + .sem_ir = &sem_ir, + .cpp_ast = &cpp_ast}}}; + + auto getter = [this]() -> const Parse::TreeAndSubtrees& { + return *tree_and_subtrees_; + }; llvm::IntrusiveRefCntPtr fs = new llvm::vfs::InMemoryFileSystem; // TODO: Include the prelude. diff --git a/toolchain/language_server/testdata/text_document/open_with_cpp_nonexistent.carbon b/toolchain/language_server/testdata/text_document/open_with_cpp_nonexistent.carbon new file mode 100644 index 000000000000..b36408d28e4f --- /dev/null +++ b/toolchain/language_server/testdata/text_document/open_with_cpp_nonexistent.carbon @@ -0,0 +1,76 @@ +// 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 +// +// TODO: At present, this has a +// "dropping diagnostic in /test.carbon.generated.cpp_imports.h". That should be +// fixed to assign back to this file, but that may come from other fixes to C++ +// diagnostic output. +// +// AUTOUPDATE +// TIP: To test this file alone, run: +// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/language_server/testdata/text_document/open_with_cpp_nonexistent.carbon +// TIP: To dump output, run: +// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/language_server/testdata/text_document/open_with_cpp_nonexistent.carbon + +// --- STDIN +[[@LSP-CALL:initialize]] +[[@LSP-NOTIFY:textDocument/didOpen: + "textDocument": {"uri": "file:/test.carbon", "languageId": "carbon", + "text": "import Cpp library \"nonexistent.h\";"} +]] +[[@LSP-NOTIFY:textDocument/didClose: + "textDocument": {"uri": "file:/test.carbon"} +]] +[[@LSP-CALL:shutdown]] +[[@LSP-NOTIFY:exit]] + +// --- AUTOUPDATE-SPLIT + +// CHECK:STDERR: /test.carbon: warning: dropping diagnostic in /test.carbon.generated.cpp_imports.h: +// CHECK:STDERR: /test.carbon.generated.cpp_imports.h:1: error: C++: +// CHECK:STDERR: /test.carbon.generated.cpp_imports.h:1:10: fatal error: 'nonexistent.h' file not found +// CHECK:STDERR: 1 | #include "nonexistent.h" +// CHECK:STDERR: | ^~~~~~~~~~~~~~~ +// CHECK:STDERR: +// CHECK:STDERR: /test.carbon:1:1: note: in `Cpp` import +// CHECK:STDERR: import Cpp library "nonexistent.h"; +// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +// CHECK:STDERR: [LanguageServerDiagnosticInWrongFile] +// CHECK:STDERR: +// CHECK:STDOUT: Content-Length: 146{{\r}} +// CHECK:STDOUT: {{\r}} +// CHECK:STDOUT: { +// CHECK:STDOUT: "id": 1, +// CHECK:STDOUT: "jsonrpc": "2.0", +// CHECK:STDOUT: "result": { +// CHECK:STDOUT: "capabilities": { +// CHECK:STDOUT: "documentSymbolProvider": true, +// CHECK:STDOUT: "textDocumentSync": 2 +// CHECK:STDOUT: } +// CHECK:STDOUT: } +// 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: }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: }Content-Length: 51{{\r}} +// CHECK:STDOUT: {{\r}} +// CHECK:STDOUT: { +// CHECK:STDOUT: "id": 2, +// CHECK:STDOUT: "jsonrpc": "2.0", +// CHECK:STDOUT: "result": null +// CHECK:STDOUT: }