add E0722 to warn on mixing ?? with > or == #1233

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");
+}
+```
+
+
+

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"

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),
+        },
+      },
+    },
 };
 }
 

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];
 }

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
 

src/quick-lint-js/fe/parse-expression.cpp

@@ -77,6 +77,7 @@ void Parser::visit_expression(Expression* ast, Parse_Visitor_Base& v,
         expression_cast<Expression::Binary_Operator*>(ast));
     this->warn_on_unintuitive_bitshift_precedence(
         expression_cast<Expression::Binary_Operator*>(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()) {

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"

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,

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,