From c5fdcd0230452fc1eb35543a6095a3b65b7f3d5c Mon Sep 17 00:00:00 2001 From: "Matthew \"strager\" Glazar" Date: Fri, 15 Dec 2023 23:11:16 -0500 Subject: [PATCH] feat(typescript): improve diagnostic on duplicate 'in' or 'out' modifiers --- docs/CHANGELOG.md | 2 + po/messages.pot | 8 ++ .../diag/diagnostic-metadata-generated.cpp | 18 ++++ .../diag/diagnostic-metadata-generated.h | 3 +- src/quick-lint-js/diag/diagnostic-types-2.h | 10 ++ src/quick-lint-js/fe/parse-statement.cpp | 96 +++++++++++++------ .../i18n/translation-table-generated.cpp | 6 +- .../i18n/translation-table-generated.h | 6 +- .../i18n/translation-table-test-generated.h | 24 ++++- test/test-parse-typescript-generic.cpp | 67 +++++++++++++ 10 files changed, 208 insertions(+), 32 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 1317511ce8..62f70f01b7 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -31,6 +31,8 @@ Semantic Versioning. clause, or in a `typeof` type now reports [E0429][] instead of misleading diagnostics. * Properties in object literal types can now be named with number literals. + * Repeating `in` or `out` generic parameter modifiers now reports [E0432][] + ("'in' or 'out' variance specifier cannot be listed twice"). ### Fixed diff --git a/po/messages.pot b/po/messages.pot index 00354ccdc2..064330f8d7 100644 --- a/po/messages.pot +++ b/po/messages.pot @@ -1773,6 +1773,14 @@ msgstr "" msgid "'out in' is not allowed; write 'in out' instead" msgstr "" +#: src/quick-lint-js/diag/diagnostic-metadata-generated.cpp +msgid "'{0}' variance specifier cannot be listed twice" +msgstr "" + +#: src/quick-lint-js/diag/diagnostic-metadata-generated.cpp +msgid "'{0}' already written here" +msgstr "" + #: src/quick-lint-js/diag/diagnostic-metadata-generated.cpp msgid "unclosed block comment" msgstr "" diff --git a/src/quick-lint-js/diag/diagnostic-metadata-generated.cpp b/src/quick-lint-js/diag/diagnostic-metadata-generated.cpp index ca5f4d0b7f..dd3a85e1cf 100644 --- a/src/quick-lint-js/diag/diagnostic-metadata-generated.cpp +++ b/src/quick-lint-js/diag/diagnostic-metadata-generated.cpp @@ -5243,6 +5243,24 @@ const QLJS_CONSTINIT Diagnostic_Info all_diagnostic_infos[] = { }, }, + // Diag_TypeScript_Variance_Keyword_Repeated + { + .code = 432, + .severity = Diagnostic_Severity::error, + .message_formats = { + QLJS_TRANSLATABLE("'{0}' variance specifier cannot be listed twice"), + QLJS_TRANSLATABLE("'{0}' already written here"), + }, + .message_args = { + { + Diagnostic_Message_Arg_Info(offsetof(Diag_TypeScript_Variance_Keyword_Repeated, second_keyword), Diagnostic_Arg_Type::source_code_span), + }, + { + Diagnostic_Message_Arg_Info(offsetof(Diag_TypeScript_Variance_Keyword_Repeated, first_keyword), Diagnostic_Arg_Type::source_code_span), + }, + }, + }, + // Diag_Unclosed_Block_Comment { .code = 37, diff --git a/src/quick-lint-js/diag/diagnostic-metadata-generated.h b/src/quick-lint-js/diag/diagnostic-metadata-generated.h index b20c8342fc..d0f43bbb52 100644 --- a/src/quick-lint-js/diag/diagnostic-metadata-generated.h +++ b/src/quick-lint-js/diag/diagnostic-metadata-generated.h @@ -359,6 +359,7 @@ namespace quick_lint_js { QLJS_DIAG_TYPE_NAME(Diag_TypeScript_Spread_Element_Cannot_Be_Optional) \ QLJS_DIAG_TYPE_NAME(Diag_TypeScript_Style_Const_Field) \ QLJS_DIAG_TYPE_NAME(Diag_TypeScript_Variance_Keywords_In_Wrong_Order) \ + QLJS_DIAG_TYPE_NAME(Diag_TypeScript_Variance_Keyword_Repeated) \ QLJS_DIAG_TYPE_NAME(Diag_Unclosed_Block_Comment) \ QLJS_DIAG_TYPE_NAME(Diag_Unclosed_Class_Block) \ QLJS_DIAG_TYPE_NAME(Diag_Unclosed_Code_Block) \ @@ -453,7 +454,7 @@ namespace quick_lint_js { /* END */ // clang-format on -inline constexpr int Diag_Type_Count = 439; +inline constexpr int Diag_Type_Count = 440; extern const Diagnostic_Info all_diagnostic_infos[Diag_Type_Count]; } diff --git a/src/quick-lint-js/diag/diagnostic-types-2.h b/src/quick-lint-js/diag/diagnostic-types-2.h index 37e0a73d65..cfde629f55 100644 --- a/src/quick-lint-js/diag/diagnostic-types-2.h +++ b/src/quick-lint-js/diag/diagnostic-types-2.h @@ -2731,6 +2731,16 @@ struct Diag_TypeScript_Variance_Keywords_In_Wrong_Order { Source_Code_Span out_keyword; }; +struct Diag_TypeScript_Variance_Keyword_Repeated { + [[qljs::diag("E0432", Diagnostic_Severity::error)]] // + [[qljs::message("'{0}' variance specifier cannot be listed twice", + ARG(second_keyword))]] // + [[qljs::message("'{0}' already written here", + ARG(first_keyword))]] // + Source_Code_Span first_keyword; + Source_Code_Span second_keyword; +}; + struct Diag_Unclosed_Block_Comment { [[qljs::diag("E0037", Diagnostic_Severity::error)]] // [[qljs::message("unclosed block comment", ARG(comment_open))]] // diff --git a/src/quick-lint-js/fe/parse-statement.cpp b/src/quick-lint-js/fe/parse-statement.cpp index 6fbddf6690..5e9664483e 100644 --- a/src/quick-lint-js/fe/parse-statement.cpp +++ b/src/quick-lint-js/fe/parse-statement.cpp @@ -1662,33 +1662,32 @@ void Parser::parse_and_visit_typescript_generic_parameters( this->buffering_visitor_stack_.push(); next_parameter: - std::optional in_keyword; - std::optional out_keyword; - - if (this->peek().type == Token_Type::kw_in) { + // 'in' and 'out' keywords. + struct Modifier { + Identifier identifier; + Token_Type token_type; + }; + Vector modifiers( + "parse_and_visit_typescript_generic_parameters modifiers", + &this->temporary_memory_); + for (;;) { + switch (this->peek().type) { // // - in_keyword = this->peek().identifier_name(); - this->skip(); - } - - if (this->peek().type == Token_Type::kw_out) { - // - // - out_keyword = this->peek().identifier_name(); - this->skip(); - - if (!in_keyword.has_value() && this->peek().type == Token_Type::kw_in) { - // // Invalid. - in_keyword = this->peek().identifier_name(); - this->diag_reporter_->report( - Diag_TypeScript_Variance_Keywords_In_Wrong_Order{ - .in_keyword = in_keyword->span(), - .out_keyword = out_keyword->span(), - }); + case Token_Type::kw_in: + case Token_Type::kw_out: + modifiers.push_back(Modifier{ + .identifier = this->peek().identifier_name(), + .token_type = this->peek().type, + }); this->skip(); + break; + + default: + goto done_parsing_modifiers; } } +done_parsing_modifiers: std::optional parameter_name; switch (this->peek().type) { @@ -1712,7 +1711,6 @@ void Parser::parse_and_visit_typescript_generic_parameters( case Token_Type::kw_module: case Token_Type::kw_namespace: case Token_Type::kw_of: - case Token_Type::kw_out: case Token_Type::kw_override: case Token_Type::kw_readonly: case Token_Type::kw_require: @@ -1726,20 +1724,64 @@ void Parser::parse_and_visit_typescript_generic_parameters( break; case Token_Type::kw_in: - QLJS_PARSER_UNIMPLEMENTED(); + case Token_Type::kw_out: + QLJS_UNREACHABLE(); break; default: - if (out_keyword.has_value()) { + if (!modifiers.empty() && + modifiers.back().token_type == Token_Type::kw_out) { // - parameter_name = out_keyword; - out_keyword = std::nullopt; + parameter_name = modifiers.back().identifier; + modifiers.pop_back(); } else { QLJS_PARSER_UNIMPLEMENTED(); } break; } + // Report bad modifiers. + { + std::optional first_in_keyword; + std::optional first_out_keyword; + bool reported_out_in_misorder = false; + for (const Modifier &modifier : modifiers) { + if (modifier.token_type == Token_Type::kw_in) { + if (first_in_keyword.has_value()) { + this->diag_reporter_->report( + Diag_TypeScript_Variance_Keyword_Repeated{ + .first_keyword = *first_in_keyword, + .second_keyword = modifier.identifier.span(), + }); + } else { + first_in_keyword = modifier.identifier.span(); + } + } + if (modifier.token_type == Token_Type::kw_out) { + if (first_out_keyword.has_value()) { + this->diag_reporter_->report( + Diag_TypeScript_Variance_Keyword_Repeated{ + .first_keyword = *first_out_keyword, + .second_keyword = modifier.identifier.span(), + }); + } else { + first_out_keyword = modifier.identifier.span(); + } + } + if (!reported_out_in_misorder && + modifier.token_type == Token_Type::kw_in && + first_out_keyword.has_value()) { + // // Invalid. + this->diag_reporter_->report( + Diag_TypeScript_Variance_Keywords_In_Wrong_Order{ + .in_keyword = modifier.identifier.span(), + .out_keyword = *first_out_keyword, + }); + reported_out_in_misorder = true; + } + } + } + if (this->peek().type == Token_Type::kw_extends || this->peek().type == Token_Type::colon) { // // Invalid. diff --git a/src/quick-lint-js/i18n/translation-table-generated.cpp b/src/quick-lint-js/i18n/translation-table-generated.cpp index c4aec62eec..e6dc30f20c 100644 --- a/src/quick-lint-js/i18n/translation-table-generated.cpp +++ b/src/quick-lint-js/i18n/translation-table-generated.cpp @@ -92,7 +92,8 @@ const Translation_Table translation_data = { {15, 14, 0, 13, 0, 13}, // {0, 0, 0, 19, 0, 17}, // {0, 0, 0, 0, 0, 25}, // - {15, 38, 0, 25, 0, 42}, // + {0, 0, 0, 0, 0, 42}, // + {15, 38, 0, 25, 0, 27}, // {0, 0, 0, 0, 0, 17}, // {0, 0, 0, 0, 0, 11}, // {15, 17, 0, 22, 0, 38}, // @@ -102,6 +103,7 @@ const Translation_Table translation_data = { {0, 0, 0, 0, 0, 32}, // {69, 48, 0, 36, 0, 25}, // {0, 0, 0, 0, 0, 62}, // + {0, 0, 0, 0, 0, 48}, // {0, 0, 0, 0, 0, 39}, // {68, 25, 0, 65, 0, 28}, // {0, 0, 0, 0, 0, 62}, // @@ -1904,6 +1906,7 @@ const Translation_Table translation_data = { u8"'with' statement\0" u8"'{0} []' is always '{1}'\0" u8"'{0}' access specifier must precede '{1}'\0" + u8"'{0}' already written here\0" u8"'{0}' found here\0" u8"'{0}' here\0" u8"'{0}' is missing on overloaded method\0" @@ -1913,6 +1916,7 @@ const Translation_Table translation_data = { u8"'{0}' is not allowed with '{1}'\0" u8"'{0}' must precede '{1}'\0" u8"'{0}' operator cannot be used before '**' without parentheses\0" + u8"'{0}' variance specifier cannot be listed twice\0" u8"'{1}' is missing on overload signature\0" u8"'{1}' statement starts here\0" u8"'}' is not allowed directly in JSX text; write {{'}'} instead\0" diff --git a/src/quick-lint-js/i18n/translation-table-generated.h b/src/quick-lint-js/i18n/translation-table-generated.h index 9609de7384..d299569832 100644 --- a/src/quick-lint-js/i18n/translation-table-generated.h +++ b/src/quick-lint-js/i18n/translation-table-generated.h @@ -18,8 +18,8 @@ namespace quick_lint_js { using namespace std::literals::string_view_literals; constexpr std::uint32_t translation_table_locale_count = 5; -constexpr std::uint16_t translation_table_mapping_table_size = 541; -constexpr std::size_t translation_table_string_table_size = 80778; +constexpr std::uint16_t translation_table_mapping_table_size = 543; +constexpr std::size_t translation_table_string_table_size = 80853; constexpr std::size_t translation_table_locale_table_size = 35; QLJS_CONSTEVAL std::uint16_t translation_table_const_look_up( @@ -107,6 +107,7 @@ QLJS_CONSTEVAL std::uint16_t translation_table_const_look_up( "'with' statement"sv, "'{0} []' is always '{1}'"sv, "'{0}' access specifier must precede '{1}'"sv, + "'{0}' already written here"sv, "'{0}' found here"sv, "'{0}' here"sv, "'{0}' is missing on overloaded method"sv, @@ -116,6 +117,7 @@ QLJS_CONSTEVAL std::uint16_t translation_table_const_look_up( "'{0}' is not allowed with '{1}'"sv, "'{0}' must precede '{1}'"sv, "'{0}' operator cannot be used before '**' without parentheses"sv, + "'{0}' variance specifier cannot be listed twice"sv, "'{1}' is missing on overload signature"sv, "'{1}' statement starts here"sv, "'}' is not allowed directly in JSX text; write {{'}'} instead"sv, diff --git a/src/quick-lint-js/i18n/translation-table-test-generated.h b/src/quick-lint-js/i18n/translation-table-test-generated.h index 2d67b7d84c..7a3ef5903a 100644 --- a/src/quick-lint-js/i18n/translation-table-test-generated.h +++ b/src/quick-lint-js/i18n/translation-table-test-generated.h @@ -27,7 +27,7 @@ struct Translated_String { }; // clang-format off -inline const Translated_String test_translation_table[540] = { +inline const Translated_String test_translation_table[542] = { { "\"global-groups\" entries must be strings"_translatable, u8"\"global-groups\" entries must be strings", @@ -919,6 +919,17 @@ inline const Translated_String test_translation_table[540] = { u8"'{0}' access specifier must precede '{1}'", }, }, + { + "'{0}' already written here"_translatable, + u8"'{0}' already written here", + { + u8"'{0}' already written here", + u8"'{0}' already written here", + u8"'{0}' already written here", + u8"'{0}' already written here", + u8"'{0}' already written here", + }, + }, { "'{0}' found here"_translatable, u8"'{0}' found here", @@ -1018,6 +1029,17 @@ inline const Translated_String test_translation_table[540] = { u8"'{0}' operator cannot be used before '**' without parentheses", }, }, + { + "'{0}' variance specifier cannot be listed twice"_translatable, + u8"'{0}' variance specifier cannot be listed twice", + { + u8"'{0}' variance specifier cannot be listed twice", + u8"'{0}' variance specifier cannot be listed twice", + u8"'{0}' variance specifier cannot be listed twice", + u8"'{0}' variance specifier cannot be listed twice", + u8"'{0}' variance specifier cannot be listed twice", + }, + }, { "'{1}' is missing on overload signature"_translatable, u8"'{1}' is missing on overload signature", diff --git a/test/test-parse-typescript-generic.cpp b/test/test-parse-typescript-generic.cpp index f27a786537..0c353bcceb 100644 --- a/test/test-parse-typescript-generic.cpp +++ b/test/test-parse-typescript-generic.cpp @@ -450,6 +450,27 @@ TEST_F(Test_Parse_TypeScript_Generic, variance_specifiers_in_wrong_order) { } } +TEST_F(Test_Parse_TypeScript_Generic, duplicate_variance_specifiers) { + test_parse_and_visit_typescript_generic_parameters( + u8""_sv, // + u8" ^^ Diag_TypeScript_Variance_Keyword_Repeated.second_keyword\n"_diag // + u8" ^^ .first_keyword"_diag, // + typescript_options); + test_parse_and_visit_typescript_generic_parameters( + u8""_sv, // + u8" ^^^ Diag_TypeScript_Variance_Keyword_Repeated.second_keyword\n"_diag // + u8" ^^^ .first_keyword"_diag, // + typescript_options); + + test_parse_and_visit_typescript_generic_parameters( + u8""_sv, // + u8" ^^^ Diag_TypeScript_Variance_Keyword_Repeated.second_keyword\n"_diag // + u8" ^^^ .first_keyword"_diag, // + u8" ^^^ Diag_TypeScript_Variance_Keyword_Repeated.second_keyword\n"_diag // + u8" ^^^ .first_keyword"_diag, // + typescript_options); +} + TEST_F(Test_Parse_TypeScript_Generic, parameters_can_be_named_contextual_keywords) { for (String8 name : @@ -510,6 +531,52 @@ TEST_F(Test_Parse_TypeScript_Generic, } } +TEST_F(Test_Parse_TypeScript_Generic, parameter_can_be_named_out) { + { + Spy_Visitor v = test_parse_and_visit_typescript_generic_parameters( + u8""_sv, no_diags, typescript_options); + EXPECT_THAT(v.variable_declarations, + ElementsAreArray({generic_param_decl(u8"out"_sv)})); + } + + { + Spy_Visitor v = test_parse_and_visit_typescript_generic_parameters( + u8""_sv, no_diags, typescript_options); + EXPECT_THAT(v.variable_declarations, + ElementsAreArray({generic_param_decl(u8"out"_sv)})); + } + + { + Spy_Visitor v = test_parse_and_visit_typescript_generic_parameters( + u8""_sv, no_diags, typescript_options); + EXPECT_THAT(v.variable_declarations, + ElementsAreArray({generic_param_decl(u8"out"_sv), + generic_param_decl(u8"other"_sv)})); + } + + { + Spy_Visitor v = test_parse_and_visit_typescript_generic_parameters( + u8""_sv, no_diags, typescript_options); + EXPECT_THAT(v.variable_declarations, + ElementsAreArray({generic_param_decl(u8"out"_sv), + generic_param_decl(u8"other"_sv)})); + } + + { + Spy_Visitor v = test_parse_and_visit_typescript_generic_parameters( + u8""_sv, no_diags, typescript_options); + EXPECT_THAT(v.variable_declarations, + ElementsAreArray({generic_param_decl(u8"out"_sv)})); + } + + { + Spy_Visitor v = test_parse_and_visit_typescript_generic_parameters( + u8""_sv, no_diags, typescript_options); + EXPECT_THAT(v.variable_declarations, + ElementsAreArray({generic_param_decl(u8"out"_sv)})); + } +} + TEST_F(Test_Parse_TypeScript_Generic, function_call_with_generic_arguments) { { Test_Parser p(u8"foo(p)"_sv, typescript_options);