[bug] fixed ter-newline-after-var rule (closes #355, closes #366)

src/rules/terNewlineAfterVarRule.ts

@@ -2,13 +2,41 @@ import * as ts from 'typescript';
 import * as Lint from 'tslint';
 
 const RULE_NAME: string = 'ter-newline-after-var';
+const EXPECTED_BLANK_LINE_MESSAGE: string = 'Expected blank line after variable declarations.';
+const UNEXPECTED_BLANK_LINE_MESSAGE: string = 'Unexpected blank line after variable declarations.';
+const enum SymbolCodes {
+  TAB = 9,
+  NEWLINE = 10,
+  CARRIAGE_RETURN = 13,
+  WHITESPACE = 32
+}
+const enum Symbols {
+  NEWLINE = '\n'
+}
 
 interface ITerNewlineAfterVarOptions {
   always: boolean;
 }
 
-const EXPECTED_BLANK_LINE_MESSAGE: string = 'Expected blank line after variable declarations.';
-const UNEXPECTED_BLANK_LINE_MESSAGE: string = 'Unexpected blank line after variable declarations.';
+function formatOptions ([alwaysOrNever]: string[]): ITerNewlineAfterVarOptions {
+  return {
+    always: alwaysOrNever !== 'never'
+  };
+}
+
+function getNextSiblingNode (node: ts.Node): ts.Node|void {
+  let breakOnSibling: boolean = false;
+
+  return ts.forEachChild(node.parent, (sibling: ts.Node): ts.Node|void => {
+    if (breakOnSibling) {
+
+      // stop looking for a next node
+      return sibling;
+    }
+
+    breakOnSibling = sibling === node;
+  });
+}
 
 export class Rule extends Lint.Rules.AbstractRule {
   public static metadata: Lint.IRuleMetadata = {
@@ -48,101 +76,108 @@ export class Rule extends Lint.Rules.AbstractRule {
     type: 'style'
   };
 
-  private formatOptions ([alwaysOrNever]: string[]): ITerNewlineAfterVarOptions {
-    return {
-      always: alwaysOrNever !== 'never'
-    };
-  }
-
   public apply (sourceFile: ts.SourceFile): Lint.RuleFailure[] {
-    const opt = this.formatOptions(this.ruleArguments);
+    const opt = formatOptions(this.ruleArguments);
     const walker = new RuleWalker(sourceFile, this.ruleName, opt);
+
     return this.applyWithWalker(walker);
   }
 }
 
 class RuleWalker extends Lint.AbstractWalker<ITerNewlineAfterVarOptions> {
-  // last variable statement node
-  private lastVariableStatementNode: ts.Node|undefined;
-  private sourceFileText: string;
-
   public walk (sourceFile: ts.SourceFile) {
-    this.sourceFileText = sourceFile.getFullText();
-
-    const onNode = (node: ts.Node): void => {
-      const { lastVariableStatementNode, sourceFileText } = this;
-
-      // save the variable statement
+    const isNewLineAlwaysRequired: boolean = this.options.always;
+    const sourceFileText: string = sourceFile.getFullText();
+    const onNode = (node: ts.Node): ts.Node|void => {
       if (node.kind === ts.SyntaxKind.VariableStatement) {
-        this.lastVariableStatementNode = node;
-        return;
-      }
-
-      if (node.kind === ts.SyntaxKind.EndOfFileToken) {
-        this.lastVariableStatementNode = undefined;
-        return;
-      }
-
-      if (lastVariableStatementNode) {
-        const unexpectedLineFixes: Lint.Replacement[] = [];
-        const expectedLineFixes: Lint.Replacement[] = [];
-        const isNewLineRequired: boolean = this.options.always;
-        let expectedLinePos: number = lastVariableStatementNode.end;
-        let newLinesCount: number = 0;
+        const nextSibling: ts.Node|void = getNextSiblingNode(node);
+        const nextSiblingKind: number|void = nextSibling && nextSibling.kind;
+
+        if (
+          !(isNewLineAlwaysRequired && nextSiblingKind === ts.SyntaxKind.VariableStatement) &&
+
+          // prevent a conflict with "eofline" rule
+          !(nextSibling && nextSiblingKind === ts.SyntaxKind.EndOfFileToken)
+        ) {
+          const isNewLineRequired: boolean = isNewLineAlwaysRequired && nextSiblingKind !== ts.SyntaxKind.VariableStatement;
+          const unexpectedLineFixes: Lint.Replacement[] = [];
+          const expectedLineFixes: Lint.Replacement[] = [];
+          const parentNodeEnd: number = node.parent.end;
+          const nodeEnd: number = node.end;
+          const trailingComments: ts.CommentRange[]|undefined = ts.getTrailingCommentRanges(
+            sourceFileText.slice(nodeEnd, parentNodeEnd),
+            0
+          );
+          const lastTrailingComment: ts.CommentRange|undefined = trailingComments && trailingComments.pop();
+          const newLinesSearchStart: number = lastTrailingComment
+            ? nodeEnd + lastTrailingComment.end
+            : nextSibling
+              ? nextSibling.pos
+              : node.end;
+          const newLinesSearchEnd: number = nextSibling ? nextSibling.getStart() : parentNodeEnd;
+          let expectedLinePos: number = newLinesSearchStart;
+          let newLinesCount: number = 0;
+
+          if (isNewLineRequired && newLinesSearchStart === newLinesSearchEnd) {
+            expectedLineFixes.push(Lint.Replacement.appendText(expectedLinePos, Symbols.NEWLINE));
+          }
 
-        for (let i = lastVariableStatementNode.end; i < node.end; i++) {
-          const code: number = sourceFileText.charCodeAt(i);
+          for (let i = newLinesSearchStart; i <= newLinesSearchEnd; i++) {
+            const code: number = sourceFileText.charCodeAt(i);
 
-          if (code === 10) {
-            newLinesCount++;
+            if (code === SymbolCodes.NEWLINE) {
+              newLinesCount++;
 
-            if (!isNewLineRequired && newLinesCount > 1) {
-              unexpectedLineFixes.push(Lint.Replacement.deleteText(i, 1));
-            }
-          } else if (code !== 9 && code !== 13 && code !== 32) {
-            const leadingComments: ts.CommentRange[]|undefined = ts.getLeadingCommentRanges(
-              `\n${ sourceFileText.slice(i) }`,
-              0
-            );
-            const lastLeadingComment: ts.CommentRange|undefined = leadingComments && leadingComments.pop();
-
-            if (lastLeadingComment && (!isNewLineRequired || (isNewLineRequired && newLinesCount < 2))) {
-              newLinesCount = 0;
-              expectedLinePos = i - 1 + lastLeadingComment.end;
-              i = expectedLinePos - 1;
-            } else {
-              if (isNewLineRequired && newLinesCount < 2) {
-                expectedLineFixes.push(Lint.Replacement.appendText(expectedLinePos, '\n'));
+              if (!isNewLineRequired && newLinesCount > 1) {
+                unexpectedLineFixes.push(Lint.Replacement.deleteText(i, 1));
+              }
+            } else if (
+              code !== SymbolCodes.TAB &&
+              code !== SymbolCodes.CARRIAGE_RETURN &&
+              code !== SymbolCodes.WHITESPACE
+            ) {
+              const leadingComments: ts.CommentRange[]|undefined = ts.getLeadingCommentRanges(
+                `\n${ sourceFileText.slice(i, newLinesSearchEnd) }`,
+                0
+              );
+              const lastLeadingComment: ts.CommentRange|undefined = leadingComments && leadingComments.pop();
+
+              if (lastLeadingComment && (!isNewLineRequired || (isNewLineRequired && newLinesCount < 2))) {
+                newLinesCount = 0;
+                expectedLinePos = i - 1 + lastLeadingComment.end;
+                i = expectedLinePos - 1;
+              } else {
+                if (isNewLineRequired && newLinesCount < 2) {
+                  expectedLineFixes.push(Lint.Replacement.appendText(expectedLinePos, Symbols.NEWLINE));
+                }
+
+                break;
               }
-
-              break;
             }
           }
-        }
 
-        if (isNewLineRequired && expectedLineFixes[0]) {
-          // add the an empty line after previous node
-          this.addFailureAt(
-            lastVariableStatementNode.getStart(),
-            1,
-            EXPECTED_BLANK_LINE_MESSAGE,
-            expectedLineFixes
-          );
-        } else if (unexpectedLineFixes[0]) {
-          this.addFailureAt(
-            lastVariableStatementNode.getStart(),
-            1,
-            UNEXPECTED_BLANK_LINE_MESSAGE,
-            unexpectedLineFixes
-          );
+          if (isNewLineRequired && expectedLineFixes[0]) {
+            // add the an empty line after previous node
+            this.addFailureAt(
+              node.getStart(),
+              1,
+              EXPECTED_BLANK_LINE_MESSAGE,
+              expectedLineFixes
+            );
+          } else if (unexpectedLineFixes[0]) {
+            this.addFailureAt(
+              node.getStart(),
+              1,
+              UNEXPECTED_BLANK_LINE_MESSAGE,
+              unexpectedLineFixes
+            );
+          }
         }
-
-        this.lastVariableStatementNode = undefined;
       }
 
-      return ts.forEachChild(node, onNode);
+      ts.forEachChild(node, onNode);
     };
 
-    return ts.forEachChild(sourceFile, onNode);
+    ts.forEachChild(sourceFile, onNode);
   }
 }

src/test/rules/terNewlineAfterVarRuleTests.ts

@@ -20,6 +20,29 @@ function expecting (errors: ['expectedBlankLine' | 'unexpectedBlankLine', number
 }
 
 ruleTester.addTestGroup('always', 'should always require an empty line after variable declarations ', [
+  {
+    code: dedent`
+      const foo1 = 1;
+
+      const bar1 = 2; // Inline comment
+
+      const foo2 = 3; /* Multiline
+      comment */
+
+      const bar2 = 4;
+      `,
+    output: dedent`
+      const foo1 = 1;
+
+      const bar1 = 2; // Inline comment
+
+      const foo2 = 3; /* Multiline
+      comment */
+
+      const bar2 = 4;
+      `,
+    options: []
+  },
   {
     code: dedent`
       var greet = "hello,",
@@ -102,7 +125,6 @@ ruleTester.addTestGroup('always', 'should always require an empty line after var
 
       console.log(greet, name);
       var greet = "hello,";
-
       var name = "world";
       `,
     options: ['always']
@@ -182,10 +204,77 @@ ruleTester.addTestGroup('always', 'should always require an empty line after var
     errors: expecting([
       ['expectedBlankLine', 1]
     ])
+  },
+  {
+    code: dedent`
+     const myFunc = () => {
+      const x = 2;
+      const y = 3;
+      return x + y;
+     }`,
+    output: dedent`
+     const myFunc = () => {
+      const x = 2;
+      const y = 3;
+
+      return x + y;
+     }`,
+    options: ['always'],
+    errors: expecting([
+      ['expectedBlankLine', 3]
+    ])
+  },
+  {
+    code: dedent`
+     let pendingStatus: string[] = []; // contains all pending checks from travis as multiple are sent
+
+     /**
+      * @param  {string} s The commit hash string
+      * @return {string}   Returns formatted commit hash string
+      */
+     function fmt_url(s: string): string {
+       return \`\x0302\x1F\${s}\x0F\`;
+     }`,
+    output: dedent`
+     let pendingStatus: string[] = []; // contains all pending checks from travis as multiple are sent
+
+     /**
+      * @param  {string} s The commit hash string
+      * @return {string}   Returns formatted commit hash string
+      */
+     function fmt_url(s: string): string {
+       return \`\x0302\x1F\${s}\x0F\`;
+     }`,
+    options: ['always']
   }
 ]);
 
 ruleTester.addTestGroup('never', 'should disallow empty lines after variable declarations ', [
+  {
+    code: dedent`
+      const foo1 = 1;
+
+      const bar1 = 2; // Inline comment
+
+      const foo2 = 3; /* Multiline
+      comment */
+
+      const bar2 = 4;
+      `,
+    output: dedent`
+      const foo1 = 1;
+      const bar1 = 2; // Inline comment
+      const foo2 = 3; /* Multiline
+      comment */
+      const bar2 = 4;
+      `,
+    options: ['never'],
+    errors: expecting([
+      ['unexpectedBlankLine', 1],
+      ['unexpectedBlankLine', 3],
+      ['unexpectedBlankLine', 5]
+    ])
+  },
   {
     code: dedent`
       var greet = "hello,",

src/test/rules/terNoSelfCompareRuleTests.ts

@@ -4,7 +4,7 @@ const ruleTester = new RuleTester('ter-no-self-compare');
 
 // Change this function to better test the rule. In some cases the message never changes so we
 // can avoid passing it in. See other rule tests for examples.
-function expecting(errors: [number, number, number][]): Failure[] {
+function expecting(errors: [number, number, number, number?][]): Failure[] {
   return errors.map((err) => {
     return {
       failure: 'Comparing to itself is potentially pointless.',