add rule no-same-line-conditional

README.md

@@ -30,6 +30,7 @@ Code Smells, or maintainability issues, are raised for places of code which migh
 * Functions should not have identical implementations ([`no-identical-functions`])
 * Boolean checks should not be inverted ([`no-inverted-boolean-check`])
 * Boolean literals should not be redundant ([`no-redundant-boolean`])
+* Conditionals should start on new lines ([`no-same-line-conditional`])
 * "switch" statements should have at least 3 "case" clauses ([`no-small-switch`])
 * "catch" clauses should do more than rethrow ([`no-useless-catch`])
 * Local variables should not be declared and then immediately returned or thrown ([`prefer-immediate-return`]) (:wrench: *fixable*)
@@ -51,6 +52,7 @@ Code Smells, or maintainability issues, are raised for places of code which migh
 [`no-inverted-boolean-check`]: ./docs/rules/no-inverted-boolean-check.md
 [`no-one-iteration-loop`]: ./docs/rules/no-one-iteration-loop.md
 [`no-redundant-boolean`]: ./docs/rules/no-redundant-boolean.md
+[`no-same-line-conditional`]: ./docs/rules/no-same-line-conditiona.md
 [`no-small-switch`]: ./docs/rules/no-small-switch.md
 [`no-use-of-empty-return-value`]: ./docs/rules/no-use-of-empty-return-value.md
 [`no-useless-catch`]: ./docs/rules/no-useless-catch.md

docs/rules/no-same-line-conditional.md

@@ -0,0 +1,35 @@
+# no-same-line-conditional
+
+Code is clearest when each statement has its own line. Nonetheless, it is a common pattern to combine on the same line an `if` and its resulting *then* statement. However, when an `if` is placed on the same line as the closing `}` from a preceding *then*, *else* or *else if* part, it is either an error - `else` is missing - or the invitation to a future error as maintainers fail to understand that the two statements are unconnected.
+
+## Noncompliant Code Example
+
+```javascript
+if (condition1) {
+  // ...
+} if (condition2) {  // Noncompliant
+  //...
+}
+```
+
+## Compliant Solution
+
+```javascript
+if (condition1) {
+  // ...
+} else if (condition2) {
+  //...
+}
+```
+
+Or
+
+```javascript
+if (condition1) {
+  // ...
+}
+
+if (condition2) {
+  //...
+}
+```

ruling/snapshots/no-same-line-conditional

@@ -0,0 +1,8 @@
+src/brackets/src/extensions/default/JavaScriptCodeHints/thirdparty/requirejs/require.js: 11,12,20
+src/three.js/src/renderers/webgl/WebGLRenderLists.js: 37
+src/vue/packages/vue-server-renderer/basic.js: 5095
+src/vue/packages/vue-server-renderer/build.js: 4838
+src/vue/packages/vue-template-compiler/browser.js: 4337
+src/vue/packages/vue-template-compiler/build.js: 3939
+src/vue/packages/weex-template-compiler/build.js: 3356
+src/vue/src/compiler/codegen/index.js: 438

src/index.ts

