Feature/warn missing fallthrough

docs/errors/E0423.md

@@ -0,0 +1,29 @@
+# E0423: missing fallthrough comment in switch 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:
+      foo();
+    default:
+      bar();
+  }
+}
+```
+
+To fix this error, place a comment at the end of `case 1` indicating fallthrough
+
+```javascript
+function test (c) {
+  switch (c) {
+    case 1:
+      foo();
+      //fallthrough
+    default:
+      bar();
+  }
+}
+```

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 fallthrough comment"
+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_Explicit_Fallthrough_Comment_In_Switch
+    {
+      .code = 423,
+      .severity = Diagnostic_Severity::warning,
+      .message_formats = {
+        QLJS_TRANSLATABLE("missing fallthrough comment"),
+      },
+      .message_args = {
+        {
+          Diagnostic_Message_Arg_Info(offsetof(Diag_Explicit_Fallthrough_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_Explicit_Fallthrough_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) \
@@ -442,7 +443,7 @@ namespace quick_lint_js {
   /* END */
 // clang-format on
 
-inline constexpr int Diag_Type_Count = 428;
+inline constexpr int Diag_Type_Count = 429;
 
 extern const Diagnostic_Info all_diagnostic_infos[Diag_Type_Count];
 }

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

@@ -759,6 +759,13 @@ struct Diag_Duplicated_Cases_In_Switch_Statement {
   Source_Code_Span duplicated_switch_case;
 };
 
+struct Diag_Explicit_Fallthrough_Comment_In_Switch {
+  [[qljs::diag("E0423", Diagnostic_Severity::warning)]]  //
+  [[qljs::message("missing fallthrough comment",
+                  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/parse-statement.cpp

@@ -2766,15 +2766,51 @@ void Parser::parse_and_visit_switch(Parse_Visitor_Base &v) {
 
   bool keep_going = true;
   bool is_before_first_switch_case = true;
+  Token prev_token;
   Hash_Set<String8_View> cases;
   while (keep_going) {
+    auto is_comment_line = [&]() -> bool {
+      const Char8 *th = this->lexer().end_of_previous_token();
+      bool f = false;
+      int i = 0;
+      for (;;) {
+        switch (th[i]) {
+        case '/':
+          if (th[i + 1] == '/') {
+            f = true;
+          }
+          goto exit_loop;
+        // skip empty lines to reach comment
+        case ' ':
+        case '\n':
+        case '\r':
+        case '\t':
+        case '\f':
+        case '\v':
+          break;
+        default:
+          goto exit_loop;
+        }
+        i++;
+      }
+    exit_loop:;
+      return f;
+    };
     switch (this->peek().type) {
     case Token_Type::right_curly:
       this->skip();
       keep_going = false;
       break;
 
     case Token_Type::kw_case: {
+      if (!is_before_first_switch_case &&
+          prev_token.type != Token_Type::kw_break &&
+          prev_token.type != Token_Type::kw_case && !is_comment_line()) {
+        this->diag_reporter_->report(
+            Diag_Explicit_Fallthrough_Comment_In_Switch{.end_of_case =
+                                                            prev_token.span()});
+      }
+      prev_token = this->peek();
       is_before_first_switch_case = false;
       Source_Code_Span case_token_span = this->peek().span();
       this->skip();
@@ -2806,6 +2842,13 @@ void Parser::parse_and_visit_switch(Parse_Visitor_Base &v) {
     }
 
     case Token_Type::kw_default:
+      if (!is_before_first_switch_case &&
+          prev_token.type != Token_Type::kw_break &&
+          prev_token.type != Token_Type::kw_case && !is_comment_line()) {
+        this->diag_reporter_->report(
+            Diag_Explicit_Fallthrough_Comment_In_Switch{.end_of_case =
+                                                            prev_token.span()});
+      }
       is_before_first_switch_case = false;
       this->skip();
       QLJS_PARSER_UNIMPLEMENTED_IF_NOT_TOKEN(Token_Type::colon);
@@ -2818,6 +2861,7 @@ void Parser::parse_and_visit_switch(Parse_Visitor_Base &v) {
             .unexpected_statement = this->peek().span(),
         });
       }
+      prev_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/i18n/translation-table-generated.cpp

@@ -363,7 +363,8 @@ const Translation_Table translation_data = {
         {0, 0, 0, 0, 0, 35},                 //
         {36, 34, 39, 35, 40, 37},            //
         {0, 0, 0, 0, 0, 39},                 //
-        {33, 7, 40, 40, 33, 58},             //
+        {0, 0, 0, 0, 0, 58},                 //
+        {33, 7, 40, 40, 33, 28},             //
         {24, 11, 33, 22, 23, 24},            //
         {34, 63, 43, 41, 33, 32},            //
         {41, 11, 49, 43, 0, 39},             //
@@ -2156,6 +2157,7 @@ const Translation_Table translation_data = {
         u8"missing exported name in import type\0"
         u8"missing expression between parentheses\0"
         u8"missing expression in placeholder within template literal\0"
+        u8"missing fallthrough comment\0"
         u8"missing for loop header\0"
         u8"missing function parameter list\0"
         u8"missing header and body for 'for' loop\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 = 522;
-constexpr std::size_t translation_table_string_table_size = 79941;
+constexpr std::uint16_t translation_table_mapping_table_size = 523;
+constexpr std::size_t translation_table_string_table_size = 79969;
 constexpr std::size_t translation_table_locale_table_size = 35;
 
 QLJS_CONSTEVAL std::uint16_t translation_table_const_look_up(
@@ -378,6 +378,7 @@ QLJS_CONSTEVAL std::uint16_t translation_table_const_look_up(
           "missing exported name in import type"sv,
           "missing expression between parentheses"sv,
           "missing expression in placeholder within template literal"sv,
+          "missing fallthrough comment"sv,
           "missing for loop header"sv,
           "missing function parameter list"sv,
           "missing header and body for 'for' loop"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[521] = {
+inline const Translated_String test_translation_table[522] = {
     {
         "\"global-groups\" entries must be strings"_translatable,
         u8"\"global-groups\" entries must be strings",
@@ -3900,6 +3900,17 @@ inline const Translated_String test_translation_table[521] = {
             u8"missing expression in placeholder within template literal",
         },
     },
+    {
+        "missing fallthrough comment"_translatable,
+        u8"missing fallthrough comment",
+        {
+            u8"missing fallthrough comment",
+            u8"missing fallthrough comment",
+            u8"missing fallthrough comment",
+            u8"missing fallthrough comment",
+            u8"missing fallthrough comment",
+        },
+    },
     {
         "missing for loop header"_translatable,
         u8"missing for loop header",

test/test-parse-warning.cpp

@@ -436,6 +436,33 @@ TEST_F(Test_Parse_Warning, warn_on_xor_operation_used_as_exponentiation) {
   test_parse_and_visit_expression(u8"4 ^ 3"_sv, no_diags);
   test_parse_and_visit_expression(u8"(x+2)^a"_sv, no_diags);
 }
+TEST_F(Test_Parse_Warning, Diag_Explicit_Fallthrough_Comment_In_Switch) {
+  test_parse_and_visit_statement(
+      u8"switch(cond1){case 1:\nfoo()\ncase 2:\nbar() //fallthrough\ndefault:}"_sv,  //
+      u8"                       ^^^ Diag_Explicit_Fallthrough_Comment_In_Switch"_diag);
+  test_parse_and_visit_statement(
+      u8"switch(cond1){case 1:\nfoo()\ncase 2:\nlongBarFn()\ndefault:}"_sv,  //
+      u8"                                       ^^^^^^^^^ Diag_Explicit_Fallthrough_Comment_In_Switch"_diag,  //
+      u8"                       ^^^ Diag_Explicit_Fallthrough_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()
       }