Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
fix: ignore directives for no-fallthrough (#16757)
Fallthrough comment pattern detection was
incorrectly matching to eslint directives.

Fixes #16650
  • Loading branch information
gfyoung committed Jan 15, 2023
1 parent 5981296 commit b4f8329
Show file tree
Hide file tree
Showing 6 changed files with 73 additions and 5 deletions.
4 changes: 2 additions & 2 deletions docs/src/rules/no-fallthrough.md
Expand Up @@ -32,7 +32,7 @@ switch(foo) {
}
```

That works fine when you don't want a fallthrough, but what if the fallthrough is intentional, there is no way to indicate that in the language. It's considered a best practice to always indicate when a fallthrough is intentional using a comment which matches the `/falls?\s?through/i` regular expression:
That works fine when you don't want a fallthrough, but what if the fallthrough is intentional, there is no way to indicate that in the language. It's considered a best practice to always indicate when a fallthrough is intentional using a comment which matches the `/falls?\s?through/i` regular expression but isn't a directive:

```js
switch(foo) {
Expand Down Expand Up @@ -169,7 +169,7 @@ Note that the last `case` statement in these examples does not cause a warning b

This rule has an object option:

* Set the `commentPattern` option to a regular expression string to change the test for intentional fallthrough comment.
* Set the `commentPattern` option to a regular expression string to change the test for intentional fallthrough comment. If the fallthrough comment matches a directive, that takes precedence over `commentPattern`.

* 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.

Expand Down
5 changes: 4 additions & 1 deletion lib/linter/linter.js
Expand Up @@ -18,6 +18,9 @@ const
merge = require("lodash.merge"),
pkg = require("../../package.json"),
astUtils = require("../shared/ast-utils"),
{
directivesPattern
} = require("../shared/directives"),
{
Legacy: {
ConfigOps,
Expand Down Expand Up @@ -377,7 +380,7 @@ function getDirectiveComments(ast, ruleMapper, warnInlineConfig) {
ast.comments.filter(token => token.type !== "Shebang").forEach(comment => {
const { directivePart, justificationPart } = extractDirectiveComment(comment.value);

const match = /^(eslint(?:-env|-enable|-disable(?:(?:-next)?-line)?)?|exported|globals?)(?:\s|$)/u.exec(directivePart);
const match = directivesPattern.exec(directivePart);

if (!match) {
return;
Expand Down
20 changes: 18 additions & 2 deletions lib/rules/no-fallthrough.js
Expand Up @@ -4,12 +4,28 @@
*/
"use strict";

//------------------------------------------------------------------------------
// Requirements
//------------------------------------------------------------------------------

const { directivesPattern } = require("../shared/directives");

//------------------------------------------------------------------------------
// Helpers
//------------------------------------------------------------------------------

const DEFAULT_FALLTHROUGH_COMMENT = /falls?\s?through/iu;

/**
* Checks whether or not a given comment string is really a fallthrough comment and not an ESLint directive.
* @param {string} comment The comment string to check.
* @param {RegExp} fallthroughCommentPattern The regular expression used for checking for fallthrough comments.
* @returns {boolean} `true` if the comment string is truly a fallthrough comment.
*/
function isFallThroughComment(comment, fallthroughCommentPattern) {
return fallthroughCommentPattern.test(comment) && !directivesPattern.test(comment.trim());
}

/**
* Checks whether or not a given case has a fallthrough comment.
* @param {ASTNode} caseWhichFallsThrough SwitchCase node which falls through.
Expand All @@ -25,14 +41,14 @@ function hasFallthroughComment(caseWhichFallsThrough, subsequentCase, context, f
const trailingCloseBrace = sourceCode.getLastToken(caseWhichFallsThrough.consequent[0]);
const commentInBlock = sourceCode.getCommentsBefore(trailingCloseBrace).pop();

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

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

return Boolean(comment && fallthroughCommentPattern.test(comment.value));
return Boolean(comment && isFallThroughComment(comment.value, fallthroughCommentPattern));
}

/**
Expand Down
15 changes: 15 additions & 0 deletions lib/shared/directives.js
@@ -0,0 +1,15 @@
/**
* @fileoverview Common utils for directives.
*
* This file contains only shared items for directives.
* If you make a utility for rules, please see `../rules/utils/ast-utils.js`.
*
* @author gfyoung <https://github.com/gfyoung>
*/
"use strict";

const directivesPattern = /^(eslint(?:-env|-enable|-disable(?:(?:-next)?-line)?)?|exported|globals?)(?:\s|$)/u;

module.exports = {
directivesPattern
};
21 changes: 21 additions & 0 deletions tests/lib/linter/linter.js
Expand Up @@ -4189,6 +4189,27 @@ var a = "test2";
assert.strictEqual(suppressedMessages[0].ruleId, "no-alert");
});

it("reports no problems for no-fallthrough despite comment pattern match", () => {
const code = "switch (foo) { case 0: a(); \n// eslint-disable-next-line no-fallthrough\n case 1: }";
const config = {
reportUnusedDisableDirectives: true,
rules: {
"no-fallthrough": 2
}
};

const messages = linter.verify(code, config, {
filename,
allowInlineConfig: true
});
const suppressedMessages = linter.getSuppressedMessages();

assert.strictEqual(messages.length, 0);

assert.strictEqual(suppressedMessages.length, 1);
assert.strictEqual(suppressedMessages[0].ruleId, "no-fallthrough");
});

describe("autofix", () => {
const alwaysReportsRule = {
create(context) {
Expand Down
13 changes: 13 additions & 0 deletions tests/lib/rules/no-fallthrough.js
Expand Up @@ -63,6 +63,7 @@ ruleTester.run("no-fallthrough", rule, {
"switch (foo) { case 0: try {} finally { break; } default: b(); }",
"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 no-fallthrough\n case 1: }",
{
code: "switch(foo) { case 0: a(); /* no break */ case 1: b(); }",
options: [{
Expand Down Expand Up @@ -297,6 +298,18 @@ ruleTester.run("no-fallthrough", rule, {
column: 34
}
]
},
{
code: "switch (foo) { case 0: a(); \n// eslint-enable no-fallthrough\n case 1: }",
options: [{}],
errors: [
{
messageId: "case",
type: "SwitchCase",
line: 3,
column: 2
}
]
}
]
});

0 comments on commit b4f8329

Please sign in to comment.