Skip to content

Commit

Permalink
refactor(configuration): use Diag_List instead of Buffering_Diag_Repo…
Browse files Browse the repository at this point in the history
…rter
  • Loading branch information
strager committed Jan 3, 2024
1 parent 3ddd74d commit 40ecdc9
Show file tree
Hide file tree
Showing 13 changed files with 114 additions and 52 deletions.
9 changes: 8 additions & 1 deletion benchmark/benchmark-configuration.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@

#include <benchmark/benchmark.h>
#include <quick-lint-js/configuration/configuration.h>
#include <quick-lint-js/container/monotonic-allocator.h>
#include <quick-lint-js/container/padded-string.h>
#include <quick-lint-js/diag/diag-list.h>
#include <quick-lint-js/diag/diag-reporter.h>

using namespace std::literals::string_view_literals;
Expand All @@ -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();
}
}
Expand Down
2 changes: 1 addition & 1 deletion plugin/vscode/quick-lint-js/vscode/qljs-document.h
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
};
Expand Down
11 changes: 8 additions & 3 deletions src/quick-lint-js/c-api.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <quick-lint-js/configuration/configuration.h>
#include <quick-lint-js/container/c-string-list.h>
#include <quick-lint-js/container/padded-string.h>
#include <quick-lint-js/diag/diag-list.h>
#include <quick-lint-js/diag/diagnostic-types.h>
#include <quick-lint-js/fe/linter.h>
#include <quick-lint-js/lsp/lsp-document-text.h>
Expand Down Expand Up @@ -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_);
Expand Down
2 changes: 1 addition & 1 deletion src/quick-lint-js/cli/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions src/quick-lint-js/configuration/configuration-loader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,7 @@ Span<Configuration_Change> 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;
Expand Down
6 changes: 4 additions & 2 deletions src/quick-lint-js/configuration/configuration-loader.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
#include <quick-lint-js/container/monotonic-allocator.h>
#include <quick-lint-js/container/result.h>
#include <quick-lint-js/container/vector.h>
#include <quick-lint-js/diag/buffering-diag-reporter.h>
#include <quick-lint-js/diag/diag-list.h>
#include <quick-lint-js/io/file-canonical.h>
#include <quick-lint-js/io/file.h>
#include <quick-lint-js/port/memory-resource.h>
Expand All @@ -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;
Expand Down
46 changes: 23 additions & 23 deletions src/quick-lint-js/configuration/configuration.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
#include <algorithm>
#include <array>
#include <quick-lint-js/configuration/configuration.h>
#include <quick-lint-js/diag/diag-reporter.h>
#include <quick-lint-js/diag/diag-list.h>
#include <quick-lint-js/fe/global-declared-variable-set.h>
#include <quick-lint-js/fe/global-variables.h>
#include <quick-lint-js/fe/variable-analyzer.h>
Expand All @@ -28,7 +28,7 @@ Source_Code_Span span_of_json_value(::simdjson::ondemand::value&);
template <class Error>
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(); }
Expand Down Expand Up @@ -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 =
Expand All @@ -81,15 +81,15 @@ void Configuration::load_from_json(Padded_String_View json,
narrow_cast<std::size_t>(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;
Expand All @@ -98,16 +98,16 @@ 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;
}

auto globals = document["globals"];
::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;
Expand All @@ -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;
Expand All @@ -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;
}
}
Expand All @@ -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) {
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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{
Expand All @@ -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;
Expand Down Expand Up @@ -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()),
});
}
Expand All @@ -427,13 +427,13 @@ Source_Code_Span span_of_json_value(::simdjson::ondemand::value& value) {
template <class Error>
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;
Expand Down
12 changes: 7 additions & 5 deletions src/quick-lint-js/configuration/configuration.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
#include <vector>

namespace quick_lint_js {
class Diag_Reporter;
class Diag_List;

class Configuration {
public:
Expand All @@ -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_;
Expand Down
3 changes: 2 additions & 1 deletion src/quick-lint-js/container/linked-bump-allocator.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
10 changes: 7 additions & 3 deletions src/quick-lint-js/diag/diag-list.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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<Diag_Type> ignored_types, const Rewind_State &r) {
for (Node_Base *node = r.last_ == nullptr ? this->first_ : r.last_;
Expand All @@ -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.
Expand Down
3 changes: 3 additions & 0 deletions src/quick-lint-js/diag/diag-list.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,12 @@ class Diag_List {
}
}

bool empty() const;
bool reported_any_diagnostic_except_since(
std::initializer_list<Diag_Type> ignored_types, const Rewind_State &);

void clear();

private:
template <std::size_t Diag_Size>
void add_impl(Diag_Type type, void *diag);
Expand Down
2 changes: 1 addition & 1 deletion src/quick-lint-js/lsp/lsp-server.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Loading

0 comments on commit 40ecdc9

Please sign in to comment.