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.

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

feat: new diag for using ‘.’ after ‘?.’ #1133

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
24 changes: 24 additions & 0 deletions docs/errors/E0717.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# E0717: using a '.' after a '?.' might fail, since '?.' might return 'undefined'

```config-for-examples
{
"globals": {
}
}
```

In JavaScript, `x?.y` ignores `y` and returns `undefined` if `x` is `null` or `undefined`. If `?.` returns `undefined`, then using `.` after will throw an error:

```js
let bug = { milestone: null };
console.log(bug.milestone); // null
console.log(bug.milestone?.name); // undefined
console.log(bug.milestone?.name.trim()); // throws an error
```

Replacing the `.` with `?.` will prevent the errors from being thrown at runtime:

```js
let bug = { milestone: null };
console.log(bug.milestone?.name?.trim()); // undefined
```
4 changes: 4 additions & 0 deletions po/messages.pot
Original file line number Diff line number Diff line change
Expand Up @@ -2357,6 +2357,10 @@ msgstr ""
msgid "'&' here"
msgstr ""

#: src/quick-lint-js/diag/diagnostic-metadata-generated.cpp
msgid "using a '.' after a '?.' might fail, since '?.' might return 'undefined'."
msgstr ""

#: test/test-diagnostic-formatter.cpp
#: test/test-vim-qflist-json-diag-reporter.cpp
msgid "something happened"
Expand Down
15 changes: 15 additions & 0 deletions src/quick-lint-js/diag/diagnostic-metadata-generated.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6722,6 +6722,21 @@ const QLJS_CONSTINIT Diagnostic_Info all_diagnostic_infos[] = {
},
},
},

// Diag_Using_Dot_After_Optional_Chaining
{
.code = 717,
.severity = Diagnostic_Severity::warning,
.message_formats = {
QLJS_TRANSLATABLE("using a '.' after a '?.' might fail, since '?.' might return 'undefined'."),
},
.message_args = {
{
Diagnostic_Message_Arg_Info(offsetof(Diag_Using_Dot_After_Optional_Chaining, dot_op), Diagnostic_Arg_Type::source_code_span),
Diagnostic_Message_Arg_Info(offsetof(Diag_Using_Dot_After_Optional_Chaining, optional_chain_op), Diagnostic_Arg_Type::source_code_span),
},
},
},
};
}

Expand Down
3 changes: 2 additions & 1 deletion src/quick-lint-js/diag/diagnostic-metadata-generated.h
Original file line number Diff line number Diff line change
Expand Up @@ -459,10 +459,11 @@ namespace quick_lint_js {
QLJS_DIAG_TYPE_NAME(Diag_Class_Async_On_Getter_Or_Setter) \
QLJS_DIAG_TYPE_NAME(Diag_Multiple_Export_Defaults) \
QLJS_DIAG_TYPE_NAME(Diag_Unintuitive_Bitshift_Precedence) \
QLJS_DIAG_TYPE_NAME(Diag_Using_Dot_After_Optional_Chaining) \
/* END */
// clang-format on

inline constexpr int Diag_Type_Count = 448;
inline constexpr int Diag_Type_Count = 449;

extern const Diagnostic_Info all_diagnostic_infos[Diag_Type_Count];
}
Expand Down
9 changes: 9 additions & 0 deletions src/quick-lint-js/diag/diagnostic-types-2.h
Original file line number Diff line number Diff line change
Expand Up @@ -3485,6 +3485,15 @@ struct Diag_Unintuitive_Bitshift_Precedence {
Source_Code_Span bitshift_operator;
Source_Code_Span and_operator;
};

struct Diag_Using_Dot_After_Optional_Chaining {
[[qljs::diag("E0717", Diagnostic_Severity::warning)]]
[[qljs::message("using a '.' after a '?.' might fail, "
"since '?.' might return 'undefined'.",
ARG(dot_op), ARG(optional_chain_op))]]
Source_Code_Span dot_op;
Source_Code_Span optional_chain_op;
};
}
QLJS_WARNING_POP

