Skip to content

Commit

Permalink
fix(fe): don't diagnose x?.y.z
Browse files Browse the repository at this point in the history
quick-lint-js reports a diagnostic for x?.y.z. The diagnostic is
incorrect. Even the JavaScript example in the documentation (written by
me) does not do what the documentation claims it does. x?.y.z does not
throw an error if x is null or undefined.

Fix quick-lint-js's false positives by reverting commit
bdfccbd. Leave a tombstone so that the
diagnostic's code is not reused.
  • Loading branch information
strager committed Jan 25, 2024
1 parent 7796545 commit 434d085
Show file tree
Hide file tree
Showing 12 changed files with 7 additions and 149 deletions.
23 changes: 2 additions & 21 deletions docs/errors/E0718.md
Original file line number Diff line number Diff line change
@@ -1,24 +1,5 @@
# E0718: using a '.' after a '?.' might fail, since '?.' might return 'undefined'

```config-for-examples
{
"globals": {
}
}
```
This diagnostic has been removed in quick-lint-js version 3.2.0.

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
```
<!-- QLJS_NO_CHECK_CODE -->
4 changes: 0 additions & 4 deletions po/messages.pot
Original file line number Diff line number Diff line change
Expand Up @@ -2413,10 +2413,6 @@ msgstr ""
msgid "namespace alias cannot use 'import type'"
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: 0 additions & 15 deletions src/quick-lint-js/diag/diagnostic-metadata-generated.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6912,21 +6912,6 @@ const QLJS_CONSTINIT Diagnostic_Info all_diagnostic_infos[] = {
},
},
},

// Diag_Using_Dot_After_Optional_Chaining
{
.code = 718,
.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: 1 addition & 2 deletions src/quick-lint-js/diag/diagnostic-metadata-generated.h
Original file line number Diff line number Diff line change
Expand Up @@ -472,11 +472,10 @@ namespace quick_lint_js {
QLJS_DIAG_TYPE_NAME(Diag_Multiple_Export_Defaults) \
QLJS_DIAG_TYPE_NAME(Diag_Unintuitive_Bitshift_Precedence) \
QLJS_DIAG_TYPE_NAME(Diag_TypeScript_Namespace_Alias_Cannot_Use_Import_Type) \
QLJS_DIAG_TYPE_NAME(Diag_Using_Dot_After_Optional_Chaining) \
/* END */
// clang-format on

inline constexpr int Diag_Type_Count = 462;
inline constexpr int Diag_Type_Count = 461;

extern const Diagnostic_Info all_diagnostic_infos[Diag_Type_Count];
}
Expand Down
12 changes: 1 addition & 11 deletions src/quick-lint-js/diag/diagnostic-types-2.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ QLJS_RESERVED_DIAG("E0271")
QLJS_RESERVED_DIAG("E0279")
QLJS_RESERVED_DIAG("E0391")
QLJS_RESERVED_DIAG("E0707")
QLJS_RESERVED_DIAG("E0718")

struct Diag_Abstract_Field_Cannot_Have_Initializer {
[[qljs::diag("E0295", Diagnostic_Severity::error)]] //
Expand Down Expand Up @@ -3588,17 +3589,6 @@ struct Diag_TypeScript_Namespace_Alias_Cannot_Use_Import_Type {
ARG(type_keyword))]] //
Source_Code_Span type_keyword;
};

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

Expand Down
2 changes: 0 additions & 2 deletions src/quick-lint-js/fe/parse-expression.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -172,8 +172,6 @@ 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
51 changes: 0 additions & 51 deletions src/quick-lint-js/fe/parse.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -494,57 +494,6 @@ 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_span();
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_span();
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
1 change: 0 additions & 1 deletion src/quick-lint-js/fe/parse.h
Original file line number Diff line number Diff line change
Expand Up @@ -607,7 +607,6 @@ 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
2 changes: 0 additions & 2 deletions src/quick-lint-js/i18n/translation-table-generated.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -594,7 +594,6 @@ const Translation_Table translation_data = {
{0, 0, 0, 47, 0, 60}, //
{0, 0, 0, 55, 0, 59}, //
{0, 0, 0, 0, 0, 59}, //
{0, 0, 0, 0, 0, 74}, //
{57, 29, 48, 46, 41, 9}, //
{37, 26, 31, 33, 35, 31}, //
{0, 0, 0, 0, 0, 41}, //
Expand Down Expand Up @@ -2465,7 +2464,6 @@ const Translation_Table translation_data = {
u8"using '{0}' against an array literal does not compare items\0"
u8"using '{0}' against an arrow function always returns '{1}'\0"
u8"using '{0}' against an object literal always returns '{1}'\0"
u8"using a '.' after a '?.' might fail, since '?.' might return 'undefined'.\0"
u8"variable\0"
u8"variable already declared here\0"
u8"variable assigned before its declaration\0"
Expand Down
5 changes: 2 additions & 3 deletions src/quick-lint-js/i18n/translation-table-generated.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 = 606;
constexpr std::size_t translation_table_string_table_size = 82482;
constexpr std::uint16_t translation_table_mapping_table_size = 605;
constexpr std::size_t translation_table_string_table_size = 82408;
constexpr std::size_t translation_table_locale_table_size = 35;

QLJS_CONSTEVAL std::uint16_t translation_table_const_look_up(
Expand Down Expand Up @@ -608,7 +608,6 @@ QLJS_CONSTEVAL std::uint16_t translation_table_const_look_up(
"using '{0}' against an array literal does not compare items"sv,
"using '{0}' against an arrow function always returns '{1}'"sv,
"using '{0}' against an object literal always returns '{1}'"sv,
"using a '.' after a '?.' might fail, since '?.' might return 'undefined'."sv,
"variable"sv,
"variable already declared here"sv,
"variable assigned before its declaration"sv,
Expand Down
13 changes: 1 addition & 12 deletions src/quick-lint-js/i18n/translation-table-test-generated.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ struct Translated_String {
};

// clang-format off
inline const Translated_String test_translation_table[605] = {
inline const Translated_String test_translation_table[604] = {
{
"\"global-groups\" entries must be strings"_translatable,
u8"\"global-groups\" entries must be strings",
Expand Down Expand Up @@ -6430,17 +6430,6 @@ inline const Translated_String test_translation_table[605] = {
u8"using '{0}' against an object literal always returns '{1}'",
},
},
{
"using a '.' after a '?.' might fail, since '?.' might return 'undefined'."_translatable,
u8"using a '.' after a '?.' might fail, since '?.' might return 'undefined'.",
{
u8"using a '.' after a '?.' might fail, since '?.' might return 'undefined'.",
u8"using a '.' after a '?.' might fail, since '?.' might return 'undefined'.",
u8"using a '.' after a '?.' might fail, since '?.' might return 'undefined'.",
u8"using a '.' after a '?.' might fail, since '?.' might return 'undefined'.",
u8"using a '.' after a '?.' might fail, since '?.' might return 'undefined'.",
},
},
{
"variable"_translatable,
u8"variable",
Expand Down
25 changes: 0 additions & 25 deletions test/test-parse-warning.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -507,31 +507,6 @@ TEST_F(Test_Parse_Warning, early_exit_does_not_trigger_fallthrough_warning) {
no_diags);
}
}

TEST_F(Test_Parse_Warning, Diag_Using_Dot_After_Optional_Chaining) {
test_parse_and_visit_expression(
u8"a?.b.c"_sv,
u8" ^ Diag_Using_Dot_After_Optional_Chaining.dot_op\n"_diag
u8" ^^ .optional_chain_op"_diag);
test_parse_and_visit_expression(u8"a?.b?.c"_sv, no_diags);
test_parse_and_visit_expression(
u8"a?.b?.a?.c.d"_sv,
u8" ^ Diag_Using_Dot_After_Optional_Chaining.dot_op\n"_diag
u8" ^^ .optional_chain_op"_diag);
test_parse_and_visit_expression(u8"a?.b?.a?.c"_sv, no_diags);

test_parse_and_visit_expression(u8"_a?.(a)?.(_a)?.(a)"_sv, no_diags);
test_parse_and_visit_expression(
u8"_a?.(a).b"_sv,
u8" ^ Diag_Using_Dot_After_Optional_Chaining.dot_op\n"_diag
u8" ^^ .optional_chain_op"_diag);

test_parse_and_visit_expression(u8"g?.[1]"_sv, no_diags);
test_parse_and_visit_expression(
u8"g?.[1]?.[2].b"_sv,
u8" ^ Diag_Using_Dot_After_Optional_Chaining.dot_op\n"_diag
u8" ^^ .optional_chain_op"_diag);
}
}
}

Expand Down

0 comments on commit 434d085

Please sign in to comment.