From 37147b65b9f777bfccc86e088f4c726f57a2fb7f Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Tue, 12 Mar 2019 20:52:18 -0400 Subject: [PATCH 1/5] remove unnecessary-else from tslint:latest --- src/configs/all.ts | 2 +- src/configs/latest.ts | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/configs/all.ts b/src/configs/all.ts index f6a7589202d..3845aff90ea 100644 --- a/src/configs/all.ts +++ b/src/configs/all.ts @@ -154,7 +154,6 @@ export const rules = { "unnecessary-constructor": true, "use-default-type-parameter": true, "use-isnan": true, - "unnecessary-else": true, // Maintainability @@ -266,6 +265,7 @@ export const rules = { "switch-final-break": true, "type-literal-delimiter": true, "unnecessary-bind": true, + "unnecessary-else": true, "variable-name": { options: ["ban-keywords", "check-format", "require-const-for-all-caps"] }, whitespace: { options: [ diff --git a/src/configs/latest.ts b/src/configs/latest.ts index 9421324f1a0..eae3fa67fb0 100644 --- a/src/configs/latest.ts +++ b/src/configs/latest.ts @@ -67,7 +67,6 @@ export const rules = { // added in v5.12 "function-constructor": true, "unnecessary-bind": true, - "unnecessary-else": true, }; // tslint:enable object-literal-sort-keys From 50c11ee5eeb67be3031eacbeeeefc61ca61e367b Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Tue, 12 Mar 2019 20:52:35 -0400 Subject: [PATCH 2/5] Revert "Added unnecessary-else Rule (#4502)" This reverts commit 88916a220a28fb044a7496a9c1c44ff94b8b742c. --- src/configuration.ts | 76 +++--- src/formatterLoader.ts | 8 +- src/language/utils.ts | 9 +- src/language/walker/ruleWalker.ts | 3 +- src/linter.ts | 6 +- src/rules/callableTypesRule.ts | 3 +- .../code-examples/unnecessaryElse.examples.ts | 76 ------ src/rules/curlyRule.ts | 11 +- src/rules/noBooleanLiteralCompareRule.ts | 6 +- src/rules/noDefaultImportRule.ts | 7 +- src/rules/noInvalidThisRule.ts | 17 +- src/rules/noRestrictedGlobalsRule.ts | 8 +- src/rules/noSparseArraysRule.ts | 3 +- src/rules/noUnnecessaryQualifierRule.ts | 3 +- src/rules/noUnusedExpressionRule.ts | 15 +- src/rules/noUnusedVariableRule.ts | 105 +++---- src/rules/orderedImportsRule.ts | 24 +- src/rules/preferConditionalExpressionRule.ts | 9 +- src/rules/preferSwitchRule.ts | 5 +- src/rules/preferTemplateRule.ts | 20 +- src/rules/quotemarkRule.ts | 6 +- src/rules/restrictPlusOperandsRule.ts | 12 +- src/rules/returnUndefinedRule.ts | 6 +- src/rules/typeLiteralDelimiterRule.ts | 3 +- src/rules/typedefRule.ts | 9 +- src/rules/unifiedSignaturesRule.ts | 3 +- src/rules/unnecessaryElseRule.ts | 100 ------- src/rules/useDefaultTypeParameterRule.ts | 3 +- src/rules/whitespaceRule.ts | 16 +- src/utils.ts | 20 +- src/verify/lines.ts | 9 +- src/verify/lintError.ts | 12 +- src/verify/parse.ts | 13 +- test/rules/unnecessary-else/test.ts.lint | 258 ------------------ test/rules/unnecessary-else/tslint.json | 5 - 35 files changed, 232 insertions(+), 657 deletions(-) delete mode 100644 src/rules/code-examples/unnecessaryElse.examples.ts delete mode 100644 src/rules/unnecessaryElseRule.ts delete mode 100644 test/rules/unnecessary-else/test.ts.lint delete mode 100644 test/rules/unnecessary-else/tslint.json diff --git a/src/configuration.ts b/src/configuration.ts index 7ef245161aa..486761c38b7 100644 --- a/src/configuration.ts +++ b/src/configuration.ts @@ -155,40 +155,42 @@ export function findConfigurationPath( throw new FatalError( `Could not find config file at: ${path.resolve(suppliedConfigFilePath)}`, ); + } else { + return path.resolve(suppliedConfigFilePath); } - 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()) { + } 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 useDirName = true; } - } catch (e) { - // throws if file doesn't exist - useDirName = true; - } - if (useDirName) { - inputFilePath = path.dirname(inputFilePath!); - } + 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; } /** @@ -244,15 +246,16 @@ 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. */ @@ -263,8 +266,9 @@ 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 a45dbfba295..951b145ddda 100644 --- a/src/formatterLoader.ts +++ b/src/formatterLoader.ts @@ -29,8 +29,7 @@ export function findFormatter( ): FormatterConstructor | undefined { if (typeof name === "function") { return name; - } - if (typeof name === "string") { + } else if (typeof name === "string") { name = name.trim(); const camelizedName = camelize(`${name}Formatter`); @@ -50,9 +49,10 @@ 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 b13acac5966..9160d82b508 100644 --- a/src/language/utils.ts +++ b/src/language/utils.ts @@ -79,8 +79,9 @@ 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` */ @@ -98,8 +99,9 @@ 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; } @@ -145,8 +147,9 @@ 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 a566bc2a249..f99c56b57a2 100644 --- a/src/language/walker/ruleWalker.ts +++ b/src/language/walker/ruleWalker.ts @@ -66,8 +66,9 @@ 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 cdb670f3466..378a133f0ab 100644 --- a/src/linter.ts +++ b/src/linter.ts @@ -283,8 +283,9 @@ 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); @@ -322,8 +323,9 @@ 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 3a9f358d381..5cc3e15285e 100644 --- a/src/rules/callableTypesRule.ts +++ b/src/rules/callableTypesRule.ts @@ -115,8 +115,9 @@ 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 deleted file mode 100644 index 260b14b596e..00000000000 --- a/src/rules/code-examples/unnecessaryElse.examples.ts +++ /dev/null @@ -1,76 +0,0 @@ -/** - * @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 01c1a0e82ab..2b51c2b5df8 100644 --- a/src/rules/curlyRule.ts +++ b/src/rules/curlyRule.ts @@ -172,11 +172,12 @@ 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 deed9492477..bbafa974c80 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; - } - if (needsParenthesesForNegate(expression)) { + } else 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 aded2c8b39d..dc7c629df59 100644 --- a/src/rules/noDefaultImportRule.ts +++ b/src/rules/noDefaultImportRule.ts @@ -104,10 +104,11 @@ 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 eb2e602603e..98c925b0cb1 100644 --- a/src/rules/noInvalidThisRule.ts +++ b/src/rules/noInvalidThisRule.ts @@ -102,15 +102,16 @@ 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 17534f0cee7..154c2788a3f 100644 --- a/src/rules/noRestrictedGlobalsRule.ts +++ b/src/rules/noRestrictedGlobalsRule.ts @@ -73,8 +73,14 @@ 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 6b93d1ea479..5654ddeb75e 100644 --- a/src/rules/noSparseArraysRule.ts +++ b/src/rules/noSparseArraysRule.ts @@ -54,8 +54,9 @@ 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 ca786500875..e4bd28d3c23 100644 --- a/src/rules/noUnnecessaryQualifierRule.ts +++ b/src/rules/noUnnecessaryQualifierRule.ts @@ -122,8 +122,7 @@ function walk(ctx: Lint.WalkContext, checker: ts.TypeChecker): void { const symbolDeclarations = symbol.getDeclarations(); if (symbolDeclarations === undefined) { return false; - } - if (symbolDeclarations.some(decl => namespacesInScope.some(ns => ns === decl))) { + } else if (symbolDeclarations.some(decl => namespacesInScope.some(ns => ns === decl))) { return true; } diff --git a/src/rules/noUnusedExpressionRule.ts b/src/rules/noUnusedExpressionRule.ts index 7bf0eb0a1c0..0a9a7883909 100644 --- a/src/rules/noUnusedExpressionRule.ts +++ b/src/rules/noUnusedExpressionRule.ts @@ -91,12 +91,10 @@ function walk(ctx: Lint.WalkContext) { if (checking) { if (isParenthesizedExpression(node) || isVoidExpression(node)) { return cb(node.expression); - } - if (isConditionalExpression(node)) { + } else if (isConditionalExpression(node)) { noCheck(node.condition, cb); return both(node.whenTrue, node.whenFalse); - } - if (isBinaryExpression(node)) { + } else if (isBinaryExpression(node)) { switch (node.operatorToken.kind) { case ts.SyntaxKind.CommaToken: if (isIndirectEval(node)) { @@ -121,8 +119,7 @@ function walk(ctx: Lint.WalkContext) { } allowFastNullChecks = true; return false; - } - if (isVoidExpression(node)) { + } else if (isVoidExpression(node)) { // allow `void 0` and `void(0)` if ( !isLiteralZero( @@ -134,8 +131,7 @@ function walk(ctx: Lint.WalkContext) { check(node.expression); } return false; - } - if ( + } else if ( isBinaryExpression(node) && node.operatorToken.kind === ts.SyntaxKind.CommaToken && !isIndirectEval(node) @@ -171,8 +167,9 @@ 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 ebd23020175..75f222a250b 100644 --- a/src/rules/noUnusedVariableRule.ts +++ b/src/rules/noUnusedVariableRule.ts @@ -299,8 +299,9 @@ function addImportSpecifierFailures( const lineStarts = ctx.sourceFile.getLineStarts(); if (nextLine < lineStarts.length) { return lineStarts[nextLine]; + } else { + return position; } - return position; } }); @@ -365,12 +366,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); - } - if (utils.isSignatureDeclaration(node) && node.type === undefined) { + } else 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; @@ -408,56 +409,58 @@ function findImports( if (i.kind === ts.SyntaxKind.ImportEqualsDeclaration) { return [i.name]; - } - 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]; - } - - // 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)); + } else { + if (i.importClause === undefined) { + // Error node + return undefined; } - 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]; + + const { name: defaultName, namedBindings } = i.importClause; + if ( + namedBindings !== undefined && + namedBindings.kind === ts.SyntaxKind.NamespaceImport + ) { + return [namedBindings.name]; } - for (const element of namedBindings.elements) { - if (isInRange(element, pos)) { - return [element.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]; + } + + 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 29bde1f0ee0..8316b542c36 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,8 +707,7 @@ function flipCase(str: string): string { .map(char => { if (char >= "a" && char <= "z") { return char.toUpperCase(); - } - if (char >= "A" && char <= "Z") { + } else if (char >= "A" && char <= "Z") { return char.toLowerCase(); } return char; @@ -736,14 +735,11 @@ function compare(a: string, b: string): 0 | 1 | -1 { } if (isLow(a) && !isLow(b)) { return 1; - } - if (!isLow(a) && isLow(b)) { + } else if (!isLow(a) && isLow(b)) { return -1; - } - if (a > b) { + } else if (a > b) { return 1; - } - if (a < b) { + } else if (a < b) { return -1; } return 0; diff --git a/src/rules/preferConditionalExpressionRule.ts b/src/rules/preferConditionalExpressionRule.ts index ad26f00219e..2365167f6b0 100644 --- a/src/rules/preferConditionalExpressionRule.ts +++ b/src/rules/preferConditionalExpressionRule.ts @@ -114,13 +114,11 @@ function detectAssignment( } const elze = detectAssignment(statement.elseStatement, sourceFile, checkElseIf, true); return elze !== undefined && nodeEquals(then, elze, sourceFile) ? then : undefined; - } - if (isBlock(statement)) { + } else if (isBlock(statement)) { return statement.statements.length === 1 ? detectAssignment(statement.statements[0], sourceFile, checkElseIf, inElse) : undefined; - } - if (isExpressionStatement(statement) && isBinaryExpression(statement.expression)) { + } else if (isExpressionStatement(statement) && isBinaryExpression(statement.expression)) { const { operatorToken: { kind }, left, @@ -130,8 +128,9 @@ 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 059fbac21c8..17f819ec1e8 100644 --- a/src/rules/preferSwitchRule.ts +++ b/src/rules/preferSwitchRule.ts @@ -81,9 +81,10 @@ 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 808102323ad..179c3b9e47f 100644 --- a/src/rules/preferTemplateRule.ts +++ b/src/rules/preferTemplateRule.ts @@ -89,20 +89,19 @@ function getError(node: ts.Node, allowSingleConcat: boolean): string | undefined return containsNewline(left as StringLike) || containsNewline(right as StringLike) ? Rule.FAILURE_STRING_MULTILINE : undefined; - } - if (!l && !r) { + } else if (!l && !r) { // Watch out for `"a" + b + c`. Parsed as `("a" + b) + c`. return containsAnyStringLiterals(left) ? Rule.FAILURE_STRING : undefined; - } - if (l) { + } else 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; @@ -110,8 +109,9 @@ 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 a772e2c8326..dd41efcf7bb 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; - } - if (node.text.includes(fixQuotemark)) { + } else 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 e8bbb6801df..b4b8d7e4827 100644 --- a/src/rules/restrictPlusOperandsRule.ts +++ b/src/rules/restrictPlusOperandsRule.ts @@ -55,8 +55,9 @@ 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); @@ -69,18 +70,15 @@ function getBaseTypeOfLiteralType(type: ts.Type): "string" | "number" | "invalid isTypeFlagSet(type, ts.TypeFlags.String) ) { return "string"; - } - if ( + } else if ( isTypeFlagSet(type, ts.TypeFlags.NumberLiteral) || isTypeFlagSet(type, ts.TypeFlags.Number) ) { return "number"; - } - if (isUnionType(type) && !isTypeFlagSet(type, ts.TypeFlags.Enum)) { + } else if (isUnionType(type) && !isTypeFlagSet(type, ts.TypeFlags.Enum)) { const types = type.types.map(getBaseTypeOfLiteralType); return allSame(types) ? types[0] : "invalid"; - } - if (isTypeFlagSet(type, ts.TypeFlags.EnumLiteral)) { + } else 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 014d0f4d6b8..315cc50f00e 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; - } - if (isIdentifier(node.expression) && node.expression.text === "undefined") { + } else 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 f3defe04f6f..629f624c020 100644 --- a/src/rules/typeLiteralDelimiterRule.ts +++ b/src/rules/typeLiteralDelimiterRule.ts @@ -61,8 +61,9 @@ 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 89c8f990eff..778e3000309 100644 --- a/src/rules/typedefRule.ts +++ b/src/rules/typedefRule.ts @@ -159,14 +159,13 @@ class TypedefWalker extends Lint.AbstractWalker { const option = (() => { if (!isArrowFunction) { return "parameter"; - } - if (isTypedPropertyDeclaration(parent.parent)) { + } else if (isTypedPropertyDeclaration(parent.parent)) { return undefined; - } - if (utils.isPropertyDeclaration(parent.parent)) { + } else 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 32dcf7ca9fb..ab35dd6d77c 100644 --- a/src/rules/unifiedSignaturesRule.ts +++ b/src/rules/unifiedSignaturesRule.ts @@ -95,8 +95,9 @@ 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 deleted file mode 100644 index b7690a0021a..00000000000 --- a/src/rules/unnecessaryElseRule.ts +++ /dev/null @@ -1,100 +0,0 @@ -/** - * @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 12ccce9ef96..4879a65583e 100644 --- a/src/rules/useDefaultTypeParameterRule.ts +++ b/src/rules/useDefaultTypeParameterRule.ts @@ -76,8 +76,9 @@ 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 1973c37f342..0fabfc46f56 100644 --- a/src/rules/whitespaceRule.ts +++ b/src/rules/whitespaceRule.ts @@ -283,14 +283,14 @@ function walk(ctx: Lint.WalkContext) { let prevTokenShouldBeFollowedByWhitespace = false; utils.forEachTokenWithTrivia(sourceFile, (_text, tokenKind, range, parent) => { - switch (tokenKind) { - case ts.SyntaxKind.WhitespaceTrivia: - case ts.SyntaxKind.NewLineTrivia: - case ts.SyntaxKind.EndOfFileToken: - prevTokenShouldBeFollowedByWhitespace = false; - return; - } - if (prevTokenShouldBeFollowedByWhitespace) { + if ( + tokenKind === ts.SyntaxKind.WhitespaceTrivia || + tokenKind === ts.SyntaxKind.NewLineTrivia || + tokenKind === ts.SyntaxKind.EndOfFileToken + ) { + prevTokenShouldBeFollowedByWhitespace = false; + return; + } else if (prevTokenShouldBeFollowedByWhitespace) { addMissingWhitespaceErrorAt(range.pos); prevTokenShouldBeFollowedByWhitespace = false; } diff --git a/src/utils.ts b/src/utils.ts index a0da79e75d4..4ca158a47b1 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; - } - if (arg != undefined) { + } else if (arg != undefined) { return [arg]; + } else { + return []; } - return []; } /** @@ -39,8 +39,9 @@ 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 { @@ -105,17 +106,18 @@ export function stripComments(content: string): string { if (m3 !== undefined) { // A block comment. Replace with nothing return ""; - } - if (m4 !== undefined) { + } else 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 ""; } - return ""; + } else { + // We match a string + return match; } - // We match a string - return match; }, ); return result; diff --git a/src/verify/lines.ts b/src/verify/lines.ts index 7d550454e16..47e1f865d21 100644 --- a/src/verify/lines.ts +++ b/src/verify/lines.ts @@ -110,8 +110,7 @@ export function printLine(fileName: string, line: Line, code?: string): string | const tildes = "~".repeat(code.length - leadingSpaces.length); return `${leadingSpaces}${tildes}`; - } - if (line instanceof EndErrorLine) { + } else if (line instanceof EndErrorLine) { let tildes = "~".repeat(line.endCol - line.startCol); if (code.length < line.endCol) { // Better than crashing in String.repeat @@ -126,11 +125,9 @@ export function printLine(fileName: string, line: Line, code?: string): string | } return `${leadingSpaces}${tildes}${endSpaces} [${line.message}]`; } - } - if (line instanceof MessageSubstitutionLine) { + } else if (line instanceof MessageSubstitutionLine) { return `[${line.key}]: ${line.message}`; - } - if (line instanceof CodeLine) { + } else if (line instanceof CodeLine) { return line.contents; } return undefined; diff --git a/src/verify/lintError.ts b/src/verify/lintError.ts index 8094c68a288..c36bfa759de 100644 --- a/src/verify/lintError.ts +++ b/src/verify/lintError.ts @@ -29,17 +29,15 @@ export interface LintError { export function errorComparator(err1: LintError, err2: LintError) { if (err1.startPos.line !== err2.startPos.line) { return err1.startPos.line - err2.startPos.line; - } - if (err1.startPos.col !== err2.startPos.col) { + } else if (err1.startPos.col !== err2.startPos.col) { return err1.startPos.col - err2.startPos.col; - } - if (err1.endPos.line !== err2.endPos.line) { + } else if (err1.endPos.line !== err2.endPos.line) { return err1.endPos.line - err2.endPos.line; - } - if (err1.endPos.col !== err2.endPos.col) { + } else 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 45eb411f4d5..2bffe3a1fe1 100644 --- a/src/verify/parse.ts +++ b/src/verify/parse.ts @@ -165,13 +165,14 @@ export function parseErrorsFromMarkup(text: string): LintError[] { errorStartPos.col } does not end correctly.`, ); - } - const nextErrorLine = errorLinesForCodeLines[nextLineNo].shift(); + } else { + 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 deleted file mode 100644 index 959dd5ce5ff..00000000000 --- a/test/rules/unnecessary-else/test.ts.lint +++ /dev/null @@ -1,258 +0,0 @@ -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 deleted file mode 100644 index 5e3f2360d32..00000000000 --- a/test/rules/unnecessary-else/tslint.json +++ /dev/null @@ -1,5 +0,0 @@ -{ - "rules": { - "unnecessary-else": true - } -} From 5365547bf8e31decdce54f9d9d1f7251cb6a0836 Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Tue, 12 Mar 2019 21:21:48 -0400 Subject: [PATCH 3/5] Add back unnecessary-else implementation --- .../code-examples/unnecessaryElse.examples.ts | 76 ++++++ src/rules/unnecessaryElseRule.ts | 100 +++++++ test/rules/unnecessary-else/test.ts.lint | 258 ++++++++++++++++++ test/rules/unnecessary-else/tslint.json | 5 + 4 files changed, 439 insertions(+) create mode 100644 src/rules/code-examples/unnecessaryElse.examples.ts create mode 100644 src/rules/unnecessaryElseRule.ts create mode 100644 test/rules/unnecessary-else/test.ts.lint create mode 100644 test/rules/unnecessary-else/tslint.json 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/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/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 + } +} From 6e2977dcd525684f3d5a7e286b5473a35adb4efe Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Tue, 12 Mar 2019 21:21:56 -0400 Subject: [PATCH 4/5] Disable the rule for tslint itself --- tslint.json | 1 + 1 file changed, 1 insertion(+) diff --git a/tslint.json b/tslint.json index 820279c960c..2a92471d37c 100644 --- a/tslint.json +++ b/tslint.json @@ -22,6 +22,7 @@ "no-parameter-reassignment": false, "no-unused-variable": false, "typedef": false, + "unnecessary-else": false, // TODO: Enable stricter options for these "comment-format": { From 3a96c20e9be0c6662977a7bb2d6e43f42091ccb7 Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Tue, 12 Mar 2019 21:24:03 -0400 Subject: [PATCH 5/5] =?UTF-8?q?It=E2=80=99s=20a=20style=20rule?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/rules/unnecessaryElseRule.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rules/unnecessaryElseRule.ts b/src/rules/unnecessaryElseRule.ts index b7690a0021a..02f4a712b64 100644 --- a/src/rules/unnecessaryElseRule.ts +++ b/src/rules/unnecessaryElseRule.ts @@ -35,7 +35,7 @@ export class Rule extends Lint.Rules.AbstractRule { 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", + type: "style", typescriptOnly: false, codeExamples, };