From 40ecdc9907269b61e2dd818f8ee0b46e9e38bfd1 Mon Sep 17 00:00:00 2001 From: "Matthew \"strager\" Glazar" Date: Wed, 3 Jan 2024 14:27:34 -0500 Subject: [PATCH] refactor(configuration): use Diag_List instead of Buffering_Diag_Reporter --- benchmark/benchmark-configuration.cpp | 9 ++- .../quick-lint-js/vscode/qljs-document.h | 2 +- src/quick-lint-js/c-api.cpp | 11 +++- src/quick-lint-js/cli/main.cpp | 2 +- .../configuration/configuration-loader.cpp | 1 + .../configuration/configuration-loader.h | 6 +- .../configuration/configuration.cpp | 46 +++++++-------- .../configuration/configuration.h | 12 ++-- .../container/linked-bump-allocator.h | 3 +- src/quick-lint-js/diag/diag-list.cpp | 10 +++- src/quick-lint-js/diag/diag-list.h | 3 + src/quick-lint-js/lsp/lsp-server.cpp | 2 +- test/test-configuration.cpp | 59 +++++++++++++++---- 13 files changed, 114 insertions(+), 52 deletions(-) diff --git a/benchmark/benchmark-configuration.cpp b/benchmark/benchmark-configuration.cpp index 053de5cfce..89307a8422 100644 --- a/benchmark/benchmark-configuration.cpp +++ b/benchmark/benchmark-configuration.cpp @@ -3,7 +3,9 @@ #include #include +#include #include +#include #include using namespace std::literals::string_view_literals; @@ -16,9 +18,14 @@ void benchmark_parse_config_json(::benchmark::State& state, Null_Diag_Reporter diag_reporter; Configuration config; + Monotonic_Allocator allocator("benchmark"); for (auto _ : state) { + Monotonic_Allocator::Rewind_Guard allocator_rewind = + allocator.make_rewind_guard(); + config.reset(); - config.load_from_json(&config_json_string, &diag_reporter); + Diag_List diags(&allocator); + config.load_from_json(&config_json_string, &diags); ::benchmark::ClobberMemory(); } } diff --git a/plugin/vscode/quick-lint-js/vscode/qljs-document.h b/plugin/vscode/quick-lint-js/vscode/qljs-document.h index 4f1b2cc678..4b047fe205 100644 --- a/plugin/vscode/quick-lint-js/vscode/qljs-document.h +++ b/plugin/vscode/quick-lint-js/vscode/qljs-document.h @@ -129,7 +129,7 @@ class QLJS_Config_Document : public QLJS_Document_Base { LSP_Locator locator(&loaded_config->file_content); VSCode_Diag_Reporter diag_reporter(vscode, env, &locator, this->uri()); - loaded_config->errors.copy_into(&diag_reporter); + diag_reporter.report(loaded_config->errors); return std::move(diag_reporter).diagnostics(); } }; diff --git a/src/quick-lint-js/c-api.cpp b/src/quick-lint-js/c-api.cpp index bb9a1fdf85..fd74dc4b91 100644 --- a/src/quick-lint-js/c-api.cpp +++ b/src/quick-lint-js/c-api.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -65,18 +66,22 @@ void qljs_web_demo_set_locale(QLJS_Web_Demo_Document* p, const char* locale) { } const QLJS_Web_Demo_Diagnostic* qljs_web_demo_lint(QLJS_Web_Demo_Document* p) { + Monotonic_Allocator temp_memory("qljs_web_demo_lint"); + if (p->need_update_config_) { p->config_.reset(); if (p->config_document_) { - p->config_.load_from_json(&p->config_document_->text_, - &Null_Diag_Reporter::instance); + Diag_List diags(&temp_memory); + p->config_.load_from_json(&p->config_document_->text_, &diags); } } p->diag_reporter_.reset(); p->diag_reporter_.set_input(&p->text_); if (p->is_config_json_) { - Configuration().load_from_json(&p->text_, &p->diag_reporter_); + Diag_List diags(&temp_memory); + Configuration().load_from_json(&p->text_, &diags); + p->diag_reporter_.report(diags); } else { parse_and_lint(&p->text_, p->diag_reporter_, p->config_.globals(), p->linter_options_); diff --git a/src/quick-lint-js/cli/main.cpp b/src/quick-lint-js/cli/main.cpp index 7e5e8a5f30..cb8759a911 100644 --- a/src/quick-lint-js/cli/main.cpp +++ b/src/quick-lint-js/cli/main.cpp @@ -283,7 +283,7 @@ void run(Options o) { .is_stdin = false, .vim_bufnr = std::nullopt, }); - config_file->errors.copy_into(reporter->get()); + reporter->get()->report(config_file->errors); // To avoid repeating errors for a given config file, remember that we // already reported errors for this config file. loaded_config_files.insert(config_file); diff --git a/src/quick-lint-js/configuration/configuration-loader.cpp b/src/quick-lint-js/configuration/configuration-loader.cpp index 658a902d1b..b3e98c52a8 100644 --- a/src/quick-lint-js/configuration/configuration-loader.cpp +++ b/src/quick-lint-js/configuration/configuration-loader.cpp @@ -373,6 +373,7 @@ Span Configuration_Loader::refresh( loaded_config.file_content = std::move(*latest_json); loaded_config.config.reset(); loaded_config.errors.clear(); + loaded_config.memory.release(); loaded_config.config.load_from_json(&loaded_config.file_content, &loaded_config.errors); config_file = &loaded_config; diff --git a/src/quick-lint-js/configuration/configuration-loader.h b/src/quick-lint-js/configuration/configuration-loader.h index 2492243869..3ffd5c102f 100644 --- a/src/quick-lint-js/configuration/configuration-loader.h +++ b/src/quick-lint-js/configuration/configuration-loader.h @@ -13,7 +13,7 @@ #include #include #include -#include +#include #include #include #include @@ -38,13 +38,15 @@ class Configuration_Filesystem { struct Loaded_Config_File { explicit Loaded_Config_File(); + Monotonic_Allocator memory{"Loaded_Config_File"}; + Configuration config; // The content of the quick-lint-js.config file. Padded_String file_content; // Errors discovered while parsing file_content. - Buffering_Diag_Reporter errors; + Diag_List errors; // The path to the quick-lint-js.config file. Never nullptr. const Canonical_Path* config_path; diff --git a/src/quick-lint-js/configuration/configuration.cpp b/src/quick-lint-js/configuration/configuration.cpp index 4f41f163ca..9a13b861dc 100644 --- a/src/quick-lint-js/configuration/configuration.cpp +++ b/src/quick-lint-js/configuration/configuration.cpp @@ -4,7 +4,7 @@ #include #include #include -#include +#include #include #include #include @@ -28,7 +28,7 @@ Source_Code_Span span_of_json_value(::simdjson::ondemand::value&); template static bool get_bool_or_default( ::simdjson::simdjson_result<::simdjson::ondemand::value>&& value, bool* out, - bool default_value, Diag_Reporter*); + bool default_value, Diag_List*); } Configuration::Configuration() { this->reset(); } @@ -71,7 +71,7 @@ void Configuration::remove_global_variable(String8_View name) { } void Configuration::load_from_json(Padded_String_View json, - Diag_Reporter* reporter) { + Diag_List* out_diags) { ::simdjson::ondemand::parser json_parser; ::simdjson::ondemand::document document; ::simdjson::error_code parse_error = @@ -81,15 +81,15 @@ void Configuration::load_from_json(Padded_String_View json, narrow_cast(json.padded_size())) .get(document); if (parse_error != ::simdjson::SUCCESS) { - this->report_json_error(json, reporter); + this->report_json_error(json, out_diags); return; } ::simdjson::ondemand::value global_groups_value; switch (document["global-groups"].get(global_groups_value)) { case ::simdjson::error_code::SUCCESS: - if (!this->load_global_groups_from_json(global_groups_value, reporter)) { - this->report_json_error(json, reporter); + if (!this->load_global_groups_from_json(global_groups_value, out_diags)) { + this->report_json_error(json, out_diags); return; } break; @@ -98,7 +98,7 @@ void Configuration::load_from_json(Padded_String_View json, break; default: - this->report_json_error(json, reporter); + this->report_json_error(json, out_diags); return; } @@ -106,8 +106,8 @@ void Configuration::load_from_json(Padded_String_View json, ::simdjson::ondemand::object globals_value; switch (globals.get(globals_value)) { case ::simdjson::error_code::SUCCESS: - if (!this->load_globals_from_json(globals_value, reporter)) { - this->report_json_error(json, reporter); + if (!this->load_globals_from_json(globals_value, out_diags)) { + this->report_json_error(json, out_diags); return; } break; @@ -118,11 +118,11 @@ void Configuration::load_from_json(Padded_String_View json, ::simdjson::ondemand::value v; if (globals.get(v) == ::simdjson::SUCCESS && v.type().error() == ::simdjson::SUCCESS) { - reporter->report(Diag_Config_Globals_Type_Mismatch{ + out_diags->add(Diag_Config_Globals_Type_Mismatch{ .value = span_of_json_value(v), }); } else { - this->report_json_error(json, reporter); + this->report_json_error(json, out_diags); return; } break; @@ -132,7 +132,7 @@ void Configuration::load_from_json(Padded_String_View json, break; default: - this->report_json_error(json, reporter); + this->report_json_error(json, out_diags); return; } } @@ -149,7 +149,7 @@ void Configuration::reset() { } bool Configuration::load_global_groups_from_json( - ::simdjson::ondemand::value& global_groups_value, Diag_Reporter* reporter) { + ::simdjson::ondemand::value& global_groups_value, Diag_List* out_diags) { ::simdjson::ondemand::json_type global_groups_value_type; if (global_groups_value.type().get(global_groups_value_type) != ::simdjson::SUCCESS) { @@ -198,7 +198,7 @@ bool Configuration::load_global_groups_from_json( if (global_group_value.type().error() != ::simdjson::SUCCESS) { return false; } - reporter->report(Diag_Config_Global_Groups_Group_Type_Mismatch{ + out_diags->add(Diag_Config_Global_Groups_Group_Type_Mismatch{ .group = span_of_json_value(global_group_value), }); break; @@ -214,7 +214,7 @@ bool Configuration::load_global_groups_from_json( case ::simdjson::ondemand::json_type::number: case ::simdjson::ondemand::json_type::object: case ::simdjson::ondemand::json_type::string: - reporter->report(Diag_Config_Global_Groups_Type_Mismatch{ + out_diags->add(Diag_Config_Global_Groups_Type_Mismatch{ .value = span_of_json_value(global_groups_value), }); break; @@ -223,7 +223,7 @@ bool Configuration::load_global_groups_from_json( } bool Configuration::load_globals_from_json( - ::simdjson::ondemand::object& globals_value, Diag_Reporter* reporter) { + ::simdjson::ondemand::object& globals_value, Diag_List* out_diags) { for (simdjson::simdjson_result<::simdjson::ondemand::field> global_field : globals_value) { std::string_view key; @@ -271,12 +271,12 @@ bool Configuration::load_globals_from_json( if (!get_bool_or_default< Diag_Config_Globals_Descriptor_Shadowable_Type_Mismatch>( descriptor_object["shadowable"], &is_shadowable, true, - reporter)) { + out_diags)) { ok = false; } if (!get_bool_or_default< Diag_Config_Globals_Descriptor_Writable_Type_Mismatch>( - descriptor_object["writable"], &is_writable, true, reporter)) { + descriptor_object["writable"], &is_writable, true, out_diags)) { ok = false; } this->add_global_variable(Global_Declared_Variable{ @@ -293,7 +293,7 @@ bool Configuration::load_globals_from_json( } default: - reporter->report(Diag_Config_Globals_Descriptor_Type_Mismatch{ + out_diags->add(Diag_Config_Globals_Descriptor_Type_Mismatch{ .descriptor = span_of_json_value(descriptor), }); break; @@ -398,11 +398,11 @@ bool Configuration::should_remove_global_variable(String8_View name) { } void Configuration::report_json_error(Padded_String_View json, - Diag_Reporter* reporter) { + Diag_List* out_diags) { // TODO(strager): Produce better error messages. simdjson provides no location // information for errors: // https://github.com/simdjson/simdjson/issues/237 - reporter->report(Diag_Config_Json_Syntax_Error{ + out_diags->add(Diag_Config_Json_Syntax_Error{ .where = Source_Code_Span::unit(json.data()), }); } @@ -427,13 +427,13 @@ Source_Code_Span span_of_json_value(::simdjson::ondemand::value& value) { template bool get_bool_or_default( ::simdjson::simdjson_result<::simdjson::ondemand::value>&& value, bool* out, - bool default_value, Diag_Reporter* reporter) { + bool default_value, Diag_List* out_diags) { ::simdjson::ondemand::value v; ::simdjson::error_code error = value.get(v); switch (error) { case ::simdjson::SUCCESS: if (v.get(*out) != ::simdjson::SUCCESS) { - reporter->report(Error{span_of_json_value(v)}); + out_diags->add(Error{span_of_json_value(v)}); *out = default_value; } return true; diff --git a/src/quick-lint-js/configuration/configuration.h b/src/quick-lint-js/configuration/configuration.h index 95a949b0b2..cad294ae98 100644 --- a/src/quick-lint-js/configuration/configuration.h +++ b/src/quick-lint-js/configuration/configuration.h @@ -16,7 +16,7 @@ #include namespace quick_lint_js { -class Diag_Reporter; +class Diag_List; class Configuration { public: @@ -34,19 +34,21 @@ class Configuration { // name must live as long as this Configuration object. void remove_global_variable(String8_View name); - void load_from_json(Padded_String_View, Diag_Reporter*); + void load_from_json(Padded_String_View, Diag_List* out_diags); void reset(); private: - bool load_global_groups_from_json(simdjson::ondemand::value&, Diag_Reporter*); - bool load_globals_from_json(simdjson::ondemand::object&, Diag_Reporter*); + bool load_global_groups_from_json(simdjson::ondemand::value&, + Diag_List* out_diags); + bool load_globals_from_json(simdjson::ondemand::object&, + Diag_List* out_diags); bool should_remove_global_variable(String8_View name); [[gnu::noinline]] void build_globals_from_groups(); - void report_json_error(Padded_String_View json, Diag_Reporter*); + void report_json_error(Padded_String_View json, Diag_List* out_diags); Monotonic_Allocator allocator_{"Configuration::allocator_"}; Global_Declared_Variable_Set globals_; diff --git a/src/quick-lint-js/container/linked-bump-allocator.h b/src/quick-lint-js/container/linked-bump-allocator.h index 9988c0475e..2cb678e739 100644 --- a/src/quick-lint-js/container/linked-bump-allocator.h +++ b/src/quick-lint-js/container/linked-bump-allocator.h @@ -37,7 +37,8 @@ class Linked_Bump_Allocator final : public Memory_Resource { ~Linked_Bump_Allocator() override; - // Deallocate previously-allocated memory. + // Deallocate previously-allocated memory, resetting this + // Linked_Bump_Allocator back to its initial state. void release(); struct Rewind_State { diff --git a/src/quick-lint-js/diag/diag-list.cpp b/src/quick-lint-js/diag/diag-list.cpp index a2c1600908..018516ea49 100644 --- a/src/quick-lint-js/diag/diag-list.cpp +++ b/src/quick-lint-js/diag/diag-list.cpp @@ -25,9 +25,7 @@ static_assert(alignof(Diag_List::Node) == alignof(Diag_List::Node_Base), Diag_List::Diag_List(Memory_Resource *memory) : memory_(memory) {} -Diag_List::~Diag_List() { - // Leak. this->memory should be a Linked_Bump_Allocator managed by the caller. -} +Diag_List::~Diag_List() { this->clear(); } Diag_List::Rewind_State Diag_List::prepare_for_rewind() { return Rewind_State{ @@ -64,6 +62,8 @@ void Diag_List::add_impl(Diag_Type type, void *diag) { this->last_ = node; } +bool Diag_List::empty() const { return this->first_ == nullptr; } + bool Diag_List::reported_any_diagnostic_except_since( std::initializer_list ignored_types, const Rewind_State &r) { for (Node_Base *node = r.last_ == nullptr ? this->first_ : r.last_; @@ -74,6 +74,10 @@ bool Diag_List::reported_any_diagnostic_except_since( } return false; } + +void Diag_List::clear() { + // Leak. this->memory should be a Linked_Bump_Allocator managed by the caller. +} } // quick-lint-js finds bugs in JavaScript programs. diff --git a/src/quick-lint-js/diag/diag-list.h b/src/quick-lint-js/diag/diag-list.h index 44c4216391..979ca0316c 100644 --- a/src/quick-lint-js/diag/diag-list.h +++ b/src/quick-lint-js/diag/diag-list.h @@ -58,9 +58,12 @@ class Diag_List { } } + bool empty() const; bool reported_any_diagnostic_except_since( std::initializer_list ignored_types, const Rewind_State &); + void clear(); + private: template void add_impl(Diag_Type type, void *diag); diff --git a/src/quick-lint-js/lsp/lsp-server.cpp b/src/quick-lint-js/lsp/lsp-server.cpp index a3d2247ae4..f73bd46dd8 100644 --- a/src/quick-lint-js/lsp/lsp-server.cpp +++ b/src/quick-lint-js/lsp/lsp-server.cpp @@ -637,7 +637,7 @@ void Linting_LSP_Server_Handler::get_config_file_diagnostics_notification( notification_json.append_copy(u8R"--(,"diagnostics":)--"_sv); LSP_Diag_Reporter diag_reporter(qljs_messages, notification_json, &config_file->file_content); - config_file->errors.copy_into(&diag_reporter); + diag_reporter.report(config_file->errors); diag_reporter.finish(); notification_json.append_copy(u8R"--(},"jsonrpc":"2.0"})--"_sv); diff --git a/test/test-configuration.cpp b/test/test-configuration.cpp index ea64cb00e6..f54ae79183 100644 --- a/test/test-configuration.cpp +++ b/test/test-configuration.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -597,6 +598,8 @@ TEST(Test_Configuration_JSON, false_global_overrides_global_group) { } TEST(Test_Configuration_JSON, invalid_json_reports_error) { + Monotonic_Allocator temp_memory("test"); + // TODO(strager): The following are erroneously treated as schema // errors, but should be JSON parse errors: // u8R"({"global-groups": {42}})"_sv, @@ -617,9 +620,12 @@ TEST(Test_Configuration_JSON, invalid_json_reports_error) { Configuration c; Padded_String json(json_string); - Diag_Collector errors; - c.load_from_json(&json, &errors); + Diag_List diags(&temp_memory); + c.load_from_json(&json, &diags); + // TODO(#1154): Remove Diag_Collector and use Diag_List directly. + Diag_Collector errors; + errors.report(diags); // TODO(strager): Check Diag_Config_Json_Syntax_Error::where. EXPECT_THAT(errors.errors, ElementsAreArray({DIAG_TYPE(Diag_Config_Json_Syntax_Error)})); @@ -627,11 +633,16 @@ TEST(Test_Configuration_JSON, invalid_json_reports_error) { } TEST(Test_Configuration_JSON, bad_schema_in_globals_reports_error) { + Monotonic_Allocator temp_memory("test"); + { Padded_String json(u8R"({"globals":["myGlobalVariable"]})"_sv); Configuration c; + Diag_List diags(&temp_memory); + c.load_from_json(&json, &diags); + // TODO(#1154): Remove Diag_Collector and use Diag_List directly. Diag_Collector errors; - c.load_from_json(&json, &errors); + errors.report(diags); EXPECT_THAT(errors.errors, ElementsAreArray({DIAG_TYPE_OFFSETS( &json, Diag_Config_Globals_Type_Mismatch, // @@ -644,8 +655,11 @@ TEST(Test_Configuration_JSON, bad_schema_in_globals_reports_error) { Padded_String json( u8R"({"globals":{"testBefore":true,"testBad":"string","testAfter":true}})"_sv); Configuration c; + Diag_List diags(&temp_memory); + c.load_from_json(&json, &diags); + // TODO(#1154): Remove Diag_Collector and use Diag_List directly. Diag_Collector errors; - c.load_from_json(&json, &errors); + errors.report(diags); EXPECT_THAT(errors.errors, ElementsAreArray({DIAG_TYPE_OFFSETS( &json, Diag_Config_Globals_Descriptor_Type_Mismatch, // @@ -665,8 +679,11 @@ TEST(Test_Configuration_JSON, bad_schema_in_globals_reports_error) { Padded_String json( u8R"({"globals":{"testBefore":true,"testBad":{"writable":false,"shadowable":"string"},"testAfter":true}})"_sv); Configuration c; + Diag_List diags(&temp_memory); + c.load_from_json(&json, &diags); + // TODO(#1154): Remove Diag_Collector and use Diag_List directly. Diag_Collector errors; - c.load_from_json(&json, &errors); + errors.report(diags); EXPECT_THAT( errors.errors, ElementsAreArray({DIAG_TYPE_OFFSETS( @@ -693,8 +710,11 @@ TEST(Test_Configuration_JSON, bad_schema_in_globals_reports_error) { Padded_String json( u8R"({"globals":{"testBefore":true,"testBad":{"writable":"string","shadowable":false},"testAfter":true}})"_sv); Configuration c; + Diag_List diags(&temp_memory); + c.load_from_json(&json, &diags); + // TODO(#1154): Remove Diag_Collector and use Diag_List directly. Diag_Collector errors; - c.load_from_json(&json, &errors); + errors.report(diags); EXPECT_THAT( errors.errors, ElementsAreArray({DIAG_TYPE_OFFSETS( @@ -719,11 +739,16 @@ TEST(Test_Configuration_JSON, bad_schema_in_globals_reports_error) { } TEST(Test_Configuration_JSON, bad_schema_in_global_groups_reports_error) { + Monotonic_Allocator temp_memory("test"); + { Padded_String json(u8R"({"global-groups":{"browser":true}})"_sv); Configuration c; + Diag_List diags(&temp_memory); + c.load_from_json(&json, &diags); + // TODO(#1154): Remove Diag_Collector and use Diag_List directly. Diag_Collector errors; - c.load_from_json(&json, &errors); + errors.report(diags); EXPECT_THAT(errors.errors, ElementsAreArray({DIAG_TYPE_OFFSETS( &json, Diag_Config_Global_Groups_Type_Mismatch, // @@ -736,8 +761,11 @@ TEST(Test_Configuration_JSON, bad_schema_in_global_groups_reports_error) { Padded_String json( u8R"({"global-groups":["browser",false,"ecmascript"]})"_sv); Configuration c; + Diag_List diags(&temp_memory); + c.load_from_json(&json, &diags); + // TODO(#1154): Remove Diag_Collector and use Diag_List directly. Diag_Collector errors; - c.load_from_json(&json, &errors); + errors.report(diags); EXPECT_THAT(errors.errors, ElementsAreArray({DIAG_TYPE_OFFSETS( &json, Diag_Config_Global_Groups_Group_Type_Mismatch, // @@ -761,13 +789,18 @@ TEST(Test_Configuration_JSON, bad_global_error_excludes_trailing_whitespace) { // simdjson's raw_json_token function returns trailing whitespace by default. // Ensure the whitespace is not included in error messages. + Monotonic_Allocator temp_memory("test"); + // According to RFC 8259, whitespace characters are U+0009, U+000A, U+000D, // and U+0020. Padded_String json(u8"{ \"globals\": { \"a\": \"b\" \n\t\r }}"_sv); Configuration c; - Diag_Collector errors; - c.load_from_json(&json, &errors); + Diag_List diags(&temp_memory); + c.load_from_json(&json, &diags); + // TODO(#1154): Remove Diag_Collector and use Diag_List directly. + Diag_Collector errors; + errors.report(diags); EXPECT_THAT( errors.errors, ElementsAreArray({DIAG_TYPE_OFFSETS( @@ -776,8 +809,12 @@ TEST(Test_Configuration_JSON, bad_global_error_excludes_trailing_whitespace) { } void load_from_json(Configuration& config, Padded_String_View json) { + Monotonic_Allocator temp_memory("test"); + Diag_List diags(&temp_memory); + config.load_from_json(json, &diags); + // TODO(#1154): Remove Diag_Collector and use Diag_List directly. Diag_Collector errors; - config.load_from_json(json, &errors); + errors.report(diags); EXPECT_THAT(errors.errors, ::testing::IsEmpty()); }