Skip to content

Commit b80023e

Browse files
stragerstrager
committed
refactor(lsp): allow lsp_linter to send any number of LSP messages
lsp_linter is forced to send exactly one LSP message. This restriction is arbitrary, and we have an ugly workaround because of this. Give lsp_linter::lint_and_get_diagnostics_notification an outgoing_lsp_message_queue instead of a output byte_buffer, allowing an lsp_linter to send zero messages if it wants.
1 parent eef2513 commit b80023e

File tree

3 files changed

+61
-49
lines changed

3 files changed

+61
-49
lines changed

‎src/quick-lint-js/lsp/lsp-server.cpp

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -382,9 +382,8 @@ void linting_lsp_server_handler::config_document::on_text_changed(
382382

383383
void linting_lsp_server_handler::lintable_document::on_text_changed(
384384
linting_lsp_server_handler& handler, string8_view document_uri_json) {
385-
byte_buffer& notification_json = handler.outgoing_messages_.new_message();
386385
handler.linter_.lint_and_get_diagnostics_notification(
387-
*this, document_uri_json, notification_json);
386+
*this, document_uri_json, handler.outgoing_messages_);
388387
}
389388

390389
void linting_lsp_server_handler::unknown_document::on_text_changed(
@@ -483,9 +482,8 @@ void linting_lsp_server_handler::handle_text_document_did_open_notification(
483482
this->write_configuration_loader_error_notification(
484483
document_path, config_file.error_to_string(), message_json);
485484
}
486-
byte_buffer& notification_json = this->outgoing_messages_.new_message();
487-
this->linter_.lint_and_get_diagnostics_notification(*doc, uri->json,
488-
notification_json);
485+
this->linter_.lint_and_get_diagnostics_notification(
486+
*doc, uri->json, this->outgoing_messages_);
489487

490488
doc_ptr = std::move(doc);
491489
} else if (this->config_loader_.is_config_file_path(document_path)) {
@@ -581,13 +579,12 @@ void linting_lsp_server_handler::lintable_document::on_config_file_changed(
581579
configuration* config = change.config_file ? &change.config_file->config
582580
: &handler.default_config_;
583581
this->config = config;
584-
byte_buffer& notification_json = handler.outgoing_messages_.new_message();
585582
// TODO(strager): Don't copy document_uri if it contains only non-special
586583
// characters.
587584
// TODO(strager): Cache the result of to_json_escaped_string?
588585
handler.linter_.lint_and_get_diagnostics_notification(
589586
*this, to_json_escaped_string_with_quotes(document_uri),
590-
notification_json);
587+
handler.outgoing_messages_);
591588
}
592589

593590
void linting_lsp_server_handler::unknown_document::on_config_file_changed(
@@ -727,16 +724,17 @@ lsp_linter::~lsp_linter() = default;
727724

728725
void lsp_linter::lint_and_get_diagnostics_notification(
729726
linting_lsp_server_handler::lintable_document& doc, string8_view uri_json,
730-
byte_buffer& notification_json) {
727+
outgoing_lsp_message_queue& outgoing_messages) {
731728
this->lint_and_get_diagnostics_notification(
732729
*doc.config, doc.lint_options, doc.doc.string(), uri_json,
733-
doc.version_json, notification_json);
730+
doc.version_json, outgoing_messages);
734731
}
735732

736733
void lsp_javascript_linter::lint_and_get_diagnostics_notification(
737734
configuration& config, linter_options lint_options, padded_string_view code,
738735
string8_view uri_json, string8_view version_json,
739-
byte_buffer& notification_json) {
736+
outgoing_lsp_message_queue& outgoing_messages) {
737+
byte_buffer& notification_json = outgoing_messages.new_message();
740738
// clang-format off
741739
notification_json.append_copy(
742740
u8R"--({)--"

‎src/quick-lint-js/lsp/lsp-server.h

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -214,14 +214,15 @@ class lsp_linter {
214214

215215
virtual ~lsp_linter();
216216

217+
// TODO(strager): Rename to just 'lint'.
217218
virtual void lint_and_get_diagnostics_notification(
218219
configuration& config, linter_options lint_options,
219220
padded_string_view code, string8_view uri_json, string8_view version_json,
220-
byte_buffer& notification_json) = 0;
221+
outgoing_lsp_message_queue&) = 0;
221222

222223
void lint_and_get_diagnostics_notification(
223224
linting_lsp_server_handler::lintable_document&, string8_view uri_json,
224-
byte_buffer& notification_json);
225+
outgoing_lsp_message_queue&);
225226
};
226227

