diff --git a/docs/errors/E0459.md b/docs/errors/E0459.md new file mode 100644 index 000000000..4791a344c --- /dev/null +++ b/docs/errors/E0459.md @@ -0,0 +1,30 @@ +# E0459: equality check result is unused; did you mean to use assignment (=) instead? + +Using `==` as a statement rather than a comparison is almost never intended. + +```javascript +let x = 4; +let y = 5; + +// set x to y - oops, one = instead of two so no assignment is made! +x == y; +``` + +To resolve this, either correct to `=` instead: +```javascript +let x = 4; +let y = 5; + +x = y; // no error +``` + +Or use/discard the result: +```javascript +let x = 4; +let y = 5; +function f(_) { } + +let _ = x == y; +void (x == y); +f(x == y); +``` diff --git a/po/messages.pot b/po/messages.pot index 6c591842f..c186aa730 100644 --- a/po/messages.pot +++ b/po/messages.pot @@ -2389,6 +2389,10 @@ msgstr "" msgid "typeof result is of type string and so will never equal undefined; use 'undefined' instead" msgstr "" +#: src/quick-lint-js/diag/diagnostic-metadata-generated.cpp +msgid "equality check result is unused; did you mean to use assignment (=) instead?" +msgstr "" + #: src/quick-lint-js/diag/diagnostic-metadata-generated.cpp msgid "missing expression in placeholder within template literal" msgstr "" diff --git a/src/quick-lint-js/diag/diagnostic-metadata-generated.cpp b/src/quick-lint-js/diag/diagnostic-metadata-generated.cpp index 24f01f806..4e0fb4d12 100644 --- a/src/quick-lint-js/diag/diagnostic-metadata-generated.cpp +++ b/src/quick-lint-js/diag/diagnostic-metadata-generated.cpp @@ -6848,6 +6848,20 @@ const QLJS_CONSTINIT Diagnostic_Info all_diagnostic_infos[] = { }, }, + // Diag_Equality_Check_Used_As_Statement + { + .code = 459, + .severity = Diagnostic_Severity::warning, + .message_formats = { + QLJS_TRANSLATABLE("equality check result is unused; did you mean to use assignment (=) instead?"), + }, + .message_args = { + { + Diagnostic_Message_Arg_Info(offsetof(Diag_Equality_Check_Used_As_Statement, equals_operator), Diagnostic_Arg_Type::source_code_span), + }, + }, + }, + // Diag_Expected_Expression_In_Template_Literal { .code = 711, diff --git a/src/quick-lint-js/diag/diagnostic-metadata-generated.h b/src/quick-lint-js/diag/diagnostic-metadata-generated.h index d9eadc717..91e462e1d 100644 --- a/src/quick-lint-js/diag/diagnostic-metadata-generated.h +++ b/src/quick-lint-js/diag/diagnostic-metadata-generated.h @@ -468,6 +468,7 @@ namespace quick_lint_js { QLJS_DIAG_TYPE_NAME(Diag_Variable_Assigned_To_Self_Is_Noop) \ QLJS_DIAG_TYPE_NAME(Diag_Xor_Used_As_Exponentiation) \ QLJS_DIAG_TYPE_NAME(Diag_Typeof_Variable_Equals_Undefined) \ + QLJS_DIAG_TYPE_NAME(Diag_Equality_Check_Used_As_Statement) \ QLJS_DIAG_TYPE_NAME(Diag_Expected_Expression_In_Template_Literal) \ QLJS_DIAG_TYPE_NAME(Diag_Missing_Comma_Between_Array_Elements) \ QLJS_DIAG_TYPE_NAME(Diag_Class_Generator_On_Getter_Or_Setter) \ @@ -479,7 +480,7 @@ namespace quick_lint_js { /* 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..724308b0a 100644 --- a/src/quick-lint-js/diag/diagnostic-types-2.h +++ b/src/quick-lint-js/diag/diagnostic-types-2.h @@ -3557,6 +3557,16 @@ struct Diag_Typeof_Variable_Equals_Undefined { Source_Code_Span undefined; }; +struct Diag_Equality_Check_Used_As_Statement { + [[qljs::diag("E0459", Diagnostic_Severity::warning)]] // + // clang-format off + [[qljs::message("equality check result is unused; did you mean to use " + "assignment (=) instead?", + ARG(equals_operator))]] // + // clang-format on + Source_Code_Span equals_operator; +}; + struct Diag_Expected_Expression_In_Template_Literal { [[qljs::diag("E0711", Diagnostic_Severity::error)]] // [[qljs::message("missing expression in placeholder within template literal", diff --git a/src/quick-lint-js/fe/parse-statement.cpp b/src/quick-lint-js/fe/parse-statement.cpp index bc283f071..24fc9f6bf 100644 --- a/src/quick-lint-js/fe/parse-statement.cpp +++ b/src/quick-lint-js/fe/parse-statement.cpp @@ -509,6 +509,9 @@ bool Parser::parse_and_visit_statement(Parse_Visitor_Base &v, Expression *ast = this->make_expression(ident, ident_token_type); ast = this->parse_expression_remainder(v, ast, Precedence{}); + + this->warn_on_equality_check_used_as_statement(ast); + this->visit_expression(ast, v, Variable_Context::rhs); this->parse_end_of_expression_statement(); break; diff --git a/src/quick-lint-js/fe/parse.cpp b/src/quick-lint-js/fe/parse.cpp index 94a02ee49..5bad25168 100644 --- a/src/quick-lint-js/fe/parse.cpp +++ b/src/quick-lint-js/fe/parse.cpp @@ -406,6 +406,27 @@ void Parser::warn_on_unintuitive_bitshift_precedence(Expression* ast) { } } } +void Parser::warn_on_equality_check_used_as_statement(Expression* ast) { + ast = ast->without_paren(); + auto is_comparison_operator = [](String8_View s) { + return s == u8"=="_sv || s == u8"==="_sv || s == u8"!="_sv || + s == u8"!=="_sv; + }; + + if (ast->kind() != Expression_Kind::Binary_Operator) return; + auto* binary_op = static_cast(ast); + + // only interested in the first operator, others may just be ordinary + // comparisons + if (ast->child_count() <= 1) return; + Source_Code_Span eq_span = binary_op->operator_spans_[0]; + + if (is_comparison_operator(eq_span.string_view())) { + this->diag_reporter_->report( + quick_lint_js::Diag_Equality_Check_Used_As_Statement{.equals_operator = + eq_span}); + } +} void Parser::error_on_pointless_string_compare( Expression::Binary_Operator* ast) { auto is_comparison_operator = [](String8_View s) { diff --git a/src/quick-lint-js/fe/parse.h b/src/quick-lint-js/fe/parse.h index bdd5df1d7..ff68d4c2b 100644 --- a/src/quick-lint-js/fe/parse.h +++ b/src/quick-lint-js/fe/parse.h @@ -609,6 +609,7 @@ class Parser { void warn_on_xor_operator_as_exponentiation(Expression::Binary_Operator *); void warn_on_typeof_variable_equals_undefined(Expression::Binary_Operator *); void warn_on_unintuitive_bitshift_precedence(Expression *ast); + void warn_on_equality_check_used_as_statement(Expression *ast); void error_on_pointless_string_compare(Expression::Binary_Operator *); void error_on_pointless_compare_against_literal( Expression::Binary_Operator *); diff --git a/src/quick-lint-js/i18n/translation-table-generated.cpp b/src/quick-lint-js/i18n/translation-table-generated.cpp index 74c41a186..020fdb9d3 100644 --- a/src/quick-lint-js/i18n/translation-table-generated.cpp +++ b/src/quick-lint-js/i18n/translation-table-generated.cpp @@ -279,7 +279,8 @@ const Translation_Table translation_data = { {0, 0, 0, 0, 0, 43}, // {0, 0, 0, 33, 0, 5}, // {0, 0, 0, 47, 0, 35}, // - {31, 20, 35, 43, 34, 30}, // + {0, 0, 0, 0, 0, 30}, // + {31, 20, 35, 43, 34, 77}, // {64, 53, 0, 54, 0, 48}, // {74, 36, 0, 56, 0, 60}, // {63, 41, 0, 51, 0, 42}, // @@ -2154,6 +2155,7 @@ const Translation_Table translation_data = { u8"enum\0" u8"enum member name cannot be numeric\0" u8"enum member needs initializer\0" + u8"equality check result is unused; did you mean to use assignment (=) instead?\0" u8"escaped character is not allowed in identifiers\0" u8"escaping '-' is not allowed in tag names; write '-' instead\0" u8"event attributes must be camelCase: '{1}'\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..69c0e0830 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 = 82764; constexpr std::size_t translation_table_locale_table_size = 35; QLJS_CONSTEVAL std::uint16_t translation_table_const_look_up( @@ -294,6 +294,7 @@ QLJS_CONSTEVAL std::uint16_t translation_table_const_look_up( "enum"sv, "enum member name cannot be numeric"sv, "enum member needs initializer"sv, + "equality check result is unused; did you mean to use assignment (=) instead?"sv, "escaped character is not allowed in identifiers"sv, "escaping '-' is not allowed in tag names; write '-' instead"sv, "event attributes must be camelCase: '{1}'"sv, diff --git a/src/quick-lint-js/i18n/translation-table-test-generated.h b/src/quick-lint-js/i18n/translation-table-test-generated.h index 504c22755..33f0a7c33 100644 --- a/src/quick-lint-js/i18n/translation-table-test-generated.h +++ b/src/quick-lint-js/i18n/translation-table-test-generated.h @@ -27,7 +27,7 @@ struct Translated_String { }; // clang-format off -inline const Translated_String test_translation_table[608] = { +inline const Translated_String test_translation_table[609] = { { "\"global-groups\" entries must be strings"_translatable, u8"\"global-groups\" entries must be strings", @@ -2976,6 +2976,17 @@ inline const Translated_String test_translation_table[608] = { u8"enum member needs initializer", }, }, + { + "equality check result is unused; did you mean to use assignment (=) instead?"_translatable, + u8"equality check result is unused; did you mean to use assignment (=) instead?", + { + u8"equality check result is unused; did you mean to use assignment (=) instead?", + u8"equality check result is unused; did you mean to use assignment (=) instead?", + u8"equality check result is unused; did you mean to use assignment (=) instead?", + u8"equality check result is unused; did you mean to use assignment (=) instead?", + u8"equality check result is unused; did you mean to use assignment (=) instead?", + }, + }, { "escaped character is not allowed in identifiers"_translatable, u8"escaped character is not allowed in identifiers", diff --git a/test/test-parse-typescript.cpp b/test/test-parse-typescript.cpp index f15825e5f..85b0d509f 100644 --- a/test/test-parse-typescript.cpp +++ b/test/test-parse-typescript.cpp @@ -64,11 +64,12 @@ TEST_F(Test_Parse_TypeScript, warn_on_mistyped_strict_inequality_operator) { TEST_F(Test_Parse_TypeScript, mistyped_strict_inequality_operator_is_suppressable) { - test_parse_and_visit_statement(u8"(x!) == y"_sv, no_diags, - typescript_options); - test_parse_and_visit_statement(u8"x! /**/ == y"_sv, no_diags, - typescript_options); - test_parse_and_visit_statement(u8"x!\n== y"_sv, no_diags, typescript_options); + test_parse_and_visit_expression(u8"(x!) == y"_sv, no_diags, + typescript_options); + test_parse_and_visit_expression(u8"x! /**/ == y"_sv, no_diags, + typescript_options); + test_parse_and_visit_expression(u8"x!\n== y"_sv, no_diags, + typescript_options); } TEST_F(Test_Parse_TypeScript, unicode_next_line_is_whitespace) { diff --git a/test/test-parse-warning.cpp b/test/test-parse-warning.cpp index 22d76141b..9c0c6b13e 100644 --- a/test/test-parse-warning.cpp +++ b/test/test-parse-warning.cpp @@ -153,15 +153,15 @@ TEST_F(Test_Error_Equals_Does_Not_Distribute_Over_Or, non_constant) { } TEST_F(Test_Parse_Warning, warn_on_pointless_string_compare) { - test_parse_and_visit_statement(u8"s.toLowerCase() == 'banana'"_sv, no_diags); - test_parse_and_visit_statement( + test_parse_and_visit_expression(u8"s.toLowerCase() == 'banana'"_sv, no_diags); + test_parse_and_visit_expression( u8"s.toLowerCase() == 'BANANA'"_sv, // u8" ^^ Diag_Pointless_String_Comp_Contains_Upper"_diag); - test_parse_and_visit_statement(u8"s.toUpperCase() == 'BANANA'"_sv, no_diags); - test_parse_and_visit_statement( + test_parse_and_visit_expression(u8"s.toUpperCase() == 'BANANA'"_sv, no_diags); + test_parse_and_visit_expression( u8"s.toUpperCase() == 'banana'"_sv, // u8" ^^ Diag_Pointless_String_Comp_Contains_Lower"_diag); - test_parse_and_visit_statement( + test_parse_and_visit_expression( u8"s.toLowerCase() == \"BANANA\""_sv, // u8" ^^ Diag_Pointless_String_Comp_Contains_Upper"_diag); } @@ -171,7 +171,7 @@ TEST_F(Test_Parse_Warning, warn_on_pointless_string_compare_all_operators) { for (String8_View op : {u8"=="_sv, u8"==="_sv, u8"!="_sv, u8"!=="_sv}) { Test_Parser p(concat(u8"s.toLowerCase() "_sv, op, u8" 'Banana'"_sv), capture_diags); - p.parse_and_visit_statement(); + p.parse_and_visit_expression(); assert_diagnostics( p.code, p.errors, { @@ -186,14 +186,14 @@ TEST_F(Test_Parse_Warning, warn_on_pointless_string_compare_all_operators) { TEST_F(Test_Parse_Warning, warn_on_pointless_string_compare_function_signatures) { - test_parse_and_visit_statement(u8"s.tolowercase() == 'BANANA'"_sv, no_diags); - test_parse_and_visit_statement(u8"s.myToLowerCase() == 'BANANA'"_sv, - no_diags); - test_parse_and_visit_statement( + test_parse_and_visit_expression(u8"s.tolowercase() == 'BANANA'"_sv, no_diags); + test_parse_and_visit_expression(u8"s.myToLowerCase() == 'BANANA'"_sv, + no_diags); + test_parse_and_visit_expression( u8"stringBuilder.build().toLowerCase() == 'BANANA'"_sv, // u8" ^^ Diag_Pointless_String_Comp_Contains_Upper"_diag); - test_parse_and_visit_statement(u8"o.arr[0]() == 'BANANA'"_sv, no_diags); - test_parse_and_visit_statement( + test_parse_and_visit_expression(u8"o.arr[0]() == 'BANANA'"_sv, no_diags); + test_parse_and_visit_expression( u8"'BANANA' == s.toLowerCase()"_sv, // u8" ^^ Diag_Pointless_String_Comp_Contains_Upper"_diag); } @@ -255,23 +255,23 @@ TEST_F(Test_Parse_Warning, test_parse_and_visit_expression( u8"(((s.toLowerCase())) === ((('BANANA'))))"_sv, // u8" ^^^ Diag_Pointless_String_Comp_Contains_Upper"_diag); - test_parse_and_visit_statement( + test_parse_and_visit_expression( u8"s.toLowerCase() == 'BANANA' && s.toUpperCase() !== 'orange'"_sv, // u8" ^^^ Diag_Pointless_String_Comp_Contains_Lower"_diag, // u8" ^^ Diag_Pointless_String_Comp_Contains_Upper"_diag); - test_parse_and_visit_statement( + test_parse_and_visit_expression( u8"((s.toLowerCase() == 'BANANA') && s.toUpperCase() !== 'orange')"_sv, // u8" ^^^ Diag_Pointless_String_Comp_Contains_Lower"_diag, // u8" ^^ Diag_Pointless_String_Comp_Contains_Upper"_diag); } TEST_F(Test_Parse_Warning, warn_on_pointless_string_compare_literals) { - test_parse_and_visit_statement(u8"s.toLowerCase() == 'Ba\\u006Eana'"_sv, - no_diags); - test_parse_and_visit_statement( + test_parse_and_visit_expression(u8"s.toLowerCase() == 'Ba\\u006Eana'"_sv, + no_diags); + test_parse_and_visit_expression( u8"s.toLowerCase() == 0xeF || s.toUpperCase() == 0xeF"_sv, no_diags); - test_parse_and_visit_statement(u8"s.toUpperCase() == 'C:\\User\\tom'"_sv, - no_diags); + test_parse_and_visit_expression(u8"s.toUpperCase() == 'C:\\User\\tom'"_sv, + no_diags); } TEST_F(Test_Parse_Warning, @@ -442,6 +442,35 @@ TEST_F(Test_Parse_Warning, warn_on_typeof_variable_equals_undefined) { no_diags); } +TEST_F(Test_Parse_Warning, warn_on_equality_check_used_as_statement) { + test_parse_and_visit_statement( + u8"x == y;"_sv, // + u8" ^^ Diag_Equality_Check_Used_As_Statement"_diag); + test_parse_and_visit_statement( + u8"x != y;"_sv, // + u8" ^^ Diag_Equality_Check_Used_As_Statement"_diag); + test_parse_and_visit_statement( + u8"x === y;"_sv, // + u8" ^^^ Diag_Equality_Check_Used_As_Statement"_diag); + test_parse_and_visit_statement( + u8"x !== y;"_sv, // + u8" ^^^ Diag_Equality_Check_Used_As_Statement"_diag); + test_parse_and_visit_statement( + u8"x == y(42);"_sv, // + u8" ^^ Diag_Equality_Check_Used_As_Statement"_diag); + test_parse_and_visit_statement( + u8"x == y == z;"_sv, // + u8" ^^ Diag_Equality_Check_Used_As_Statement"_diag); + + test_parse_and_visit_statement(u8"let _ = x == y;"_sv, no_diags); + test_parse_and_visit_statement(u8"void (x == y);"_sv, no_diags); + test_parse_and_visit_statement(u8"f(x == y);"_sv, no_diags); + + test_parse_and_visit_statement(u8"x + y == z;"_sv, no_diags); + test_parse_and_visit_statement(u8"x = y;"_sv, no_diags); + test_parse_and_visit_statement(u8"x + y;"_sv, no_diags); +} + TEST_F(Test_Parse_Warning, warn_on_xor_operation_used_as_exponentiation) { test_parse_and_visit_expression( u8"2 ^ 8"_sv, //