New diagnostic type, documentation, and test for if("x" || "y"), case "x" || "y":

docs/errors/E0373.md

@@ -0,0 +1,29 @@
+# E0373: Pointless Comparison Against String Literal
+
+A condition with a string literal will always evaluate to true
+
+```typescript
+if("x" || "y"){
+    let x = y;
+}
+```
+
+To fix this warning, remove the condition.
+
+```typescript
+let x = y;
+```
+
+Or, 
+
+```typescript
+case "x" || "y":
+```
+
+To fix this warning, make separate cases
+
+```typescript
+case "x":
+
+case "y":
+```

src/quick-lint-js/diag/diagnostic-types.h

@@ -2960,6 +2960,16 @@
                                 "expression literal always returns '{1}'"),     \
               equals_operator, comparison_result))                              \
                                                                                 \
+  QLJS_DIAG_TYPE(                                                               \
+      diag_pointless_comp_against_string_expression_literal, "E0373",           \
+      diagnostic_severity::warning,                                             \
+      {                                                                         \
+        source_code_span or_operator;                                           \
+      },                                                                        \
+      MESSAGE(QLJS_TRANSLATABLE("comparison with a non-empty string literal "   \
+                                " always returns '{1}'"),                       \
+              or_operator))                                                     \
+                                                                                \
   QLJS_DIAG_TYPE(                                                               \
       diag_unexpected_function_parameter_is_parenthesized, "E0349",             \
       diagnostic_severity::error,                                               \
@@ -2976,13 +2986,6 @@
       diag_unexpected_colon_after_generic_definition, "E0331",                  \
       diagnostic_severity::error, { source_code_span colon; },                  \
       MESSAGE(QLJS_TRANSLATABLE("':' should be 'extends' instead"), colon))     \
-                                                                                \
-  QLJS_DIAG_TYPE(                                                               \
-      diag_pointless_nullish_coalescing_operator, "E0369",                      \
-      diagnostic_severity::warning, { source_code_span question_question; },    \
-      MESSAGE(QLJS_TRANSLATABLE("nullish coalescing operator does nothing "     \
-                                "when left operand is never null"),             \
-              question_question))                                               \
   /* END */
 
 // QLJS_X_RESERVED_DIAG_TYPES lists reserved error codes. These codes were used

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

@@ -334,6 +334,39 @@ void parser::error_on_sketchy_condition(expression* ast) {
   }
 }
 