@@ -34,6 +34,7 @@ const sonarjsRules: [string, Linter.RuleLevel][] = [
   ["no-inverted-boolean-check", "error"],
   ["no-one-iteration-loop", "error"],
   ["no-redundant-boolean", "error"],
+  ["no-same-line-conditional", "error"],
   ["no-small-switch", "error"],
   ["no-use-of-empty-return-value", "error"],
   ["no-useless-catch", "error"],

src/rules/no-same-line-conditional.ts

@@ -0,0 +1,84 @@
+/*
+ * eslint-plugin-sonarjs
+ * Copyright (C) 2018 SonarSource SA
+ * mailto:info AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+// https://jira.sonarsource.com/browse/RSPEC-3972
+
+import { AST, Rule } from "eslint";
+import * as estree from "estree";
+import { toEncodedMessage } from "../utils/locations";
+
+interface SiblingIfStatement {
+  first: estree.IfStatement;
+  following: estree.IfStatement;
+}
+
+const rule: Rule.RuleModule = {
+  meta: {
+    schema: [
+      {
+        // internal parameter
+        enum: ["sonar-runtime"],
+      },
+    ],
+  },
+  create(context: Rule.RuleContext) {
+    function checkStatements(statements: estree.Node[]) {
+      const sourceCode = context.getSourceCode();
+      const siblingIfStatements = getSiblingIfStatements(statements);
+
+      siblingIfStatements.forEach(siblingIfStatement => {
+        const precedingIf = siblingIfStatement.first;
+        const followingIf = siblingIfStatement.following;
+        if (
+          !!precedingIf.loc &&
+          !!followingIf.loc &&
+          precedingIf.loc.end.line === followingIf.loc.start.line &&
+          precedingIf.loc.start.line !== followingIf.loc.end.line
+        ) {
+          const precedingIfLastToken = sourceCode.getLastToken(precedingIf) as AST.Token;
+          const followingIfToken = sourceCode.getFirstToken(followingIf) as AST.Token;
+          context.report({
+            message: toEncodedMessage(`Move this "if" to a new line or add the missing "else".`, [
+              precedingIfLastToken,
+            ]),
+            loc: followingIfToken.loc,
+          });
+        }
+      });
+    }
+
+    return {
+      Program: (node: estree.Node) => checkStatements((node as estree.Program).body),
+      BlockStatement: (node: estree.Node) => checkStatements((node as estree.BlockStatement).body),
+      SwitchCase: (node: estree.Node) => checkStatements((node as estree.SwitchCase).consequent),
+    };
+  },
+};
+
+function getSiblingIfStatements(statements: estree.Node[]): SiblingIfStatement[] {
+  return statements.reduce<SiblingIfStatement[]>((siblingsArray, statement, currentIndex) => {
+    const previousStatement = statements[currentIndex - 1];
+    if (statement.type === "IfStatement" && !!previousStatement && previousStatement.type === "IfStatement") {
+      return [{ first: previousStatement, following: statement }, ...siblingsArray];
+    }
+    return siblingsArray;
+  }, []);
+}
+
+export = rule;

src/utils/locations.ts

@@ -115,6 +115,32 @@ export function issueLocation(
   };
 }
 
+export function toEncodedMessage(
+  message: string,
+  secondaryLocationsHolder: Array<AST.Token | estree.Node>,
+  secondaryMessages?: string[],
+  cost?: number,
+): string {
+  const encodedMessage: EncodedMessage = {
+    message,
+    cost,
+    secondaryLocations: secondaryLocationsHolder.map((locationHolder, index) =>
+      toSecondaryLocation(locationHolder, secondaryMessages ? secondaryMessages[index] : undefined),
+    ),
+  };
+  return JSON.stringify(encodedMessage);
+}
+
+function toSecondaryLocation(locationHolder: AST.Token | estree.Node, message?: string): IssueLocation {
+  return {
+    message,
+    column: locationHolder.loc!.start.column,
+    line: locationHolder.loc!.start.line,
+    endColumn: locationHolder.loc!.end.column,
+    endLine: locationHolder.loc!.end.line,
+  };
+}
+
 function getTokenByValue(node: estree.Node, value: string, context: Rule.RuleContext) {
   return context
     .getSourceCode()

tests/rules/no-same-line-conditional.test.ts

@@ -0,0 +1,187 @@
+/*
+ * eslint-plugin-sonarjs
+ * Copyright (C) 2018 SonarSource SA
+ * mailto:info AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+import { RuleTester } from "eslint";
+
+const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 2018, sourceType: "module" } });
+import rule = require("../../src/rules/no-same-line-conditional");
+
+ruleTester.run("Conditionals should start on new lines", rule, {
+  valid: [
+    {
+      code: `
+      if (cond1)
+        if (cond2) {
+          if (cond3) {
+          }
+        }`,
+    },
+    {
+      code: `
+      if (cond1) {
+      } else if (cond2) {
+      } else if (cond3) {
+      }`,
+    },
+    {
+      code: `
+      if (cond1) {
+      }
+      if (cond2) {
+      } else if (cond3) {
+      }`,
+    },
+    {
+      code: `
+      if (cond1)
+        doSomething();
+      if (cond2) {
+      }`,
+    },
+    {
+      code: `foo(); if (cond) bar();`,
+    },
+    {
+      // OK if everything is on one line
+      code: `if (cond1) foo(); if (cond2) bar();`,
+    },
+    {
+      code: `
+      function myFunc() {
+        if (cond1) {
+        } else if (cond2) {
+        } else if (cond3) {
+        }
+      }`,
+    },
+    {
+      code: `
+      switch(x) {
+        case 1:
+          if (cond1) {
+          } else if (cond2) {
+          } else if (cond3) {
+          }
+          break;
+        default:
+          if (cond1) {
+          } else if (cond2) {
+          } else if (cond3) {
+          }
+          break;
+      }`,
+    },
+  ],
+  invalid: [
+    {
+      code: `
+      if (cond1) {
+      } if (cond2) {
+      }`,
+      errors: [
+        {
+          message:
+            '{"message":"Move this \\"if\\" to a new line or add the missing \\"else\\".","secondaryLocations":[{"column":6,"line":3,"endColumn":7,"endLine":3}]}',
+          line: 3,
+          endLine: 3,
+          column: 9,
+          endColumn: 11,
+        },
+      ],
+    },
+    {
+      code: `
+      switch(x) {
+        case 1:
+          if (cond1) {
+          } else if (cond2) {
+          } if (cond3) {
+          }
+          break;
+        default:
+          if (cond1) {
+          } if (cond2) {
+          } else if (cond3) {
+          }
+          break;
+      }`,
+      errors: [
+        {
+          message:
+            '{"message":"Move this \\"if\\" to a new line or add the missing \\"else\\".","secondaryLocations":[{"column":10,"line":6,"endColumn":11,"endLine":6}]}',
+          line: 6,
+          endLine: 6,
+          column: 13,
+          endColumn: 15,
+        },
+        {
+          message:
+            '{"message":"Move this \\"if\\" to a new line or add the missing \\"else\\".","secondaryLocations":[{"column":10,"line":11,"endColumn":11,"endLine":11}]}',
+          line: 11,
+          endLine: 11,
+          column: 13,
+          endColumn: 15,
+        },
+      ],
+    },
+    {
+      code: `
+      if (cond1) {
+      } else if (cond2) {
+      } if (cond3) {
+      }`,
+      errors: [
+        {
+          message:
+            '{"message":"Move this \\"if\\" to a new line or add the missing \\"else\\".","secondaryLocations":[{"column":6,"line":4,"endColumn":7,"endLine":4}]}',
+        },
+      ],
+    },
+    {
+      code: `
+      if (cond1)
+        if (cond2) {
+          if (cond3) {
+          } if (cond4) {
+          }
+        }`,
+      errors: [
+        {
+          message:
+            '{"message":"Move this \\"if\\" to a new line or add the missing \\"else\\".","secondaryLocations":[{"column":10,"line":5,"endColumn":11,"endLine":5}]}',
+        },
+      ],
+    },
+    {
+      code: `
+      function myFunc() {
+        if (cond1) {
+        } else if (cond2) {
+        } if (cond3) {
+        }
+      }`,
+      errors: [
+        {
+          message:
+            '{"message":"Move this \\"if\\" to a new line or add the missing \\"else\\".","secondaryLocations":[{"column":8,"line":5,"endColumn":9,"endLine":5}]}',
+        },
+      ],
+    },
+  ],
+});