diff --git a/src/configs/all.ts b/src/configs/all.ts index 53353d216da..5c6c78bb41d 100644 --- a/src/configs/all.ts +++ b/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 diff --git a/src/configs/latest.ts b/src/configs/latest.ts index eae3fa67fb0..9421324f1a0 100644 --- a/src/configs/latest.ts +++ b/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 diff --git a/src/configuration.ts b/src/configuration.ts index 486761c38b7..7ef245161aa 100644 --- a/src/configuration.ts +++ b/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 diff --git a/src/formatterLoader.ts b/src/formatterLoader.ts index 951b145ddda..a45dbfba295 100644 --- a/src/formatterLoader.ts +++ b/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( diff --git a/src/language/utils.ts b/src/language/utils.ts index 9160d82b508..b13acac5966 100644 --- a/src/language/utils.ts +++ b/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; } /** diff --git a/src/language/walker/ruleWalker.ts b/src/language/walker/ruleWalker.ts index f99c56b57a2..a566bc2a249 100644 --- a/src/language/walker/ruleWalker.ts +++ b/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. */ diff --git a/src/linter.ts b/src/linter.ts index 5df8cc47ce0..2d6fa3357c3 100644 --- a/src/linter.ts +++ b/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); } } diff --git a/src/rules/callableTypesRule.ts b/src/rules/callableTypesRule.ts index 5cc3e15285e..3a9f358d381 100644 --- a/src/rules/callableTypesRule.ts +++ b/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; } diff --git a/src/rules/code-examples/unnecessaryElse.examples.ts b/src/rules/code-examples/unnecessaryElse.examples.ts new file mode 100644 index 00000000000..260b14b596e --- /dev/null +++ b/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 + } + `, + }, +]; diff --git a/src/rules/curlyRule.ts b/src/rules/curlyRule.ts index 2b51c2b5df8..01c1a0e82ab 100644 --- a/src/rules/curlyRule.ts +++ b/src/rules/curlyRule.ts @@ -172,12 +172,11 @@ class CurlyWalker extends Lint.AbstractWalker { 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}}`), + ]; } } diff --git a/src/rules/noBooleanLiteralCompareRule.ts b/src/rules/noBooleanLiteralCompareRule.ts index bbafa974c80..deed9492477 100644 --- a/src/rules/noBooleanLiteralCompareRule.ts +++ b/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 { diff --git a/src/rules/noDefaultImportRule.ts b/src/rules/noDefaultImportRule.ts index dc7c629df59..aded2c8b39d 100644 --- a/src/rules/noDefaultImportRule.ts +++ b/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("^\\./|^\\.\\./"), + }; } } diff --git a/src/rules/noInvalidThisRule.ts b/src/rules/noInvalidThisRule.ts index 98c925b0cb1..eb2e602603e 100644 --- a/src/rules/noInvalidThisRule.ts +++ b/src/rules/noInvalidThisRule.ts @@ -102,16 +102,15 @@ function walk(ctx: Lint.WalkContext): 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)) { diff --git a/src/rules/noRestrictedGlobalsRule.ts b/src/rules/noRestrictedGlobalsRule.ts index 154c2788a3f..17534f0cee7 100644 --- a/src/rules/noRestrictedGlobalsRule.ts +++ b/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()); } } diff --git a/src/rules/noSparseArraysRule.ts b/src/rules/noSparseArraysRule.ts index 5654ddeb75e..6b93d1ea479 100644 --- a/src/rules/noSparseArraysRule.ts +++ b/src/rules/noSparseArraysRule.ts @@ -54,9 +54,8 @@ function walk(ctx: Lint.WalkContext): void { // Ignore LHS of assignments. traverseExpressionsInLHS(node.left, cb); return cb(node.right); - } else { - return ts.forEachChild(node, cb); } + return ts.forEachChild(node, cb); } for (const element of node.elements) { diff --git a/src/rules/noUnnecessaryQualifierRule.ts b/src/rules/noUnnecessaryQualifierRule.ts index e4bd28d3c23..ca786500875 100644 --- a/src/rules/noUnnecessaryQualifierRule.ts +++ b/src/rules/noUnnecessaryQualifierRule.ts @@ -122,7 +122,8 @@ function walk(ctx: Lint.WalkContext, checker: ts.TypeChecker): void { const symbolDeclarations = symbol.getDeclarations(); if (symbolDeclarations === undefined) { return false; - } else if (symbolDeclarations.some(decl => namespacesInScope.some(ns => ns === decl))) { + } + if (symbolDeclarations.some(decl => namespacesInScope.some(ns => ns === decl))) { return true; } diff --git a/src/rules/noUnusedExpressionRule.ts b/src/rules/noUnusedExpressionRule.ts index 0a9a7883909..7bf0eb0a1c0 100644 --- a/src/rules/noUnusedExpressionRule.ts +++ b/src/rules/noUnusedExpressionRule.ts @@ -91,10 +91,12 @@ function walk(ctx: Lint.WalkContext) { if (checking) { if (isParenthesizedExpression(node) || isVoidExpression(node)) { return cb(node.expression); - } else if (isConditionalExpression(node)) { + } + if (isConditionalExpression(node)) { noCheck(node.condition, cb); return both(node.whenTrue, node.whenFalse); - } else if (isBinaryExpression(node)) { + } + if (isBinaryExpression(node)) { switch (node.operatorToken.kind) { case ts.SyntaxKind.CommaToken: if (isIndirectEval(node)) { @@ -119,7 +121,8 @@ function walk(ctx: Lint.WalkContext) { } allowFastNullChecks = true; return false; - } else if (isVoidExpression(node)) { + } + if (isVoidExpression(node)) { // allow `void 0` and `void(0)` if ( !isLiteralZero( @@ -131,7 +134,8 @@ function walk(ctx: Lint.WalkContext) { check(node.expression); } return false; - } else if ( + } + if ( isBinaryExpression(node) && node.operatorToken.kind === ts.SyntaxKind.CommaToken && !isIndirectEval(node) @@ -167,9 +171,8 @@ function walk(ctx: Lint.WalkContext) { if (cb(one)) { if (cb(two)) { return true; - } else { - ctx.addFailureAtNode(one, Rule.FAILURE_STRING); } + ctx.addFailureAtNode(one, Rule.FAILURE_STRING); } else if (cb(two)) { ctx.addFailureAtNode(two, Rule.FAILURE_STRING); } diff --git a/src/rules/noUnusedVariableRule.ts b/src/rules/noUnusedVariableRule.ts index 75f222a250b..ebd23020175 100644 --- a/src/rules/noUnusedVariableRule.ts +++ b/src/rules/noUnusedVariableRule.ts @@ -299,9 +299,8 @@ function addImportSpecifierFailures( const lineStarts = ctx.sourceFile.getLineStarts(); if (nextLine < lineStarts.length) { return lineStarts[nextLine]; - } else { - return position; } + return position; } }); @@ -366,12 +365,12 @@ function getImplicitType(node: ts.Node, checker: ts.TypeChecker): ts.Type | unde (utils.isBindingElement(node) && node.name.kind === ts.SyntaxKind.Identifier) ) { return checker.getTypeAtLocation(node); - } else if (utils.isSignatureDeclaration(node) && node.type === undefined) { + } + if (utils.isSignatureDeclaration(node) && node.type === undefined) { const sig = checker.getSignatureFromDeclaration(node); return sig === undefined ? undefined : sig.getReturnType(); - } else { - return undefined; } + return undefined; } type ImportLike = ts.ImportDeclaration | ts.ImportEqualsDeclaration; @@ -409,58 +408,56 @@ function findImports( if (i.kind === ts.SyntaxKind.ImportEqualsDeclaration) { return [i.name]; - } else { - if (i.importClause === undefined) { - // Error node - return undefined; - } + } + if (i.importClause === undefined) { + // Error node + return undefined; + } - const { name: defaultName, namedBindings } = i.importClause; - if ( - namedBindings !== undefined && - namedBindings.kind === ts.SyntaxKind.NamespaceImport - ) { - return [namedBindings.name]; - } + const { name: defaultName, namedBindings } = i.importClause; + if (namedBindings !== undefined && namedBindings.kind === ts.SyntaxKind.NamespaceImport) { + return [namedBindings.name]; + } - // Starting from TS2.8, when all imports in an import node are not used, - // TS emits only 1 diagnostic object for the whole line as opposed - // to the previous behavior of outputting a diagnostic with kind == 6192 - // (UnusedKind.VARIABLE_OR_PARAMETER) for every unused import. - // From TS2.8, in the case of none of the imports in a line being used, - // the single diagnostic TS outputs are different between the 1 import - // and 2+ imports cases: - // - 1 import in node: - // - diagnostic has kind == 6133 (UnusedKind.VARIABLE_OR_PARAMETER) - // - the text range is the whole node (`import { ... } from "..."`) - // whereas pre-TS2.8, the text range was for the import node. so - // `name.getStart()` won't equal `pos` like in pre-TS2.8 - // - 2+ imports in node: - // - diagnostic has kind == 6192 (UnusedKind.DECLARATION) - // - we know that all of these are unused - if (kind === UnusedKind.DECLARATION) { - const imp: ts.Identifier[] = []; - if (defaultName !== undefined) { - imp.push(defaultName); - } - if (namedBindings !== undefined) { - imp.push(...namedBindings.elements.map(el => el.name)); - } - return imp.length > 0 ? imp : undefined; - } else if ( - defaultName !== undefined && - (isInRange(defaultName, pos) || namedBindings === undefined) // defaultName is the only option - ) { - return [defaultName]; - } else if (namedBindings !== undefined) { - if (namedBindings.elements.length === 1) { - return [namedBindings.elements[0].name]; - } + // Starting from TS2.8, when all imports in an import node are not used, + // TS emits only 1 diagnostic object for the whole line as opposed + // to the previous behavior of outputting a diagnostic with kind == 6192 + // (UnusedKind.VARIABLE_OR_PARAMETER) for every unused import. + // From TS2.8, in the case of none of the imports in a line being used, + // the single diagnostic TS outputs are different between the 1 import + // and 2+ imports cases: + // - 1 import in node: + // - diagnostic has kind == 6133 (UnusedKind.VARIABLE_OR_PARAMETER) + // - the text range is the whole node (`import { ... } from "..."`) + // whereas pre-TS2.8, the text range was for the import node. so + // `name.getStart()` won't equal `pos` like in pre-TS2.8 + // - 2+ imports in node: + // - diagnostic has kind == 6192 (UnusedKind.DECLARATION) + // - we know that all of these are unused + if (kind === UnusedKind.DECLARATION) { + const imp: ts.Identifier[] = []; + if (defaultName !== undefined) { + imp.push(defaultName); + } + if (namedBindings !== undefined) { + imp.push(...namedBindings.elements.map(el => el.name)); + } + return imp.length > 0 ? imp : undefined; + } + if ( + defaultName !== undefined && + (isInRange(defaultName, pos) || namedBindings === undefined) // defaultName is the only option + ) { + return [defaultName]; + } + if (namedBindings !== undefined) { + if (namedBindings.elements.length === 1) { + return [namedBindings.elements[0].name]; + } - for (const element of namedBindings.elements) { - if (isInRange(element, pos)) { - return [element.name]; - } + for (const element of namedBindings.elements) { + if (isInRange(element, pos)) { + return [element.name]; } } } diff --git a/src/rules/orderedImportsRule.ts b/src/rules/orderedImportsRule.ts index 8316b542c36..29bde1f0ee0 100644 --- a/src/rules/orderedImportsRule.ts +++ b/src/rules/orderedImportsRule.ts @@ -266,13 +266,13 @@ function parseOptions(ruleArguments: any[]): Options { const compiledGroups = groups.map((g, idx) => { if (typeof g === "string") { return { name: `/${g}/`, match: new RegExp(g), order: idx }; - } else { - return { - match: new RegExp(g.match), - name: g.name !== undefined ? g.name : `/${g.match}/`, - order: g.order, - }; } + + return { + match: new RegExp(g.match), + name: g.name !== undefined ? g.name : `/${g.match}/`, + order: g.order, + }; }); return { @@ -707,7 +707,8 @@ function flipCase(str: string): string { .map(char => { if (char >= "a" && char <= "z") { return char.toUpperCase(); - } else if (char >= "A" && char <= "Z") { + } + if (char >= "A" && char <= "Z") { return char.toLowerCase(); } return char; @@ -735,11 +736,14 @@ function compare(a: string, b: string): 0 | 1 | -1 { } if (isLow(a) && !isLow(b)) { return 1; - } else if (!isLow(a) && isLow(b)) { + } + if (!isLow(a) && isLow(b)) { return -1; - } else if (a > b) { + } + if (a > b) { return 1; - } else if (a < b) { + } + if (a < b) { return -1; } return 0; diff --git a/src/rules/preferConditionalExpressionRule.ts b/src/rules/preferConditionalExpressionRule.ts index 2365167f6b0..ad26f00219e 100644 --- a/src/rules/preferConditionalExpressionRule.ts +++ b/src/rules/preferConditionalExpressionRule.ts @@ -114,11 +114,13 @@ function detectAssignment( } const elze = detectAssignment(statement.elseStatement, sourceFile, checkElseIf, true); return elze !== undefined && nodeEquals(then, elze, sourceFile) ? then : undefined; - } else if (isBlock(statement)) { + } + if (isBlock(statement)) { return statement.statements.length === 1 ? detectAssignment(statement.statements[0], sourceFile, checkElseIf, inElse) : undefined; - } else if (isExpressionStatement(statement) && isBinaryExpression(statement.expression)) { + } + if (isExpressionStatement(statement) && isBinaryExpression(statement.expression)) { const { operatorToken: { kind }, left, @@ -128,9 +130,8 @@ function detectAssignment( isSameLine(sourceFile, right.getStart(sourceFile), right.end) ? left : undefined; - } else { - return undefined; } + return undefined; } function nodeEquals(a: ts.Node, b: ts.Node, sourceFile: ts.SourceFile): boolean { diff --git a/src/rules/preferSwitchRule.ts b/src/rules/preferSwitchRule.ts index 17f819ec1e8..059fbac21c8 100644 --- a/src/rules/preferSwitchRule.ts +++ b/src/rules/preferSwitchRule.ts @@ -81,10 +81,9 @@ function check(node: ts.IfStatement, sourceFile: ts.SourceFile, minCases: number casesSeen++; if (switchVariable !== undefined) { return nodeEquals(expr, switchVariable, sourceFile); - } else { - switchVariable = expr; - return true; } + switchVariable = expr; + return true; }); return couldBeSwitch && casesSeen >= minCases; } diff --git a/src/rules/preferTemplateRule.ts b/src/rules/preferTemplateRule.ts index 179c3b9e47f..808102323ad 100644 --- a/src/rules/preferTemplateRule.ts +++ b/src/rules/preferTemplateRule.ts @@ -89,19 +89,20 @@ function getError(node: ts.Node, allowSingleConcat: boolean): string | undefined return containsNewline(left as StringLike) || containsNewline(right as StringLike) ? Rule.FAILURE_STRING_MULTILINE : undefined; - } else if (!l && !r) { + } + if (!l && !r) { // Watch out for `"a" + b + c`. Parsed as `("a" + b) + c`. return containsAnyStringLiterals(left) ? Rule.FAILURE_STRING : undefined; - } else if (l) { + } + if (l) { // `"x" + y` return !allowSingleConcat ? Rule.FAILURE_STRING : undefined; - } else { - // `? + "b"` - // If LHS consists of only string literals (as in `"a" + "b" + "c"`, allow it.) - return !containsOnlyStringLiterals(left) && (!allowSingleConcat || isPlusExpression(left)) - ? Rule.FAILURE_STRING - : undefined; } + // `? + "b"` + // If LHS consists of only string literals (as in `"a" + "b" + "c"`, allow it.) + return !containsOnlyStringLiterals(left) && (!allowSingleConcat || isPlusExpression(left)) + ? Rule.FAILURE_STRING + : undefined; } type StringLike = ts.StringLiteral | ts.TemplateLiteral; @@ -109,9 +110,8 @@ type StringLike = ts.StringLiteral | ts.TemplateLiteral; function containsNewline(node: StringLike): boolean { if (node.kind === ts.SyntaxKind.TemplateExpression) { return node.templateSpans.some(({ literal: { text } }) => text.includes("\n")); - } else { - return node.text.includes("\n"); } + return node.text.includes("\n"); } function containsOnlyStringLiterals(node: ts.Expression): boolean { diff --git a/src/rules/quotemarkRule.ts b/src/rules/quotemarkRule.ts index dd41efcf7bb..a772e2c8326 100644 --- a/src/rules/quotemarkRule.ts +++ b/src/rules/quotemarkRule.ts @@ -161,13 +161,13 @@ function walk(ctx: Lint.WalkContext) { if (fixQuotemark === actualQuotemark) { // We were already using the best quote mark for this scenario return; - } else if (node.text.includes(fixQuotemark)) { + } + if (node.text.includes(fixQuotemark)) { // It contains all of the other kinds of quotes. Escaping is unavoidable, sadly. return; } - } else { - fixQuotemark = alternativeFixQuotemark; } + fixQuotemark = alternativeFixQuotemark; } const start = node.getStart(sourceFile); diff --git a/src/rules/restrictPlusOperandsRule.ts b/src/rules/restrictPlusOperandsRule.ts index b4b8d7e4827..e8bbb6801df 100644 --- a/src/rules/restrictPlusOperandsRule.ts +++ b/src/rules/restrictPlusOperandsRule.ts @@ -55,9 +55,8 @@ function walk(ctx: Lint.WalkContext, tc: ts.TypeChecker) { node, Rule.INVALID_TYPES_ERROR + Rule.SUGGEST_TEMPLATE_LITERALS, ); - } else { - return ctx.addFailureAtNode(node, Rule.INVALID_TYPES_ERROR); } + return ctx.addFailureAtNode(node, Rule.INVALID_TYPES_ERROR); } } return ts.forEachChild(node, cb); @@ -70,15 +69,18 @@ function getBaseTypeOfLiteralType(type: ts.Type): "string" | "number" | "invalid isTypeFlagSet(type, ts.TypeFlags.String) ) { return "string"; - } else if ( + } + if ( isTypeFlagSet(type, ts.TypeFlags.NumberLiteral) || isTypeFlagSet(type, ts.TypeFlags.Number) ) { return "number"; - } else if (isUnionType(type) && !isTypeFlagSet(type, ts.TypeFlags.Enum)) { + } + if (isUnionType(type) && !isTypeFlagSet(type, ts.TypeFlags.Enum)) { const types = type.types.map(getBaseTypeOfLiteralType); return allSame(types) ? types[0] : "invalid"; - } else if (isTypeFlagSet(type, ts.TypeFlags.EnumLiteral)) { + } + if (isTypeFlagSet(type, ts.TypeFlags.EnumLiteral)) { // Compatibility for TypeScript pre-2.4, which used EnumLiteralType instead of LiteralType getBaseTypeOfLiteralType(((type as any) as { baseType: ts.LiteralType }).baseType); } diff --git a/src/rules/returnUndefinedRule.ts b/src/rules/returnUndefinedRule.ts index 315cc50f00e..014d0f4d6b8 100644 --- a/src/rules/returnUndefinedRule.ts +++ b/src/rules/returnUndefinedRule.ts @@ -87,11 +87,11 @@ function walk(ctx: Lint.WalkContext, checker: ts.TypeChecker) { function returnKindFromReturn(node: ts.ReturnStatement): ReturnKind | undefined { if (node.expression === undefined) { return ReturnKind.Void; - } else if (isIdentifier(node.expression) && node.expression.text === "undefined") { + } + if (isIdentifier(node.expression) && node.expression.text === "undefined") { return ReturnKind.Value; - } else { - return undefined; } + return undefined; } enum ReturnKind { diff --git a/src/rules/typeLiteralDelimiterRule.ts b/src/rules/typeLiteralDelimiterRule.ts index 629f624c020..f3defe04f6f 100644 --- a/src/rules/typeLiteralDelimiterRule.ts +++ b/src/rules/typeLiteralDelimiterRule.ts @@ -61,9 +61,8 @@ export class Rule extends Lint.Rules.AbstractRule { private getRuleOptions(): Options { if (this.ruleArguments[0] === undefined) { return {}; - } else { - return this.ruleArguments[0] as Options; } + return this.ruleArguments[0] as Options; } } diff --git a/src/rules/typedefRule.ts b/src/rules/typedefRule.ts index 778e3000309..89c8f990eff 100644 --- a/src/rules/typedefRule.ts +++ b/src/rules/typedefRule.ts @@ -159,13 +159,14 @@ class TypedefWalker extends Lint.AbstractWalker { const option = (() => { if (!isArrowFunction) { return "parameter"; - } else if (isTypedPropertyDeclaration(parent.parent)) { + } + if (isTypedPropertyDeclaration(parent.parent)) { return undefined; - } else if (utils.isPropertyDeclaration(parent.parent)) { + } + if (utils.isPropertyDeclaration(parent.parent)) { return "member-variable-declaration"; - } else { - return "arrow-parameter"; } + return "arrow-parameter"; })(); if (option !== undefined) { diff --git a/src/rules/unifiedSignaturesRule.ts b/src/rules/unifiedSignaturesRule.ts index ab35dd6d77c..32dcf7ca9fb 100644 --- a/src/rules/unifiedSignaturesRule.ts +++ b/src/rules/unifiedSignaturesRule.ts @@ -95,9 +95,8 @@ function walk(ctx: Lint.WalkContext): void { return body === undefined && name !== undefined ? { signature: statement, key: name.text } : undefined; - } else { - return undefined; } + return undefined; }), ); } diff --git a/src/rules/unnecessaryElseRule.ts b/src/rules/unnecessaryElseRule.ts new file mode 100644 index 00000000000..b7690a0021a --- /dev/null +++ b/src/rules/unnecessaryElseRule.ts @@ -0,0 +1,100 @@ +/** + * @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 utils from "tsutils"; +import * as ts from "typescript"; + +import * as Lint from "../index"; + +import { codeExamples } from "./code-examples/unnecessaryElse.examples"; + +export class Rule extends Lint.Rules.AbstractRule { + /* tslint:disable:object-literal-sort-keys */ + public static metadata: Lint.IRuleMetadata = { + description: Lint.Utils.dedent` + Disallows \`else\` blocks following \`if\` blocks ending with a \`break\`, \`continue\`, \`return\`, or \`throw\` statement.`, + descriptionDetails: "", + optionExamples: [true], + options: null, + optionsDescription: "Not configurable.", + rationale: Lint.Utils.dedent` + When an \`if\` block is guaranteed to exit control flow when entered, + it is unnecessary to add an \`else\` statement. + The contents that would be in the \`else\` block can be placed after the end of the \`if\` block.`, + ruleName: "unnecessary-else", + type: "functionality", + typescriptOnly: false, + codeExamples, + }; + /* tslint:disable:object-literal-sort-keys */ + + public static FAILURE_STRING(name: string): string { + return `The preceding \`if\` block ends with a \`${name}\` statement. This \`else\` is unnecessary.`; + } + + public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { + return this.applyWithFunction(sourceFile, walk); + } +} + +function walk(ctx: Lint.WalkContext): void { + ts.forEachChild(ctx.sourceFile, function cb(node: ts.Node): void { + if (utils.isIfStatement(node)) { + const jumpStatement = utils.isBlock(node.thenStatement) + ? getJumpStatement(getLastStatement(node.thenStatement)) + : getJumpStatement(node.thenStatement); + + if (jumpStatement !== undefined && node.elseStatement !== undefined) { + const elseKeyword = getPositionOfElseKeyword(node, ts.SyntaxKind.ElseKeyword); + ctx.addFailureAtNode(elseKeyword, Rule.FAILURE_STRING(jumpStatement)); + } + } + return ts.forEachChild(node, cb); + }); +} + +function getPositionOfElseKeyword(node: ts.Node, kind: ts.SyntaxKind) { + return node.getChildren().filter(child => child.kind === kind)[0]; +} + +function getJumpStatement(node: ts.Statement): string | undefined { + switch (node.kind) { + case ts.SyntaxKind.BreakStatement: + return "break"; + case ts.SyntaxKind.ContinueStatement: + return "continue"; + case ts.SyntaxKind.ThrowStatement: + return "throw"; + case ts.SyntaxKind.ReturnStatement: + return "return"; + default: + return undefined; + } +} + +function getLastStatement(clause: ts.Block): ts.Statement { + const block = clause.statements[0]; + const statements = + clause.statements.length === 1 && utils.isBlock(block) + ? block.statements + : clause.statements; + + return last(statements); +} + +function last(arr: ReadonlyArray): T { + return arr[arr.length - 1]; +} diff --git a/src/rules/useDefaultTypeParameterRule.ts b/src/rules/useDefaultTypeParameterRule.ts index 4879a65583e..12ccce9ef96 100644 --- a/src/rules/useDefaultTypeParameterRule.ts +++ b/src/rules/useDefaultTypeParameterRule.ts @@ -76,9 +76,8 @@ function walk(ctx: Lint.WalkContext, checker: ts.TypeChecker): void { function createFix(): Lint.Fix { if (i === 0) { return Lint.Replacement.deleteFromTo(typeArguments.pos - 1, typeArguments.end + 1); - } else { - return Lint.Replacement.deleteFromTo(typeArguments[i - 1].end, arg.end); } + return Lint.Replacement.deleteFromTo(typeArguments[i - 1].end, arg.end); } } } diff --git a/src/rules/whitespaceRule.ts b/src/rules/whitespaceRule.ts index 6872b3d3ce4..5c04cc0a610 100644 --- a/src/rules/whitespaceRule.ts +++ b/src/rules/whitespaceRule.ts @@ -267,14 +267,14 @@ function walk(ctx: Lint.WalkContext) { let prevTokenShouldBeFollowedByWhitespace = false; utils.forEachTokenWithTrivia(sourceFile, (_text, tokenKind, range, parent) => { - if ( - tokenKind === ts.SyntaxKind.WhitespaceTrivia || - tokenKind === ts.SyntaxKind.NewLineTrivia || - tokenKind === ts.SyntaxKind.EndOfFileToken - ) { - prevTokenShouldBeFollowedByWhitespace = false; - return; - } else if (prevTokenShouldBeFollowedByWhitespace) { + switch (tokenKind) { + case ts.SyntaxKind.WhitespaceTrivia: + case ts.SyntaxKind.NewLineTrivia: + case ts.SyntaxKind.EndOfFileToken: + prevTokenShouldBeFollowedByWhitespace = false; + return; + } + if (prevTokenShouldBeFollowedByWhitespace) { addMissingWhitespaceErrorAt(range.pos); prevTokenShouldBeFollowedByWhitespace = false; } diff --git a/src/utils.ts b/src/utils.ts index 4ca158a47b1..a0da79e75d4 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -25,11 +25,11 @@ import * as ts from "typescript"; export function arrayify(arg?: T | T[]): T[] { if (Array.isArray(arg)) { return arg; - } else if (arg != undefined) { + } + if (arg != undefined) { return [arg]; - } else { - return []; } + return []; } /** @@ -39,9 +39,8 @@ export function arrayify(arg?: T | T[]): T[] { export function objectify(arg: any): any { if (typeof arg === "object" && arg != undefined) { return arg; - } else { - return {}; } + return {}; } export function hasOwnProperty(arg: {}, key: string): boolean { @@ -106,18 +105,17 @@ export function stripComments(content: string): string { if (m3 !== undefined) { // A block comment. Replace with nothing return ""; - } else if (m4 !== undefined) { + } + if (m4 !== undefined) { // A line comment. If it ends in \r?\n then keep it. const length = m4.length; if (length > 2 && m4[length - 1] === "\n") { return m4[length - 2] === "\r" ? "\r\n" : "\n"; - } else { - return ""; } - } else { - // We match a string - return match; + return ""; } + // We match a string + return match; }, ); return result; diff --git a/src/verify/lines.ts b/src/verify/lines.ts index 47e1f865d21..7d550454e16 100644 --- a/src/verify/lines.ts +++ b/src/verify/lines.ts @@ -110,7 +110,8 @@ export function printLine(fileName: string, line: Line, code?: string): string | const tildes = "~".repeat(code.length - leadingSpaces.length); return `${leadingSpaces}${tildes}`; - } else if (line instanceof EndErrorLine) { + } + if (line instanceof EndErrorLine) { let tildes = "~".repeat(line.endCol - line.startCol); if (code.length < line.endCol) { // Better than crashing in String.repeat @@ -125,9 +126,11 @@ export function printLine(fileName: string, line: Line, code?: string): string | } return `${leadingSpaces}${tildes}${endSpaces} [${line.message}]`; } - } else if (line instanceof MessageSubstitutionLine) { + } + if (line instanceof MessageSubstitutionLine) { return `[${line.key}]: ${line.message}`; - } else if (line instanceof CodeLine) { + } + if (line instanceof CodeLine) { return line.contents; } return undefined; diff --git a/src/verify/lintError.ts b/src/verify/lintError.ts index c36bfa759de..8094c68a288 100644 --- a/src/verify/lintError.ts +++ b/src/verify/lintError.ts @@ -29,15 +29,17 @@ export interface LintError { export function errorComparator(err1: LintError, err2: LintError) { if (err1.startPos.line !== err2.startPos.line) { return err1.startPos.line - err2.startPos.line; - } else if (err1.startPos.col !== err2.startPos.col) { + } + if (err1.startPos.col !== err2.startPos.col) { return err1.startPos.col - err2.startPos.col; - } else if (err1.endPos.line !== err2.endPos.line) { + } + if (err1.endPos.line !== err2.endPos.line) { return err1.endPos.line - err2.endPos.line; - } else if (err1.endPos.col !== err2.endPos.col) { + } + if (err1.endPos.col !== err2.endPos.col) { return err1.endPos.col - err2.endPos.col; - } else { - return err1.message.localeCompare(err2.message); } + return err1.message.localeCompare(err2.message); } export function lintSyntaxError(message: string) { diff --git a/src/verify/parse.ts b/src/verify/parse.ts index 2bffe3a1fe1..45eb411f4d5 100644 --- a/src/verify/parse.ts +++ b/src/verify/parse.ts @@ -165,14 +165,13 @@ export function parseErrorsFromMarkup(text: string): LintError[] { errorStartPos.col } does not end correctly.`, ); - } else { - const nextErrorLine = errorLinesForCodeLines[nextLineNo].shift(); + } + const nextErrorLine = errorLinesForCodeLines[nextLineNo].shift(); - // if end of multiline error, add it it list of errors - if (nextErrorLine instanceof EndErrorLine) { - addError(nextErrorLine, errorStartPos, nextLineNo); - break; - } + // if end of multiline error, add it it list of errors + if (nextErrorLine instanceof EndErrorLine) { + addError(nextErrorLine, errorStartPos, nextLineNo); + break; } } } diff --git a/test/rules/unnecessary-else/test.ts.lint b/test/rules/unnecessary-else/test.ts.lint new file mode 100644 index 00000000000..959dd5ce5ff --- /dev/null +++ b/test/rules/unnecessary-else/test.ts.lint @@ -0,0 +1,258 @@ +const testReturn = (a) => { + if (a===0) { + return 0; + } else { + ~~~~ [return] + return a; + } +} + +const testReturn = (a) => { + if (a===0) return 0; + else return a; + ~~~~ [return] +} + +const testReturn = (a) => { + if (a===0) + return 0; + else + ~~~~ [return] + return a; +} + +const testReturn = (a) => { + if (a>0) { + if (a%2 ===0) { + return "even" ; + } else { + ~~~~ [return] + return "odd"; + } + } + return "negative"; +} + +const testReturn = (a) => { + if (a===0) { + return 0; + } + return a; +} + +const testReturn = (a) => { + if (a<0) { + return; + } else if (a>0) { + ~~~~ [return] + if (a%2 === 0) { + return ; + } else { + ~~~~ [return] + return ; + } + } + return; +} + +const testReturn = (a) => { + if (a<0) { + return; + } + if (a===1) { + return ; + } else { + ~~~~ [return] + return ; + } +} + +const testReturn = (a) => { + if (a>0) { + if (a%3===0) { + return; + } else { + ~~~~ [return] + console.log(a) + } + } + else { + console.log("negative"); + } +} + +const testThrow = (a) => { + if ( a===0 ) { + throw "error"; + } else { + ~~~~ [throw] + return 100/a; + } +} + +const testThrow = (a) => { + if (a===0) + throw "error; + else + ~~~~ [throw] + console.log(100/a); +} + +const testThrow = (a) => { + if (a===0) throw "error; + else console.log(100/a); + ~~~~ [throw] +} + +const testThrow = (a) => { + switch (a) { + case 1: + break; + case 2: + if (true) { + throw "error"; + } else { + ~~~~ [throw] + break; + } + default : + break; + } +} + +const testThrow = (a) => { + let i = 1; + do { + if (a-i === 0) { + throw "error; + } else { + ~~~~ [throw] + console.log(i/a-i); + } + ++i; + } +} + +const testThrow = (a) => { + if (a===0) { + throw "error"; + } + return 100/a; +} + +const testThrow = (a) => { + if (a===0) throw "error"; + return 100/a; +} + +const testContinue = () => { + for (let i = 1; i < 10; i++) { + if (i===8) { + continue ; + } else { + ~~~~ [continue] + console.log(i); + } + } +} + +const testContinue = () => { + for (let i = 1; i < 10; i++) { + if (i===8) continue ; + else console.log(i); + ~~~~ [continue] + } +} + +const testContinue = () => { + for (let i = 1; i < 10; i++) { + if (i===8) + continue ; + else + ~~~~ [continue] + console.log(i); + } +} + +const testContinue = () => { + for (let i = 1; i < 10; i++) { + if (i===4) { + continue ; + } + console.log(i); + } +} + +const testContinue = () => { + for (let i = 1; i < 10; i++) { + if (i===4) + continue ; + console.log(i); + } +} + +const testBreak = (a) => { + let i = 0; + while(i < 20) { + if (i === a) { + break ; + } else { + ~~~~ [break] + i++; + } + } + return i-1; +} + +const testBreak = (a) => { + let i = 0; + while(i < 20) { + if (i === a) { + break ; + } + i++; + } + return i-1; +} + +const testBreak = (a) => { + let i = 0; + while(i < 20) { + if (i === a) + break ; + i++; + } + return i-1; +} + +const testBreak = (a) => { + let i = 0; + while(i < 20) { + if (i === a) break ; + i++; + } + return i-1; +} + +const testBreak = (a) => { + let i = 0; + while(i < 20) { + if (i === a) break ; + else i++; + ~~~~ [break] + } + return i-1; +} + +const testNoJump = (a) => { + if (a % 2 === 0) { + console.log(a); + } else { + console.log(a * 2); + } +} + +[return]: The preceding `if` block ends with a `return` statement. This `else` is unnecessary. +[throw]: The preceding `if` block ends with a `throw` statement. This `else` is unnecessary. +[break]: The preceding `if` block ends with a `break` statement. This `else` is unnecessary. +[continue]: The preceding `if` block ends with a `continue` statement. This `else` is unnecessary. diff --git a/test/rules/unnecessary-else/tslint.json b/test/rules/unnecessary-else/tslint.json new file mode 100644 index 00000000000..5e3f2360d32 --- /dev/null +++ b/test/rules/unnecessary-else/tslint.json @@ -0,0 +1,5 @@ +{ + "rules": { + "unnecessary-else": true + } +}