Refactor error list out of configuration class

Each configuration object stores a buffering_error_reporter of errors
encountered while parsing the config JSON. I think the configuration
class is the wrong place for this. Move the error list into the
configuration_loader class, storing it in loaded_config_file.

This commit should not change behavior.

Author: strager

src/configuration-loader.cpp

@@ -127,7 +127,8 @@ configuration_loader::load_config_file(const char* config_path) {
   loaded_config_file* config_file = &config_it->second;
   config_file->config_path = &config_it->first;
   config_file->file_content = std::move(*config_json);
-  config_file->config.load_from_json(&config_file->file_content);
+  config_file->config.load_from_json(&config_file->file_content,
+                                     &config_file->errors);
   return config_file;
 }
 
@@ -179,7 +180,8 @@ configuration_loader::find_and_load_config_file_in_directory_and_ancestors(
   loaded_config_file* config_file = &config_it->second;
   config_file->config_path = &config_it->first;
   config_file->file_content = std::move(found->file_content);
-  config_file->config.load_from_json(&config_file->file_content);
+  config_file->config.load_from_json(&config_file->file_content,
+                                     &config_file->errors);
   return config_file;
 }
 
@@ -359,7 +361,9 @@ std::vector<configuration_change> configuration_loader::refresh() {
         loaded_config.config_path = &config_it->first;
         loaded_config.file_content = std::move(*latest_json);
         loaded_config.config.reset();
-        loaded_config.config.load_from_json(&loaded_config.file_content);
+        loaded_config.errors.clear();
+        loaded_config.config.load_from_json(&loaded_config.file_content,
+                                            &loaded_config.errors);
         config_file = &loaded_config;
       } else {
         config_file = &loaded_config_it->second;
@@ -426,7 +430,9 @@ std::vector<configuration_change> configuration_loader::refresh() {
           loaded_config.config_path = &config_it->first;
           loaded_config.file_content = std::move(latest->file_content);
           loaded_config.config.reset();
-          loaded_config.config.load_from_json(&loaded_config.file_content);
+          loaded_config.errors.clear();
+          loaded_config.config.load_from_json(&loaded_config.file_content,
+                                              &loaded_config.errors);
           config_file = &loaded_config;
         } else {
           config_file = &loaded_config_it->second;
@@ -462,7 +468,9 @@ std::vector<configuration_change> configuration_loader::refresh() {
       QLJS_ASSERT(*loaded_config.config_path == config_path);
       loaded_config.file_content = std::move(*config_json);
       loaded_config.config.reset();
-      loaded_config.config.load_from_json(&loaded_config.file_content);
+      loaded_config.errors.clear();
+      loaded_config.config.load_from_json(&loaded_config.file_content,
+                                          &loaded_config.errors);
 
       for (const watched_config_path& watch : this->watched_config_paths_) {
         if (watch.actual_config_path == config_path) {

src/configuration.cpp

@@ -4,6 +4,7 @@
 #include <algorithm>
 #include <quick-lint-js/char8.h>
 #include <quick-lint-js/configuration.h>
+#include <quick-lint-js/error.h>
 #include <quick-lint-js/global-variables.h>
 #include <quick-lint-js/lint.h>
 #include <quick-lint-js/narrow-cast.h>
@@ -175,7 +176,8 @@ void configuration::remove_global_variable(string8_view name) {
   this->globals_to_remove_.emplace_back(name);
 }
 
-void configuration::load_from_json(padded_string_view json) {
+void configuration::load_from_json(padded_string_view json,
+                                   error_reporter* reporter) {
   ::simdjson::ondemand::parser json_parser;
   ::simdjson::ondemand::document document;
   ::simdjson::error_code parse_error =
@@ -185,15 +187,15 @@ void configuration::load_from_json(padded_string_view json) {
                    narrow_cast<std::size_t>(json.padded_size()))
           .get(document);
   if (parse_error != ::simdjson::error_code::SUCCESS) {
-    this->report_json_error(json);
+    this->report_json_error(json, reporter);
     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)) {
-      this->report_json_error(json);
+    if (!this->load_global_groups_from_json(global_groups_value, reporter)) {
+      this->report_json_error(json, reporter);
       return;
     }
     break;
@@ -202,16 +204,16 @@ void configuration::load_from_json(padded_string_view json) {
     break;
 
   default:
-    this->report_json_error(json);
+    this->report_json_error(json, reporter);
     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)) {
-      this->report_json_error(json);
+    if (!this->load_globals_from_json(globals_value, reporter)) {
+      this->report_json_error(json, reporter);
       return;
     }
     break;
@@ -222,11 +224,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) {
-      this->errors_.report(error_config_globals_type_mismatch{
+      reporter->report(error_config_globals_type_mismatch{
           .value = span_of_json_value(v),
       });
     } else {
-      this->report_json_error(json);
+      this->report_json_error(json, reporter);
       return;
     }
     break;
@@ -236,36 +238,23 @@ void configuration::load_from_json(padded_string_view json) {
     break;
 
   default:
-    this->report_json_error(json);
+    this->report_json_error(json, reporter);
     return;
   }
 }
 
-void configuration::report_errors(error_reporter* reporter) {
-  this->errors_.copy_into(reporter);
-  this->errors_were_reported_ = true;
-}
-
-bool configuration::have_errors() const noexcept {
-  return !this->errors_.empty();
-}
-
-bool configuration::errors_were_reported() const noexcept {
-  return this->errors_were_reported_;
-}
-
 void configuration::reset() {
   // TODO(strager): Make this more efficient by avoiding reallocations.
   this->globals_ = global_declared_variable_set();
   this->globals_to_remove_.clear();
   this->add_global_group_node_js_ = true;
   this->add_global_group_ecmascript_ = true;
   this->string_allocator_.memory_resource()->release();
-  this->errors_.clear();
 }
 
 bool configuration::load_global_groups_from_json(
-    ::simdjson::ondemand::value& global_groups_value) {
+    ::simdjson::ondemand::value& global_groups_value,
+    error_reporter* reporter) {
   ::simdjson::fallback::ondemand::json_type global_groups_value_type;
   if (global_groups_value.type().get(global_groups_value_type) !=
       ::simdjson::SUCCESS) {
@@ -314,7 +303,7 @@ bool configuration::load_global_groups_from_json(
         if (global_group_value.type().error() != ::simdjson::SUCCESS) {
           return false;
         }
-        this->errors_.report(error_config_global_groups_group_type_mismatch{
+        reporter->report(error_config_global_groups_group_type_mismatch{
             .group = span_of_json_value(global_group_value),
         });
         break;
@@ -330,7 +319,7 @@ bool configuration::load_global_groups_from_json(
   case ::simdjson::fallback::ondemand::json_type::number:
   case ::simdjson::fallback::ondemand::json_type::object:
   case ::simdjson::fallback::ondemand::json_type::string:
-    this->errors_.report(error_config_global_groups_type_mismatch{
+    reporter->report(error_config_global_groups_type_mismatch{
         .value = span_of_json_value(global_groups_value),
     });
     break;
@@ -339,7 +328,7 @@ bool configuration::load_global_groups_from_json(
 }
 
 bool configuration::load_globals_from_json(
-    ::simdjson::ondemand::object& globals_value) {
+    ::simdjson::ondemand::object& globals_value, error_reporter* reporter) {
   for (simdjson::simdjson_result<::simdjson::fallback::ondemand::field>
            global_field : globals_value) {
     std::string_view key;
@@ -379,20 +368,22 @@ bool configuration::load_globals_from_json(
       global_declared_variable* var = add_global_variable(global_name);
       if (!this->get_bool_or_default<
               error_config_globals_descriptor_shadowable_type_mismatch>(
-              descriptor_object["shadowable"], &var->is_shadowable, true)) {
+              descriptor_object["shadowable"], &var->is_shadowable, true,
+              reporter)) {
         return false;
       }
       if (!this->get_bool_or_default<
               error_config_globals_descriptor_writable_type_mismatch>(
-              descriptor_object["writable"], &var->is_writable, true)) {
+              descriptor_object["writable"], &var->is_writable, true,
+              reporter)) {
         return false;
       }
 
       break;
     }
 
     default:
-      this->errors_.report(error_config_globals_descriptor_type_mismatch{
+      reporter->report(error_config_globals_descriptor_type_mismatch{
           .descriptor = span_of_json_value(descriptor),
       });
       break;
@@ -418,7 +409,7 @@ bool configuration::should_remove_global_variable(string8_view name) {
 template <class Error>
 bool configuration::get_bool_or_default(
     ::simdjson::simdjson_result<::simdjson::ondemand::value>&& value, bool* out,
-    bool default_value) {
+    bool default_value, error_reporter* reporter) {
   ::simdjson::fallback::ondemand::value v;
   ::simdjson::error_code error = value.get(v);
   switch (error) {
@@ -428,7 +419,7 @@ bool configuration::get_bool_or_default(
       return false;
     }
     if (type != ::simdjson::fallback::ondemand::json_type::boolean) {
-      this->errors_.report(Error{span_of_json_value(v)});
+      reporter->report(Error{span_of_json_value(v)});
       *out = default_value;
       return true;
     }
@@ -447,11 +438,12 @@ bool configuration::get_bool_or_default(
   }
 }
 
-void configuration::report_json_error(padded_string_view json) {
+void configuration::report_json_error(padded_string_view json,
+                                      error_reporter* reporter) {
   // TODO(strager): Produce better error messages. simdjson provides no location
   // information for errors:
   // https://github.com/simdjson/simdjson/issues/237
-  this->errors_.report(error_config_json_syntax_error{
+  reporter->report(error_config_json_syntax_error{
       .where = source_code_span(json.data(), json.data()),
   });
 }

src/lsp-server.cpp

@@ -302,7 +302,7 @@ void linting_lsp_server_handler<Linter>::
     if (config_file.ok()) {
       if (*config_file) {
         doc.config = &(*config_file)->config;
-        if (doc.config->have_errors()) {
+        if (!(*config_file)->errors.empty()) {
           byte_buffer& message_json = notification_jsons.emplace_back();
           this->write_configuration_errors_notification(
               document_path, *config_file, message_json);
@@ -424,7 +424,7 @@ void linting_lsp_server_handler<Linter>::
   notification_json.append_copy(u8R"--(,"diagnostics":)--");
   lsp_error_reporter error_reporter(notification_json,
                                     &config_file->file_content);
-  config_file->config.report_errors(&error_reporter);
+  config_file->errors.copy_into(&error_reporter);
   error_reporter.finish();
 
   notification_json.append_copy(u8R"--(},"jsonrpc":"2.0"})--");

src/main.cpp

@@ -192,15 +192,16 @@ void handle_options(quick_lint_js::options o) {
       std::exit(1);
     }
     loaded_config_file *config_file = *config_result;
-    if (config_file && !config_file->config.errors_were_reported()) {
+    if (config_file && !config_file->config.errors_were_reported) {
       reporter.set_source(&config_file->file_content,
                           file_to_lint{
                               .path = config_file->config_path->c_str(),
                               .config_file = nullptr,
                               .is_stdin = false,
                               .vim_bufnr = std::nullopt,
                           });
-      config_file->config.report_errors(reporter.get());
+      config_file->errors.copy_into(reporter.get());
+      config_file->config.errors_were_reported = true;
     }
   }
 

src/quick-lint-js/configuration-loader.h

@@ -5,6 +5,7 @@
 #define QUICK_LINT_JS_CONFIGURATION_LOADER_H
 
 #include <optional>
+#include <quick-lint-js/buffering-error-reporter.h>
 #include <quick-lint-js/configuration.h>
 #include <quick-lint-js/file-canonical.h>
 #include <quick-lint-js/file.h>
@@ -44,6 +45,9 @@ struct loaded_config_file {
   // The content of the quick-lint-js.config file.
   padded_string file_content;
 
+  // Errors discovered while parsing file_content.
+  buffering_error_reporter errors;
+
   // The path to the quick-lint-js.config file. Never nullptr.
   const canonical_path* config_path;
 };

src/quick-lint-js/configuration.h

@@ -5,7 +5,6 @@
 #define QUICK_LINT_JS_CONFIGURATION_H
 
 #include <optional>
-#include <quick-lint-js/buffering-error-reporter.h>
 #include <quick-lint-js/char8.h>
 #include <quick-lint-js/file-canonical.h>
 #include <quick-lint-js/lint.h>
@@ -27,36 +26,35 @@ class configuration {
   global_declared_variable* add_global_variable(string8_view name);
   void remove_global_variable(string8_view name);
 
-  void load_from_json(padded_string_view);
-
-  void report_errors(error_reporter*);
-  bool have_errors() const noexcept;
-  bool errors_were_reported() const noexcept;
+  void load_from_json(padded_string_view, error_reporter*);
 
   void reset();
 
+  // TODO(strager): Move this out of the configuration class. It's only used by
+  // the CLI.
+  bool errors_were_reported = false;
+
  private:
-  bool load_global_groups_from_json(simdjson::ondemand::value&);
-  bool load_globals_from_json(simdjson::ondemand::object&);
+  bool load_global_groups_from_json(simdjson::ondemand::value&,
+                                    error_reporter*);
+  bool load_globals_from_json(simdjson::ondemand::object&, error_reporter*);
 
   bool should_remove_global_variable(string8_view name);
 
   // Returns false on parse error, and true otherwise.
   template <class Error>
   bool get_bool_or_default(
       ::simdjson::simdjson_result<::simdjson::ondemand::value>&& value,
-      bool* out, bool default_value);
+      bool* out, bool default_value, error_reporter*);
 
-  void report_json_error(padded_string_view json);
+  void report_json_error(padded_string_view json, error_reporter*);
 
   global_declared_variable_set globals_;
   std::vector<string8> globals_to_remove_;
   bool add_global_group_browser_ = true;
   bool add_global_group_node_js_ = true;
   bool add_global_group_ecmascript_ = true;
   monotonic_allocator string_allocator_;
-  buffering_error_reporter errors_;
-  bool errors_were_reported_ = false;
 
   string8_view save_string(std::string_view s);
 };