Expand Down
19 changes: 13 additions & 6 deletions src/quick-lint-js/fe/expression.h
Original file line number Diff line number Diff line change
Expand Up @@ -612,11 +612,13 @@ class Expression::Call final : public Expression {
static constexpr Expression_Kind kind = Expression_Kind::Call;

explicit Call(Expression_Arena::Array_Ptr<Expression *> children,
Source_Code_Span left_paren_span, const Char8 *span_end)
Source_Code_Span left_paren_span, const Char8 *span_end,
std::optional<Source_Code_Span> optional_chaining_op_)
: Expression(kind),
call_left_paren_begin_(left_paren_span.begin()),
span_end_(span_end),
children_(children) {
children_(children),
optional_chaining_operator_(optional_chaining_op_) {
QLJS_ASSERT(left_paren_span.size() == 1);
}

Expand All @@ -628,6 +630,7 @@ class Expression::Call final : public Expression {
const Char8 *call_left_paren_begin_;
const Char8 *span_end_;
Expression_Arena::Array_Ptr<Expression *> children_;
std::optional<Source_Code_Span> optional_chaining_operator_;
};
static_assert(Expression_Arena::is_allocatable<Expression::Call>);

Expand All @@ -647,11 +650,12 @@ class Expression::Dot final : public Expression {
public:
static constexpr Expression_Kind kind = Expression_Kind::Dot;

explicit Dot(Expression *lhs, Identifier rhs)
: Expression(kind), variable_identifier_(rhs), child_(lhs) {}
explicit Dot(Expression *lhs, Identifier rhs, Source_Code_Span op_span)
: Expression(kind), variable_identifier_(rhs), child_(lhs), operator_span_(op_span) {}

Identifier variable_identifier_;
Expression *child_;
Source_Code_Span operator_span_;
};
static_assert(Expression_Arena::is_allocatable<Expression::Dot>);

Expand Down Expand Up @@ -682,15 +686,18 @@ class Expression::Index final : public Expression {
static constexpr Expression_Kind kind = Expression_Kind::Index;

explicit Index(Expression *container, Expression *subscript,
Source_Code_Span left_square_span, const Char8 *subscript_end)
Source_Code_Span left_square_span, const Char8 *subscript_end,
std::optional<Source_Code_Span> optional_chaining_op_)
: Expression(kind),
index_subscript_end_(subscript_end),
left_square_span(left_square_span),
children_{container, subscript} {}
children_{container, subscript},
optional_chaining_operator_(optional_chaining_op_) {}

const Char8 *index_subscript_end_;
Source_Code_Span left_square_span;
std::array<Expression *, 2> children_;
std::optional<Source_Code_Span> optional_chaining_operator_;
};
static_assert(Expression_Arena::is_allocatable<Expression::Index>);

Expand Down
35 changes: 22 additions & 13 deletions src/quick-lint-js/fe/parse-expression.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,8 @@ void Parser::visit_expression(Expression* ast, Parse_Visitor_Base& v,
break;
case Expression_Kind::Dot:
this->visit_expression(ast->child_0(), v, Variable_Context::rhs);
this->warn_on_dot_operator_after_optional_chain(
expression_cast<Expression::Dot*>(ast));
break;
case Expression_Kind::Index:
this->visit_expression(ast->child_0(), v, Variable_Context::rhs);
Expand Down Expand Up @@ -957,7 +959,8 @@ Expression* Parser::parse_async_expression_only(
Expression* call_ast = this->make_expression<Expression::Call>(
this->expressions_.make_array(std::move(call_children)),
/*left_paren_span=*/left_paren_span,
/*span_end=*/right_paren_span.end());
/*span_end=*/right_paren_span.end(),
/*optional_chaining_op=*/std::nullopt);
return call_ast;
} else if (is_await) {
// await is an operator.
Expand Down Expand Up @@ -1636,7 +1639,7 @@ Expression* Parser::parse_expression_remainder(Parse_Visitor_Base& v,
.func_start = func_start_span});
}
Expression* callee = binary_builder.last_expression();
Expression::Call* call = this->parse_call_expression_remainder(v, callee);
Expression::Call* call = this->parse_call_expression_remainder(v, callee, std::nullopt);
binary_builder.replace_last(call);

if (this->peek().type == Token_Type::equal_greater) {
Expand Down Expand Up @@ -1734,7 +1737,7 @@ Expression* Parser::parse_expression_remainder(Parse_Visitor_Base& v,
});
}
binary_builder.replace_last(this->make_expression<Expression::Dot>(
binary_builder.last_expression(), this->peek().identifier_name()));
binary_builder.last_expression(), this->peek().identifier_name(), dot_span));
this->skip();
goto next;