227228
class lsp_javascript_linter final : public lsp_linter {
@@ -231,7 +232,7 @@ class lsp_javascript_linter final : public lsp_linter {
231232
void lint_and_get_diagnostics_notification(
232233
configuration&, linter_options, padded_string_view code,
233234
string8_view uri_json, string8_view version_json,
234-
byte_buffer& notification_json) override;
235+
outgoing_lsp_message_queue&) override;
235236

236237
private:
237238
void lint_and_get_diagnostics(configuration&, linter_options,

‎test/test-lsp-server.cpp

Lines changed: 49 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ class mock_lsp_linter final : public lsp_linter {
6565
using lint_and_get_diagnostics_notification_type =
6666
void(configuration&, linter_options, padded_string_view code,
6767
string8_view uri_json, string8_view version_json,
68-
byte_buffer& notification_json);
68+
outgoing_lsp_message_queue& outgoing_messages);
6969

7070
explicit mock_lsp_linter() = default;
7171

@@ -81,9 +81,9 @@ class mock_lsp_linter final : public lsp_linter {
8181
void lint_and_get_diagnostics_notification(
8282
configuration& config, linter_options lint_options,
8383
padded_string_view code, string8_view uri_json, string8_view version_json,
84-
byte_buffer& notification_json) override {
84+
outgoing_lsp_message_queue& outgoing_messages) override {
8585
this->callback_(config, lint_options, code, uri_json, version_json,
86-
notification_json);
86+
outgoing_messages);
8787
}
8888

8989
private:
@@ -101,11 +101,12 @@ class test_linting_lsp_server : public ::testing::Test, public filesystem_test {
101101
this->linter = mock_lsp_linter(
102102
[this](configuration& config, linter_options lint_options,
103103
padded_string_view code, string8_view uri_json,
104-
string8_view version_json, byte_buffer& notification_json) {
104+
string8_view version_json,
105+
outgoing_lsp_message_queue& outgoing_messages) {
105106
this->lint_calls.emplace_back(code.string_view());
106107
if (this->lint_callback) {
107108
this->lint_callback(config, lint_options, code, uri_json,
108-
version_json, notification_json);
109+
version_json, outgoing_messages);
109110
}
110111
});
111112
this->handler =
@@ -117,7 +118,7 @@ class test_linting_lsp_server : public ::testing::Test, public filesystem_test {
117118

118119
heap_function<void(configuration&, linter_options, padded_string_view code,
119120
string8_view uri_json, string8_view version,
120-
byte_buffer& notification_json)>
121+
outgoing_lsp_message_queue& outgoing_messages)>
121122
lint_callback;
122123
std::vector<string8> lint_calls;
123124

@@ -517,11 +518,12 @@ TEST_F(test_linting_lsp_server, opening_document_lints) {
517518
this->lint_callback = [&](configuration&, linter_options,
518519
padded_string_view code, string8_view uri_json,
519520
string8_view version,
520-
byte_buffer& notification_json) {
521+
outgoing_lsp_message_queue& outgoing_messages) {
521522
EXPECT_EQ(code, u8"let x = x;"_sv);
522523
EXPECT_EQ(uri_json, u8"\"file:///test.js\""_sv);
523524
EXPECT_EQ(version, u8"10"_sv);
524525

526+
byte_buffer& notification_json = outgoing_messages.new_message();
525527
notification_json.append_copy(
526528
u8R"--(
527529
{
@@ -591,7 +593,7 @@ TEST_F(test_linting_lsp_server, javascript_language_ids_enable_jsx) {
591593

592594
this->lint_callback = [&](configuration&, linter_options lint_options,
593595
padded_string_view, string8_view, string8_view,
594-
byte_buffer&) {
596+
outgoing_lsp_message_queue&) {
595597
EXPECT_TRUE(lint_options.jsx);
596598
EXPECT_FALSE(lint_options.typescript);
597599
};
@@ -622,7 +624,7 @@ TEST_F(test_linting_lsp_server, typescript_language_ids_enable_typescript) {
622624

623625
this->lint_callback = [&](configuration&, linter_options lint_options,
624626
padded_string_view, string8_view, string8_view,
625-
byte_buffer&) {
627+
outgoing_lsp_message_queue&) {
626628
EXPECT_TRUE(lint_options.typescript);
627629
EXPECT_FALSE(lint_options.jsx);
628630
};
@@ -653,7 +655,7 @@ TEST_F(test_linting_lsp_server, tsx_language_ids_enable_typescript_jsx) {
653655

654656
this->lint_callback = [&](configuration&, linter_options lint_options,
655657
padded_string_view, string8_view, string8_view,
656-
byte_buffer&) {
658+
outgoing_lsp_message_queue&) {
657659
EXPECT_TRUE(lint_options.typescript);
658660
EXPECT_TRUE(lint_options.jsx);
659661
};
@@ -828,7 +830,7 @@ TEST_F(test_linting_lsp_server, linting_uses_config_from_file) {
828830

829831
this->lint_callback = [&](configuration& config, linter_options,
830832
padded_string_view, string8_view, string8_view,
831-
byte_buffer&) {
833+
outgoing_lsp_message_queue&) {
832834
EXPECT_TRUE(config.globals().find(u8"testGlobalVariable"_sv));
833835
};
834836

@@ -915,7 +917,7 @@ TEST_F(
915917
this->lint_calls.clear();
916918
this->lint_callback = [&](configuration& config, linter_options,
917919
padded_string_view, string8_view, string8_view,
918-
byte_buffer&) {
920+
outgoing_lsp_message_queue&) {
919921
EXPECT_FALSE(config.globals().find(u8"testGlobalVariableBefore"_sv));
920922
EXPECT_TRUE(config.globals().find(u8"testGlobalVariableAfter"_sv));
921923
};
@@ -948,7 +950,7 @@ TEST_F(test_linting_lsp_server,
948950

949951
this->lint_callback = [&](configuration& config, linter_options,
950952
padded_string_view, string8_view, string8_view,
951-
byte_buffer&) {
953+
outgoing_lsp_message_queue&) {
952954
EXPECT_TRUE(config.globals().find(u8"testGlobalVariable"_sv));
953955
};
954956

@@ -974,7 +976,7 @@ TEST_F(test_linting_lsp_server,
974976
TEST_F(test_linting_lsp_server, linting_uses_already_opened_config_file) {
975977
this->lint_callback = [&](configuration& config, linter_options,
976978
padded_string_view, string8_view, string8_view,
977-
byte_buffer&) {
979+
outgoing_lsp_message_queue&) {
978980
EXPECT_TRUE(config.globals().find(u8"modified"_sv));
979981
};
980982

@@ -1018,7 +1020,7 @@ TEST_F(test_linting_lsp_server,
10181020
linting_uses_already_opened_shadowing_config_file) {
10191021
this->lint_callback = [&](configuration& config, linter_options,
10201022
padded_string_view, string8_view, string8_view,
1021-
byte_buffer&) {
1023+
outgoing_lsp_message_queue&) {
10221024
EXPECT_TRUE(config.globals().find(u8"haveInnerConfig"_sv));
10231025
EXPECT_FALSE(config.globals().find(u8"haveOuterConfig"_sv));
10241026
};
@@ -1064,7 +1066,8 @@ TEST_F(test_linting_lsp_server, editing_config_relints_open_js_file) {
10641066

10651067
this->lint_callback = [&](configuration& config, linter_options,
10661068
padded_string_view, string8_view uri_json,
1067-
string8_view version_json, byte_buffer&) {
1069+
string8_view version_json,
1070+
outgoing_lsp_message_queue&) {
10681071
if (config.globals().find(u8"after"_sv)) {
10691072
EXPECT_FALSE(config.globals().find(u8"before"_sv));
10701073
EXPECT_EQ(version_json, u8"10");
@@ -1192,7 +1195,9 @@ TEST_F(test_linting_lsp_server,
11921195

11931196
this->lint_callback = [&](configuration&, linter_options, padded_string_view,
11941197
string8_view, string8_view version_json,
1195-
byte_buffer&) { EXPECT_EQ(version_json, u8"11"); };
1198+
outgoing_lsp_message_queue&) {
1199+
EXPECT_EQ(version_json, u8"11");
1200+
};
11961201
this->lint_calls.clear();
11971202

11981203
// Change 'before' to 'after'.
@@ -1225,7 +1230,8 @@ TEST_F(test_linting_lsp_server,
12251230
TEST_F(test_linting_lsp_server, editing_config_relints_many_open_js_files) {
12261231
this->lint_callback = [&](configuration&, linter_options, padded_string_view,
12271232
string8_view uri_json, string8_view version_json,
1228-
byte_buffer& notification_json) {
1233+
outgoing_lsp_message_queue& outgoing_messages) {
1234+
byte_buffer& notification_json = outgoing_messages.new_message();
12291235
notification_json.append_copy(
12301236
u8R"(
12311237
{
@@ -1337,7 +1343,8 @@ TEST_F(test_linting_lsp_server, editing_config_relints_many_open_js_files) {
13371343
TEST_F(test_linting_lsp_server, editing_config_relints_only_affected_js_files) {
13381344
this->lint_callback = [&](configuration&, linter_options, padded_string_view,
13391345
string8_view uri_json, string8_view version_json,
1340-
byte_buffer& notification_json) {
1346+
outgoing_lsp_message_queue& outgoing_messages) {
1347+
byte_buffer& notification_json = outgoing_messages.new_message();
13411348
notification_json.append_copy(
13421349
u8R"(
13431350
{
@@ -1514,7 +1521,8 @@ TEST_F(test_linting_lsp_server,
15141521
this->lint_calls.clear();
15151522
this->lint_callback = [&](configuration& config, linter_options,
15161523
padded_string_view, string8_view,
1517-
string8_view version_json, byte_buffer&) {
1524+
string8_view version_json,
1525+
outgoing_lsp_message_queue&) {
15181526
EXPECT_EQ(version_json, u8"11");
15191527
EXPECT_FALSE(config.globals().find(u8"before"_sv));
15201528
EXPECT_TRUE(config.globals().find(u8"after"_sv));
@@ -1547,7 +1555,8 @@ TEST_F(test_linting_lsp_server, opening_config_relints_open_js_files) {
15471555

15481556
this->lint_callback = [&](configuration& config, linter_options,
15491557
padded_string_view, string8_view uri_json,
1550-
string8_view version_json, byte_buffer&) {
1558+
string8_view version_json,
1559+
outgoing_lsp_message_queue&) {
15511560
if (config.globals().find(u8"after"_sv)) {
15521561
EXPECT_FALSE(config.globals().find(u8"before"_sv));
15531562
EXPECT_EQ(version_json, u8"10");
@@ -1617,13 +1626,14 @@ TEST_F(test_linting_lsp_server,
16171626
this->lint_callback = [&](configuration& config, linter_options,
16181627
padded_string_view, string8_view uri_json,
16191628
string8_view version_json,
1620-
byte_buffer& notification_json) {
1629+
outgoing_lsp_message_queue& outgoing_messages) {
16211630
EXPECT_TRUE(config.globals().find(u8"after"_sv));
16221631
EXPECT_FALSE(config.globals().find(u8"before"_sv));
16231632
EXPECT_EQ(version_json, u8"10");
16241633
EXPECT_EQ(uri_json, u8"\"" + this->fs.file_uri_prefix_8() + u8"test.js\"");
16251634
after_config_was_loaded = true;
16261635

1636+
byte_buffer& notification_json = outgoing_messages.new_message();
16271637
notification_json.append_copy(
16281638
u8R"({
16291639
"method": "textDocument/publishDiagnostics",
@@ -1658,7 +1668,7 @@ TEST_F(
16581668
linting_uses_config_from_filesystem_if_config_is_opened_then_closed_before_opening_js_file) {
16591669
this->lint_callback = [&](configuration& config, linter_options,
16601670
padded_string_view, string8_view, string8_view,
1661-
byte_buffer&) {
1671+
outgoing_lsp_message_queue&) {
16621672
EXPECT_TRUE(config.globals().find(u8"v1"_sv));
16631673
EXPECT_FALSE(config.globals().find(u8"v2"_sv));
16641674
};
@@ -1749,9 +1759,10 @@ TEST_F(test_linting_lsp_server,
17491759
this->lint_calls.clear();
17501760
this->lint_callback = [&](configuration& config, linter_options,
17511761
padded_string_view, string8_view, string8_view,
1752-
byte_buffer& notification_json) {
1762+
outgoing_lsp_message_queue& outgoing_messages) {
17531763
EXPECT_TRUE(config.globals().find(u8"configFromFilesystem"_sv));
17541764
EXPECT_FALSE(config.globals().find(u8"configFromLSP"_sv));
1765+
byte_buffer& notification_json = outgoing_messages.new_message();
17551766
notification_json.append_copy(
17561767
u8R"({
17571768
"method": "textDocument/publishDiagnostics",
@@ -1793,11 +1804,12 @@ TEST_F(test_linting_lsp_server, opening_js_file_with_unreadable_config_lints) {
17931804
this->lint_callback = [&](configuration& config, linter_options,
17941805
padded_string_view, string8_view uri_json,
17951806
string8_view version_json,
1796-
byte_buffer& notification_json) {
1807+
outgoing_lsp_message_queue& outgoing_messages) {
17971808
EXPECT_TRUE(config.globals().find(u8"Array"_sv))
17981809
<< "config should be default";
17991810
EXPECT_FALSE(config.globals().find(u8"undeclaredVariable"_sv))
18001811
<< "config should be default";
1812+
byte_buffer& notification_json = outgoing_messages.new_message();
18011813
notification_json.append_copy(
18021814
u8R"({
18031815
"method": "textDocument/publishDiagnostics",
@@ -1855,11 +1867,12 @@ TEST_F(test_linting_lsp_server,
18551867
this->lint_callback = [&](configuration& config, linter_options,
18561868
padded_string_view, string8_view uri_json,
18571869
string8_view version_json,
1858-
byte_buffer& notification_json) {
1870+
outgoing_lsp_message_queue& outgoing_messages) {
18591871
EXPECT_TRUE(config.globals().find(u8"Array"_sv))
18601872
<< "config should be default";
18611873
EXPECT_FALSE(config.globals().find(u8"undeclaredVariable"_sv))
18621874
<< "config should be default";
1875+
byte_buffer& notification_json = outgoing_messages.new_message();
18631876
notification_json.append_copy(
18641877
u8R"({
18651878
"method": "textDocument/publishDiagnostics",
@@ -1943,9 +1956,10 @@ TEST_F(test_linting_lsp_server, making_config_file_unreadable_relints) {
19431956
this->lint_callback = [&](configuration& config, linter_options,
19441957
padded_string_view, string8_view uri_json,
19451958
string8_view version_json,
1946-
byte_buffer& notification_json) {
1959+
outgoing_lsp_message_queue& outgoing_messages) {
19471960
EXPECT_FALSE(config.globals().find(u8"configFromFilesystem"_sv))
19481961
<< "config should be default";
1962+
byte_buffer& notification_json = outgoing_messages.new_message();
19491963
notification_json.append_copy(
19501964
u8R"({
19511965
"method": "textDocument/publishDiagnostics",
@@ -2484,21 +2498,20 @@ TEST_F(test_linting_lsp_server, invalid_notification_is_ignored) {
24842498

24852499
TEST(test_lsp_javascript_linter, linting_gives_diagnostics) {
24862500
padded_string code(u8"let x = x;"_sv);
2487-
byte_buffer notification_json_buffer;
2501+
outgoing_lsp_message_queue notifications;
24882502
configuration config;
24892503

24902504
lsp_javascript_linter linter;
2491-
linter.lint_and_get_diagnostics_notification(
2492-
config, linter_options(), &code, u8"\"file:///test.js\""_sv, u8"10"_sv,
2493-
notification_json_buffer);
2505+
linter.lint_and_get_diagnostics_notification(config, linter_options(), &code,
2506+
u8"\"file:///test.js\""_sv,
2507+
u8"10"_sv, notifications);
24942508

2495-
std::string notification_json;
2496-
notification_json.resize(notification_json_buffer.size());
2497-
notification_json_buffer.copy_to(notification_json.data());
2509+
spy_lsp_endpoint_remote endpoint;
2510+
notifications.send(endpoint);
2511+
::boost::json::object notification = endpoint.notifications().at(0);
24982512

2499-
::boost::json::value notification = parse_boost_json(notification_json);
25002513
EXPECT_EQ(look_up(notification, "method"), "textDocument/publishDiagnostics");
2501-
EXPECT_FALSE(notification.as_object().contains("error"));
2514+
EXPECT_FALSE(notification.contains("error"));
25022515
// LSP PublishDiagnosticsParams:
25032516
EXPECT_EQ(look_up(notification, "params", "uri"), "file:///test.js");
25042517
EXPECT_EQ(look_up(notification, "params", "version"), 10);

0 commit comments

Comments
 (0)