From 6a1c7a0dac050ea5876972c50563a7eb867b38d3 Mon Sep 17 00:00:00 2001 From: Kevin Gibbons Date: Fri, 18 Jun 2021 12:42:22 -0700 Subject: [PATCH] Fix: allow fallthrough comment inside block (fixes #14701) (#14702) * Fix: allow fallthrough comment inside block (fixes #14701) * address comments --- docs/rules/no-fallthrough.md | 22 +++++++++++++++ lib/rules/no-fallthrough.js | 23 ++++++++++++---- tests/lib/rules/no-fallthrough.js | 46 +++++++++++++++++++++++++++++++ 3 files changed, 85 insertions(+), 6 deletions(-) diff --git a/docs/rules/no-fallthrough.md b/docs/rules/no-fallthrough.md index 15ca77f661b..81b428cf070 100644 --- a/docs/rules/no-fallthrough.md +++ b/docs/rules/no-fallthrough.md @@ -54,6 +54,17 @@ switch(foo) { case 2: doSomethingElse(); } + +switch(foo) { + case 1: { + doSomething(); + // falls through + } + + case 2: { + doSomethingElse(); + } +} ``` 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. @@ -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. diff --git a/lib/rules/no-fallthrough.js b/lib/rules/no-fallthrough.js index e8016e93e59..3b949acd1da 100644 --- a/lib/rules/no-fallthrough.js +++ b/lib/rules/no-fallthrough.js @@ -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)); } @@ -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 diff --git a/tests/lib/rules/no-fallthrough.js b/tests/lib/rules/no-fallthrough.js index e154c31d5f7..0b98d3293e9 100644 --- a/tests/lib/rules/no-fallthrough.js +++ b/tests/lib/rules/no-fallthrough.js @@ -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(); } }", @@ -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 + }, { code: "switch(foo) { case 0:\n // comment\n default: b() }", errors: errorsDefault @@ -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 + } + ] } ] });