Repository: eslint/eslint
Pull request #8149 (2017-02-24 21:02:39)

Fix: padded-blocks autofix problems with comments
**What is the purpose of this pull request? (put an "X" next to item)**

[ ] Documentation update
[X] Bug fix ([template](https://raw.githubusercontent.com/eslint/eslint/master/templates/bug-report.md))
[ ] New rule ([template](https://raw.githubusercontent.com/eslint/eslint/master/templates/rule-proposal.md))
[ ] Changes an existing rule ([template](https://raw.githubusercontent.com/eslint/eslint/master/templates/rule-change-proposal.md))
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

<!--
    If the item you've checked above has a template, please paste the template questions below and answer them. (If this pull request is addressing an issue, you can just paste a link to the issue here instead.)
-->

<!--
    Please ensure your pull request is ready:

    - Read the pull request guide (http://eslint.org/docs/developer-guide/contributing/pull-requests)
    - Include tests for this change
    - Update documentation for this change (if appropriate)
-->

<!--
    The following is required for all pull requests:
-->
Thi is an alternative to https://github.com/eslint/eslint/pull/8145 covering some edge cases.

**What changes did you make? (Give an overview)**
Ignore the tokens and comments starting on the same line as the brace, and repeat until there is a token or comment on a different line.

**Is there anything you'd like reviewers to focus on?**
Review the edge cases with comments spanning multiple lines and where a comment follows (or precedes) another one.



eslintbot: LGTM
mention-bot: @alberto, thanks for your PR! By analyzing the history of the files in this pull request, we identified @vitorbal, @nzakas and @lo1tuma to be potential reviewers.
eslintbot: LGTM

Patch:
diff --git a/docs/rules/nonblock-statement-body-position.md b/docs/rules/nonblock-statement-body-position.md
index ee4cc4726..0a05d2945 100644
--- a/docs/rules/nonblock-statement-body-position.md
+++ b/docs/rules/nonblock-statement-body-position.md
@@ -1,7 +1,5 @@
 # enforce the location of single-line statements (nonblock-statement-body-position)
 
-(fixable) The `--fix` option on the [command line](../user-guide/command-line-interface#fix) automatically fixes problems reported by this rule.
-
 When writing `if`, `else`, `while`, `do-while`, and `for` statements, the body can be a single statement instead of a block. It can be useful to enforce a consistent location for these single statements.
 
 For example, some developers avoid writing code like this:
diff --git a/lib/rules/no-use-before-define.js b/lib/rules/no-use-before-define.js
index 4bb52030b..1a779b86f 100644
--- a/lib/rules/no-use-before-define.js
+++ b/lib/rules/no-use-before-define.js
@@ -166,7 +166,7 @@ module.exports = {
         const options = parseOptions(context.options[0]);
 
         /**
-         * Determines whether a given use-before-define case should be reportedaccording to the options.
+         * Determines whether a given use-before-define case should be reported according to the options.
          * @param {escope.Variable} variable The variable that gets used before being defined
          * @param {escope.Reference} reference The reference to the variable
          * @returns {boolean} `true` if the usage should be reported
diff --git a/lib/rules/padded-blocks.js b/lib/rules/padded-blocks.js
index 27657360e..36036aec4 100644
--- a/lib/rules/padded-blocks.js
+++ b/lib/rules/padded-blocks.js
@@ -90,23 +90,32 @@ module.exports = {
             return node.type === "Line" || node.type === "Block";
         }
 
+        /**
+         * Checks if there is padding between two tokens
+         * @param {Token} first The first token
+         * @param {Token} second The second token
+         * @returns {boolean} True if there is at least a line between the tokens
+         */
+        function isPaddingBetweenTokens(first, second) {
+            return second.loc.start.line - first.loc.end.line >= 2;
+        }
+
+
         /**
          * Checks if the given token has a blank line after it.
          * @param {Token} token The token to check.
          * @returns {boolean} Whether or not the token is followed by a blank line.
          */
-        function isTokenTopPadded(token) {
-            const tokenStartLine = token.loc.start.line,
-                expectedFirstLine = tokenStartLine + 2;
-            let first = token;
+        function getFirstBlockToken(token) {
+            let prev = token,
+                first = token;
 
             do {
+                prev = first;
                 first = sourceCode.getTokenAfter(first, { includeComments: true });
-            } while (isComment(first) && first.loc.start.line === tokenStartLine);
-
-            const firstLine = first.loc.start.line;
+            } while (isComment(first) && first.loc.start.line === prev.loc.end.line);
 
-            return expectedFirstLine <= firstLine;
+            return first;
         }
 
         /**
@@ -114,18 +123,16 @@ module.exports = {
          * @param {Token} token The token to check
          * @returns {boolean} Whether or not the token is preceeded by a blank line
          */
-        function isTokenBottomPadded(token) {
-            const blockEnd = token.loc.end.line,
-                expectedLastLine = blockEnd - 2;
-            let last = token;
+        function getLastBlockToken(token) {
+            let last = token,
+                next = token;
 
             do {
+                next = last;
                 last = sourceCode.getTokenBefore(last, { includeComments: true });
-            } while (isComment(last) && last.loc.end.line === blockEnd);
+            } while (isComment(last) && last.loc.end.line === next.loc.start.line);
 
-            const lastLine = last.loc.end.line;
-
-            return lastLine <= expectedLastLine;
+            return last;
         }
 
         /**
@@ -155,17 +162,21 @@ module.exports = {
          */
         function checkPadding(node) {
             const openBrace = getOpenBrace(node),
+                firstBlockToken = getFirstBlockToken(openBrace),
+                tokenBeforeFirst = sourceCode.getTokenBefore(firstBlockToken, { includeComments: true }),
                 closeBrace = sourceCode.getLastToken(node),
-                blockHasTopPadding = isTokenTopPadded(openBrace),
-                blockHasBottomPadding = isTokenBottomPadded(closeBrace);
+                lastBlockToken = getLastBlockToken(closeBrace),
+                tokenAfterLast = sourceCode.getTokenAfter(lastBlockToken, { includeComments: true }),
+                blockHasTopPadding = isPaddingBetweenTokens(tokenBeforeFirst, firstBlockToken),
+                blockHasBottomPadding = isPaddingBetweenTokens(lastBlockToken, tokenAfterLast);
 
             if (requirePaddingFor(node)) {
                 if (!blockHasTopPadding) {
                     context.report({
                         node,
-                        loc: { line: openBrace.loc.start.line, column: openBrace.loc.start.column },
+                        loc: { line: tokenBeforeFirst.loc.start.line, column: tokenBeforeFirst.loc.start.column },
                         fix(fixer) {
-                            return fixer.insertTextAfter(openBrace, "\n");
+                            return fixer.insertTextAfter(tokenBeforeFirst, "\n");
                         },
                         message: ALWAYS_MESSAGE
                     });
@@ -173,39 +184,34 @@ module.exports = {
                 if (!blockHasBottomPadding) {
                     context.report({
                         node,
-                        loc: { line: closeBrace.loc.end.line, column: closeBrace.loc.end.column - 1 },
+                        loc: { line: tokenAfterLast.loc.end.line, column: tokenAfterLast.loc.end.column - 1 },
                         fix(fixer) {
-                            return fixer.insertTextBefore(closeBrace, "\n");
+                            return fixer.insertTextBefore(tokenAfterLast, "\n");
                         },
                         message: ALWAYS_MESSAGE
                     });
                 }
             } else {
                 if (blockHasTopPadding) {
-                    const nextToken = sourceCode.getTokenAfter(openBrace, { includeComments: true });
 
                     context.report({
                         node,
-                        loc: { line: openBrace.loc.start.line, column: openBrace.loc.start.column },
+                        loc: { line: tokenBeforeFirst.loc.start.line, column: tokenBeforeFirst.loc.start.column },
                         fix(fixer) {
-
-                            // FIXME: The start of this range is sometimes larger than the end.
-                            // https://github.com/eslint/eslint/issues/8116
-                            return fixer.replaceTextRange([openBrace.end, nextToken.start - nextToken.loc.start.column], "\n");
+                            return fixer.replaceTextRange([tokenBeforeFirst.end, firstBlockToken.start - firstBlockToken.loc.start.column], "\n");
                         },
                         message: NEVER_MESSAGE
                     });
                 }
 
                 if (blockHasBottomPadding) {
-                    const previousToken = sourceCode.getTokenBefore(closeBrace, { includeComments: true });
 
                     context.report({
                         node,
-                        loc: { line: closeBrace.loc.end.line, column: closeBrace.loc.end.column - 1 },
+                        loc: { line: tokenAfterLast.loc.end.line, column: tokenAfterLast.loc.end.column - 1 },
                         message: NEVER_MESSAGE,
                         fix(fixer) {
-                            return fixer.replaceTextRange([previousToken.end, closeBrace.start - closeBrace.loc.start.column], "\n");
+                            return fixer.replaceTextRange([lastBlockToken.end, tokenAfterLast.start - tokenAfterLast.loc.start.column], "\n");
                         }
                     });
                 }


Test patch:
diff --git a/tests/lib/rules/padded-blocks.js b/tests/lib/rules/padded-blocks.js
index 5281172a2..c9208183b 100644
--- a/tests/lib/rules/padded-blocks.js
+++ b/tests/lib/rules/padded-blocks.js
@@ -30,6 +30,11 @@ ruleTester.run("padded-blocks", rule, {
         { code: "{\n\na()\n//comment\n\n}" },
         { code: "{\n\na = 1\n\n}" },
         { code: "{//comment\n\na();\n\n}" },
+        { code: "{ /* comment */\n\na();\n\n}" },
+        { code: "{ /* comment \n */\n\na();\n\n}" },
+        { code: "{ /* comment \n */ /* another comment \n */\n\na();\n\n}" },
+        { code: "{ /* comment \n */ /* another comment \n */\n\na();\n\n/* comment \n */ /* another comment \n */}" },
+
         { code: "{\n\na();\n\n/* comment */ }" },
         { code: "{\n\na();\n\n/* comment */ }", options: ["always"] },
         { code: "{\n\na();\n\n/* comment */ }", options: [{ blocks: "always" }] },
@@ -85,6 +90,17 @@ ruleTester.run("padded-blocks", rule, {
                 }
             ]
         },
+        {
+            code: "{ //comment\na();\n\n}",
+            output: "{ //comment\n\na();\n\n}",
+            errors: [
+                {
+                    message: ALWAYS_MESSAGE,
+                    line: 1,
+                    column: 3
+                }
+            ]
+        },
         {
             code: "{\n\na();\n//comment\n}",
             output: "{\n\na();\n//comment\n\n}",
@@ -466,9 +482,39 @@ ruleTester.run("padded-blocks", rule, {
         },
         {
             code: "function foo() { // a\n\n  b;\n}",
-            output: "function foo() { // a\n\n  b;\n}",
+            output: "function foo() { // a\n  b;\n}",
+            options: ["never"],
+            errors: [NEVER_MESSAGE]
+        },
+        {
+            code: "function foo() { /* a\n */\n\n  bar;\n}",
+            output: "function foo() { /* a\n */\n  bar;\n}",
             options: ["never"],
             errors: [NEVER_MESSAGE]
+        },
+        {
+            code: "function foo() {\n\n  bar;\n/* a\n */}",
+            output: "function foo() {\n\n  bar;\n\n/* a\n */}",
+            options: ["always"],
+            errors: [ALWAYS_MESSAGE]
+        },
+        {
+            code: "function foo() { /* a\n */\n/* b\n */\n  bar;\n}",
+            output: "function foo() { /* a\n */\n\n/* b\n */\n  bar;\n\n}",
+            options: ["always"],
+            errors: [ALWAYS_MESSAGE, ALWAYS_MESSAGE]
+        },
+        {
+            code: "function foo() { /* a\n */ /* b\n */\n  bar;\n}",
+            output: "function foo() { /* a\n */ /* b\n */\n\n  bar;\n\n}",
+            options: ["always"],
+            errors: [ALWAYS_MESSAGE, ALWAYS_MESSAGE]
+        },
+        {
+            code: "function foo() { /* a\n */ /* b\n */\n  bar;\n/* c\n *//* d\n */}",
+            output: "function foo() { /* a\n */ /* b\n */\n\n  bar;\n\n/* c\n *//* d\n */}",
+            options: ["always"],
+            errors: [ALWAYS_MESSAGE, ALWAYS_MESSAGE]
         }
     ]
 });