+void parser::warn_on_pointless_string_condition(
+    expression::conditional* ast) {
+  auto is_or_operator = [](string8_view s) {
+    return s == u8"||"_sv;
+  };
+
+  for (span_size i = 0; i < ast->child_count() + 1; i++) {
+    expression* lhs = ast->child(i)->without_paren();
+    expression* rhs = ast->child(i + 1)->without_paren();
+
+    if ((lhs->kind() == expression_kind::literal &&
+         rhs->kind() == expression_kind::literal) ){
+      source_code_span op_span = ast->operator_spans_[i];
+      if (!is_or_operator(op_span.string_view())) {
+        continue;
+      }
+      auto char_is_a_quote = [](const char8* s) {
+        return *s == '"' || *s == '\'' || *s == '`';
+      };
+
+      if (!char_is_a_quote(rhs->span().begin())) {
+        continue;
+      }
+
+      // string8_view literal1 = lhs->child_0()->variable_identifier().span().string_view();
+      // string8_view literal2 = rhs->span().string_view();
+
+      this->diag_reporter_->report(
+        diag_pointless_comp_against_string_expression_literal{op_span});
+    }
+  }
+}
+
 void parser::warn_on_comma_operator_in_conditional_statement(expression* ast) {
   if (ast->kind() != expression_kind::binary_operator) return;
 
@@ -435,7 +468,6 @@ void parser::error_on_invalid_as_const(expression* ast,
   case expression_kind::dot:
   case expression_kind::array:
   case expression_kind::object:
-  case expression_kind::_template:
     break;
 
   case expression_kind::literal: {
@@ -735,82 +767,6 @@ void parser::consume_semicolon() {
   }
 }
 
-void parser::error_on_pointless_nullish_coalescing_operator(
-    expression::binary_operator* ast) {
-  auto is_nullish_operator = [](string8_view s) -> bool {
-    return s == u8"??"_sv;
-  };
-
-  for (span_size i = 0; i < ast->child_count() - 1; i++) {
-    source_code_span op_span = ast->operator_spans_[i];
-    if (is_nullish_operator(op_span.string_view())) {
-      if (i >= 1) {
-        // lhs is a multi-child expression
-        return;
-      } else {
-        this->check_lhs_for_null_potential(ast->child(i)->without_paren(),
-                                           op_span);
-      }
-    }
-  }
-}
-
-void parser::check_lhs_for_null_potential(expression* lhs,
-                                          source_code_span op_span) {
-  auto binary_operator_is_never_null =
-      [](expression::binary_operator* expr) -> bool {
-    // these 4 binary operators can resolve to a null value
-    string8_view can_resolve_to_null[4] = {u8"&&"_sv, u8"??"_sv, u8","_sv,
-                                           u8"||"_sv};
-    for (span_size i = 0; i < expr->child_count() - 1; i++) {
-      string8_view expr_op_span = expr->operator_spans_[i].string_view();
-      for (span_size j = 0; j < 4; j++) {
-        if (expr_op_span == can_resolve_to_null[j]) {
-          return false;
-        }
-      }
-    }
-    return true;
-  };
-
-  bool report_diag = false;
-  switch (lhs->kind()) {
-  case expression_kind::literal:
-    if (lhs->span().string_view() == u8"null"_sv) {
-      break;
-    }
-    if (lhs->span().string_view() == u8"undefined"_sv) {
-      break;
-    }
-    report_diag = true;
-    break;
-  case expression_kind::rw_unary_suffix:
-    report_diag = true;
-    break;
-  case expression_kind::unary_operator: {
-    auto* maybe_void_lhs = static_cast<expression::unary_operator*>(lhs);
-    if (maybe_void_lhs->is_void_operator() == false) {
-      report_diag = true;
-    }
-    break;
-  }
-  case expression_kind::_typeof:
-    report_diag = true;
-    break;
-  case expression_kind::binary_operator: {
-    auto* operator_lhs = static_cast<expression::binary_operator*>(lhs);
-    report_diag = binary_operator_is_never_null(operator_lhs);
-    break;
-  }
-  default:
-    break;
-  }
-  if (report_diag == true) {
-    this->diag_reporter_->report(diag_pointless_nullish_coalescing_operator{
-        .question_question = op_span});
-  }
-}
-
 template void
 parser::consume_semicolon<diag_missing_semicolon_after_abstract_method>();
 template void

src/quick-lint-js/fe/parse.h

@@ -411,6 +411,7 @@ class parser {
                                                 source_code_span token);
 
   void error_on_sketchy_condition(expression *);
+  void warn_on_pointless_string_condition(expression::conditional* ast);
   void warn_on_comma_operator_in_conditional_statement(expression *);
   void warn_on_comma_operator_in_index(expression *, source_code_span);
   void error_on_pointless_string_compare(expression::binary_operator *);
@@ -688,11 +689,6 @@ class parser {
   template <class MissingSemicolonDiagnostic>
   void consume_semicolon();
 
-  void error_on_pointless_nullish_coalescing_operator(
-      expression::binary_operator *);
-
-  void check_lhs_for_null_potential(expression *, source_code_span op_span);
-
   const token &peek() const noexcept { return this->lexer_.peek(); }
   void skip() noexcept { this->lexer_.skip(); }
 

test/test-parse-warning.cpp

@@ -673,73 +673,22 @@ TEST_F(test_parse_warning,
   }
 }
 
-TEST_F(test_parse_warning, warn_on_pointless_nullish_coalescing_operator) {
+TEST_F(test_parse_warning, warn_on_pointless_string_literal_comparison) {
   {
-    test_parser p(u8"true ?? false"_sv, capture_diags);
-    p.parse_and_visit_expression();
-
-    EXPECT_THAT(p.errors,
-                ElementsAreArray({
-                    DIAG_TYPE_OFFSETS(
-                        p.code, diag_pointless_nullish_coalescing_operator,
-                        question_question, strlen(u8"true "), u8"??"_sv),
-                }));
-  }
-  {
-    test_parser p(u8"(a < b) ?? false"_sv, capture_diags);
-    p.parse_and_visit_expression();
-
-    EXPECT_THAT(p.errors,
-                ElementsAreArray({
-                    DIAG_TYPE_OFFSETS(
-                        p.code, diag_pointless_nullish_coalescing_operator,
-                        question_question, strlen(u8"(a < b) "), u8"??"_sv),
-                }));
-  }
-  {
-    test_parser p(u8"!b ?? false"_sv, capture_diags);
-    p.parse_and_visit_expression();
-    EXPECT_THAT(p.errors,
-                ElementsAreArray({
-                    DIAG_TYPE_OFFSETS(
-                        p.code, diag_pointless_nullish_coalescing_operator,
-                        question_question, strlen(u8"!b "), u8"??"_sv),
-                }));
-  }
-  {
-    test_parser p(u8"'hi' ?? true"_sv, capture_diags);
-    p.parse_and_visit_expression();
-    EXPECT_THAT(p.errors,
-                ElementsAreArray({
-                    DIAG_TYPE_OFFSETS(
-                        p.code, diag_pointless_nullish_coalescing_operator,
-                        question_question, strlen(u8"'hi' "), u8"??"_sv),
-                }));
-  }
-  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);
+    test_parser p(u8"if('x' || 'y')"_sv, capture_diags);
     p.parse_and_visit_expression();
+    EXPECT_THAT(
+        p.errors,
+        ElementsAreArray({
+            DIAG_TYPE_OFFSETS(p.code, diag_pointless_comp_against_string_expression_literal,
+                              or_operator, strlen(u8"'x' "), u8"||"_sv),
+        }));
   }
 }
 }
 }
 
+
 // quick-lint-js finds bugs in JavaScript programs.
 // Copyright (C) 2020  Matthew "strager" Glazar
 //