From 7a17bc9ef0b56c91fb72385ff090c6e0afa13f85 Mon Sep 17 00:00:00 2001 From: "Matthew \"strager\" Glazar" Date: Sun, 10 Dec 2023 03:08:03 -0500 Subject: [PATCH] fix(typescript): fix parsing of 'c ? (p): T => b : f' ':' serves two purposes: mark type annotations (e.g. for arrow function return types, as in '(p): T => b') and delimit the false branch of a conditional expression (as in 'c ? t : f'). Our parser gets confused in cases where the two are mixed. Follow TypeScript's behavior and use a transaction, backtracking when needed. --- docs/CHANGELOG.md | 4 ++ src/quick-lint-js/fe/parse-expression.cpp | 51 ++++++++++++++- src/quick-lint-js/fe/parse.h | 10 +++ test/test-parse-expression-typescript.cpp | 75 +++++++++++++++++++++++ 4 files changed, 138 insertions(+), 2 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 784c7c7700..1fd14270a2 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -23,6 +23,10 @@ Semantic Versioning. [E0426][] ("type predicates are only allowed as function return types"). * `export default` now supports separately-declared TypeScript interfaces and types. + * `cond ? (param): ReturnType => body : f` is now correctly parsed as a + conditional expression with a function in the truthy branch. + (`cond ? (t) : param => body` continues to be parsed as a conditional + expression with a function in the falsy branch.) ### Fixed diff --git a/src/quick-lint-js/fe/parse-expression.cpp b/src/quick-lint-js/fe/parse-expression.cpp index bea42a4263..6264069177 100644 --- a/src/quick-lint-js/fe/parse-expression.cpp +++ b/src/quick-lint-js/fe/parse-expression.cpp @@ -1952,7 +1952,8 @@ Expression* Parser::parse_expression_remainder(Parse_Visitor_Base& v, } else { true_expression = this->parse_expression( v, Precedence{ - .colon_type_annotation = Allow_Type_Annotations::never, + .colon_type_annotation = Allow_Type_Annotations:: + typescript_only_if_legal_as_conditional_true_branch, }); } @@ -2105,8 +2106,14 @@ Expression* Parser::parse_expression_remainder(Parse_Visitor_Base& v, // x: Type // TypeScript only. case Token_Type::colon: { + Expression* child = binary_builder.last_expression(); bool should_parse_annotation; switch (prec.colon_type_annotation) { + case Allow_Type_Annotations:: + typescript_only_if_legal_as_conditional_true_branch: + should_parse_annotation = + this->options_.typescript && child->kind() == Expression_Kind::Paren; + break; case Allow_Type_Annotations::typescript_only: should_parse_annotation = this->options_.typescript; break; @@ -2120,10 +2127,11 @@ Expression* Parser::parse_expression_remainder(Parse_Visitor_Base& v, if (!should_parse_annotation) { break; } - Expression* child = binary_builder.last_expression(); Source_Code_Span colon_span = this->peek().span(); Buffering_Visitor type_visitor(&this->type_expression_memory_); + Parser_Transaction transaction = this->begin_transaction(); + // If an arrow function has a return type, then the return type must *not* // be parenthesized. Exception: an possibly-parenthesized arrow return type // is okay. @@ -2143,6 +2151,45 @@ Expression* Parser::parse_expression_remainder(Parse_Visitor_Base& v, .allow_assertion_signature_or_type_predicate = is_possibly_arrow_function_return_type_annotation, }); + if (prec.colon_type_annotation == + Allow_Type_Annotations:: + typescript_only_if_legal_as_conditional_true_branch && + this->options_.typescript && + this->peek().type == Token_Type::equal_greater) { + // cond ? (t) : param => body + // cond ? (param): RetType => body : f + // ^^ + + // TODO(strager): We should call validate_arrow_function_parameter_list, + // similar to if we did 'goto arrow_function;'. + + this->skip(); + Stacked_Buffering_Visitor arrow_body_visits = + this->buffering_visitor_stack_.push(); + Expression* arrow_function = + this->parse_arrow_function_expression_remainder( + arrow_body_visits.visitor(), /*generic_parameter_visits=*/nullptr, + child, + /*return_type_visits=*/&type_visitor, /*prec=*/prec); + + if (this->peek().type == Token_Type::colon) { + // (cond ? (param): RetType => body : f) + // ^ + // We correctly interpreted ': RetType' as a return type annotation. + this->commit_transaction(std::move(transaction)); + arrow_body_visits.visitor().move_into(v); + binary_builder.replace_last(arrow_function); + break; + } else { + // (cond ? (x) : param => body) + // ^ + // We incorrectly interpreted ': param' as a return type annotation. + this->roll_back_transaction(std::move(transaction)); + break; + } + } + this->commit_transaction(std::move(transaction)); + const Char8* type_end = this->lexer_.end_of_previous_token(); binary_builder.replace_last( this->make_expression( diff --git a/src/quick-lint-js/fe/parse.h b/src/quick-lint-js/fe/parse.h index 5d60980130..0f2c40be72 100644 --- a/src/quick-lint-js/fe/parse.h +++ b/src/quick-lint-js/fe/parse.h @@ -652,6 +652,16 @@ class Parser { // > 'unsigned char:2' in initialization enum Allow_Type_Annotations : std::uint8_t { typescript_only, + // Used inside 't' in 'c ? t : f': + // + // c ? (param): Type => body : f; // 'Type' is a type annotation. + // c ? x : y => body : f; // 'y' is not a type annotation because + // // 'x' is not parenthesized. Invalid. + // c ? (x) : param => body; // 'param' is not a type annotation + // // because no ':' follows 'body'. + // Only parse annotations if they are valid return type annotations inside + // the '?:'. + typescript_only_if_legal_as_conditional_true_branch, always, never, }; diff --git a/test/test-parse-expression-typescript.cpp b/test/test-parse-expression-typescript.cpp index 73a4af55fe..3c85a54946 100644 --- a/test/test-parse-expression-typescript.cpp +++ b/test/test-parse-expression-typescript.cpp @@ -66,6 +66,81 @@ TEST_F(Test_Parse_Expression_TypeScript, Expression* ast = p.parse_expression(); EXPECT_EQ(summarize(ast), "cond(var cond, var x, var Type)"); } + + { + Test_Parser p(u8"cond ? t : param => body"_sv, typescript_options); + Expression* ast = p.parse_expression(); + EXPECT_EQ(summarize(ast), "cond(var cond, var t, arrowfunc(var param))"); + } + + { + Test_Parser p(u8"cond1 ? cond2 ? t2 : param => body : f1"_sv, + typescript_options); + Expression* ast = p.parse_expression(); + EXPECT_EQ(summarize(ast), + "cond(var cond1, cond(var cond2, var t2, arrowfunc(var param)), " + "var f1)"); + } +} + +TEST_F(Test_Parse_Expression_TypeScript, + colon_in_conditional_can_be_arrow_return_type_annotation) { + { + Test_Parser p(u8"cond ? (param): ReturnType => body : f"_sv, + typescript_options); + Expression* ast = p.parse_expression(); + EXPECT_EQ(summarize(ast), "cond(var cond, arrowfunc(var param), var f)"); + } + + { + Test_Parser p(u8"cond ? async (param): ReturnType => body : f"_sv, + typescript_options); + Expression* ast = p.parse_expression(); + EXPECT_EQ(summarize(ast), + "cond(var cond, asyncarrowfunc(var param), var f)"); + } +} + +TEST_F( + Test_Parse_Expression_TypeScript, + colon_in_conditional_can_be_arrow_return_type_annotation_despite_following_syntax_error) { + { + // TypeScript resolves this ambiguity as a syntax error, so we should too. + // TypeScript's rule seems to be that '(t2)' is not a parameter list if + // 'body' is followed by a ':'. + test_parse_and_visit_expression( + u8"cond1 ? cond2 ? (t2) : param => body : f1"_sv, // + u8" ` Diag_Missing_Colon_In_Conditional_Expression.expected_colon\n"_diag + u8" ^ .question"_diag, + typescript_options); + } +} + +TEST_F( + Test_Parse_Expression_TypeScript, + colon_in_conditional_true_branch_cannot_be_arrow_return_type_annotation_if_arrow_body_not_followed_by_colon) { + { + Test_Parser p(u8"cond ? (t) : param => body"_sv, typescript_options); + Expression* ast = p.parse_expression(); + EXPECT_EQ(summarize(ast), + "cond(var cond, paren(var t), arrowfunc(var param))"); + } + + { + // This example triggers backtracking in the parser. Ensure that the + // backtracking walks back any speculative visits. + Spy_Visitor p = test_parse_and_visit_expression( + u8"cond ? (t) : param => body"_sv, no_diags, typescript_options); + EXPECT_THAT(p.visits, ElementsAreArray({ + "visit_enter_function_scope", // + "visit_variable_declaration", // param + "visit_enter_function_scope_body", // + "visit_variable_use", // body + "visit_exit_function_scope", // + "visit_variable_use", // cond, + "visit_variable_use", // t + })); + } } TEST_F(Test_Parse_Expression_TypeScript, non_null_assertion) {