feat: warn missing fallthrough

Add and report a diagnostic for missing fallthrough within switch statements.
Fixes #1081

Author: yashmasani

docs/errors/E0427.md

@@ -0,0 +1,29 @@
+# E0427: missing 'break;' or '// fallthrough' comment between statement and 'case'
+
+Switch Cases in javascript fallthrough to the next case if the `break` statement is not added at the end of the case.
+Since there is no explicit way of communication whether the fallthrough is intentional or not, it is recommended to use a comment indicating fallthrough.
+
+```javascript
+function test (c) {
+  switch (c) {
+    case 1:
+      console.log('case 1');
+    default:
+      console.log('default');
+  }
+}
+```
+
+To fix this error, place a comment at the end of `case 1` indicating fallthrough
+
+```javascript
+function test (c) {
+  switch (c) {
+    case 1:
+      console.log('case 1');
+      //fallthrough
+    default:
+      console.log('default');
+  }
+}
+```

po/messages.pot

@@ -557,6 +557,10 @@ msgstr ""
 msgid "this case will run instead"
 msgstr ""
 
+#: src/quick-lint-js/diag/diagnostic-metadata-generated.cpp
+msgid "missing 'break;' or '// fallthrough' comment between statement and 'case'"
+msgstr ""
+
 #: src/quick-lint-js/diag/diagnostic-metadata-generated.cpp
 msgid "'else' has no corresponding 'if'"
 msgstr ""

src/quick-lint-js/diag/diagnostic-metadata-generated.cpp

@@ -1423,6 +1423,20 @@ const QLJS_CONSTINIT Diagnostic_Info all_diagnostic_infos[] = {
       },
     },
 
