Skip to content

Commit

Permalink
feat: new diag for using ‘.’ after ‘?.’
Browse files Browse the repository at this point in the history
This closes #1128 -- it warns the user about using a dot After an optional chain. This cover optional chain with function call as well as optional chain with index expressions
  • Loading branch information
pepplejoshua authored and strager committed Jan 3, 2024
1 parent b05e1cc commit 6904e85
Show file tree
Hide file tree
Showing 14 changed files with 202 additions and 27 deletions.
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 @@ -2401,6 +2401,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 @@ -6870,6 +6870,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 @@ -469,10 +469,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 = 458;
inline constexpr int Diag_Type_Count = 459;

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 @@ -3567,6 +3567,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 @@ -172,6 +172,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 @@ -964,7 +966,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 @@ -1650,7 +1653,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 @@ -1748,7 +1751,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 @@ -1779,7 +1782,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 @@ -1813,6 +1816,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 @@ -1821,7 +1825,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 @@ -1836,7 +1840,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 @@ -1852,13 +1857,13 @@ Expression* Parser::parse_expression_remainder(Parse_Visitor_Base& v,
}
this->parse_and_visit_typescript_generic_arguments(v, /*in_jsx=*/false);
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 @@ -1871,7 +1876,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 @@ -2622,7 +2627,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 @@ -2663,11 +2669,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 @@ -2695,7 +2703,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 @@ -2941,14 +2941,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 @@ -2961,7 +2962,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 @@ -469,6 +469,61 @@ void Parser::error_on_invalid_as_const(Expression* ast,
}
}

void Parser::warn_on_dot_operator_after_optional_chain(
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 @@ -587,6 +587,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 @@ -851,9 +852,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

0 comments on commit 6904e85

Please sign in to comment.