Added unnecessary-else Rule (#4502)

* Added no-unnecessary-else Rule

* Changed year to 2019 under License

* Changed name of the rule to 'unnecessary-else'

* Removed comments like valid from test.ts.lint file

* Modified the description, rationale and the FAILURE_STRING

* Added codeExamples

* Changed the Logic and Added new testcases

* Incorporated review comments and Added more testcases

* Revert "Incorporated review comments and Added more testcases"

This reverts commit 2e0825f.

* Incorporated review comments and Added more Testcases

* Modified the logic and error message

* Modified Code Examples

* Fixed linting errors

* Optimized the rule logic

src/configs/all.ts

@@ -153,6 +153,7 @@ export const rules = {
     "unnecessary-constructor": true,
     "use-default-type-parameter": true,
     "use-isnan": true,
+    "unnecessary-else": true,
 
     // Maintainability
 

src/configs/latest.ts

@@ -67,6 +67,7 @@ export const rules = {
     // added in v5.12
     "function-constructor": true,
     "unnecessary-bind": true,
+    "unnecessary-else": true,
 };
 // tslint:enable object-literal-sort-keys
 

src/configuration.ts

@@ -155,42 +155,40 @@ export function findConfigurationPath(
             throw new FatalError(
                 `Could not find config file at: ${path.resolve(suppliedConfigFilePath)}`,
             );
-        } else {
-            return path.resolve(suppliedConfigFilePath);
         }
-    } else {
-        // convert to dir if it's a file or doesn't exist
-        let useDirName = false;
-        try {
-            const stats = fs.statSync(inputFilePath!);
-            if (stats.isFile()) {
-                useDirName = true;
-            }
-        } catch (e) {
-            // throws if file doesn't exist
+        return path.resolve(suppliedConfigFilePath);
+    }
+    // convert to dir if it's a file or doesn't exist
+    let useDirName = false;
+    try {
+        const stats = fs.statSync(inputFilePath!);
+        if (stats.isFile()) {
             useDirName = true;
         }
-        if (useDirName) {
-            inputFilePath = path.dirname(inputFilePath!);
-        }
+    } catch (e) {
+        // throws if file doesn't exist
+        useDirName = true;
+    }
+    if (useDirName) {
+        inputFilePath = path.dirname(inputFilePath!);
+    }
 
-        // search for tslint.json from input file location
-        let configFilePath = findup(CONFIG_FILENAMES, path.resolve(inputFilePath!));
-        if (configFilePath !== undefined) {
-            return configFilePath;
-        }
+    // search for tslint.json from input file location
+    let configFilePath = findup(CONFIG_FILENAMES, path.resolve(inputFilePath!));
+    if (configFilePath !== undefined) {
+        return configFilePath;
+    }
 
-        // search for tslint.json in home directory
-        const homeDir = os.homedir();
-        for (const configFilename of CONFIG_FILENAMES) {
-            configFilePath = path.join(homeDir, configFilename);
-            if (fs.existsSync(configFilePath)) {
-                return path.resolve(configFilePath);
-            }
+    // search for tslint.json in home directory
+    const homeDir = os.homedir();
+    for (const configFilename of CONFIG_FILENAMES) {
+        configFilePath = path.join(homeDir, configFilename);
+        if (fs.existsSync(configFilePath)) {
+            return path.resolve(configFilePath);
         }
-        // no path could be found
-        return undefined;
     }
+    // no path could be found
+    return undefined;
 }
 
 /**
@@ -246,16 +244,15 @@ function findup(filenames: string[], directory: string): string | undefined {
 export function loadConfigurationFromPath(configFilePath?: string, _originalFilePath?: string) {
     if (configFilePath == undefined) {
         return DEFAULT_CONFIG;
-    } else {
-        const resolvedConfigFilePath = resolveConfigurationPath(configFilePath);
-        const rawConfigFile = readConfigurationFile(resolvedConfigFilePath);
-
-        return parseConfigFile(
-            rawConfigFile,
-            path.dirname(resolvedConfigFilePath),
-            readConfigurationFile,
-        );
     }
+    const resolvedConfigFilePath = resolveConfigurationPath(configFilePath);
+    const rawConfigFile = readConfigurationFile(resolvedConfigFilePath);
+
+    return parseConfigFile(
+        rawConfigFile,
+        path.dirname(resolvedConfigFilePath),
+        readConfigurationFile,
+    );
 }
 
 /** Reads the configuration file from disk and parses it as raw JSON, YAML or JS depending on the extension. */
@@ -266,9 +263,8 @@ export function readConfigurationFile(filepath: string): RawConfigFile {
         try {
             if (resolvedConfigFileExt === ".json") {
                 return JSON.parse(stripComments(fileContent)) as RawConfigFile;
-            } else {
-                return yaml.safeLoad(fileContent) as RawConfigFile;
             }
+            return yaml.safeLoad(fileContent) as RawConfigFile;
         } catch (e) {
             const error = e as Error;
             // include the configuration file being parsed in the error since it may differ from the directly referenced config

src/formatterLoader.ts

@@ -29,7 +29,8 @@ export function findFormatter(
 ): FormatterConstructor | undefined {
     if (typeof name === "function") {
         return name;
-    } else if (typeof name === "string") {
+    }
+    if (typeof name === "string") {
         name = name.trim();
         const camelizedName = camelize(`${name}Formatter`);
 
@@ -49,10 +50,9 @@ export function findFormatter(
 
         // else try to resolve as module
         return loadFormatterModule(name);
-    } else {
-        // If an something else is passed as a name (e.g. object)
-        throw new Error(`Name of type ${typeof name} is not supported.`);
     }
+    // If an something else is passed as a name (e.g. object)
+    throw new Error(`Name of type ${typeof name} is not supported.`);
 }
 
 function loadFormatter(

src/language/utils.ts

@@ -79,9 +79,8 @@ export function isBlockScopedVariable(
             parent.kind === ts.SyntaxKind.CatchClause ||
             isBlockScopedVariableDeclarationList(parent)
         );
-    } else {
-        return isBlockScopedVariableDeclarationList(node.declarationList);
     }
+    return isBlockScopedVariableDeclarationList(node.declarationList);
 }
 
 /** @deprecated use `isBlockScopedVariableDeclarationList` and `getDeclarationOfBindingElement` from `tsutils` */
@@ -99,9 +98,8 @@ export function getBindingElementVariableDeclaration(
     while (currentParent.kind !== ts.SyntaxKind.VariableDeclaration) {
         if (currentParent.parent === undefined) {
             return null; // function parameter, no variable declaration
-        } else {
-            currentParent = currentParent.parent;
         }
+        currentParent = currentParent.parent;
     }
     return currentParent as ts.VariableDeclaration;
 }
@@ -147,9 +145,8 @@ export function isAssignment(node: ts.Node) {
             binaryExpression.operatorToken.kind >= ts.SyntaxKind.FirstAssignment &&
             binaryExpression.operatorToken.kind <= ts.SyntaxKind.LastAssignment
         );
-    } else {
-        return false;
     }
+    return false;
 }
 
 /**

src/language/walker/ruleWalker.ts

@@ -66,9 +66,8 @@ export class RuleWalker extends SyntaxWalker implements IWalker {
     public hasOption(option: string): boolean {
         if (this.options !== undefined) {
             return this.options.indexOf(option) !== -1;
-        } else {
-            return false;
         }
+        return false;
     }
 
     /** @deprecated Prefer `addFailureAt` and its variants. */

src/linter.ts

@@ -283,9 +283,8 @@ export class Linter {
         try {
             if (this.program !== undefined && isTypedRule(rule)) {
                 return rule.applyWithProgram(sourceFile, this.program);
-            } else {
-                return rule.apply(sourceFile);
             }
+            return rule.apply(sourceFile);
         } catch (error) {
             if (isError(error) && error.stack !== undefined) {
                 showRuleCrashWarning(error.stack, rule.getOptions().ruleName, sourceFile.fileName);
@@ -323,9 +322,8 @@ export class Linter {
                 throw new FatalError(INVALID_SOURCE_ERROR);
             }
             return sourceFile;
-        } else {
-            return utils.getSourceFile(fileName, source);
         }
+        return utils.getSourceFile(fileName, source);
     }
 }
 

src/rules/callableTypesRule.ts

@@ -115,9 +115,8 @@ function renderSuggestion(
                 parent.name.pos,
                 parent.typeParameters.end + 1,
             )} = ${suggestion}`;
-        } else {
-            return `type ${parent.name.text} = ${suggestion}`;
         }
+        return `type ${parent.name.text} = ${suggestion}`;
     }
     return suggestion.endsWith(";") ? suggestion.slice(0, -1) : suggestion;
 }

src/rules/code-examples/unnecessaryElse.examples.ts

@@ -0,0 +1,76 @@
+/**
+ * @license
+ * Copyright 2019 Palantir Technologies, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import * as Lint from "../../index";
+
+// tslint:disable: object-literal-sort-keys
+export const codeExamples = [
+    {
+        description:
+            'Disallows "else" following "if" blocks ending with "return", "break", "continue" or "throw" statement. ',
+        config: Lint.Utils.dedent`
+            "rules": { "unnecessary-else": true }
+        `,
+        pass: Lint.Utils.dedent`
+            if (someCondition()) {
+                return;
+            }
+            // some code here
+
+            if (someCondition()) {
+                continue;
+            }
+            // some code here
+
+            if (someCondition()) {
+                throw;
+            }
+            // some code here
+
+            if (someCondition()) {
+                break;
+            }
+            // some code here
+
+        `,
+        fail: Lint.Utils.dedent`
+            if (someCondition()) {
+                return;
+            } else {
+                // some code here
+            }
+
+            if (someCondition()) {
+                break;
+            } else {
+                // some code here
+            }
+
+            if (someCondition()) {
+                throw;
+            } else {
+                // some code here
+            }
+
+            if (someCondition()) {
+                continue;
+            } else {
+                // some code here
+            }
+        `,
+    },
+];

src/rules/curlyRule.ts

@@ -172,12 +172,11 @@ class CurlyWalker extends Lint.AbstractWalker<Options> {
                 Lint.Replacement.appendText(statement.pos, " {"),
                 Lint.Replacement.appendText(statement.end, " }"),
             ];
-        } else {
-            const newLine = newLineWithIndentation(node, this.sourceFile);
-            return [
-                Lint.Replacement.appendText(statement.pos, " {"),
-                Lint.Replacement.appendText(statement.end, `${newLine}}`),
-            ];
         }
+        const newLine = newLineWithIndentation(node, this.sourceFile);
+        return [
+            Lint.Replacement.appendText(statement.pos, " {"),
+            Lint.Replacement.appendText(statement.end, `${newLine}}`),
+        ];
     }
 }

src/rules/noBooleanLiteralCompareRule.ts

@@ -89,15 +89,15 @@ function fix(node: ts.BinaryExpression, { negate, expression }: Compare): Lint.F
             : Lint.Replacement.deleteFromTo(node.getStart(), node.right.getStart());
     if (!negate) {
         return deleted;
-    } else if (needsParenthesesForNegate(expression)) {
+    }
+    if (needsParenthesesForNegate(expression)) {
         return [
             deleted,
             Lint.Replacement.appendText(node.getStart(), "!("),
             Lint.Replacement.appendText(node.getEnd(), ")"),
         ];
-    } else {
-        return [deleted, Lint.Replacement.appendText(node.getStart(), "!")];
     }
+    return [deleted, Lint.Replacement.appendText(node.getStart(), "!")];
 }
 
 function needsParenthesesForNegate(node: ts.Expression): boolean {

src/rules/noDefaultImportRule.ts

@@ -104,11 +104,10 @@ export class Rule extends Lint.Rules.AbstractRule {
                     fromModuleConfigOption[fromModulesConfigOptionName],
                 ),
             };
-        } else {
-            return {
-                [fromModulesConfigOptionName]: new RegExp("^\\./|^\\.\\./"),
-            };
         }
+        return {
+            [fromModulesConfigOptionName]: new RegExp("^\\./|^\\.\\./"),
+        };
     }
 }
 

src/rules/noInvalidThisRule.ts

@@ -102,16 +102,15 @@ function walk(ctx: Lint.WalkContext<boolean>): void {
                     ts.forEachChild(node, cb);
                     currentParent = originalParent;
                     return;
-                } else {
-                    currentParent = (node as ts.FunctionLikeDeclaration).parameters.some(
-                        isThisParameter,
-                    )
-                        ? ParentType.BoundRegularFunction
-                        : ParentType.UnboundRegularFunction;
-                    ts.forEachChild(node, cb);
-                    currentParent = originalParent;
-                    return;
                 }
+                currentParent = (node as ts.FunctionLikeDeclaration).parameters.some(
+                    isThisParameter,
+                )
+                    ? ParentType.BoundRegularFunction
+                    : ParentType.UnboundRegularFunction;
+                ts.forEachChild(node, cb);
+                currentParent = originalParent;
+                return;
 
             case ts.SyntaxKind.ThisKeyword:
                 if (!thisAllowedParents.has(currentParent)) {

src/rules/noRestrictedGlobalsRule.ts

@@ -73,14 +73,8 @@ export class Rule extends Lint.Rules.TypedRule {
         const bannedGlobals = new Set(bannedList);
         if (sourceFile.isDeclarationFile) {
             return [];
-        } else {
-            return this.applyWithFunction(
-                sourceFile,
-                walk,
-                bannedGlobals,
-                program.getTypeChecker(),
-            );
         }
+        return this.applyWithFunction(sourceFile, walk, bannedGlobals, program.getTypeChecker());
     }
 }