Expand Down Expand Up @@ -1765,7 +1768,7 @@ Expression* Parser::parse_expression_remainder(Parse_Visitor_Base& v,
case Token_Type::semicolon: {
Source_Code_Span empty_property_name(dot_span.end(), dot_span.end());
binary_builder.replace_last(this->make_expression<Expression::Dot>(
binary_builder.last_expression(), Identifier(empty_property_name)));
binary_builder.last_expression(), Identifier(empty_property_name), dot_span));
this->diag_reporter_->report(Diag_Missing_Property_Name_For_Dot_Operator{
.dot = dot_span,
});
Expand Down Expand Up @@ -1799,6 +1802,7 @@ Expression* Parser::parse_expression_remainder(Parse_Visitor_Base& v,
// array?.[index]
// f?.(x, y)
case Token_Type::question_dot: {
Source_Code_Span question_dot_span = this->peek().span();
this->skip();
switch (this->peek().type) {
// x?.y
Expand All @@ -1807,7 +1811,7 @@ Expression* Parser::parse_expression_remainder(Parse_Visitor_Base& v,
case Token_Type::reserved_keyword_with_escape_sequence:
QLJS_CASE_KEYWORD:
binary_builder.replace_last(this->make_expression<Expression::Dot>(
binary_builder.last_expression(), this->peek().identifier_name()));
binary_builder.last_expression(), this->peek().identifier_name(), question_dot_span));
this->skip();
goto next;

Expand All @@ -1822,7 +1826,8 @@ Expression* Parser::parse_expression_remainder(Parse_Visitor_Base& v,
// f?.(x, y)
case Token_Type::left_paren:
binary_builder.replace_last(this->parse_call_expression_remainder(
v, binary_builder.last_expression()));
v, binary_builder.last_expression(),
std::optional<Source_Code_Span>(question_dot_span)));
goto next;

// f?.<T>(x, y) // TypeScript only
Expand All @@ -1838,13 +1843,13 @@ Expression* Parser::parse_expression_remainder(Parse_Visitor_Base& v,
}
this->parse_and_visit_typescript_generic_arguments(v);
binary_builder.replace_last(this->parse_call_expression_remainder(
v, binary_builder.last_expression()));
v, binary_builder.last_expression(), std::nullopt));
goto next;

// array?.[index]
case Token_Type::left_square:
binary_builder.replace_last(this->parse_index_expression_remainder(
v, binary_builder.last_expression()));
v, binary_builder.last_expression(), std::optional<Source_Code_Span>(question_dot_span)));
goto next;

default:
Expand All @@ -1857,7 +1862,7 @@ Expression* Parser::parse_expression_remainder(Parse_Visitor_Base& v,
// o[key] // Indexing expression.
case Token_Type::left_square: {
binary_builder.replace_last(this->parse_index_expression_remainder(
v, binary_builder.last_expression()));
v, binary_builder.last_expression(), std::nullopt));
goto next;
}

Expand Down Expand Up @@ -2610,7 +2615,8 @@ Expression* Parser::parse_arrow_function_expression_remainder(
}

Expression::Call* Parser::parse_call_expression_remainder(Parse_Visitor_Base& v,
Expression* callee) {
Expression* callee,
std::optional<Source_Code_Span> optional_chaining_op) {
Source_Code_Span left_paren_span = this->peek().span();
Expression_Arena::Vector<Expression*> call_children(
"parse_expression_remainder call children",
Expand Down Expand Up @@ -2651,11 +2657,13 @@ Expression::Call* Parser::parse_call_expression_remainder(Parse_Visitor_Base& v,
this->make_expression<Expression::Call>(
this->expressions_.make_array(std::move(call_children)),
/*left_paren_span=*/left_paren_span,
/*span_end=*/call_span_end));
/*span_end=*/call_span_end,
/*optional_chaining_op=*/optional_chaining_op));
}

Expression* Parser::parse_index_expression_remainder(Parse_Visitor_Base& v,
Expression* lhs) {
Expression* lhs,
std::optional<Source_Code_Span> optional_chaining_op) {
QLJS_ASSERT(this->peek().type == Token_Type::left_square);
Source_Code_Span left_square_span = this->peek().span();
this->skip();
Expand Down Expand Up @@ -2683,7 +2691,8 @@ Expression* Parser::parse_index_expression_remainder(Parse_Visitor_Base& v,
break;
}
return this->make_expression<Expression::Index>(lhs, subscript,
left_square_span, end);
left_square_span, end,
optional_chaining_op);
}

