diff --git a/docs/errors/E0373.md b/docs/errors/E0373.md new file mode 100644 index 0000000000..1bd31bd068 --- /dev/null +++ b/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": +``` \ No newline at end of file diff --git a/src/quick-lint-js/diag/diagnostic-types.h b/src/quick-lint-js/diag/diagnostic-types.h index ab77b3a3b8..dae9e3606c 100644 --- a/src/quick-lint-js/diag/diagnostic-types.h +++ b/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 diff --git a/src/quick-lint-js/fe/parse.cpp b/src/quick-lint-js/fe/parse.cpp index fb46d3dc07..1e85a44b06 100644 --- a/src/quick-lint-js/fe/parse.cpp +++ b/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(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(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(); template void diff --git a/src/quick-lint-js/fe/parse.h b/src/quick-lint-js/fe/parse.h index f18b0ba1df..44364df910 100644 --- a/src/quick-lint-js/fe/parse.h +++ b/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 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(); } diff --git a/test/test-parse-warning.cpp b/test/test-parse-warning.cpp index 2e753a3b9b..3cb6c2903a 100644 --- a/test/test-parse-warning.cpp +++ b/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 //