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

feat: add reportUnusedFallthroughComment option to no-fallthrough rule #18188

Merged
merged 7 commits into from
Mar 14, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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
38 changes: 38 additions & 0 deletions docs/src/rules/no-fallthrough.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -235,6 +237,42 @@ 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;
}
}
```

:::

Comment on lines +273 to +275
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we can add a correct section here with an example having a comment that doesn't permit fallthrough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense! Added

## 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.
58 changes: 42 additions & 16 deletions lib/rules/no-fallthrough.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,23 +48,27 @@ 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") {
const trailingCloseBrace = sourceCode.getLastToken(caseWhichFallsThrough.consequent[0]);
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;
}

/**
Expand Down Expand Up @@ -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'."
}
Expand All @@ -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) {
Expand Down Expand Up @@ -168,13 +178,24 @@ 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) {
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) {
Expand All @@ -185,11 +206,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
};
Comment on lines +208 to +216
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The state can leak across multiple switches this way:

/* eslint no-fallthrough: ["error", { "reportUnusedFallthroughComment": true }] */

switch(foo){
    case 1:
        doSomething();
        break;
}

function f() {
    switch(foo){
        // falls through comment false positive
        case 1:
            if (a) {
                throw new Error();
            } else if (b) {
                break;
            } else {
                return;
            }
        case 2:
            break;
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh dear. And I thought I was being all clever by adding those test cases 🤦

Fixed!


}
};
}
Expand Down
141 changes: 141 additions & 0 deletions tests/lib/rules/no-fallthrough.js
Original file line number Diff line number Diff line change
Expand Up @@ -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: [{
Expand Down Expand Up @@ -109,8 +130,51 @@ 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 }]
}
],

invalid: [
{
code: "switch(foo) { case 0: a();\ncase 1: b() }",
Expand Down Expand Up @@ -310,6 +374,83 @@ 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"
}
]
}
]
});