From 3ddd74dc4dd2ecf8b822b8b6fec829e0f3571c1b Mon Sep 17 00:00:00 2001 From: "Matthew \"strager\" Glazar" Date: Wed, 3 Jan 2024 14:27:34 -0500 Subject: [PATCH] refactor(diag): create new Diag_List type; use in one spot I don't like Buffering_Diag_Reporter. Create a new Diag_List type, decoupled from Diag_Reporter, which we can use to "just" store a list of diagnostics (hence its name). To test the waters, use Diag_List in place of Buffering_Diag_Reporter in one spot in the Lexer. --- src/CMakeLists.txt | 2 + src/quick-lint-js/diag/diag-list.cpp | 95 +++++++++++++ src/quick-lint-js/diag/diag-list.h | 90 ++++++++++++ src/quick-lint-js/diag/diag-reporter.cpp | 7 + src/quick-lint-js/diag/diag-reporter.h | 5 + src/quick-lint-js/fe/lex.cpp | 35 +++-- src/quick-lint-js/fe/lex.h | 4 +- src/quick-lint-js/fe/token.cpp | 5 +- src/quick-lint-js/fe/token.h | 4 +- test/CMakeLists.txt | 1 + test/test-diag-list.cpp | 171 +++++++++++++++++++++++ 11 files changed, 398 insertions(+), 21 deletions(-) create mode 100644 src/quick-lint-js/diag/diag-list.cpp create mode 100644 src/quick-lint-js/diag/diag-list.h create mode 100644 test/test-diag-list.cpp diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 6b110aa922..9fb9ad8acb 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -315,6 +315,8 @@ quick_lint_js_add_library( quick-lint-js/diag/buffering-diag-reporter.h quick-lint-js/diag/diag-code-list.cpp quick-lint-js/diag/diag-code-list.h + quick-lint-js/diag/diag-list.cpp + quick-lint-js/diag/diag-list.h quick-lint-js/diag/diag-reporter.cpp quick-lint-js/diag/diag-reporter.h quick-lint-js/diag/diagnostic-formatter.cpp diff --git a/src/quick-lint-js/diag/diag-list.cpp b/src/quick-lint-js/diag/diag-list.cpp new file mode 100644 index 0000000000..a2c1600908 --- /dev/null +++ b/src/quick-lint-js/diag/diag-list.cpp @@ -0,0 +1,95 @@ +// Copyright (C) 2020 Matthew "strager" Glazar +// See end of file for extended copyright information. + +#include +#include +#include + +namespace quick_lint_js { +struct Diag_List::Node : public Node_Base { + union Underlying_Diag { + explicit Underlying_Diag() {} + +#define QLJS_DIAG_TYPE_NAME(name) \ + ::quick_lint_js::name name; \ + static_assert(std::is_trivially_copyable_v<::quick_lint_js::name>, \ + #name " should be trivially copyable"); + QLJS_X_DIAG_TYPE_NAMES +#undef QLJS_DIAG_TYPE_NAME + }; + + Underlying_Diag data; +}; +static_assert(alignof(Diag_List::Node) == alignof(Diag_List::Node_Base), + "Node_Base alignment should align any Diag type"); + +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::Rewind_State Diag_List::prepare_for_rewind() { + return Rewind_State{ + .first_ = this->first_, + .last_ = this->last_, + }; +} + +void Diag_List::rewind(Rewind_State &&r) { + // Leak nodes between r.last and this->last_. this->memory should be a + // Linked_Bump_Allocator managed by the caller. + this->first_ = r.first_; + this->last_ = r.last_; +} + +#define QLJS_DIAG_TYPE_NAME(name) \ + void Diag_List::add(name diag) { \ + this->add_impl(Diag_Type::name, &diag); \ + } +QLJS_X_DIAG_TYPE_NAMES +#undef QLJS_DIAG_TYPE_NAME + +template +void Diag_List::add_impl(Diag_Type type, void *diag) { + Node *node = this->memory_->new_object(); + node->next = nullptr; + node->type = type; + std::memcpy(&node->data, diag, Diag_Size); + if (this->last_ == nullptr) { + this->first_ = node; + } else { + this->last_->next = node; + } + this->last_ = node; +} + +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_; + node != nullptr; node = node->next) { + if (!contains(ignored_types, node->type)) { + return true; + } + } + return false; +} +} + +// quick-lint-js finds bugs in JavaScript programs. +// Copyright (C) 2020 Matthew "strager" Glazar +// +// This file is part of quick-lint-js. +// +// quick-lint-js is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// quick-lint-js is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with quick-lint-js. If not, see . diff --git a/src/quick-lint-js/diag/diag-list.h b/src/quick-lint-js/diag/diag-list.h new file mode 100644 index 0000000000..44c4216391 --- /dev/null +++ b/src/quick-lint-js/diag/diag-list.h @@ -0,0 +1,90 @@ +// Copyright (C) 2020 Matthew "strager" Glazar +// See end of file for extended copyright information. + +#pragma once + +#include +#include + +namespace quick_lint_js { +// Internally, Diag_List is implemented as a singly linked list. +class Diag_List { + public: + // Private to Diag_List. Do not use. + // + // Node_Base is split from Node to improve compile times. The union inside + // Node compiles slowly. + // + // Node_Base needs to be in the header for an efficient implementation of + // for_each. + struct Node_Base { + Node_Base *next; + Diag_Type type; + + // Returns &static_cast(this)->data. + void *get_data() { return &this[1]; } + }; + // Private to Diag_List. Do not use. + struct Node; + + struct Rewind_State { + // Private to Diag_List. Do not use. + Node_Base *first_; + Node_Base *last_; + }; + + explicit Diag_List(Memory_Resource *); + + Diag_List(Diag_List &&) = delete; + Diag_List &operator=(Diag_List &&) = delete; + + ~Diag_List(); + +#define QLJS_DIAG_TYPE_NAME(name) void add(name diag); + QLJS_X_DIAG_TYPE_NAMES +#undef QLJS_DIAG_TYPE_NAME + + Rewind_State prepare_for_rewind(); + + void rewind(Rewind_State &&r); + + // Func must have the following signature: + // + // void(Diag_Type type, const void* diag); + template + void for_each(Func &&callback) const { + for (Node_Base *node = this->first_; node != nullptr; node = node->next) { + callback(node->type, node->get_data()); + } + } + + bool reported_any_diagnostic_except_since( + std::initializer_list ignored_types, const Rewind_State &); + + private: + template + void add_impl(Diag_Type type, void *diag); + + Memory_Resource *memory_; + Node_Base *first_ = nullptr; + Node_Base *last_ = nullptr; +}; +} + +// quick-lint-js finds bugs in JavaScript programs. +// Copyright (C) 2020 Matthew "strager" Glazar +// +// This file is part of quick-lint-js. +// +// quick-lint-js is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// quick-lint-js is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with quick-lint-js. If not, see . diff --git a/src/quick-lint-js/diag/diag-reporter.cpp b/src/quick-lint-js/diag/diag-reporter.cpp index 4f2d1a1e43..9e2e36db39 100644 --- a/src/quick-lint-js/diag/diag-reporter.cpp +++ b/src/quick-lint-js/diag/diag-reporter.cpp @@ -1,6 +1,7 @@ // Copyright (C) 2020 Matthew "strager" Glazar // See end of file for extended copyright information. +#include #include #include @@ -12,6 +13,12 @@ namespace quick_lint_js { QLJS_X_DIAG_TYPE_NAMES #undef QLJS_DIAG_TYPE_NAME +void Diag_Reporter::report(const Diag_List& diags) { + diags.for_each([&](Diag_Type type, void* diag) -> void { + this->report_impl(type, diag); + }); +} + Null_Diag_Reporter Null_Diag_Reporter::instance; } diff --git a/src/quick-lint-js/diag/diag-reporter.h b/src/quick-lint-js/diag/diag-reporter.h index b2746e482a..aef8683be1 100644 --- a/src/quick-lint-js/diag/diag-reporter.h +++ b/src/quick-lint-js/diag/diag-reporter.h @@ -6,6 +6,8 @@ #include namespace quick_lint_js { +class Diag_List; + class Diag_Reporter { public: Diag_Reporter() = default; @@ -22,6 +24,9 @@ class Diag_Reporter { QLJS_X_DIAG_TYPE_NAMES #undef QLJS_DIAG_TYPE_NAME + virtual void report(const Diag_List &); + + // TODO(#1154): Delete this in favor of report(const Diag_List&). virtual void report_impl(Diag_Type type, void *diag) = 0; }; diff --git a/src/quick-lint-js/fe/lex.cpp b/src/quick-lint-js/fe/lex.cpp index 565f1e6e02..b33684e1d4 100644 --- a/src/quick-lint-js/fe/lex.cpp +++ b/src/quick-lint-js/fe/lex.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -725,11 +726,13 @@ const Char8* Lexer::parse_string_literal() { } } break; - case 'u': - c = this->parse_unicode_escape(escape_sequence_start, - this->diag_reporter_) - .end; + case 'u': { + // TODO(#1154): Remove this temporary Diag_List. + Diag_List diags(&this->allocator_); + c = this->parse_unicode_escape(escape_sequence_start, &diags).end; + this->diag_reporter_->report(diags); break; + } default: ++c; break; @@ -841,7 +844,7 @@ void Lexer::skip_in_template(const Char8* template_begin) { Lexer::Parsed_Template_Body Lexer::parse_template_body( const Char8* input, const Char8* template_begin, Diag_Reporter* diag_reporter) { - Buffering_Diag_Reporter* escape_sequence_diagnostics = nullptr; + Diag_List* escape_sequence_diagnostics = nullptr; const Char8* c = input; for (;;) { switch (*c) { @@ -878,8 +881,7 @@ Lexer::Parsed_Template_Body Lexer::parse_template_body( case 'u': { if (!escape_sequence_diagnostics) { escape_sequence_diagnostics = - this->allocator_.new_object( - &this->allocator_); + this->allocator_.new_object(&this->allocator_); } c = this->parse_unicode_escape(escape_sequence_start, escape_sequence_diagnostics) @@ -1527,8 +1529,8 @@ const Char8* Lexer::parse_hex_digits_and_underscores(const Char8* input) { QLJS_WARNING_PUSH QLJS_WARNING_IGNORE_GCC("-Wuseless-cast") -Lexer::Parsed_Unicode_Escape Lexer::parse_unicode_escape( - const Char8* input, Diag_Reporter* reporter) { +Lexer::Parsed_Unicode_Escape Lexer::parse_unicode_escape(const Char8* input, + Diag_List* out_diags) { const Char8* escape_sequence_begin = input; auto get_escape_span = [escape_sequence_begin, &input]() { return Source_Code_Span(escape_sequence_begin, input); @@ -1545,7 +1547,7 @@ Lexer::Parsed_Unicode_Escape Lexer::parse_unicode_escape( // TODO: Add an enum to Diag_Unclosed_Identifier_Escape_Sequence to // indicate whether the token is a template literal, a string literal // or an identifier. - reporter->report(Diag_Unclosed_Identifier_Escape_Sequence{ + out_diags->add(Diag_Unclosed_Identifier_Escape_Sequence{ .escape_sequence = get_escape_span()}); return Parsed_Unicode_Escape{.end = input, .code_point = std::nullopt}; } @@ -1557,7 +1559,7 @@ Lexer::Parsed_Unicode_Escape Lexer::parse_unicode_escape( code_point_hex_end = input; ++input; // Skip "}". if (found_non_hex_digit || code_point_hex_begin == code_point_hex_end) { - reporter->report(Diag_Expected_Hex_Digits_In_Unicode_Escape{ + out_diags->add(Diag_Expected_Hex_Digits_In_Unicode_Escape{ .escape_sequence = get_escape_span()}); return Parsed_Unicode_Escape{.end = input, .code_point = std::nullopt}; } @@ -1569,12 +1571,12 @@ Lexer::Parsed_Unicode_Escape Lexer::parse_unicode_escape( // TODO: Add an enum to Diag_Expected_Hex_Digits_In_Unicode_Escape to // indicate whether the token is a template literal, a string literal // or an identifier. - reporter->report(Diag_Expected_Hex_Digits_In_Unicode_Escape{ + out_diags->add(Diag_Expected_Hex_Digits_In_Unicode_Escape{ .escape_sequence = get_escape_span()}); return Parsed_Unicode_Escape{.end = input, .code_point = std::nullopt}; } if (!is_hex_digit(*input)) { - reporter->report(Diag_Expected_Hex_Digits_In_Unicode_Escape{ + out_diags->add(Diag_Expected_Hex_Digits_In_Unicode_Escape{ .escape_sequence = Source_Code_Span(escape_sequence_begin, input + 1)}); return Parsed_Unicode_Escape{.end = input, .code_point = std::nullopt}; @@ -1596,7 +1598,7 @@ Lexer::Parsed_Unicode_Escape Lexer::parse_unicode_escape( break; } if (code_point >= 0x110000) { - reporter->report(Diag_Escaped_Code_Point_In_Unicode_Out_Of_Range{ + out_diags->add(Diag_Escaped_Code_Point_In_Unicode_Out_Of_Range{ .escape_sequence = get_escape_span()}); } return Parsed_Unicode_Escape{.end = input, .code_point = code_point}; @@ -1717,8 +1719,11 @@ Lexer::Parsed_Identifier Lexer::parse_identifier_slow( auto parse_unicode_escape = [&]() { const Char8* escape_begin = input; + // TODO(#1154): Remove this temporary Diag_List. + Diag_List diags(&this->allocator_); Parsed_Unicode_Escape escape = - this->parse_unicode_escape(escape_begin, this->diag_reporter_); + this->parse_unicode_escape(escape_begin, &diags); + this->diag_reporter_->report(diags); if (escape.code_point.has_value()) { bool is_initial_identifier_character = escape_begin == identifier_begin; diff --git a/src/quick-lint-js/fe/lex.h b/src/quick-lint-js/fe/lex.h index d75a59355e..e522d6a7b7 100644 --- a/src/quick-lint-js/fe/lex.h +++ b/src/quick-lint-js/fe/lex.h @@ -206,7 +206,7 @@ class Lexer { Token_Type type; const Char8* end; // Might be null. - Buffering_Diag_Reporter* escape_sequence_diagnostics; + Diag_List* escape_sequence_diagnostics; }; // The result of parsing an identifier. @@ -284,7 +284,7 @@ class Lexer { }; Parsed_Unicode_Escape parse_unicode_escape(const Char8* input, - Diag_Reporter*); + Diag_List* out_diags); Parsed_Identifier parse_identifier(const Char8*, Identifier_Kind); const Char8* parse_identifier_fast_only(const Char8*); diff --git a/src/quick-lint-js/fe/token.cpp b/src/quick-lint-js/fe/token.cpp index d4a63c2ba6..7e34848bc7 100644 --- a/src/quick-lint-js/fe/token.cpp +++ b/src/quick-lint-js/fe/token.cpp @@ -2,7 +2,8 @@ // See end of file for extended copyright information. #include -#include +#include +#include #include #include #include @@ -45,7 +46,7 @@ void Token::report_errors_for_escape_sequences_in_template( QLJS_ASSERT(this->type == Token_Type::complete_template || this->type == Token_Type::incomplete_template); if (this->template_escape_sequence_diagnostics) { - this->template_escape_sequence_diagnostics->move_into(reporter); + reporter->report(*this->template_escape_sequence_diagnostics); } } diff --git a/src/quick-lint-js/fe/token.h b/src/quick-lint-js/fe/token.h index e703b37eb5..3802b21ac9 100644 --- a/src/quick-lint-js/fe/token.h +++ b/src/quick-lint-js/fe/token.h @@ -194,7 +194,7 @@ case ::quick_lint_js::Token_Type::question_question_equal namespace quick_lint_js { -class Buffering_Diag_Reporter; +class Diag_List; class Diag_Reporter; class Identifier; class Source_Code_Span; @@ -330,7 +330,7 @@ struct Token { // Used only if this is a reserved_keyword_with_escape_sequence token. Escape_Sequence_List* identifier_escape_sequences; // Used only if this is a complete_template or incomplete_template token. - Buffering_Diag_Reporter* template_escape_sequence_diagnostics; + Diag_List* template_escape_sequence_diagnostics; }; }; } diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 1296033539..aab90eee86 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -94,6 +94,7 @@ quick_lint_js_add_executable( test-cxx-parser.cpp test-debug-server.cpp test-diag-code-list.cpp + test-diag-list.cpp test-diag-matcher.cpp test-diagnostic-assertion.cpp test-diagnostic-formatter.cpp diff --git a/test/test-diag-list.cpp b/test/test-diag-list.cpp new file mode 100644 index 0000000000..f551000dcd --- /dev/null +++ b/test/test-diag-list.cpp @@ -0,0 +1,171 @@ +// Copyright (C) 2020 Matthew "strager" Glazar +// See end of file for extended copyright information. + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace quick_lint_js { +namespace { +TEST(Test_Diag_List, saves_all_data) { + static Padded_String let_code(u8"let"_sv); + static Padded_String expression_code(u8"2+2==5"_sv); + + Linked_Bump_Allocator memory("test"); + Diag_List diags(&memory); + diags.add(Diag_Let_With_No_Bindings{.where = span_of(let_code)}); + diags.add(Diag_Expected_Parenthesis_Around_If_Condition{ + .where = span_of(expression_code), + .token = u8'(', + }); + + int report_count = 0; + diags.for_each([&](Diag_Type type, void* diag) -> void { + report_count += 1; + switch (report_count) { + case 1: { + ASSERT_EQ(type, Diag_Type::Diag_Let_With_No_Bindings); + const auto* d = static_cast(diag); + EXPECT_TRUE(same_pointers(d->where, span_of(let_code))); + break; + } + case 2: { + ASSERT_EQ(type, Diag_Type::Diag_Expected_Parenthesis_Around_If_Condition); + const auto* d = + static_cast( + diag); + EXPECT_TRUE(same_pointers(d->where, span_of(expression_code))); + EXPECT_EQ(d->token, u8'('); + break; + } + default: + ADD_FAILURE() << "expected at most two calls to report_impl"; + break; + } + }); + EXPECT_EQ(report_count, 2); +} + +TEST(Test_Diag_List, reported_any_diagnostic_except_diag_types) { + static Padded_String code(u8"let"_sv); + Linked_Bump_Allocator memory("test"); + Diag_List diags(&memory); + Diag_List::Rewind_State begin_rewind_state = diags.prepare_for_rewind(); + + EXPECT_FALSE(diags.reported_any_diagnostic_except_since( + { + Diag_Type::Diag_Assignment_Before_Variable_Declaration, + }, + begin_rewind_state)); + diags.add(Diag_Assignment_Before_Variable_Declaration{ + .assignment = span_of(code), + .declaration = span_of(code), + }); + EXPECT_FALSE(diags.reported_any_diagnostic_except_since( + { + Diag_Type::Diag_Assignment_Before_Variable_Declaration, + }, + begin_rewind_state)); + EXPECT_TRUE(diags.reported_any_diagnostic_except_since( + { + Diag_Type::Diag_Assignment_To_Const_Global_Variable, + }, + begin_rewind_state)); + EXPECT_FALSE(diags.reported_any_diagnostic_except_since( + { + Diag_Type::Diag_Assignment_To_Const_Global_Variable, + Diag_Type::Diag_Assignment_Before_Variable_Declaration, + }, + begin_rewind_state)); + EXPECT_FALSE(diags.reported_any_diagnostic_except_since( + { + Diag_Type::Diag_Assignment_Before_Variable_Declaration, + Diag_Type::Diag_Assignment_To_Const_Global_Variable, + }, + begin_rewind_state)); +} + +TEST(Test_Diag_List, + reported_any_diagnostic_except_diag_types_since_rewind_state) { + static Padded_String code(u8"let"_sv); + Linked_Bump_Allocator memory("test"); + Diag_List diags(&memory); + diags.add(Diag_Assignment_Before_Variable_Declaration{ + .assignment = span_of(code), + .declaration = span_of(code), + }); + Diag_List::Rewind_State middle_rewind_state = diags.prepare_for_rewind(); + + EXPECT_FALSE(diags.reported_any_diagnostic_except_since( + { + Diag_Type::Diag_Assignment_Before_Variable_Declaration, + }, + middle_rewind_state)); + diags.add(Diag_Assignment_Before_Variable_Declaration{ + .assignment = span_of(code), + .declaration = span_of(code), + }); + EXPECT_FALSE(diags.reported_any_diagnostic_except_since( + { + Diag_Type::Diag_Assignment_Before_Variable_Declaration, + }, + middle_rewind_state)); + EXPECT_TRUE(diags.reported_any_diagnostic_except_since( + { + Diag_Type::Diag_Assignment_To_Const_Global_Variable, + }, + middle_rewind_state)); + EXPECT_FALSE(diags.reported_any_diagnostic_except_since( + { + Diag_Type::Diag_Assignment_To_Const_Global_Variable, + Diag_Type::Diag_Assignment_Before_Variable_Declaration, + }, + middle_rewind_state)); + EXPECT_FALSE(diags.reported_any_diagnostic_except_since( + { + Diag_Type::Diag_Assignment_Before_Variable_Declaration, + Diag_Type::Diag_Assignment_To_Const_Global_Variable, + }, + middle_rewind_state)); +} + +TEST(Test_Diag_List, not_destructing_does_not_leak) { + // This test relies on a leak checker such as Valgrind's memtest or + // Clang's LeakSanitizer. + + Linked_Bump_Allocator memory("test"); + alignas(Diag_List) std::byte diags_storage[sizeof(Diag_List)]; + Diag_List* diags = new (&diags_storage) Diag_List(&memory); + + Padded_String let_code(u8"let"_sv); + diags->add(Diag_Let_With_No_Bindings{.where = span_of(let_code)}); + + // Destruct memory, but don't destruct *diags. +} +} +} + +// quick-lint-js finds bugs in JavaScript programs. +// Copyright (C) 2020 Matthew "strager" Glazar +// +// This file is part of quick-lint-js. +// +// quick-lint-js is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// quick-lint-js is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with quick-lint-js. If not, see .