diff --git a/docs/src/rules/no-fallthrough.md b/docs/src/rules/no-fallthrough.md index 1f092097330..3076516aa65 100644 --- a/docs/src/rules/no-fallthrough.md +++ b/docs/src/rules/no-fallthrough.md @@ -178,6 +178,8 @@ This rule has an object option: * Set the `allowEmptyCase` option to `true` to allow empty cases regardless of the layout. By default, this rule does not require a fallthrough comment after an empty `case` only if the empty `case` and the next `case` are on the same line or on consecutive lines. +* Set the `reportUnusedFallthroughComment` option to `true` to prohibit a fallthrough comment from being present if the case cannot fallthrough due to being unreachable. This is mostly intended to help avoid misleading comments occurring as a result of refactoring. + ### commentPattern Examples of **correct** code for the `{ "commentPattern": "break[\\s\\w]*omitted" }` option: @@ -235,6 +237,60 @@ switch(foo){ ::: +### reportUnusedFallthroughComment + +Examples of **incorrect** code for the `{ "reportUnusedFallthroughComment": true }` option: + +::: incorrect + +```js +/* eslint no-fallthrough: ["error", { "reportUnusedFallthroughComment": true }] */ + +switch(foo){ + case 1: + doSomething(); + break; + // falls through + case 2: doSomething(); +} + +function f() { + switch(foo){ + case 1: + if (a) { + throw new Error(); + } else if (b) { + break; + } else { + return; + } + // falls through + case 2: + break; + } +} +``` + +::: + +Examples of **correct** code for the `{ "reportUnusedFallthroughComment": true }` option: + +::: correct + +```js +/* eslint no-fallthrough: ["error", { "reportUnusedFallthroughComment": true }] */ + +switch(foo){ + case 1: + doSomething(); + break; + // just a comment + case 2: doSomething(); +} +``` + +::: + ## When Not To Use It If you don't want to enforce that each `case` statement should end with a `throw`, `return`, `break`, or comment, then you can safely turn this rule off. diff --git a/lib/rules/no-fallthrough.js b/lib/rules/no-fallthrough.js index 03b5f6f7ce6..4c98ddde525 100644 --- a/lib/rules/no-fallthrough.js +++ b/lib/rules/no-fallthrough.js @@ -48,9 +48,9 @@ function isFallThroughComment(comment, fallthroughCommentPattern) { * @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 case has a valid fallthrough comment. + * @returns {null | object} the comment if the case has a valid fallthrough comment, otherwise null */ -function hasFallthroughComment(caseWhichFallsThrough, subsequentCase, context, fallthroughCommentPattern) { +function getFallthroughComment(caseWhichFallsThrough, subsequentCase, context, fallthroughCommentPattern) { const sourceCode = context.sourceCode; if (caseWhichFallsThrough.consequent.length === 1 && caseWhichFallsThrough.consequent[0].type === "BlockStatement") { @@ -58,13 +58,17 @@ function hasFallthroughComment(caseWhichFallsThrough, subsequentCase, context, f const commentInBlock = sourceCode.getCommentsBefore(trailingCloseBrace).pop(); if (commentInBlock && isFallThroughComment(commentInBlock.value, fallthroughCommentPattern)) { - return true; + return commentInBlock; } } const comment = sourceCode.getCommentsBefore(subsequentCase).pop(); - return Boolean(comment && isFallThroughComment(comment.value, fallthroughCommentPattern)); + if (comment && isFallThroughComment(comment.value, fallthroughCommentPattern)) { + return comment; + } + + return null; } /** @@ -103,12 +107,17 @@ module.exports = { allowEmptyCase: { type: "boolean", default: false + }, + reportUnusedFallthroughComment: { + type: "boolean", + default: false } }, additionalProperties: false } ], messages: { + unusedFallthroughComment: "Found a comment that would permit fallthrough, but case cannot fall through.", case: "Expected a 'break' statement before 'case'.", default: "Expected a 'break' statement before 'default'." } @@ -120,12 +129,13 @@ module.exports = { let currentCodePathSegments = new Set(); const sourceCode = context.sourceCode; const allowEmptyCase = options.allowEmptyCase || false; + const reportUnusedFallthroughComment = options.reportUnusedFallthroughComment || false; /* * We need to use leading comments of the next SwitchCase node because * trailing comments is wrong if semicolons are omitted. */ - let fallthroughCase = null; + let previousCase = null; let fallthroughCommentPattern = null; if (options.commentPattern) { @@ -168,13 +178,23 @@ module.exports = { * And reports the previous fallthrough node if that does not exist. */ - if (fallthroughCase && (!hasFallthroughComment(fallthroughCase, node, context, fallthroughCommentPattern))) { - context.report({ - messageId: node.test ? "case" : "default", - node - }); + if (previousCase && previousCase.node.parent === node.parent) { + const previousCaseFallthroughComment = getFallthroughComment(previousCase.node, node, context, fallthroughCommentPattern); + + if (previousCase.isFallthrough && !(previousCaseFallthroughComment)) { + context.report({ + messageId: node.test ? "case" : "default", + node + }); + } else if (reportUnusedFallthroughComment && !previousCase.isSwitchExitReachable && previousCaseFallthroughComment) { + context.report({ + messageId: "unusedFallthroughComment", + node: previousCaseFallthroughComment + }); + } + } - fallthroughCase = null; + previousCase = null; }, "SwitchCase:exit"(node) { @@ -185,11 +205,16 @@ module.exports = { * `break`, `return`, or `throw` are unreachable. * And allows empty cases and the last case. */ - if (isAnySegmentReachable(currentCodePathSegments) && - (node.consequent.length > 0 || (!allowEmptyCase && hasBlankLinesBetween(node, nextToken))) && - node.parent.cases.at(-1) !== node) { - fallthroughCase = node; - } + const isSwitchExitReachable = isAnySegmentReachable(currentCodePathSegments); + const isFallthrough = isSwitchExitReachable && (node.consequent.length > 0 || (!allowEmptyCase && hasBlankLinesBetween(node, nextToken))) && + node.parent.cases.at(-1) !== node; + + previousCase = { + node, + isSwitchExitReachable, + isFallthrough + }; + } }; } diff --git a/tests/lib/rules/no-fallthrough.js b/tests/lib/rules/no-fallthrough.js index f922a76fcac..ac3806964a6 100644 --- a/tests/lib/rules/no-fallthrough.js +++ b/tests/lib/rules/no-fallthrough.js @@ -64,6 +64,27 @@ ruleTester.run("no-fallthrough", rule, { "switch (foo) { case 0: try { throw 0; } catch (err) { break; } default: b(); }", "switch (foo) { case 0: do { throw 0; } while(a); default: b(); }", "switch (foo) { case 0: a(); \n// eslint-disable-next-line rule-to-test/no-fallthrough\n case 1: }", + ` + switch (foo) { + case 0: + a(); + break; + /* falls through */ + case 1: + b(); + } + `, + ` + switch (foo) { + case 0: + a(); + break; + // eslint-disable-next-line rule-to-test/no-fallthrough + /* falls through */ + case 1: + b(); + } + `, { code: "switch(foo) { case 0: a(); /* no break */ case 1: b(); }", options: [{ @@ -109,8 +130,128 @@ ruleTester.run("no-fallthrough", rule, { { code: "switch (a) {\n case 1: ; break; \n case 3: }", options: [{ allowEmptyCase: false }] + }, + ` +switch (foo) { + case 0: + a(); +} +switch (bar) { + case 1: + b(); +} + `, + { + code: + ` +switch (foo) { + case 0: + a(); + break; + // falls through +} +switch (bar) { + case 1: + b(); +} + `, + options: [{ reportUnusedFallthroughComment: true }] + }, + { + code: + ` +switch (foo) { + case 0: + a(); + break; + /* falls through */ +} +switch (bar) { + case 1: + b(); +} + `, + options: [{ reportUnusedFallthroughComment: true }] + }, + { + code: ` +switch(foo){ + case 1: + doSomething(); + break; + // just a comment + case 2: doSomething(); +} + `, + options: [{ reportUnusedFallthroughComment: true }] + + }, + { + code: ` +switch(foo){ + case 1: + doSomething(); + break; +} + +function f() { + switch(foo){ + // falls through comment should not false positive + case 1: + if (a) { + throw new Error(); + } else if (b) { + break; + } else { + return; + } + case 2: + break; + } +} + `, + options: [{ reportUnusedFallthroughComment: true }] + }, + { + code: ` +switch(foo){ + case 1: + doSomething(); + break; +} + +function f() { + switch(foo){ + /* falls through comment should not false positive */ + case 1: + if (a) { + throw new Error(); + } else if (b) { + break; + } else { + return; + } + case 2: + break; + } +} + `, + options: [{ reportUnusedFallthroughComment: true }] + }, + { + code: ` +switch(foo){ + case 1: + doSomething(); + // falls through + case 2: doSomething(); +} + `, + options: [{ reportUnusedFallthroughComment: true }] + } ], + invalid: [ { code: "switch(foo) { case 0: a();\ncase 1: b() }", @@ -310,6 +451,102 @@ ruleTester.run("no-fallthrough", rule, { column: 2 } ] + }, + { + code: ` +switch (foo) { + case 0: + a(); + break; + /* falls through */ + case 1: + b(); +}`, + options: [{ reportUnusedFallthroughComment: true }], + errors: [ + { + line: 6, + messageId: "unusedFallthroughComment" + } + ] + }, + { + code: ` +switch (foo) { + default: + a(); + break; + /* falls through */ + case 1: + b(); +}`, + options: [{ reportUnusedFallthroughComment: true }], + errors: [ + { + line: 6, + messageId: "unusedFallthroughComment" + } + ] + }, + { + code: ` +switch(foo){ + case 1: + doSomething(); + break; + // falls through + case 2: doSomething(); +}`, + options: [{ reportUnusedFallthroughComment: true }], + errors: [ + { + line: 6, + messageId: "unusedFallthroughComment" + } + ] + }, { + code: ` +function f() { + switch(foo){ + case 1: + if (a) { + throw new Error(); + } else if (b) { + break; + } else { + return; + } + // falls through + case 2: + break; + } +}`, + options: [{ reportUnusedFallthroughComment: true }], + errors: [ + { + line: 12, + messageId: "unusedFallthroughComment" + } + ] + }, + { + code: ` +switch (foo) { + case 0: { + a(); + break; + // falls through + } + case 1: + b(); +}`, + options: [{ reportUnusedFallthroughComment: true }], + errors: [ + { + line: 6, + messageId: "unusedFallthroughComment" + } + ] } ] });