Skip to content

Commit 20c2059

Browse files
jonmeowjonmeow
authored
Fix language-server crash with cpp_ast (#5604)
Removes the nullptr default for safety. Note, I think cpp support doesn't allow things like `<version>` or inline code yet, and language-server support doesn't allow non-hermetic files, so the best I can test is an error.
1 parent c0cdc71 commit 20c2059

File tree

4 files changed

+102
-16
lines changed

4 files changed

+102
-16
lines changed

‎toolchain/check/check.h

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,8 @@
1515

1616
namespace Carbon::Check {
1717

18-
// Checking information that's tracked per file.
18+
// Checking information that's tracked per file. All members are caller-owned.
19+
// Other than `timings`, members must be non-null.
1920
struct Unit {
2021
Diagnostics::Consumer* consumer;
2122
SharedValueStores* value_stores;
@@ -25,8 +26,9 @@ struct Unit {
2526
// The unit's SemIR, provided as empty and filled in by CheckParseTrees.
2627
SemIR::File* sem_ir;
2728

28-
// The Clang AST owned by `CompileSubcommand`.
29-
std::unique_ptr<clang::ASTUnit>* cpp_ast = nullptr;
29+
// Storage for the unit's Clang AST. The unique_ptr should start empty, and
30+
// can be assigned as part of checking.
31+
std::unique_ptr<clang::ASTUnit>* cpp_ast;
3032
};
3133

3234
// Checks a group of parse trees. This will use imports to decide the order of

‎toolchain/diagnostics/coverage_test.cpp

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,20 @@ constexpr Kind Kinds[] = {
1919
#include "toolchain/diagnostics/diagnostic_kind.def"
2020
};
2121

22+
// TODO: LanguageServerDiagnosticInWrongFile currently has coverage, but
23+
// mainly due to incorrect behavior in C++ diagnostics. See
24+
// language_server/testdata/text_document/open_with_cpp_nonexistent.carbon.
25+
// Leaving this TODO here until it's more precisely tested, because the C++
26+
// diagnostics should be fixed (removing coverage); see below TODO.
27+
//
28+
// TODO: This can only fire if the first message in a diagnostic is rooted
29+
// in a file other than the file being compiled. The language server
30+
// currently only supports compiling one file at a time. Do one of:
31+
// - When imports are supported, find a diagnostic whose first message isn't
32+
// in the current file.
33+
// - Require all diagnostics produced by compiling have their first location
34+
// be in the file being compiled, never an import.
35+
// Kind::LanguageServerDiagnosticInWrongFile,
2236
constexpr Kind UntestedKinds[] = {
2337
// These exist only for unit tests.
2438
Kind::TestDiagnostic,
@@ -42,15 +56,6 @@ constexpr Kind UntestedKinds[] = {
4256

4357
// This is a little long but is tested in lex/numeric_literal_test.cpp.
4458
Kind::TooManyDigits,
45-
46-
// TODO: This can only fire if the first message in a diagnostic is rooted
47-
// in a file other than the file being compiled. The language server
48-
// currently only supports compiling one file at a time. Do one of:
49-
// - When imports are supported, find a diagnostic whose first message isn't
50-
// in the current file.
51-
// - Require all diagnostics produced by compiling have their first location
52-
// be in the file being compiled, never an import.
53-
Kind::LanguageServerDiagnosticInWrongFile,
5459
};
5560

5661
// Looks for diagnostic kinds that aren't covered by a file_test.

‎toolchain/language_server/context.cpp

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -140,14 +140,17 @@ auto Context::File::SetText(Context& context, std::optional<int64_t> version,
140140

141141
SemIR::File sem_ir(tree_.get(), SemIR::CheckIRId(0), tree_->packaging_decl(),
142142
*value_stores_, uri_.file().str());
143-
auto getter = [this]() -> const Parse::TreeAndSubtrees& {
144-
return *tree_and_subtrees_;
145-
};
143+
std::unique_ptr<clang::ASTUnit> cpp_ast;
146144
// TODO: Support cross-file checking when multiple files have edits.
147145
llvm::SmallVector<Check::Unit> units = {{{.consumer = &consumer,
148146
.value_stores = value_stores_.get(),
149147
.timings = nullptr,
150-
.sem_ir = &sem_ir}}};
148+
.sem_ir = &sem_ir,
149+
.cpp_ast = &cpp_ast}}};
150+
151+
auto getter = [this]() -> const Parse::TreeAndSubtrees& {
152+
return *tree_and_subtrees_;
153+
};
151154
llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> fs =
152155
new llvm::vfs::InMemoryFileSystem;
153156
// TODO: Include the prelude.

‎toolchain/language_server/testdata/text_document/open_with_cpp_nonexistent.carbon

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
2+
// Exceptions. See /LICENSE for license information.
3+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
4+
//
5+
// TODO: At present, this has a
6+
// "dropping diagnostic in /test.carbon.generated.cpp_imports.h". That should be
7+
// fixed to assign back to this file, but that may come from other fixes to C++
8+
// diagnostic output.
9+
//
10+
// AUTOUPDATE
11+
// TIP: To test this file alone, run:
12+
// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/language_server/testdata/text_document/open_with_cpp_nonexistent.carbon
13+
// TIP: To dump output, run:
14+
// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/language_server/testdata/text_document/open_with_cpp_nonexistent.carbon
15+
16+
// --- STDIN
17+
[[@LSP-CALL:initialize]]
18+
[[@LSP-NOTIFY:textDocument/didOpen:
19+
"textDocument": {"uri": "file:/test.carbon", "languageId": "carbon",
20+
"text": "import Cpp library \"nonexistent.h\";"}
21+
]]
22+
[[@LSP-NOTIFY:textDocument/didClose:
23+
"textDocument": {"uri": "file:/test.carbon"}
24+
]]
25+
[[@LSP-CALL:shutdown]]
26+
[[@LSP-NOTIFY:exit]]
27+
28+
// --- AUTOUPDATE-SPLIT
29+
30+
// CHECK:STDERR: /test.carbon: warning: dropping diagnostic in /test.carbon.generated.cpp_imports.h:
31+
// CHECK:STDERR: /test.carbon.generated.cpp_imports.h:1: error: C++:
32+
// CHECK:STDERR: /test.carbon.generated.cpp_imports.h:1:10: fatal error: 'nonexistent.h' file not found
33+
// CHECK:STDERR: 1 | #include "nonexistent.h"
34+
// CHECK:STDERR: | ^~~~~~~~~~~~~~~
35+
// CHECK:STDERR:
36+
// CHECK:STDERR: /test.carbon:1:1: note: in `Cpp` import
37+
// CHECK:STDERR: import Cpp library "nonexistent.h";
38+
// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
39+
// CHECK:STDERR: [LanguageServerDiagnosticInWrongFile]
40+
// CHECK:STDERR:
41+
// CHECK:STDOUT: Content-Length: 146{{\r}}
42+
// CHECK:STDOUT: {{\r}}
43+
// CHECK:STDOUT: {
44+
// CHECK:STDOUT: "id": 1,
45+
// CHECK:STDOUT: "jsonrpc": "2.0",
46+
// CHECK:STDOUT: "result": {
47+
// CHECK:STDOUT: "capabilities": {
48+
// CHECK:STDOUT: "documentSymbolProvider": true,
49+
// CHECK:STDOUT: "textDocumentSync": 2
50+
// CHECK:STDOUT: }
51+
// CHECK:STDOUT: }
52+
// CHECK:STDOUT: }Content-Length: 144{{\r}}
53+
// CHECK:STDOUT: {{\r}}
54+
// CHECK:STDOUT: {
55+
// CHECK:STDOUT: "jsonrpc": "2.0",
56+
// CHECK:STDOUT: "method": "textDocument/publishDiagnostics",
57+
// CHECK:STDOUT: "params": {
58+
// CHECK:STDOUT: "diagnostics": [],
59+
// CHECK:STDOUT: "uri": "file:///test.carbon"
60+
// CHECK:STDOUT: }
61+
// CHECK:STDOUT: }Content-Length: 144{{\r}}
62+
// CHECK:STDOUT: {{\r}}
63+
// CHECK:STDOUT: {
64+
// CHECK:STDOUT: "jsonrpc": "2.0",
65+
// CHECK:STDOUT: "method": "textDocument/publishDiagnostics",
66+
// CHECK:STDOUT: "params": {
67+
// CHECK:STDOUT: "diagnostics": [],
68+
// CHECK:STDOUT: "uri": "file:///test.carbon"
69+
// CHECK:STDOUT: }
70+
// CHECK:STDOUT: }Content-Length: 51{{\r}}
71+
// CHECK:STDOUT: {{\r}}
72+
// CHECK:STDOUT: {
73+
// CHECK:STDOUT: "id": 2,
74+
// CHECK:STDOUT: "jsonrpc": "2.0",
75+
// CHECK:STDOUT: "result": null
76+
// CHECK:STDOUT: }

0 commit comments

Comments
 (0)