Repository: eslint/eslint (js)

Issue:
no-fallthrough should allow the comment inside of a block when the case is a single block
**What rule do you want to change?**

no-fallthrough

**Does this change cause the rule to produce more or fewer warnings?**

fewer

**How will the change be implemented? (New option, new default behavior, etc.)?**

new default behavior

**Please provide some example code that this change will affect:**

<!-- Put your code examples here -->
```js
switch (foo) {
  case 1: {
    let x = value;
    console.log(value);
    // falls through
  }
  case 2: {
    doSomething();
  }
}
```

**What does the rule currently do for this code?**

Error

**What will the rule do after it's changed?**

Not error

**Are you willing to submit a pull request to implement this change?**

Yes

---

This has been brought up before, in #9559 and #9080, and with a PR in #11016. On each of those occasions it's been closed automatically without any interest. But those just asked for the new behavior without really explaining why.

I want to make a case the proposed change is **the right behavior**. The `fallthrough` comment is intended to replace a `break`, which means that a reader of the code will be looking for it where they would expect to find the `break`. And the convention when using a braced `case` is that the `break` occurs _within_ the braces, not outside of it. So that is where a reader should expect to find the comment, not (as eslint currently requires) _after_ the braces. The placement required by the current rule actually makes the comment easier to miss.


