diff --git a/docs/errors/E0721.md b/docs/errors/E0721.md new file mode 100644 index 000000000..60811796e --- /dev/null +++ b/docs/errors/E0721.md @@ -0,0 +1,22 @@ +# E0721: typeof comparison with invalid string literal + +The `typeof` operator will always return a string that can take a value from `["undefined", "object", "boolean", "number", "bigint", "string", "symbol", "function"]`. Therefore, comparing to another sting has a high chance of being an error +([MDN](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/typeof)) + +```javascript +let x = foo; +if (typeof x === "strng") { + // this will not run! + alert("x is a string!"); +} +``` +Instead, compare against the string `"string"`; + +```javascript +let x = "foo" +if (typeof x === "string") { + // this will run now :) + alert("x is a string"); +} +``` + diff --git a/po/messages.pot b/po/messages.pot index 6c591842f..9544b3bdd 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 "typeof comparison with invalid string literal: {0}" +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..3e5038ff8 100644 --- a/src/quick-lint-js/diag/diagnostic-metadata-generated.cpp +++ b/src/quick-lint-js/diag/diagnostic-metadata-generated.cpp @@ -6975,6 +6975,20 @@ const QLJS_CONSTINIT Diagnostic_Info all_diagnostic_infos[] = { }, }, }, + + // Diag_Typeof_Invalid_String_Comparison + { + .code = 721, + .severity = Diagnostic_Severity::warning, + .message_formats = { + QLJS_TRANSLATABLE("typeof comparison with invalid string literal: {0}"), + }, + .message_args = { + { + Diagnostic_Message_Arg_Info(offsetof(Diag_Typeof_Invalid_String_Comparison, literal), 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..6d676a0a7 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_Typeof_Invalid_String_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..6507fe1ee 100644 --- a/src/quick-lint-js/diag/diagnostic-types-2.h +++ b/src/quick-lint-js/diag/diagnostic-types-2.h @@ -788,10 +788,10 @@ struct Diag_Duplicated_Cases_In_Switch_Statement { struct Diag_Fallthrough_Without_Comment_In_Switch { [[qljs::diag("E0427", Diagnostic_Severity::warning)]] // - [ - [qljs::message("missing 'break;' or '// fallthrough' comment between " - "statement and 'case'", - ARG(end_of_case))]] // + [[qljs::message( + "missing 'break;' or '// fallthrough' comment between " + "statement and 'case'", + ARG(end_of_case))]] // Source_Code_Span end_of_case; }; @@ -2281,10 +2281,10 @@ struct Diag_TypeScript_Delete_Cannot_Delete_Variables { struct Diag_TypeScript_Definite_Assignment_Assertion_In_Ambient_Context { [[qljs::diag("E0445", Diagnostic_Severity::error)]] // - [ - [qljs::message("'!' (definite assignment assertion) is not allowed on " - "'declare' variables", - ARG(definite_assignment_assertion))]] // + [[qljs::message( + "'!' (definite assignment assertion) is not allowed on " + "'declare' variables", + ARG(definite_assignment_assertion))]] // [[qljs::message("'declare' here", ARG(declare_keyword))]] // Source_Code_Span definite_assignment_assertion; @@ -2309,10 +2309,10 @@ struct Diag_TypeScript_Definite_Assignment_Assertion_On_Const { struct Diag_TypeScript_Definite_Assignment_Assertion_With_Initializer { [[qljs::diag("E0442", Diagnostic_Severity::error)]] // - [ - [qljs::message("'!' (definite assignment assertion) cannot be used with " - "an initial value", - ARG(definite_assignment_assertion))]] // + [[qljs::message( + "'!' (definite assignment assertion) cannot be used with " + "an initial value", + ARG(definite_assignment_assertion))]] // [[qljs::message("initial value was given here", ARG(equal))]] // Source_Code_Span definite_assignment_assertion; @@ -2321,10 +2321,10 @@ struct Diag_TypeScript_Definite_Assignment_Assertion_With_Initializer { struct Diag_TypeScript_Definite_Assignment_Assertion_Without_Type_Annotation { [[qljs::diag("E0443", Diagnostic_Severity::error)]] // - [ - [qljs::message("type annotation is required when using '!' (definite " - "assignment assertion)", - ARG(definite_assignment_assertion))]] // + [[qljs::message( + "type annotation is required when using '!' (definite " + "assignment assertion)", + ARG(definite_assignment_assertion))]] // Source_Code_Span definite_assignment_assertion; }; @@ -2586,10 +2586,10 @@ struct Diag_TypeScript_Declare_Field_Not_Allowed_In_JavaScript { struct Diag_TypeScript_Declare_Field_Cannot_Use_Private_Identifier { [[qljs::diag("E0416", Diagnostic_Severity::error)]] // - [ - [qljs::message("private identifiers are not allowed for 'declare' " - "fields; use 'private' instead", - ARG(private_identifier_hash))]] // + [[qljs::message( + "private identifiers are not allowed for 'declare' " + "fields; use 'private' instead", + ARG(private_identifier_hash))]] // [[qljs::message("'declare' here", ARG(declare_keyword))]] // Source_Code_Span private_identifier_hash; Source_Code_Span declare_keyword; @@ -3621,14 +3621,21 @@ struct Diag_TypeScript_Namespace_Alias_Cannot_Use_Import_Type { struct Diag_Confusing_Let_Call { [[qljs::diag("E0720", Diagnostic_Severity::warning)]] // - [ - [qljs::message("function 'let' call may be confused for destructuring; " - "remove parentheses to declare a variable", - ARG(let_function_call))]] // + [[qljs::message( + "function 'let' call may be confused for destructuring; " + "remove parentheses to declare a variable", + ARG(let_function_call))]] // Source_Code_Span let_function_call; }; +struct Diag_Typeof_Invalid_String_Comparison { + [[qljs::diag("E0721", Diagnostic_Severity::warning)]] // + [[qljs::message("typeof comparison with invalid string literal: {0}", + ARG(literal))]] // + Source_Code_Span literal; +}; } + QLJS_WARNING_POP // quick-lint-js finds bugs in JavaScript programs. diff --git a/src/quick-lint-js/fe/parse-expression.cpp b/src/quick-lint-js/fe/parse-expression.cpp index 00f2b0700..76322e02a 100644 --- a/src/quick-lint-js/fe/parse-expression.cpp +++ b/src/quick-lint-js/fe/parse-expression.cpp @@ -75,6 +75,8 @@ void Parser::visit_expression(Expression* ast, Parse_Visitor_Base& v, expression_cast(ast)); this->warn_on_typeof_variable_equals_undefined( expression_cast(ast)); + this->warn_on_typeof_variable_equals_invalid_literal( + expression_cast(ast)); this->warn_on_unintuitive_bitshift_precedence( expression_cast(ast)); break; @@ -518,11 +520,10 @@ Expression* Parser::parse_primary_expression(Parse_Visitor_Base& v, Expression* ast = type == Token_Type::kw_delete ? this->make_expression(child, operator_span) - : type == Token_Type::kw_typeof - ? this->make_expression(child, - operator_span) - : this->make_expression( - child, operator_span); + : type == Token_Type::kw_typeof + ? this->make_expression(child, operator_span) + : this->make_expression(child, + operator_span); return ast; } @@ -1683,7 +1684,7 @@ Expression* Parser::parse_expression_remainder(Parse_Visitor_Base& v, // x += y // x[y] &&= z QLJS_CASE_COMPOUND_ASSIGNMENT_OPERATOR: - QLJS_CASE_CONDITIONAL_ASSIGNMENT_OPERATOR : { + QLJS_CASE_CONDITIONAL_ASSIGNMENT_OPERATOR: { if (!prec.math_or_logical_or_assignment) { break; } @@ -3016,7 +3017,7 @@ Expression* Parser::parse_object_literal(Parse_Visitor_Base& v) { break; } - QLJS_CASE_RESERVED_KEYWORD_EXCEPT_AWAIT_AND_YIELD : { + QLJS_CASE_RESERVED_KEYWORD_EXCEPT_AWAIT_AND_YIELD: { Expression* value = this->make_expression(key_token.span()); this->diags_.add(Diag_Missing_Value_For_Object_Literal_Entry{ @@ -3990,10 +3991,11 @@ Expression* Parser::parse_jsx_element_or_fragment(Parse_Visitor_Base& v, .opening_tag_name = tag_namespace ? Source_Code_Span(tag_namespace->span().begin(), tag_end) - : !tag_members.empty() - ? Source_Code_Span(tag_members.front().span().begin(), - tag_end) - : tag ? tag->span() : Source_Code_Span::unit(tag_end), + : !tag_members.empty() + ? Source_Code_Span(tag_members.front().span().begin(), + tag_end) + : tag ? tag->span() + : Source_Code_Span::unit(tag_end), .closing_tag_name = closing_tag_begin <= closing_tag_end ? Source_Code_Span(closing_tag_begin, closing_tag_end) diff --git a/src/quick-lint-js/fe/parse.cpp b/src/quick-lint-js/fe/parse.cpp index 965701c0c..dd4218dbc 100644 --- a/src/quick-lint-js/fe/parse.cpp +++ b/src/quick-lint-js/fe/parse.cpp @@ -538,6 +538,60 @@ void Parser::warn_on_typeof_variable_equals_undefined( } } +void Parser::warn_on_typeof_variable_equals_invalid_literal( + Expression::Binary_Operator* ast) { + static const auto existingLiterals = + std::to_array>( + {u8"undefined", u8"object", u8"boolean", u8"number", u8"bigint", + u8"string", u8"symbol", u8"function"}); + + auto is_comparison_operator = [](String8_View s) -> bool { + return s == u8"=="_sv || s == u8"==="_sv || s == u8"!="_sv || + s == u8"!=="_sv; + }; + + auto normalize = [](String8_View str) -> String8_View { + auto start = str.find_first_not_of(u8"'\"\\"); + if (start == String8_View::npos) { + return {}; + } + auto end = str.find_last_not_of(u8"'\"\\"); + if (end == String8_View::npos || end < start) { + return {}; + } + return str.substr(start, end - start + 1); + }; + + for (Span_Size i = 0; i < ast->child_count() - 1; i++) { + Source_Code_Span eq_span = ast->operator_spans_[i]; + if (is_comparison_operator(eq_span.string_view())) { + Expression* typeof_op = ast->child(i)->without_paren(); + Expression* literal = ast->child(i + 1)->without_paren(); + if (typeof_op->kind() == Expression_Kind::Typeof && + literal->kind() == Expression_Kind::Literal) { + // fallthrough + } else if (literal->kind() == Expression_Kind::Typeof && + typeof_op->kind() == Expression_Kind::Literal) { + std::swap(literal, typeof_op); + // falltrough + } else { + continue; // does not match + } + + Expression::Literal* type_literal = + expression_cast(literal); + String8_View literal_value = + normalize(type_literal->span().string_view()); + + if (std::find(existingLiterals.begin(), existingLiterals.end(), + literal_value) == existingLiterals.end()) { + this->diags_.add(Diag_Typeof_Invalid_String_Comparison{ + .literal = type_literal->span()}); + } + } + } +} + void Parser::error_on_pointless_compare_against_literal( Expression::Binary_Operator* ast) { auto is_comparison_operator = [](String8_View s) -> bool { diff --git a/src/quick-lint-js/fe/parse.h b/src/quick-lint-js/fe/parse.h index 27d280269..1ec046328 100644 --- a/src/quick-lint-js/fe/parse.h +++ b/src/quick-lint-js/fe/parse.h @@ -105,7 +105,7 @@ struct Parser_Transaction { // Parse_Visitor (visit_variable_declaration, visit_enter_function_scope, etc.). class Parser { private: - template + template class Bool_Guard; public: @@ -604,6 +604,8 @@ class Parser { void warn_on_comma_operator_in_index(Expression *, Source_Code_Span); void warn_on_xor_operator_as_exponentiation(Expression::Binary_Operator *); void warn_on_typeof_variable_equals_undefined(Expression::Binary_Operator *); + void warn_on_typeof_variable_equals_invalid_literal( + Expression::Binary_Operator *); void warn_on_unintuitive_bitshift_precedence(Expression *ast); void error_on_pointless_string_compare(Expression::Binary_Operator *); void error_on_pointless_compare_against_literal( @@ -1011,7 +1013,7 @@ class Parser { private: template - Expression *make_expression(Args &&... args) { + Expression *make_expression(Args &&...args) { return this->expressions_.make_expression( std::forward(args)...); } @@ -1040,7 +1042,7 @@ class Parser { }; private: - template + template class Bool_Guard { public: explicit Bool_Guard(Parser *p, bool old_value) diff --git a/src/quick-lint-js/i18n/translation-table-generated.cpp b/src/quick-lint-js/i18n/translation-table-generated.cpp index 74c41a186..d3a6ac384 100644 --- a/src/quick-lint-js/i18n/translation-table-generated.cpp +++ b/src/quick-lint-js/i18n/translation-table-generated.cpp @@ -544,6 +544,7 @@ const Translation_Table translation_data = { {0, 0, 0, 0, 0, 75}, // {0, 0, 0, 0, 0, 58}, // {0, 0, 0, 0, 0, 31}, // + {0, 0, 0, 0, 0, 51}, // {27, 19, 30, 29, 22, 91}, // {25, 50, 0, 36, 0, 23}, // {66, 43, 31, 36, 30, 44}, // @@ -2418,6 +2419,7 @@ const Translation_Table translation_data = { u8"type annotation is required when using '!' (definite assignment assertion)\0" u8"type predicates are only allowed as function return types\0" u8"type {0} is being defined here\0" + u8"typeof comparison with invalid string literal: {0}\0" u8"typeof result is of type string and so will never equal undefined; use 'undefined' instead\0" u8"unclosed block comment\0" u8"unclosed class; expected '}' by end of file\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..7a2d379dd 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 = 82738; constexpr std::size_t translation_table_locale_table_size = 35; QLJS_CONSTEVAL std::uint16_t translation_table_const_look_up( @@ -558,6 +558,7 @@ QLJS_CONSTEVAL std::uint16_t translation_table_const_look_up( "type annotation is required when using '!' (definite assignment assertion)"sv, "type predicates are only allowed as function return types"sv, "type {0} is being defined here"sv, + "typeof comparison with invalid string literal: {0}"sv, "typeof result is of type string and so will never equal undefined; use 'undefined' instead"sv, "unclosed block comment"sv, "unclosed class; expected '}' by end of file"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..8a6eaab97 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", @@ -5880,6 +5880,17 @@ inline const Translated_String test_translation_table[608] = { u8"type {0} is being defined here", }, }, + { + "typeof comparison with invalid string literal: {0}"_translatable, + u8"typeof comparison with invalid string literal: {0}", + { + u8"typeof comparison with invalid string literal: {0}", + u8"typeof comparison with invalid string literal: {0}", + u8"typeof comparison with invalid string literal: {0}", + u8"typeof comparison with invalid string literal: {0}", + u8"typeof comparison with invalid string literal: {0}", + }, + }, { "typeof result is of type string and so will never equal undefined; use 'undefined' instead"_translatable, u8"typeof result is of type string and so will never equal undefined; use 'undefined' instead", diff --git a/test/test-variable-analyzer-typeof.cpp b/test/test-variable-analyzer-typeof.cpp index b205036e7..f77e4bae1 100644 --- a/test/test-variable-analyzer-typeof.cpp +++ b/test/test-variable-analyzer-typeof.cpp @@ -66,6 +66,13 @@ TEST( u8"^ Diag_Use_Of_Undeclared_Variable.name"_diag, typescript_analyze_options, default_globals); } + +TEST(Test_Variable_Analyzer_Typeof, typeof_comparison_invalid_string_literals) { + test_parse_and_analyze( + u8"let v = 5; typeof v == 'nmber'"_sv, + u8" ^^^^^^^ Diag_Typeof_Invalid_String_Comparison"_diag, + typescript_analyze_options, default_globals); +} } }