Expression_Arena::Vector<Expression*>
Expand Down
5 changes: 3 additions & 2 deletions src/quick-lint-js/fe/parse-statement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2917,14 +2917,15 @@ void Parser::parse_and_visit_decorator(Parse_Visitor_Base &v) {
this->skip();

while (this->peek().type == Token_Type::dot) {
Source_Code_Span op_span_ = this->peek().span();
this->skip();
switch (this->peek().type) {
// @myNamespace.myDecorator
QLJS_CASE_KEYWORD:
case Token_Type::identifier:
case Token_Type::private_identifier:
ast = this->make_expression<Expression::Dot>(
ast, this->peek().identifier_name());
ast, this->peek().identifier_name(), op_span_);
this->skip();
break;

Expand All @@ -2937,7 +2938,7 @@ void Parser::parse_and_visit_decorator(Parse_Visitor_Base &v) {
if (this->peek().type == Token_Type::left_paren) {
// @decorator()
// @decorator(arg1, arg2)
ast = this->parse_call_expression_remainder(v, ast);
ast = this->parse_call_expression_remainder(v, ast, std::nullopt);
}

this->visit_expression(ast, v, Variable_Context::rhs);
Expand Down
55 changes: 55 additions & 0 deletions src/quick-lint-js/fe/parse.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,61 @@ void Parser::error_on_invalid_as_const(Expression* ast,
}
}

void Parser::warn_on_dot_operator_after_optional_chain(
pepplejoshua marked this conversation as resolved.
Show resolved Hide resolved
Expression::Dot *ast) {
Expression *lhs = ast->child_;
Source_Code_Span operator_span = ast->operator_span_;
auto is_optional_chain = [](String8_View s) -> bool {
return s[0] == u8'?';
};

// we know the current node is a dot operator.
// If it is a '.' and its parent is a '?.' or a '?.(' or a '?.['
// then we can report a warning
if (!is_optional_chain(operator_span.string_view())) {
switch (lhs->kind()) {
case Expression_Kind::Dot: {
auto lhs_dot = expression_cast<Expression::Dot*>(lhs);
Source_Code_Span lhs_operator_span = lhs_dot->operator_span_;
if (is_optional_chain(lhs_operator_span.string_view())) {
this->diag_reporter_->report(
Diag_Using_Dot_After_Optional_Chaining {
.dot_op = operator_span,
.optional_chain_op = lhs_operator_span,
});
}
break;
}
case Expression_Kind::Call: {
auto lhs_call = expression_cast<Expression::Call*>(lhs);
std::optional<Source_Code_Span> lhs_operator_span = lhs_call->optional_chaining_operator_;
if (lhs_operator_span.has_value()) {
this->diag_reporter_->report(
Diag_Using_Dot_After_Optional_Chaining {
.dot_op = operator_span,
.optional_chain_op = *lhs_operator_span,
});
}
break;
}
case Expression_Kind::Index: {
auto lhs_index = expression_cast<Expression::Index*>(lhs);
std::optional<Source_Code_Span> lhs_operator_span = lhs_index->optional_chaining_operator_;
if (lhs_operator_span.has_value()) {
this->diag_reporter_->report(
Diag_Using_Dot_After_Optional_Chaining {
.dot_op = operator_span,
.optional_chain_op = *lhs_operator_span,
});
}
break;
}
default:
break;
}
}
}

void Parser::warn_on_xor_operator_as_exponentiation(
Expression::Binary_Operator* ast) {
auto is_xor_operator = [](String8_View s) -> bool { return s == u8"^"_sv; };
Expand Down
7 changes: 5 additions & 2 deletions src/quick-lint-js/fe/parse.h
Original file line number Diff line number Diff line change
Expand Up @@ -555,6 +555,7 @@ class Parser {
void warn_on_comma_operator_in_conditional_statement(Expression *);
void warn_on_comma_operator_in_index(Expression *, Source_Code_Span);
void warn_on_xor_operator_as_exponentiation(Expression::Binary_Operator *);
void warn_on_dot_operator_after_optional_chain(Expression::Dot *);
void warn_on_unintuitive_bitshift_precedence(Expression *ast);
void error_on_pointless_string_compare(Expression::Binary_Operator *);
void error_on_pointless_compare_against_literal(
Expand Down Expand Up @@ -819,9 +820,11 @@ class Parser {
Expression *parameters_expression, Buffering_Visitor *return_type_visits,
Precedence);
Expression::Call *parse_call_expression_remainder(Parse_Visitor_Base &,
Expression *callee);
Expression *callee,
std::optional<Source_Code_Span>);
Expression *parse_index_expression_remainder(Parse_Visitor_Base &,
Expression *lhs);
Expression *lhs,
std::optional<Source_Code_Span>);
Expression_Arena::Vector<Expression *>
parse_arrow_function_parameters_or_call_arguments(Parse_Visitor_Base &v);
Expression *parse_arrow_function_body(
Expand Down
Loading
Loading