+    // Diag_Fallthrough_Without_Comment_In_Switch
+    {
+      .code = 427,
+      .severity = Diagnostic_Severity::warning,
+      .message_formats = {
+        QLJS_TRANSLATABLE("missing 'break;' or '// fallthrough' comment between statement and 'case'"),
+      },
+      .message_args = {
+        {
+          Diagnostic_Message_Arg_Info(offsetof(Diag_Fallthrough_Without_Comment_In_Switch, end_of_case), Diagnostic_Arg_Type::source_code_span),
+        },
+      },
+    },
+
     // Diag_Else_Has_No_If
     {
       .code = 65,

src/quick-lint-js/diag/diagnostic-metadata-generated.h

@@ -103,6 +103,7 @@ namespace quick_lint_js {
   QLJS_DIAG_TYPE_NAME(Diag_Dot_Not_Allowed_After_Generic_Arguments_In_Type) \
   QLJS_DIAG_TYPE_NAME(Diag_Dot_Dot_Is_Not_An_Operator) \
   QLJS_DIAG_TYPE_NAME(Diag_Duplicated_Cases_In_Switch_Statement) \
+  QLJS_DIAG_TYPE_NAME(Diag_Fallthrough_Without_Comment_In_Switch) \
   QLJS_DIAG_TYPE_NAME(Diag_Else_Has_No_If) \
   QLJS_DIAG_TYPE_NAME(Diag_Equals_Does_Not_Distribute_Over_Or) \
   QLJS_DIAG_TYPE_NAME(Diag_Escaped_Character_Disallowed_In_Identifiers) \
@@ -447,7 +448,7 @@ namespace quick_lint_js {
   /* END */
 // clang-format on
 
-inline constexpr int Diag_Type_Count = 433;
+inline constexpr int Diag_Type_Count = 434;
 
 extern const Diagnostic_Info all_diagnostic_infos[Diag_Type_Count];
 }

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

@@ -759,6 +759,15 @@ struct Diag_Duplicated_Cases_In_Switch_Statement {
   Source_Code_Span duplicated_switch_case;
 };
 
+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))]]  //
+  Source_Code_Span end_of_case;
+};
+
 struct Diag_Else_Has_No_If {
   [[qljs::diag("E0065", Diagnostic_Severity::error)]]                     //
   [[qljs::message("'else' has no corresponding 'if'", ARG(else_token))]]  //

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

@@ -142,6 +142,7 @@ void Lexer::parse_bom_before_shebang() {
 [[gnu::noinline]] void Lexer::parse_current_token() {
   this->last_last_token_end_ = this->last_token_.end;
   this->last_token_.has_leading_newline = false;
+  this->last_token_.has_leading_comment = false;
   this->skip_whitespace();
 
   while (!this->try_parse_current_token()) {
@@ -906,6 +907,7 @@ Lexer::Parsed_Template_Body Lexer::parse_template_body(
 void Lexer::skip_in_jsx() {
   this->last_last_token_end_ = this->last_token_.end;
   this->last_token_.has_leading_newline = false;
+  this->last_token_.has_leading_comment = false;
   this->skip_whitespace();
 
 retry:
@@ -980,6 +982,7 @@ const Char8* Lexer::find_equal_greater_in_jsx_children() const {
 void Lexer::skip_less_less_as_less() {
   QLJS_ASSERT(this->peek().type == Token_Type::less_less);
   this->last_token_.has_leading_newline = false;
+  this->last_token_.has_leading_comment = false;
   this->last_token_.type = Token_Type::less;
   this->last_token_.begin += 1;
   this->last_last_token_end_ = this->last_token_.begin;
@@ -1156,6 +1159,7 @@ void Lexer::insert_semicolon() {
 
   this->last_token_.type = Token_Type::semicolon;
   this->last_token_.has_leading_newline = false;
+  this->last_token_.has_leading_comment = false;
   this->last_token_.begin = this->input_;
   this->last_token_.end = this->input_;
 }
@@ -1951,7 +1955,7 @@ QLJS_WARNING_POP
 void Lexer::skip_block_comment() {
   QLJS_SLOW_ASSERT(this->input_[0] == '/' && this->input_[1] == '*');
   const Char8* c = this->input_ + 2;
-
+  this->last_token_.has_leading_comment = true;
 #if QLJS_HAVE_X86_SSE2
   using Bool_Vector = Bool_Vector_16_SSE2;
   using Char_Vector = Char_Vector_16_SSE2;
@@ -2049,6 +2053,7 @@ void Lexer::skip_line_comment_body() {
   using Char_Vector = Char_Vector_1;
 #endif
 
+  this->last_token_.has_leading_comment = true;
   auto found_comment_end = [&]() {
     int n = newline_character_size(this->input_);
 

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

@@ -2815,7 +2815,27 @@ void Parser::parse_and_visit_switch(Parse_Visitor_Base &v) {
 
   bool keep_going = true;
   bool is_before_first_switch_case = true;
+  std::optional<Token> previous_statement_first_token;
   Hash_Set<String8_View> cases;
+  auto is_valid_end_of_case = [](Token_Type tk) {
+    switch (tk) {
+    case Token_Type::kw_return:
+    case Token_Type::kw_continue:
+    case Token_Type::kw_throw:
+    case Token_Type::kw_break:
+    case Token_Type::kw_case:
+    // Temporarily return true to omit diag with these statments
+    case Token_Type::kw_if:
+    case Token_Type::kw_try:
+    case Token_Type::kw_while:
+    case Token_Type::kw_do:
+    case Token_Type::kw_for:
+    case Token_Type::left_curly:
+      return true;
+    default:
+      return false;
+    }
+  };
   while (keep_going) {
     switch (this->peek().type) {
     case Token_Type::right_curly:
@@ -2824,6 +2844,13 @@ void Parser::parse_and_visit_switch(Parse_Visitor_Base &v) {
       break;
 
     case Token_Type::kw_case: {
+      if (!is_before_first_switch_case &&
+          !is_valid_end_of_case(previous_statement_first_token->type) &&
+          !this->peek().has_leading_comment) {
+        this->diag_reporter_->report(Diag_Fallthrough_Without_Comment_In_Switch{
+            .end_of_case = Source_Code_Span::unit(this->peek().begin)});
+      }
+      previous_statement_first_token = this->peek();
       is_before_first_switch_case = false;
       Source_Code_Span case_token_span = this->peek().span();
       this->skip();
@@ -2855,6 +2882,12 @@ void Parser::parse_and_visit_switch(Parse_Visitor_Base &v) {
     }
 
     case Token_Type::kw_default:
+      if (!is_before_first_switch_case &&
+          !is_valid_end_of_case(previous_statement_first_token->type) &&
+          !this->peek().has_leading_comment) {
+        this->diag_reporter_->report(Diag_Fallthrough_Without_Comment_In_Switch{
+            .end_of_case = Source_Code_Span::unit(this->peek().begin)});
+      }
       is_before_first_switch_case = false;
       this->skip();
       QLJS_PARSER_UNIMPLEMENTED_IF_NOT_TOKEN(Token_Type::colon);
@@ -2867,6 +2900,7 @@ void Parser::parse_and_visit_switch(Parse_Visitor_Base &v) {
             .unexpected_statement = this->peek().span(),
         });
       }
+      previous_statement_first_token = this->peek();
       bool parsed_statement = this->parse_and_visit_statement(
           v, Parse_Statement_Options{
                  .possibly_followed_by_another_statement = true,

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

@@ -318,6 +318,7 @@ struct Token {
   Token_Type type;
 
   bool has_leading_newline;
+  bool has_leading_comment;
 
   // Used only if this is a keyword token or an identifier token.
   // If the token contains no escape sequences, .normalized_identifier is

src/quick-lint-js/i18n/translation-table-generated.cpp

@@ -339,7 +339,8 @@ const Translation_Table translation_data = {
         {35, 20, 49, 55, 39, 38},            //
         {34, 44, 0, 36, 0, 38},              //
         {58, 48, 46, 50, 28, 52},            //
-        {0, 40, 0, 28, 0, 27},               //
+        {0, 0, 0, 0, 0, 27},                 //
+        {0, 40, 0, 28, 0, 74},               //
         {29, 22, 33, 28, 26, 26},            //
         {0, 0, 0, 55, 0, 51},                //
         {48, 27, 63, 27, 0, 24},             //
@@ -2140,6 +2141,7 @@ const Translation_Table translation_data = {
         u8"missing ':' in conditional expression\0"
         u8"missing '<>' and '</>' to enclose multiple children\0"
         u8"missing '=' after variable\0"
+        u8"missing 'break;' or '// fallthrough' comment between statement and 'case'\0"
         u8"missing 'if' after 'else'\0"
         u8"missing 'while (condition)' for do-while statement\0"
         u8"missing TypeScript type\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 = 530;
-constexpr std::size_t translation_table_string_table_size = 80362;
+constexpr std::uint16_t translation_table_mapping_table_size = 531;
+constexpr std::size_t translation_table_string_table_size = 80436;
 constexpr std::size_t translation_table_locale_table_size = 35;
 
 QLJS_CONSTEVAL std::uint16_t translation_table_const_look_up(
@@ -354,6 +354,7 @@ QLJS_CONSTEVAL std::uint16_t translation_table_const_look_up(
           "missing ':' in conditional expression"sv,
           "missing '<>' and '</>' to enclose multiple children"sv,
           "missing '=' after variable"sv,
+          "missing 'break;' or '// fallthrough' comment between statement and 'case'"sv,
           "missing 'if' after 'else'"sv,
           "missing 'while (condition)' for do-while statement"sv,
           "missing TypeScript type"sv,

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[529] = {
+inline const Translated_String test_translation_table[530] = {
     {
         "\"global-groups\" entries must be strings"_translatable,
         u8"\"global-groups\" entries must be strings",
@@ -3636,6 +3636,17 @@ inline const Translated_String test_translation_table[529] = {
             u8"saknar '=' efter variabel",
         },
     },
+    {
+        "missing 'break;' or '// fallthrough' comment between statement and 'case'"_translatable,
+        u8"missing 'break;' or '// fallthrough' comment between statement and 'case'",
+        {
+            u8"missing 'break;' or '// fallthrough' comment between statement and 'case'",
+            u8"missing 'break;' or '// fallthrough' comment between statement and 'case'",
+            u8"missing 'break;' or '// fallthrough' comment between statement and 'case'",
+            u8"missing 'break;' or '// fallthrough' comment between statement and 'case'",
+            u8"missing 'break;' or '// fallthrough' comment between statement and 'case'",
+        },
+    },
     {
         "missing 'if' after 'else'"_translatable,
         u8"missing 'if' after 'else'",

test/test-parse-warning.cpp

@@ -457,6 +457,34 @@ TEST_F(Test_Parse_Warning, warn_on_unintuitive_precedence_when_using_bitshift) {
   test_parse_and_visit_expression(u8"a & (b >> 0xBEEF)"_sv, no_diags);
   test_parse_and_visit_expression(u8"a & b >> c"_sv, no_diags);
 }
+
+TEST_F(Test_Parse_Warning, Diag_Fallthrough_Without_Comment_In_Switch) {
+  test_parse_and_visit_statement(
+      u8"switch(cond1){case 1:\nfoo()\ncase 2:\nbar() //fallthrough\ndefault:}"_sv,  //
+      u8"                              ` Diag_Fallthrough_Without_Comment_In_Switch"_diag);
+  test_parse_and_visit_statement(
+      u8"switch(cond1){case 1:\nfoo()\ncase 2:\nlongBarFn()\ndefault:}"_sv,  //
+      u8"                                                    ` Diag_Fallthrough_Without_Comment_In_Switch"_diag,  //
+      u8"                              ` Diag_Fallthrough_Without_Comment_In_Switch"_diag);
+  // check for false positive
+  test_parse_and_visit_statement(
+      u8R"(switch(cond1){
+        case 1:
+        case 2:
+        default:
+      })"_sv,
+      no_diags);
+  test_parse_and_visit_statement(
+      u8R"(switch(cond1){
+      case 1:
+        foo()
+
+        //fallthrough
+      case 2:
+        bar()//fallthrough
+      default:})"_sv,
+      no_diags);
+}
 }
 }
 

test/test-parse.cpp

@@ -263,6 +263,7 @@ TEST_F(Test_Parse, asi_between_expression_statement_and_switch_label) {
       switch (x) {
         case a:
           f()
+          //fallthrough
         case b:
           g()
       }
@@ -278,6 +279,7 @@ TEST_F(Test_Parse, asi_between_expression_statement_and_switch_label) {
       switch (x) {
         case a:
           f()
+          //fallthrough
         default:
           g()
       }