Pull request:
Fix: allow fallthrough comment inside block (fixes #14701)

#### Prerequisites checklist

- [x] I have read the [contributing guidelines](https://github.com/eslint/eslint/blob/master/CONTRIBUTING.md).

#### What is the purpose of this pull request? (put an "X" next to an item)

[x] Bug fix
[x] Changes an existing rule

**What rule do you want to change?**

no-fallthrough

**Does this change cause the rule to produce more or fewer warnings?**

fewer

**How will the change be implemented? (New option, new default behavior, etc.)?**

new default

**Please provide some example code that this change will affect:**

```js
switch (foo) {
  case 1: {
    let x = value;
    console.log(value);
    // falls through
  }
  case 2: {
    doSomething();
  }
}
```

**What does the rule currently do for this code?**

warn

**What will the rule do after it's changed?**

not warn

#### What changes did you make? (Give an overview)

Now when the case consists of a single BlockStatement the code will look for the fallthrough comment to be the final comment in the case, as well where it looked previously. 

#### Is there anything you'd like reviewers to focus on?

In the situation in question - i.e., when the case consists of exactly one statement, which is a BlockStatement - I've chosen to check for the fallthrough comment in both places. This means that it becomes possible to have a comment after the fallthrough comment, as long as it's after the block, as in

```js
switch (foo) {
  case 1: {
    let x = value;
    console.log(value);
    // falls through
  }

  // some other comment
  case 2: {
    doSomething();
  }
}
```

I think that's correct, personally. However, I'm happy to be more restrictive and say that the fallthrough comment must also be the final comment: that is, to only look in the new place when there are no comments after the block.

Patch:
diff --git a/docs/rules/no-fallthrough.md b/docs/rules/no-fallthrough.md
index 15ca77f66..81b428cf0 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/docs/user-guide/configuring/language-options.md b/docs/user-guide/configuring/language-options.md
index 08b62aad5..eb3fe8a0a 100644
--- a/docs/user-guide/configuring/language-options.md
+++ b/docs/user-guide/configuring/language-options.md
@@ -187,7 +187,7 @@ For ES6 syntax, use `{ "parserOptions": { "ecmaVersion": 6 } }`; for new ES6 glo
 
 Parser options are set in your `.eslintrc.*` file by using the `parserOptions` property. The available options are:
 
-* `ecmaVersion` - set to 3, 5 (default), 6, 7, 8, 9, 10, 11, or 12 to specify the version of ECMAScript syntax you want to use. You can also set to 2015 (same as 6), 2016 (same as 7), 2017 (same as 8), 2018 (same as 9), 2019 (same as 10), 2020 (same as 11), or 2021 (same as 12) to use the year-based naming. You can also set "latest" to use the most recently supported version.
+* `ecmaVersion` - set to 3, 5 (default), 6, 7, 8, 9, 10, 11, or 12 to specify the version of ECMAScript syntax you want to use. You can also set to 2015 (same as 6), 2016 (same as 7), 2017 (same as 8), 2018 (same as 9), 2019 (same as 10), 2020 (same as 11), or 2021 (same as 12) to use the year-based naming.
 * `sourceType` - set to `"script"` (default) or `"module"` if your code is in ECMAScript modules.
 * `ecmaFeatures` - an object indicating which additional language features you'd like to use:
     * `globalReturn` - allow `return` statements in the global scope
@@ -199,7 +199,7 @@ Here's an example `.eslintrc.json` file:
 ```json
 {
     "parserOptions": {
-        "ecmaVersion": "latest",
+        "ecmaVersion": 6,
         "sourceType": "module",
         "ecmaFeatures": {
             "jsx": true
diff --git a/docs/user-guide/integrations.md b/docs/user-guide/integrations.md
index c7130bb9f..34ad2e25b 100644
--- a/docs/user-guide/integrations.md
+++ b/docs/user-guide/integrations.md
@@ -44,6 +44,7 @@
 * [Git Precommit Hook](https://coderwall.com/p/zq8jlq/eslint-pre-commit-hook)
 * [Git pre-commit hook that only lints staged changes](https://gist.github.com/dahjelle/8ddedf0aebd488208a9a7c829f19b9e8)
 * [overcommit Git hook manager](https://github.com/brigade/overcommit)
+* [Mega-Linter](https://nvuillam.github.io/mega-linter): Linters aggregator for CI, [embedding eslint](https://nvuillam.github.io/mega-linter/descriptors/javascript_eslint/)
 
 ## Testing
 
diff --git a/lib/linter/linter.js b/lib/linter/linter.js
index 77682ea95..e94b507b5 100644
--- a/lib/linter/linter.js
+++ b/lib/linter/linter.js
@@ -37,7 +37,6 @@ const
 const debug = require("debug")("eslint:linter");
 const MAX_AUTOFIX_PASSES = 10;
 const DEFAULT_PARSER_NAME = "espree";
-const DEFAULT_ECMA_VERSION = 5;
 const commentParser = new ConfigCommentParser();
 const DEFAULT_ERROR_LOC = { start: { line: 1, column: 0 }, end: { line: 1, column: 1 } };
 
@@ -433,20 +432,10 @@ function getDirectiveComments(filename, ast, ruleMapper, warnInlineConfig) {
 
 /**
  * Normalize ECMAScript version from the initial config
- * @param {Parser} parser The parser which uses this options.
- * @param {number} ecmaVersion ECMAScript version from the initial config
+ * @param  {number} ecmaVersion ECMAScript version from the initial config
  * @returns {number} normalized ECMAScript version
  */
-function normalizeEcmaVersion(parser, ecmaVersion) {
-    if (parser === espree) {
-        if (ecmaVersion === void 0) {
-            return DEFAULT_ECMA_VERSION;
-        }
-
-        if (ecmaVersion === "latest") {
-            return espree.latestEcmaVersion;
-        }
-    }
+function normalizeEcmaVersion(ecmaVersion) {
 
     /*
      * Calculate ECMAScript edition number from official year version starting with
@@ -532,13 +521,12 @@ function normalizeVerifyOptions(providedOptions, config) {
 
 /**
  * Combines the provided parserOptions with the options from environments
- * @param {Parser} parser The parser which uses this options.
+ * @param {string} parserName The parser name which uses this options.
  * @param {ParserOptions} providedOptions The provided 'parserOptions' key in a config
  * @param {Environment[]} enabledEnvironments The environments enabled in configuration and with inline comments
  * @returns {ParserOptions} Resulting parser options after merge
  */
-function resolveParserOptions(parser, providedOptions, enabledEnvironments) {
-
+function resolveParserOptions(parserName, providedOptions, enabledEnvironments) {
     const parserOptionsFromEnv = enabledEnvironments
         .filter(env => env.parserOptions)
         .reduce((parserOptions, env) => merge(parserOptions, env.parserOptions), {});
@@ -554,7 +542,12 @@ function resolveParserOptions(parser, providedOptions, enabledEnvironments) {
         mergedParserOptions.ecmaFeatures = Object.assign({}, mergedParserOptions.ecmaFeatures, { globalReturn: false });
     }
 
-    mergedParserOptions.ecmaVersion = normalizeEcmaVersion(parser, mergedParserOptions.ecmaVersion);
+    /*
+     * TODO: @aladdin-add
+     * 1. for a 3rd-party parser, do not normalize parserOptions
+     * 2. for espree, no need to do this (espree will do it)
+     */
+    mergedParserOptions.ecmaVersion = normalizeEcmaVersion(mergedParserOptions.ecmaVersion);
 
     return mergedParserOptions;
 }
@@ -613,7 +606,7 @@ function getRuleOptions(ruleConfig) {
  */
 function analyzeScope(ast, parserOptions, visitorKeys) {
     const ecmaFeatures = parserOptions.ecmaFeatures || {};
-    const ecmaVersion = parserOptions.ecmaVersion || DEFAULT_ECMA_VERSION;
+    const ecmaVersion = parserOptions.ecmaVersion || 5;
 
     return eslintScope.analyze(ast, {
         ignoreEval: true,
@@ -1130,7 +1123,7 @@ class Linter {
             .map(envName => getEnv(slots, envName))
             .filter(env => env);
 
-        const parserOptions = resolveParserOptions(parser, config.parserOptions || {}, enabledEnvs);
+        const parserOptions = resolveParserOptions(parserName, config.parserOptions || {}, enabledEnvs);
         const configuredGlobals = resolveGlobals(config.globals || {}, enabledEnvs);
         const settings = config.settings || {};
 
diff --git a/lib/rules/no-fallthrough.js b/lib/rules/no-fallthrough.js
index e8016e93e..3b949acd1 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/packages/eslint-config-eslint/default.yml b/packages/eslint-config-eslint/default.yml
index d2b31d9ed..38061e801 100644
--- a/packages/eslint-config-eslint/default.yml
+++ b/packages/eslint-config-eslint/default.yml
@@ -1,3 +1,4 @@
+reportUnusedDisableDirectives: true
 extends:
     - "eslint:recommended"
     - "plugin:node/recommended"


Test patch:
diff --git a/tests/lib/linter/linter.js b/tests/lib/linter/linter.js
index 15d310c9a..e61804d3a 100644
--- a/tests/lib/linter/linter.js
+++ b/tests/lib/linter/linter.js
@@ -11,7 +11,6 @@
 
 const assert = require("chai").assert,
     sinon = require("sinon"),
-    espree = require("espree"),
     esprima = require("esprima"),
     testParsers = require("../../fixtures/parsers/linter-test-parsers");
 
@@ -3494,70 +3493,6 @@ var a = "test2";
         });
 
         describe("ecmaVersion", () => {
-
-            it("should not support ES6 when no ecmaVersion provided", () => {
-                const messages = linter.verify("let x = 0;");
-
-                assert.strictEqual(messages.length, 1);
-            });
-
-            it("the default ECMAScript version is 5", () => {
-                let ecmaVersion = null;
-                const config = { rules: { "ecma-version": 2 } };
-
-                linter.defineRule("ecma-version", context => ({
-                    Program() {
-                        ecmaVersion = context.parserOptions.ecmaVersion;
-                    }
-                }));
-                linter.verify("", config);
-                assert.strictEqual(ecmaVersion, 5);
-            });
-
-            it("supports ECMAScript version 'latest'", () => {
-                const messages = linter.verify("let x = 5 ** 7;", {
-                    parserOptions: { ecmaVersion: "latest" }
-                });
-
-                assert.strictEqual(messages.length, 0);
-            });
-
-            it("the 'latest' is equal to espree.lastEcmaVersion", () => {
-                let ecmaVersion = null;
-                const config = { rules: { "ecma-version": 2 }, parserOptions: { ecmaVersion: "latest" } };
-
-                linter.defineRule("ecma-version", context => ({
-                    Program() {
-                        ecmaVersion = context.parserOptions.ecmaVersion;
-                    }
-                }));
-                linter.verify("", config);
-                assert.strictEqual(ecmaVersion, espree.latestEcmaVersion);
-            });
-
-            it("should pass normalized ecmaVersion to eslint-scope", () => {
-                let blockScope = null;
-
-                linter.defineRule("block-scope", context => ({
-                    BlockStatement() {
-                        blockScope = context.getScope();
-                    }
-                }));
-
-                linter.verify("{}", {
-                    rules: { "block-scope": 2 },
-                    parserOptions: { ecmaVersion: "latest" }
-                });
-
-                assert.strictEqual(blockScope.type, "block");
-
-                linter.verify("{}", {
-                    rules: { "block-scope": 2 },
-                    parserOptions: {} // ecmaVersion defaults to 5
-                });
-                assert.strictEqual(blockScope.type, "global");
-            });
-
             describe("it should properly parse let declaration when", () => {
                 it("the ECMAScript version number is 6", () => {
                     const messages = linter.verify("let x = 5;", {
diff --git a/tests/lib/rules/no-fallthrough.js b/tests/lib/rules/no-fallthrough.js
index e154c31d5..0b98d3293 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
+                }
+            ]
         }
     ]
 });