Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggest async when await is followed by arrow function. #314

Closed
wants to merge 6 commits into from
Closed

Suggest async when await is followed by arrow function. #314

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ba39488
Suggest `async` when `await` is followed by arrow function.
keyehzy May 17, 2021
0dd962d
Fix github CI error
keyehzy May 17, 2021
5e508a2
fix double error code
keyehzy May 17, 2021
a2148b2
Attempts to fix #278.
keyehzy May 17, 2021
3481de1
Combining
keyehzy May 18, 2021
798b911
Added more tests and nit things
keyehzy May 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions src/parse.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -715,6 +715,30 @@ expression* parser::parse_await_expression(token await_token, precedence prec) {
}
}();

if (this->peek().type == token_type::left_paren) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code fails to catch arrow functions which don't have parentheses around the parameter list. For example:

await x => {}

See my comment on line 760 for a possible solution.

source_code_span operator_span = await_token.span();
buffering_error_reporter temp_error_reporter;
error_reporter* old_error_reporter =
std::exchange(this->error_reporter_, &temp_error_reporter);
lexer_transaction transaction = this->lexer_.begin_transaction();

// Try to parse leading ( as arrow function.
expression* ast = this->parse_expression(prec);

this->lexer_.roll_back_transaction(std::move(transaction));
this->error_reporter_ = old_error_reporter;

bool is_arrow_kind =
(ast->kind() == expression_kind::arrow_function_with_statements) ||
(ast->kind() == expression_kind::arrow_function_with_expression);

if (is_arrow_kind) {
this->error_reporter_->report(error_await_creating_arrow_function{
.await_operator = operator_span,
});
}
}

if (is_identifier) {
return this->make_expression<expression::variable>(
await_token.identifier_name(), await_token.type);
Expand All @@ -727,11 +751,13 @@ expression* parser::parse_await_expression(token await_token, precedence prec) {
}

expression* child = this->parse_expression(prec);

if (child->kind() == expression_kind::_invalid) {
this->error_reporter_->report(error_missing_operand_for_operator{
.where = operator_span,
});
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be a lot simpler to check if child->kind() is arrow_function_with_statements or arrow_function_with_expression here? That wouldn't fix #278, but based on your tests, #278 isn't really fixed by this commit anyway.

return this->make_expression<expression::await>(child, operator_span);
}
}
Expand Down
7 changes: 7 additions & 0 deletions src/quick-lint-js/error.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,13 @@
.error(QLJS_TRANSLATABLE("'await' is only allowed in async functions"), \
await_operator)) \
\
QLJS_ERROR_TYPE( \
error_await_creating_arrow_function, "E???", \
{ source_code_span await_operator; }, \
.error(QLJS_TRANSLATABLE("'await' does not create an async arrow " \
"function; use 'async' instead"), \
await_operator)) \
\
QLJS_ERROR_TYPE( \
error_big_int_literal_contains_decimal_point, "E005", \
{ source_code_span where; }, \
Expand Down
58 changes: 58 additions & 0 deletions test/test-parse-expression.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -869,6 +869,64 @@ TEST_F(test_parse_expression, await_unary_operator_inside_async_functions) {
EXPECT_EQ(summarize(ast), "await(var x)");
EXPECT_THAT(p.errors(), IsEmpty());
}

{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I think these new tests belong in a different TEST_F than test_parse_expression.await_unary_operator_inside_async_functions. For example, the test on line 899 is for a normal (non-async) function, which is definitely not what the test's name implies.

test_parser p(u8"await () => {}"_sv);
auto guard = p.parser().enter_function(function_attributes::async);
expression* ast = p.parse_expression();
EXPECT_EQ(ast->kind(), expression_kind::await);
EXPECT_EQ(ast->child_0()->kind(),
expression_kind::arrow_function_with_statements);
EXPECT_THAT(p.errors(),
ElementsAre(ERROR_TYPE_FIELD(
error_await_creating_arrow_function, await_operator,
offsets_matcher(p.code(), 0, u8"await"))));
}

{
test_parser p(u8"await () => "_sv);
auto guard = p.parser().enter_function(function_attributes::async);
expression* ast = p.parse_expression();
EXPECT_EQ(ast->kind(), expression_kind::await);
EXPECT_EQ(ast->child_0()->kind(),
expression_kind::arrow_function_with_expression);
EXPECT_THAT(p.errors(),
ElementsAre(ERROR_TYPE_FIELD(
error_await_creating_arrow_function, await_operator,
offsets_matcher(p.code(), 0, u8"await"))));
}

{
test_parser p(u8"await () => {}"_sv);
auto guard = p.parser().enter_function(function_attributes::normal);
expression* ast = p.parse_expression();
EXPECT_EQ(ast->kind(), expression_kind::binary_operator);
EXPECT_THAT(
p.errors(),
ElementsAre(
ERROR_TYPE_FIELD(error_await_creating_arrow_function,
await_operator,
offsets_matcher(p.code(), 0, u8"await")),
ERROR_TYPE_FIELD(
error_missing_operator_between_expression_and_arrow_function,
where, offsets_matcher(p.code(), 0, u8"await ("))));
Comment on lines +903 to +912
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: This parsing doesn't seem right at all. error_missing_operator_between_expression_and_arrow_function is wrong, assuming you want to fix #278. The whole point of #278 is to avoid E063.

Also, creating a binary_operator out of thin air is weird.

}

{
test_parser p(u8"await () => "_sv);
auto guard = p.parser().enter_function(function_attributes::normal);
expression* ast = p.parse_expression();
EXPECT_EQ(ast->kind(), expression_kind::binary_operator);
EXPECT_THAT(
p.errors(),
ElementsAre(
ERROR_TYPE_FIELD(error_await_creating_arrow_function,
await_operator,
offsets_matcher(p.code(), 0, u8"await")),
ERROR_TYPE_FIELD(
error_missing_operator_between_expression_and_arrow_function,
where, offsets_matcher(p.code(), 0, u8"await ("))));
}
}

TEST_F(test_parse_expression,
Expand Down