Feature/warn missing fallthrough #1094

yashmasani · 2023-10-21T19:41:49Z

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

…ly introduced diag E0423

yashmasani · 2023-10-21T19:44:06Z

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

  Hash_Set<String8_View> cases;
  while (keep_going) {
+    auto is_comment_line = [&]() -> bool {


@strager the function to find the comment, is_comment_line skips all whitespace, which is similar to the lexer function. However, the lexer handles many more cases of characters. Do you think all those cases should be included within is_comment_line as well ?

Must fix: We should not repeat comment-parsing code like you have done here.

I see two options:

Call Lexer's code directly instead of reimplementing or copy-pasting it here; or

Teach the lexer to communicate when a comment was seen before a token.
We should try the latter solution as I think it will be more reliable and useful. I think adding a bool has_leading_comment; into the Token class should be simple enough for our needs. Token::has_leading_newline already exists, and the code for has_leading_comment might be very similar.

You are right. I went with the suggestion of adding has_leading_comment and it worked very well 👍

strager

Must fix: The following code should not report any diagnostic, but your code reports several diagnostics for it:

function f(a) {
  for (let c in a) {
    switch (c) {
    case 1:
      return 42;
    case 2:
      break;
    case 3:
      continue;
    case 4:
      throw new Error();
    case 5:
    case 6:
      if (c > 5) {
        return 1;
      } else {
        return 2;
      }
    case 7:
    }
  }
}

Fixing most of these cases should be easy, but the if looks challenging. For now, I think it's fine if we allow some false negatives by turning off the diagnostic if we see an if, for, while, try, etc.

strager · 2023-10-23T22:56:29Z

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",


Minor: This message and the message in docs/errors/E0423.md should match.

strager · 2023-10-23T23:00:06Z

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;


Nit: I think this variable's name is bad. prev_token does not refer to the token just before the current token; it refers to a prior case or default token or the first token of a statement.

Maybe previous_statement_first_token is a better name?

strager · 2023-10-23T23:17:36Z

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 {


Nit: We normally name the diagnostic struct after the problem, not the solution. Therefore this should be named something like Diag_Fallthrough_Without_Comment_In_Switch.

strager · 2023-10-23T23:18:09Z

test/test-parse-warning.cpp

+      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


strager · 2023-10-23T23:26:07Z

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

  Hash_Set<String8_View> cases;
  while (keep_going) {
+    auto is_comment_line = [&]() -> bool {


Must fix: We should not repeat comment-parsing code like you have done here.

I see two options:

Call Lexer's code directly instead of reimplementing or copy-pasting it here; or

Teach the lexer to communicate when a comment was seen before a token.
We should try the latter solution as I think it will be more reliable and useful. I think adding a bool has_leading_comment; into the Token class should be simple enough for our needs. Token::has_leading_newline already exists, and the code for has_leading_comment might be very similar.

strager · 2023-10-23T23:31:43Z

test/test-parse-warning.cpp

+      u8"switch(cond1){case 1:\nfoo()\ncase 2:\nbar() //fallthrough\ndefault:}"_sv,  //
+      u8"                       ^^^ Diag_Explicit_Fallthrough_Comment_In_Switch"_diag);


Must fix: I think the diagnostic is in the wrong place. As a user, I would expect the diagnostic to be between the statement and the following case, not at the beginning of the statement. In other words, the diagnostic should point me to where I should fix the problem. For example, the diagnostic could be just before case:

u8"switch(cond1){case 1:\nfoo()\ncase 2:\nbar() //fallthrough\ndefault:}"_sv, // u8" ` Diag_Explicit_Fallthrough_Comment_In_Switch"_diag);

You are right. I went with the suggestion of adding has_leading_comment 👍
Also switched the diag to be just before case or default.

strager · 2023-10-23T23:33:27Z

docs/errors/E0423.md

@@ -0,0 +1,29 @@
+# E0423: missing fallthrough comment in switch case


"missing fallthrough comment" is good, but if the programmer intended to write a 'break', this diagnostic message is misleading.

Maybe this is a better message:

missing 'break;' or '// fallthrough' comment between statement and 'case'

For reference, here is the message from ESLint's no-fallthrough rule:

Expected a 'break' statement before 'case'.

Sounds good.
Switched to missing 'break;' or '// fallthrough' comment between statement and 'case'
FYI: Also had to switch diag code to E0427 since E0423 was used recently.

strager · 2023-10-23T23:37:35Z

Thanks for the pull request. I think you're on the right track. There are a lot of things to consider, though, which is one reason I didn't mark the task as for-hire or good-first-issue.

yashmasani · 2023-11-10T05:27:19Z

Thanks for the pull request. I think you're on the right track. There are a lot of things to consider, though, which is one reason I didn't mark the task as for-hire or good-first-issue.

Thank you for the review. All comments were very clear and concise so I could make the changes easily.

strager

Must fix: Your patch introduces a false positive diagnostic in the example for E0140:

function getText(node) {
  switch (node.nodeType) {
    case document.TEXT_NODE:
      return node.nodeValue;
    case: {
      let result = "";
      for (let child of node.childNodes) {
        result += getText(child, document);
      }
      return result;
    }
    default:
      throw new Error("Unsupported DOM node type");
  }
}

strager · 2023-11-16T02:37:06Z

docs/errors/E0427.md

+      foo();
+    default:
+      bar();


These examples assume that globals foo and bar exist, but they don't. This causes warning E0057 ('use of undeclared variable: foo').

To fix this, I suggest writing console.log calls instead. (The console global should exist in these examples.)

strager · 2023-11-16T02:39:17Z

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

  Hash_Set<String8_View> cases;
+  auto is_valid_end_of_case = [](Token_Type tk) {


👍 Nice helper function.

strager · 2023-11-16T02:40:57Z

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

@@ -2824,6 +2843,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) &&


GCC reports a warning because it thinks previous_statement_first_token might be uninitialized here.

To fix the warning, I think making previous_statement_first_token a std::optional is a decent solution.

strager · 2023-11-16T02:45:28Z

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


strager · 2023-11-16T06:32:02Z

If you want, I can fix the remaining issues. Let me know.

yashmasani · 2023-11-20T03:47:15Z

If you want, I can fix the remaining issues. Let me know.

I addressed the remaining issues. Thanks!

strager · 2023-11-21T18:51:37Z

Landed as commit 422f3b4. 👍

yashmasani added 6 commits October 21, 2023 15:03

feat: add diag missing fallthrough comment to diagnostic types

90b6ce8

feat: add functionality to find comments and report to diag if not found

9a31c29

fix: remove unintended diag warning from other tests cases due to new…

a95534f

…ly introduced diag E0423

test: add new test cases for E0423 diag warn

221cd6a

fix: clang-format

b23aa3e

feat: add error docs for E0423

731cce5

yashmasani commented Oct 21, 2023

View reviewed changes

strager requested changes Oct 23, 2023

View reviewed changes

yashmasani added 7 commits October 28, 2023 13:01

feat: use more explicit message for E0423 diag

f8ed5fa

fix: change diagnostic name and match message with docs

a190be5

fix: change diag name in test

d73c09a

feat: check if token is valid end for switch case

2516ebc

fix: diag should display before case/default

f07f052

Merge branch 'master' into feature/warn-missing-fallthrough

f121cb6

fix: clang-format and use more correct variable name

1be8e48

yashmasani requested a review from strager November 10, 2023 21:43

strager mentioned this pull request Nov 16, 2023

Emacs Flymake: diagnostics with empty span do not appear #1104

Open

strager requested changes Nov 16, 2023

View reviewed changes

yashmasani added 2 commits November 19, 2023 22:13

fix: remove false positive diag

6cc3562

fix: use console in docs instead of undeclared functions

9f96079

strager closed this Nov 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/warn missing fallthrough #1094

Feature/warn missing fallthrough #1094

yashmasani commented Oct 21, 2023 •

edited

Loading

yashmasani Oct 21, 2023

strager Oct 23, 2023

yashmasani Nov 10, 2023

strager left a comment

strager Oct 23, 2023

strager Oct 23, 2023

strager Oct 23, 2023

strager Oct 23, 2023

strager Oct 23, 2023

strager Oct 23, 2023

yashmasani Nov 10, 2023

strager Oct 23, 2023

yashmasani Nov 10, 2023

strager commented Oct 23, 2023

yashmasani commented Nov 10, 2023

strager left a comment

strager Nov 16, 2023

strager Nov 16, 2023

strager Nov 16, 2023

strager Nov 16, 2023

strager commented Nov 16, 2023

yashmasani commented Nov 20, 2023

strager commented Nov 21, 2023

		u8"switch(cond1){case 1:\nfoo()\ncase 2:\nbar() //fallthrough\ndefault:}"_sv, //
		u8" ^^^ Diag_Explicit_Fallthrough_Comment_In_Switch"_diag);

		@@ -0,0 +1,29 @@
		# E0423: missing fallthrough comment in switch case

		Hash_Set<String8_View> cases;
		auto is_valid_end_of_case = [](Token_Type tk) {

Feature/warn missing fallthrough #1094

Feature/warn missing fallthrough #1094

Conversation

yashmasani commented Oct 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strager left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strager commented Oct 23, 2023

yashmasani commented Nov 10, 2023

strager left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strager commented Nov 16, 2023

yashmasani commented Nov 20, 2023

strager commented Nov 21, 2023

yashmasani commented Oct 21, 2023 •

edited

Loading