diff --git a/docs/errors/E0722.md b/docs/errors/E0722.md new file mode 100644 index 000000000..06ae7a44d --- /dev/null +++ b/docs/errors/E0722.md @@ -0,0 +1,30 @@ +# E0722: mixing `??` with `>` or `==` may lead to unexpected behavior; use parentheses to clarify the intended precedence + +```config-for-examples +{ + "globals": { + "config": true + } +} +``` + +In JavaScript, mixing the nullish coalescing operator ?? with comparison operators like > or == without parentheses can lead to unexpected behavior. This code may not work as intended: + +```javascript +const config = { items: null }; +if (config?.items?.length ?? 0 > 0) { + console.log("Items exist"); +} +``` + +The above code is interpreted as config?.items?.length ?? (0 > 0), which is not the intended behavior. To ensure the correct precedence, use parentheses: + +```javascript +const config = { items: null }; +if ((config?.items?.length ?? 0) > 0) { + console.log("Items exist"); +} +``` + + + diff --git a/po/messages.pot b/po/messages.pot index 6c591842f..00533feed 100644 --- a/po/messages.pot +++ b/po/messages.pot @@ -2429,6 +2429,10 @@ msgstr "" msgid "function 'let' call may be confused for destructuring; remove parentheses to declare a variable" msgstr "" +#: src/quick-lint-js/diag/diagnostic-metadata-generated.cpp +msgid "mixing `??` with `>` or `==` may lead to unexpected behavior; use parentheses to clarify the intended precedence" +msgstr "" + #: test/test-diagnostic-formatter.cpp #: test/test-vim-qflist-json-diag-reporter.cpp msgid "something happened" diff --git a/src/quick-lint-js/diag/diagnostic-metadata-generated.cpp b/src/quick-lint-js/diag/diagnostic-metadata-generated.cpp index 24f01f806..63c241a6e 100644 --- a/src/quick-lint-js/diag/diagnostic-metadata-generated.cpp +++ b/src/quick-lint-js/diag/diagnostic-metadata-generated.cpp @@ -6975,6 +6975,19 @@ const QLJS_CONSTINIT Diagnostic_Info all_diagnostic_infos[] = { }, }, }, + // Diag_Mixing_Nullish_Coalescing_With_Comparison + { + .code = 722, + .severity = Diagnostic_Severity::warning, + .message_formats = { + QLJS_TRANSLATABLE("mixing `??` with `>` or `==` may lead to unexpected behavior; use parentheses to clarify the intended precedence"), + }, + .message_args = { + { + Diagnostic_Message_Arg_Info(offsetof(Diag_Mixing_Nullish_Coalescing_With_Comparison, nullish_coalescing_expression), Diagnostic_Arg_Type::source_code_span), + }, + }, + }, }; } diff --git a/src/quick-lint-js/diag/diagnostic-metadata-generated.h b/src/quick-lint-js/diag/diagnostic-metadata-generated.h index d9eadc717..383cc7712 100644 --- a/src/quick-lint-js/diag/diagnostic-metadata-generated.h +++ b/src/quick-lint-js/diag/diagnostic-metadata-generated.h @@ -476,10 +476,11 @@ namespace quick_lint_js { 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_Confusing_Let_Call) \ + QLJS_DIAG_TYPE_NAME(Diag_Mixing_Nullish_Coalescing_With_Comparison) \ /* END */ // clang-format on -inline constexpr int Diag_Type_Count = 465; +inline constexpr int Diag_Type_Count = 466; extern const Diagnostic_Info all_diagnostic_infos[Diag_Type_Count]; } diff --git a/src/quick-lint-js/diag/diagnostic-types-2.h b/src/quick-lint-js/diag/diagnostic-types-2.h index 60ff0ceef..fc4641ed5 100644 --- a/src/quick-lint-js/diag/diagnostic-types-2.h +++ b/src/quick-lint-js/diag/diagnostic-types-2.h @@ -3628,6 +3628,13 @@ struct Diag_Confusing_Let_Call { Source_Code_Span let_function_call; }; +struct Diag_Mixing_Nullish_Coalescing_With_Comparison { + [[qljs::diag("E0721", Diagnostic_Severity::warning)]] // + [[qljs::message("mixing `??` with `>` or `==` may lead to unexpected behavior; use parentheses to clarify the intended precedence", + ARG(nullish_coalescing_expression))]] // + Source_Code_Span nullish_coalescing_expression; +}; + } QLJS_WARNING_POP diff --git a/src/quick-lint-js/fe/parse-expression.cpp b/src/quick-lint-js/fe/parse-expression.cpp index 00f2b0700..0aa19d3cf 100644 --- a/src/quick-lint-js/fe/parse-expression.cpp +++ b/src/quick-lint-js/fe/parse-expression.cpp @@ -77,6 +77,7 @@ void Parser::visit_expression(Expression* ast, Parse_Visitor_Base& v, expression_cast(ast)); this->warn_on_unintuitive_bitshift_precedence( expression_cast(ast)); + this->check_for_nullish_coalescing_with_comparison(*ast); break; case Expression_Kind::Trailing_Comma: { auto& trailing_comma_ast = @@ -306,6 +307,14 @@ void Parser::visit_compound_or_conditional_assignment_expression( this->maybe_visit_assignment(lhs, v, Variable_Assignment_Flags::none); } +void Parser::check_for_nullish_coalescing_with_comparison(const Expression &expr) { + if (expr.contains_nullish_coalescing() && expr.contains_comparison_operator()) { + this->diags_.add(Diag_Mixing_Nullish_Coalescing_With_Comparison{ + .nullish_coalescing_expression = expr.span(), + }); + } +} + void Parser::maybe_visit_assignment(Expression* ast, Parse_Visitor_Base& v, Variable_Assignment_Flags flags) { switch (ast->kind()) { diff --git a/src/quick-lint-js/i18n/translation-table-generated.cpp b/src/quick-lint-js/i18n/translation-table-generated.cpp index 74c41a186..f1ecdaf9e 100644 --- a/src/quick-lint-js/i18n/translation-table-generated.cpp +++ b/src/quick-lint-js/i18n/translation-table-generated.cpp @@ -619,6 +619,7 @@ const Translation_Table translation_data = { {0, 0, 0, 17, 0, 29}, // {0, 0, 0, 0, 0, 53}, // {0, 15, 0, 72, 0, 47}, // + {0, 0, 0, 0, 0, 60}, // }}), // clang-format off @@ -644,6 +645,7 @@ const Translation_Table translation_data = { u8"if-Anweisung\0" u8"Ung\u00fcltiges 'in' innerhalb Initialisierung der C-\u00e4hnlichen for-Schleife\0" u8"while-Schleife\0" + u8"Nullish coalescing operator '??' cannot be used with comparison operators\0" u8"with-Anweisung\0" u8"'{0}' ist hier\0" u8"'{0}' ist f\u00fcr Strings nicht erlaubt. '{1}' anstattdessen verwenden.\0" diff --git a/src/quick-lint-js/i18n/translation-table-generated.h b/src/quick-lint-js/i18n/translation-table-generated.h index 8d74ec6f4..3238096dc 100644 --- a/src/quick-lint-js/i18n/translation-table-generated.h +++ b/src/quick-lint-js/i18n/translation-table-generated.h @@ -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 = 609; -constexpr std::size_t translation_table_string_table_size = 82687; +constexpr std::uint16_t translation_table_mapping_table_size = 610; +constexpr std::size_t translation_table_string_table_size = 82744; constexpr std::size_t translation_table_locale_table_size = 35; QLJS_CONSTEVAL std::uint16_t translation_table_const_look_up( @@ -42,6 +42,7 @@ QLJS_CONSTEVAL std::uint16_t translation_table_const_look_up( "'.' is not allowed after generic arguments; write [\"{1}\"] instead"sv, "'.' operator needs a key name; use + to concatenate strings; use [] to access with a dynamic key"sv, "'...' belongs before the tuple element name, not before the type"sv, + "Nullish coalescing operator '??' cannot be used with comparison operators"sv, "'...' belongs only before the tuple element name, not also before the type"sv, "'...' goes here"sv, "':' should be 'extends' instead"sv, diff --git a/test/test-parse-warning.cpp b/test/test-parse-warning.cpp index 22d76141b..53f8ae76d 100644 --- a/test/test-parse-warning.cpp +++ b/test/test-parse-warning.cpp @@ -52,6 +52,41 @@ TEST_F(Test_Parse_Warning, condition_with_assignment_from_literal) { } } +TEST_F(Test_Parse_Warning, warn_on_pointless_nullish_coalescing_operator) { + test_parse_and_visit_expression( + u8"true ?? false"_sv, // + u8" ^^ Diag_Pointless_Nullish_Coalescing_Operator"_diag); + test_parse_and_visit_expression( + u8"(a < b) ?? false"_sv, // + u8" ^^ Diag_Pointless_Nullish_Coalescing_Operator"_diag); + test_parse_and_visit_expression( + u8"!b ?? false"_sv, // + u8" ^^ Diag_Pointless_Nullish_Coalescing_Operator"_diag); + test_parse_and_visit_expression( + u8"'hi' ?? true"_sv, // + u8" ^^ Diag_Pointless_Nullish_Coalescing_Operator"_diag); + for (String8_View code : { + u8"s.toLowerCase() ?? false"_sv, + u8"s ?? false"_sv, + u8"null ?? false"_sv, + u8"(foo) ?? false"_sv, + u8"{}.missingProp ?? false"_sv, + u8"{}['missingProp'] ?? false"_sv, + u8"await foo ?? false"_sv, + u8"void 42 ?? false"_sv, + u8"bar`hello` ?? false"_sv, + u8"this ?? false"_sv, + u8"(2+2 && null) ?? false"_sv, + u8"(2+2 || null) ?? false"_sv, + u8"(2+2 , null) ?? false"_sv, + u8"(2+2 ?? null) ?? false"_sv, + }) { + SCOPED_TRACE(out_string8(code)); + Test_Parser p(code); + p.parse_and_visit_expression(); + } +} + TEST_F(Test_Parse_Warning, non_condition_with_assignment_from_literal) { for (String8_View code : { u8"with (x = 'hello') {}"_sv,