Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: allow fallthrough comment inside block (fixes #14701) #14702

Merged
merged 2 commits into from Jun 18, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 22 additions & 0 deletions docs/rules/no-fallthrough.md
Expand Up @@ -54,6 +54,17 @@ switch(foo) {
case 2:
doSomethingElse();
}

switch(foo) {
case 1: {
doSomething();
// falls through
}

case 2: {
doSomethingElse();
}
}
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved
```

In this example, there is no confusion as to the expected behavior. It is clear that the first case is meant to fall through to the second case.
Expand Down Expand Up @@ -124,6 +135,17 @@ switch(foo) {
case 2:
doSomething();
}

switch(foo) {
case 1: {
doSomething();
// falls through
}

case 2: {
doSomethingElse();
}
}
```

Note that the last `case` statement in these examples does not cause a warning because there is nothing to fall through into.
Expand Down
23 changes: 17 additions & 6 deletions lib/rules/no-fallthrough.js
Expand Up @@ -11,15 +11,26 @@
const DEFAULT_FALLTHROUGH_COMMENT = /falls?\s?through/iu;

/**
* Checks whether or not a given node has a fallthrough comment.
* @param {ASTNode} node A SwitchCase node to get comments.
* Checks whether or not a given case has a fallthrough comment.
* @param {ASTNode} caseWhichFallsThrough SwitchCase node which falls through.
* @param {ASTNode} subsequentCase The case after caseWhichFallsThrough.
* @param {RuleContext} context A rule context which stores comments.
* @param {RegExp} fallthroughCommentPattern A pattern to match comment to.
* @returns {boolean} `true` if the node has a valid fallthrough comment.
* @returns {boolean} `true` if the case has a valid fallthrough comment.
*/
function hasFallthroughComment(node, context, fallthroughCommentPattern) {
function hasFallthroughComment(caseWhichFallsThrough, subsequentCase, context, fallthroughCommentPattern) {
const sourceCode = context.getSourceCode();
const comment = sourceCode.getCommentsBefore(node).pop();

if (caseWhichFallsThrough.consequent.length === 1 && caseWhichFallsThrough.consequent[0].type === "BlockStatement") {
const trailingCloseBrace = sourceCode.getLastToken(caseWhichFallsThrough.consequent[0]);
const commentInBlock = sourceCode.getCommentsBefore(trailingCloseBrace).pop();

if (commentInBlock && fallthroughCommentPattern.test(commentInBlock.value)) {
return true;
}
}

const comment = sourceCode.getCommentsBefore(subsequentCase).pop();

return Boolean(comment && fallthroughCommentPattern.test(comment.value));
}
Expand Down Expand Up @@ -108,7 +119,7 @@ module.exports = {
* Checks whether or not there is a fallthrough comment.
* And reports the previous fallthrough node if that does not exist.
*/
if (fallthroughCase && !hasFallthroughComment(node, context, fallthroughCommentPattern)) {
if (fallthroughCase && !hasFallthroughComment(fallthroughCase, node, context, fallthroughCommentPattern)) {
context.report({
messageId: node.test ? "case" : "default",
node
Expand Down
46 changes: 46 additions & 0 deletions tests/lib/rules/no-fallthrough.js
Expand Up @@ -30,6 +30,14 @@ ruleTester.run("no-fallthrough", rule, {
"switch(foo) { case 0: a(); /* fall through */ case 1: b(); }",
"switch(foo) { case 0: a(); /* fallthrough */ case 1: b(); }",
"switch(foo) { case 0: a(); /* FALLS THROUGH */ case 1: b(); }",
"switch(foo) { case 0: { a(); /* falls through */ } case 1: b(); }",
"switch(foo) { case 0: { a()\n /* falls through */ } case 1: b(); }",
"switch(foo) { case 0: { a(); /* fall through */ } case 1: b(); }",
"switch(foo) { case 0: { a(); /* fallthrough */ } case 1: b(); }",
"switch(foo) { case 0: { a(); /* FALLS THROUGH */ } case 1: b(); }",
"switch(foo) { case 0: { a(); } /* falls through */ case 1: b(); }",
"switch(foo) { case 0: { a(); /* falls through */ } /* comment */ case 1: b(); }",
"switch(foo) { case 0: { /* falls through */ } case 1: b(); }",
"function foo() { switch(foo) { case 0: a(); return; case 1: b(); }; }",
"switch(foo) { case 0: a(); throw 'foo'; case 1: b(); }",
"while (a) { switch(foo) { case 0: a(); continue; case 1: b(); } }",
Expand Down Expand Up @@ -133,6 +141,30 @@ ruleTester.run("no-fallthrough", rule, {
code: "switch(foo) { case 0:\n\n default: b() }",
errors: errorsDefault
},
{
code: "switch(foo) { case 0: {} default: b() }",
errors: errorsDefault
},
{
code: "switch(foo) { case 0: a(); { /* falls through */ } default: b() }",
errors: errorsDefault
},
{
code: "switch(foo) { case 0: { /* falls through */ } a(); default: b() }",
errors: errorsDefault
},
{
code: "switch(foo) { case 0: if (a) { /* falls through */ } default: b() }",
errors: errorsDefault
},
{
code: "switch(foo) { case 0: { { /* falls through */ } } default: b() }",
errors: errorsDefault
},
{
code: "switch(foo) { case 0: { /* comment */ } default: b() }",
errors: errorsDefault
},
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved
{
code: "switch(foo) { case 0:\n // comment\n default: b() }",
errors: errorsDefault
Expand Down Expand Up @@ -168,6 +200,20 @@ ruleTester.run("no-fallthrough", rule, {
column: 1
}
]
},
{
code: "switch(foo) { case 0: { a();\n/* no break */\n/* todo: fix readability */ }\ndefault: b() }",
options: [{
commentPattern: "no break"
}],
errors: [
{
messageId: "default",
type: "SwitchCase",
line: 4,
column: 1
}
]
}
]
});