From 5bd58d2708da7f24c9cbf201e48ae3dac977a21c Mon Sep 17 00:00:00 2001 From: Debsmita Date: Mon, 4 Feb 2019 21:08:10 +0530 Subject: [PATCH 01/14] Added no-unnecessary-else Rule --- src/configs/all.ts | 1 + src/configs/recommended.ts | 1 + src/rules/noUnnecessaryElseRule.ts | 96 ++++++++++++++++++ test/rules/no-unnecessary-else/test.ts.lint | 107 ++++++++++++++++++++ test/rules/no-unnecessary-else/tslint.json | 6 ++ 5 files changed, 211 insertions(+) create mode 100644 src/rules/noUnnecessaryElseRule.ts create mode 100644 test/rules/no-unnecessary-else/test.ts.lint create mode 100644 test/rules/no-unnecessary-else/tslint.json diff --git a/src/configs/all.ts b/src/configs/all.ts index a5658c6382c..203d9d3401c 100644 --- a/src/configs/all.ts +++ b/src/configs/all.ts @@ -151,6 +151,7 @@ export const rules = { "unnecessary-constructor": true, "use-default-type-parameter": true, "use-isnan": true, + "no-unnecessary-else": true, // Maintainability diff --git a/src/configs/recommended.ts b/src/configs/recommended.ts index a692d17e862..0119157386f 100644 --- a/src/configs/recommended.ts +++ b/src/configs/recommended.ts @@ -91,6 +91,7 @@ export const rules = { "no-string-throw": true, "no-switch-case-fall-through": false, "no-trailing-whitespace": true, + "no-unnecessary-else": true, "no-unnecessary-initializer": true, "no-unsafe-finally": true, "no-unused-expression": true, diff --git a/src/rules/noUnnecessaryElseRule.ts b/src/rules/noUnnecessaryElseRule.ts new file mode 100644 index 00000000000..07dcead75e7 --- /dev/null +++ b/src/rules/noUnnecessaryElseRule.ts @@ -0,0 +1,96 @@ +/** + * @license + * Copyright 2018 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 ts from "typescript"; + +import * as Lint from "../index"; + +export class Rule extends Lint.Rules.AbstractRule { + public static metadata: Lint.IRuleMetadata = { + description: Lint.Utils.dedent` + Disallows else block when the If block contains control flow statements, such as \`return\`, \`continue\`, + \`break\` and \`throws\`.`, + descriptionDetails: "", + optionExamples: [true], + options: null, + optionsDescription: "Not configurable.", + rationale: Lint.Utils.dedent` + When control flow statements, + such as \`return\`, \`continue\`, \`break\` and \`throws\` are written inside \`if\` block, + Then \`else\` block becomes unnecessary.`, + ruleName: "no-unnecessary-else", + type: "functionality", + typescriptOnly: false, + }; + + public static FAILURE_STRING(name: string): string { + return `If block contains \`${name}\` statement. Consider replacing the contents of else block outside of the block.`; + } + + public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { + return this.applyWithFunction(sourceFile, walk); + } +} + +type JumpStatement = + | ts.BreakStatement + | ts.ContinueStatement + | ts.ThrowStatement + | ts.ReturnStatement; + +function walk(ctx: Lint.WalkContext): void { + let inElse = false; + ts.forEachChild(ctx.sourceFile, function cb(node: ts.Node): void { + switch (node.kind) { + case ts.SyntaxKind.IfStatement: + const { thenStatement, elseStatement } = node as ts.IfStatement; + if (elseStatement !== undefined) { + inElse = true; + } + ts.forEachChild(thenStatement, cb); + break; + + case ts.SyntaxKind.BreakStatement: + case ts.SyntaxKind.ContinueStatement: + case ts.SyntaxKind.ThrowStatement: + case ts.SyntaxKind.ReturnStatement: + if (inElse) { + ctx.addFailureAtNode( + node, + Rule.FAILURE_STRING(printJumpKind(node as JumpStatement)), + ); + inElse = false; + } + break; + + default: + return ts.forEachChild(node, cb); + } + }); +} + +function printJumpKind(node: JumpStatement): string { + 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"; + } +} diff --git a/test/rules/no-unnecessary-else/test.ts.lint b/test/rules/no-unnecessary-else/test.ts.lint new file mode 100644 index 00000000000..e6b3cdc7c8e --- /dev/null +++ b/test/rules/no-unnecessary-else/test.ts.lint @@ -0,0 +1,107 @@ +const testReturn = () => { + if () { + return; + ~~~~~~~ [return] + } else { + return; + } +} + +//valid +const testReturn = () => { + if () { + return; + } + return; +} + +const testReturn = () => { + if () { + return; + ~~~~~~~ [return] + } else if { + if () { + return ; + } else { + return ; + } + } + return; +} + +const testReturn = () => { + if () { + return; + } + if () { + return ; + ~~~~~~~~ [return] + } else { + return ; + } +} + +//valid +const testReturn = () => { + if () { + return; + } + if () { + return ; + } + return ; +} + +function testThrow () { + if () { + throw "error"; + ~~~~~~~~~~~~~~ [throw] + } else { + } +} + +//valid +function testThrow () { + if () { + throw "error"; + } +} + +function testContinue () { + for ( ) { + if () { + continue ; + ~~~~~~~~~~ [continue] + } else { + } + } +} + +//valid +function testContinue () { + for ( ) { + if () { + continue ; + } + } +} + +function testBreak () { + if () { + break ; + ~~~~~~~ [break] + } else { + } +} + +//valid +function testBreak () { + if () { + break ; + } +} + +[return]: If block contains `return` statement. Consider replacing the contents of else block outside of the block. +[throw]: If block contains `throw` statement. Consider replacing the contents of else block outside of the block. +[break]: If block contains `break` statement. Consider replacing the contents of else block outside of the block. +[continue]: If block contains `continue` statement. Consider replacing the contents of else block outside of the block. diff --git a/test/rules/no-unnecessary-else/tslint.json b/test/rules/no-unnecessary-else/tslint.json new file mode 100644 index 00000000000..d5a4a8c2e79 --- /dev/null +++ b/test/rules/no-unnecessary-else/tslint.json @@ -0,0 +1,6 @@ +{ + "rules": { + "no-unnecessary-else": true + } + } + \ No newline at end of file From ee45076c830524d83245b806e4a65bba5a2ca5da Mon Sep 17 00:00:00 2001 From: Debsmita Date: Sat, 9 Feb 2019 21:46:27 +0530 Subject: [PATCH 02/14] Changed year to 2019 under License --- src/rules/noUnnecessaryElseRule.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rules/noUnnecessaryElseRule.ts b/src/rules/noUnnecessaryElseRule.ts index 0a80ff613ca..750268703cd 100644 --- a/src/rules/noUnnecessaryElseRule.ts +++ b/src/rules/noUnnecessaryElseRule.ts @@ -1,6 +1,6 @@ /** * @license - * Copyright 2018 Palantir Technologies, Inc. + * 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. From 8d4f035a89b74f342aa687724425c8b5406d077d Mon Sep 17 00:00:00 2001 From: Debsmita Date: Sat, 9 Feb 2019 21:51:07 +0530 Subject: [PATCH 03/14] Changed name of the rule to 'unnecessary-else' --- src/configs/all.ts | 2 +- src/configs/latest.ts | 2 +- .../{noUnnecessaryElseRule.ts => unnecessaryElseRule.ts} | 2 +- test/rules/no-unnecessary-else/tslint.json | 6 ------ .../{no-unnecessary-else => unnecessary-else}/test.ts.lint | 0 test/rules/unnecessary-else/tslint.json | 6 ++++++ 6 files changed, 9 insertions(+), 9 deletions(-) rename src/rules/{noUnnecessaryElseRule.ts => unnecessaryElseRule.ts} (98%) delete mode 100644 test/rules/no-unnecessary-else/tslint.json rename test/rules/{no-unnecessary-else => unnecessary-else}/test.ts.lint (100%) create mode 100644 test/rules/unnecessary-else/tslint.json diff --git a/src/configs/all.ts b/src/configs/all.ts index 203d9d3401c..60766993c0a 100644 --- a/src/configs/all.ts +++ b/src/configs/all.ts @@ -151,7 +151,7 @@ export const rules = { "unnecessary-constructor": true, "use-default-type-parameter": true, "use-isnan": true, - "no-unnecessary-else": true, + "unnecessary-else": true, // Maintainability diff --git a/src/configs/latest.ts b/src/configs/latest.ts index 8ccc6727bd1..9421324f1a0 100644 --- a/src/configs/latest.ts +++ b/src/configs/latest.ts @@ -67,7 +67,7 @@ export const rules = { // added in v5.12 "function-constructor": true, "unnecessary-bind": true, - "no-unnecessary-else": true, + "unnecessary-else": true, }; // tslint:enable object-literal-sort-keys diff --git a/src/rules/noUnnecessaryElseRule.ts b/src/rules/unnecessaryElseRule.ts similarity index 98% rename from src/rules/noUnnecessaryElseRule.ts rename to src/rules/unnecessaryElseRule.ts index 750268703cd..cc18692f31d 100644 --- a/src/rules/noUnnecessaryElseRule.ts +++ b/src/rules/unnecessaryElseRule.ts @@ -32,7 +32,7 @@ export class Rule extends Lint.Rules.AbstractRule { When control flow statements, such as \`return\`, \`continue\`, \`break\` or \`throw\` is written inside \`if\` block, Then \`else\` block becomes unnecessary.`, - ruleName: "no-unnecessary-else", + ruleName: "unnecessary-else", type: "functionality", typescriptOnly: false, }; diff --git a/test/rules/no-unnecessary-else/tslint.json b/test/rules/no-unnecessary-else/tslint.json deleted file mode 100644 index d5a4a8c2e79..00000000000 --- a/test/rules/no-unnecessary-else/tslint.json +++ /dev/null @@ -1,6 +0,0 @@ -{ - "rules": { - "no-unnecessary-else": true - } - } - \ No newline at end of file diff --git a/test/rules/no-unnecessary-else/test.ts.lint b/test/rules/unnecessary-else/test.ts.lint similarity index 100% rename from test/rules/no-unnecessary-else/test.ts.lint rename to test/rules/unnecessary-else/test.ts.lint diff --git a/test/rules/unnecessary-else/tslint.json b/test/rules/unnecessary-else/tslint.json new file mode 100644 index 00000000000..959c1d4cb5c --- /dev/null +++ b/test/rules/unnecessary-else/tslint.json @@ -0,0 +1,6 @@ +{ + "rules": { + "unnecessary-else": true + } + } + \ No newline at end of file From c2db576ebe200c4e74417e56954613d16a3292b9 Mon Sep 17 00:00:00 2001 From: Debsmita Date: Sat, 9 Feb 2019 23:11:25 +0530 Subject: [PATCH 04/14] Removed comments like valid from test.ts.lint file --- test/rules/unnecessary-else/test.ts.lint | 5 ----- 1 file changed, 5 deletions(-) diff --git a/test/rules/unnecessary-else/test.ts.lint b/test/rules/unnecessary-else/test.ts.lint index bbf14a60e99..59d8be517b1 100644 --- a/test/rules/unnecessary-else/test.ts.lint +++ b/test/rules/unnecessary-else/test.ts.lint @@ -18,7 +18,6 @@ const testReturn = () => { } } -//valid const testReturn = () => { if () { return; @@ -52,7 +51,6 @@ const testReturn = () => { } } -//valid const testReturn = () => { if () { return; @@ -87,7 +85,6 @@ const testThrow = () => { } } -//valid function testThrow () { if () { throw "error"; @@ -104,7 +101,6 @@ function testContinue () { } } -//valid function testContinue () { for ( ) { if () { @@ -137,7 +133,6 @@ const testBreak = () => { } } -//valid function testBreak () { if () { break ; From a1ec00c3360cd8d8b7e36e4ea7b18410a14db156 Mon Sep 17 00:00:00 2001 From: Debsmita Date: Sat, 9 Feb 2019 23:17:28 +0530 Subject: [PATCH 05/14] Modified the description, rationale and the FAILURE_STRING --- src/rules/unnecessaryElseRule.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/rules/unnecessaryElseRule.ts b/src/rules/unnecessaryElseRule.ts index cc18692f31d..92eef0ec4a2 100644 --- a/src/rules/unnecessaryElseRule.ts +++ b/src/rules/unnecessaryElseRule.ts @@ -22,16 +22,15 @@ export class Rule extends Lint.Rules.AbstractRule { /* tslint:disable:object-literal-sort-keys */ public static metadata: Lint.IRuleMetadata = { description: Lint.Utils.dedent` - Disallows else block when the If block contains control flow statements, such as \`return\`, \`continue\`, - \`break\` or \`throw\`.`, + 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 control flow statements, - such as \`return\`, \`continue\`, \`break\` or \`throw\` is written inside \`if\` block, - Then \`else\` block becomes unnecessary.`, + 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, @@ -39,7 +38,7 @@ export class Rule extends Lint.Rules.AbstractRule { /* tslint:disable:object-literal-sort-keys */ public static FAILURE_STRING(name: string): string { - return `If block contains \`${name}\` statement. Consider replacing the contents of else block outside of the block.`; + return `The preceding \`if\` block ends with a \`${name}\` statement. This \`else\` blockis unnecessary.`; } public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { From b46d5385f6d5848136581f593a1a465db4990ffe Mon Sep 17 00:00:00 2001 From: Debsmita Date: Sun, 10 Feb 2019 00:24:05 +0530 Subject: [PATCH 06/14] Added codeExamples --- .../code-examples/unnecessaryElse.examples.ts | 76 +++++++++++++++++++ src/rules/unnecessaryElseRule.ts | 5 +- 2 files changed, 80 insertions(+), 1 deletion(-) create mode 100644 src/rules/code-examples/unnecessaryElse.examples.ts diff --git a/src/rules/code-examples/unnecessaryElse.examples.ts b/src/rules/code-examples/unnecessaryElse.examples.ts new file mode 100644 index 00000000000..ca7a5ea10ce --- /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" blocks following "if" blocks ending with "return", "break", "continue" or "throw" statement. ', + config: Lint.Utils.dedent` + "rules": { "unnecessary-else": true } + `, + pass: Lint.Utils.dedent` + if () { + return ; + } + return; + + if () { + continue; + } + // some code here + + if () { + throw; + } + // some code here + + if () { + break; + } + // some code here + + `, + fail: Lint.Utils.dedent` + if () { + return; + } else { + + } + + if () { + break; + } else { + + } + + if () { + throw; + } else { + + } + + if () { + continue; + } else { + + } + `, + }, +]; diff --git a/src/rules/unnecessaryElseRule.ts b/src/rules/unnecessaryElseRule.ts index 92eef0ec4a2..6b30dc9344a 100644 --- a/src/rules/unnecessaryElseRule.ts +++ b/src/rules/unnecessaryElseRule.ts @@ -18,6 +18,8 @@ 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 = { @@ -34,11 +36,12 @@ export class Rule extends Lint.Rules.AbstractRule { 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\` blockis unnecessary.`; + return `The preceding \`if\` block ends with a \`${name}\` statement. This \`else\` block is unnecessary.`; } public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { From 89c814eb7648cbe716af317adb05add694305430 Mon Sep 17 00:00:00 2001 From: Debsmita Date: Tue, 12 Feb 2019 12:27:20 +0530 Subject: [PATCH 07/14] Changed the Logic and Added new testcases --- src/rules/unnecessaryElseRule.ts | 64 ++++---- test/rules/unnecessary-else/test.ts.lint | 180 ++++++++++++++--------- 2 files changed, 139 insertions(+), 105 deletions(-) diff --git a/src/rules/unnecessaryElseRule.ts b/src/rules/unnecessaryElseRule.ts index 6b30dc9344a..2242692e8c8 100644 --- a/src/rules/unnecessaryElseRule.ts +++ b/src/rules/unnecessaryElseRule.ts @@ -14,6 +14,7 @@ * 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"; @@ -49,44 +50,27 @@ export class Rule extends Lint.Rules.AbstractRule { } } -type JumpStatement = - | ts.BreakStatement - | ts.ContinueStatement - | ts.ThrowStatement - | ts.ReturnStatement; - function walk(ctx: Lint.WalkContext): void { - let inElse = false; ts.forEachChild(ctx.sourceFile, function cb(node: ts.Node): void { - switch (node.kind) { - case ts.SyntaxKind.IfStatement: - const { thenStatement, elseStatement } = node as ts.IfStatement; - if (elseStatement !== undefined) { - inElse = true; - } - ts.forEachChild(thenStatement, cb); - break; - - case ts.SyntaxKind.BreakStatement: - case ts.SyntaxKind.ContinueStatement: - case ts.SyntaxKind.ThrowStatement: - case ts.SyntaxKind.ReturnStatement: - if (inElse) { - ctx.addFailureAtNode( - node, - Rule.FAILURE_STRING(printJumpKind(node as JumpStatement)), - ); - inElse = false; - } - break; - - default: - return ts.forEachChild(node, cb); + if (utils.isIfStatement(node)) { + const jumpStatement = getJumpStatement( + getLastStatement(node.thenStatement as ts.Block), + ); + if (jumpStatement !== undefined && node.elseStatement !== undefined) { + ctx.addFailureAtNode( + node.elseStatement.getChildAt(0), + Rule.FAILURE_STRING(jumpStatement), + ); + ts.forEachChild(node.elseStatement, cb); + } + ts.forEachChild(node.thenStatement, cb); + } else { + return ts.forEachChild(node, cb); } }); } -function printJumpKind(node: JumpStatement): string { +function getJumpStatement(node: ts.Statement): string | undefined { switch (node.kind) { case ts.SyntaxKind.BreakStatement: return "break"; @@ -96,5 +80,21 @@ function printJumpKind(node: JumpStatement): string { 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 index 59d8be517b1..a623baa3398 100644 --- a/test/rules/unnecessary-else/test.ts.lint +++ b/test/rules/unnecessary-else/test.ts.lint @@ -1,83 +1,90 @@ -const testReturn = () => { - if () { - return; - ~~~~~~~ [return] +const testReturn = (a) => { + if (a===0) { + return 0; } else { - return; + ~ [return] + return a; } } -const testReturn = () => { - if () { - if () { - return; - ~~~~~~~ [return] +const testReturn = (a) => { + if (a>0) { + if (a%2 ===0) { + return "even" ; } else { - return; + ~ [return] + return "odd"; } } + return "negative"; } -const testReturn = () => { - if () { - return; +const testReturn = (a) => { + if (a===0) { + return 0; } - return; + return a; } -const testReturn = () => { - if () { +const testReturn = (a) => { + if (a<0) { return; - ~~~~~~~ [return] - } else if { - if () { + } else if (a>0) { + ~~ [return] + if (a%2 === 0) { return ; } else { + ~ [return] return ; } } return; } -const testReturn = () => { - if () { +const testReturn = (a) => { + if (a<0) { return; } - if () { + if (a===1) { return ; - ~~~~~~~~ [return] } else { + ~ [return] return ; } } -const testReturn = () => { - if () { - return; +const testReturn = (a) => { + if (a>0) { + if (a%3===0) { + return; + } else { + ~ [return] + console.log(a) + } } - if () { - return ; + else { + console.log("negative"); } - return ; } -function testThrow () { - if () { +const testThrow = (a) => { + if ( a===0 ) { throw "error"; - ~~~~~~~~~~~~~~ [throw] } else { + ~ [throw] + return 100/a; } } -const testThrow = () => { - switch () { - case : +const testThrow = (a) => { + switch (a) { + case 1: break; - case : - if () { + case 2: + if (true) { throw "error"; - ~~~~~~~~~~~~~~ [throw] } else { + ~ [throw] break; } default : @@ -85,61 +92,88 @@ const testThrow = () => { } } -function testThrow () { - if () { +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; } -function testContinue () { - for ( ) { - if () { +const testContinue = () => { + for (let i = 1; i < 10; i++) { + if (i===8) { continue ; - ~~~~~~~~~~ [continue] } else { + ~ [continue] + console.log(i); } } } -function testContinue () { - for ( ) { - if () { +const testContinue = () => { + for (let i = 1; i < 10; i++) { + if (i===4) { continue ; } + console.log(i); } } -function testBreak () { - if () { - break ; - ~~~~~~~ [break] - } else { - } +const testBreak = (a) => { + let i = 0; + while(i < 20) { + if (i === a) { + break ; + } else { + ~ [break] + i++; + } + } + return i-1; } -const testBreak = () => { - switch () { - case : - break; - case : - if () { - break; - ~~~~~~ [break] +const testBreak = (a) => { + let i = 0; + while(i !== a) { + if (i % 2 === 0) { + if (i % 5 === 0) { + return i; } else { - + ~ [return] + return i/2; } - default : - break; + } else { + i++; + } } + return i-1; } -function testBreak () { - if () { - break ; - } +const testBreak = (a) => { + let i = 0; + while(i < 20) { + if (i === a) { + break ; + } + i++; + } + return i-1; } -[return]: If block contains `return` statement. Consider replacing the contents of else block outside of the block. -[throw]: If block contains `throw` statement. Consider replacing the contents of else block outside of the block. -[break]: If block contains `break` statement. Consider replacing the contents of else block outside of the block. -[continue]: If block contains `continue` statement. Consider replacing the contents of else block outside of the block. +[return]: The preceding `if` block ends with a `return` statement. This `else` block is unnecessary. +[throw]: The preceding `if` block ends with a `throw` statement. This `else` block is unnecessary. +[break]: The preceding `if` block ends with a `break` statement. This `else` block is unnecessary. +[continue]: The preceding `if` block ends with a `continue` statement. This `else` block is unnecessary. From 2e0825f3f496e600d97e701f7dab7ec8d9201e6b Mon Sep 17 00:00:00 2001 From: Debsmita Date: Sun, 24 Feb 2019 16:12:49 +0530 Subject: [PATCH 08/14] Incorporated review comments and Added more testcases --- src/rules/unnecessaryElseRule.ts | 35 +++++-- test/rules/unnecessary-else/test.ts.lint | 125 ++++++++++++++++++----- 2 files changed, 128 insertions(+), 32 deletions(-) diff --git a/src/rules/unnecessaryElseRule.ts b/src/rules/unnecessaryElseRule.ts index 2242692e8c8..bce8053f577 100644 --- a/src/rules/unnecessaryElseRule.ts +++ b/src/rules/unnecessaryElseRule.ts @@ -53,24 +53,41 @@ export class Rule extends Lint.Rules.AbstractRule { function walk(ctx: Lint.WalkContext): void { ts.forEachChild(ctx.sourceFile, function cb(node: ts.Node): void { if (utils.isIfStatement(node)) { - const jumpStatement = getJumpStatement( - getLastStatement(node.thenStatement as ts.Block), - ); - if (jumpStatement !== undefined && node.elseStatement !== undefined) { - ctx.addFailureAtNode( - node.elseStatement.getChildAt(0), - Rule.FAILURE_STRING(jumpStatement), + let jumpStatement; + if ( + node.thenStatement.kind !== ts.SyntaxKind.Block && + !utils.isSameLine(ctx.sourceFile, node.thenStatement.pos, node.thenStatement.end) + ) { + jumpStatement = getJumpStatement( + node.thenStatement.getChildAt(0, ctx.sourceFile).parent, ); - ts.forEachChild(node.elseStatement, cb); + } else if ( + utils.isSameLine(ctx.sourceFile, node.thenStatement.pos, node.thenStatement.end) + ) { + jumpStatement = getJumpStatement(node.thenStatement); + } else { + jumpStatement = getJumpStatement(getLastStatement(node.thenStatement as ts.Block)); + } + if (jumpStatement !== undefined && node.elseStatement !== undefined) { + const elseKeyword = getPositionOfElseKeyword(node, ts.SyntaxKind.ElseKeyword); + ctx.addFailureAtNode(elseKeyword, Rule.FAILURE_STRING(jumpStatement)); } + ts.forEachChild(node.expression, cb); ts.forEachChild(node.thenStatement, cb); + if (node.elseStatement !== undefined) { + ts.forEachChild(node.elseStatement, cb); + } } else { return ts.forEachChild(node, cb); } }); } -function getJumpStatement(node: ts.Statement): string | undefined { +function getPositionOfElseKeyword(node: ts.Node, kind: ts.SyntaxKind) { + return node.getChildren().filter(child => child.kind === kind)[0]; +} + +function getJumpStatement(node: ts.Statement | ts.Node): string | undefined { switch (node.kind) { case ts.SyntaxKind.BreakStatement: return "break"; diff --git a/test/rules/unnecessary-else/test.ts.lint b/test/rules/unnecessary-else/test.ts.lint index a623baa3398..f99e7fd7feb 100644 --- a/test/rules/unnecessary-else/test.ts.lint +++ b/test/rules/unnecessary-else/test.ts.lint @@ -2,17 +2,31 @@ const testReturn = (a) => { if (a===0) { return 0; } else { - ~ [return] + ~~~~ [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] return "odd"; } } @@ -30,11 +44,11 @@ const testReturn = (a) => { if (a<0) { return; } else if (a>0) { - ~~ [return] + ~~~~ [return] if (a%2 === 0) { return ; } else { - ~ [return] + ~~~~ [return] return ; } } @@ -48,7 +62,7 @@ const testReturn = (a) => { if (a===1) { return ; } else { - ~ [return] + ~~~~ [return] return ; } } @@ -58,7 +72,7 @@ const testReturn = (a) => { if (a%3===0) { return; } else { - ~ [return] + ~~~~ [return] console.log(a) } } @@ -71,11 +85,25 @@ const testThrow = (a) => { if ( a===0 ) { throw "error"; } else { - ~ [throw] + ~~~~ [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: @@ -84,7 +112,7 @@ const testThrow = (a) => { if (true) { throw "error"; } else { - ~ [throw] + ~~~~ [throw] break; } default : @@ -98,7 +126,7 @@ const testThrow = (a) => { if (a-i === 0) { throw "error; } else { - ~ [throw] + ~~~~ [throw] console.log(i/a-i); } ++i; @@ -112,17 +140,40 @@ const testThrow = (a) => { 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] + ~~~~ [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) { @@ -132,13 +183,21 @@ const testContinue = () => { } } +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] + ~~~~ [break] i++; } } @@ -147,17 +206,11 @@ const testBreak = (a) => { const testBreak = (a) => { let i = 0; - while(i !== a) { - if (i % 2 === 0) { - if (i % 5 === 0) { - return i; - } else { - ~ [return] - return i/2; - } - } else { - i++; + while(i < 20) { + if (i === a) { + break ; } + i++; } return i-1; } @@ -165,14 +218,40 @@ const testBreak = (a) => { const testBreak = (a) => { let i = 0; while(i < 20) { - if (i === a) { + 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` block is unnecessary. [throw]: The preceding `if` block ends with a `throw` statement. This `else` block is unnecessary. [break]: The preceding `if` block ends with a `break` statement. This `else` block is unnecessary. From 7578c43c369d6fb5dc7dfe268e0670cad861a245 Mon Sep 17 00:00:00 2001 From: Debsmita Date: Sun, 24 Feb 2019 16:25:22 +0530 Subject: [PATCH 09/14] Revert "Incorporated review comments and Added more testcases" This reverts commit 2e0825f3f496e600d97e701f7dab7ec8d9201e6b. --- src/rules/unnecessaryElseRule.ts | 35 ++----- test/rules/unnecessary-else/test.ts.lint | 125 +++++------------------ 2 files changed, 32 insertions(+), 128 deletions(-) diff --git a/src/rules/unnecessaryElseRule.ts b/src/rules/unnecessaryElseRule.ts index bce8053f577..2242692e8c8 100644 --- a/src/rules/unnecessaryElseRule.ts +++ b/src/rules/unnecessaryElseRule.ts @@ -53,41 +53,24 @@ export class Rule extends Lint.Rules.AbstractRule { function walk(ctx: Lint.WalkContext): void { ts.forEachChild(ctx.sourceFile, function cb(node: ts.Node): void { if (utils.isIfStatement(node)) { - let jumpStatement; - if ( - node.thenStatement.kind !== ts.SyntaxKind.Block && - !utils.isSameLine(ctx.sourceFile, node.thenStatement.pos, node.thenStatement.end) - ) { - jumpStatement = getJumpStatement( - node.thenStatement.getChildAt(0, ctx.sourceFile).parent, - ); - } else if ( - utils.isSameLine(ctx.sourceFile, node.thenStatement.pos, node.thenStatement.end) - ) { - jumpStatement = getJumpStatement(node.thenStatement); - } else { - jumpStatement = getJumpStatement(getLastStatement(node.thenStatement as ts.Block)); - } + const jumpStatement = getJumpStatement( + getLastStatement(node.thenStatement as ts.Block), + ); if (jumpStatement !== undefined && node.elseStatement !== undefined) { - const elseKeyword = getPositionOfElseKeyword(node, ts.SyntaxKind.ElseKeyword); - ctx.addFailureAtNode(elseKeyword, Rule.FAILURE_STRING(jumpStatement)); - } - ts.forEachChild(node.expression, cb); - ts.forEachChild(node.thenStatement, cb); - if (node.elseStatement !== undefined) { + ctx.addFailureAtNode( + node.elseStatement.getChildAt(0), + Rule.FAILURE_STRING(jumpStatement), + ); ts.forEachChild(node.elseStatement, cb); } + ts.forEachChild(node.thenStatement, cb); } else { 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 | ts.Node): string | undefined { +function getJumpStatement(node: ts.Statement): string | undefined { switch (node.kind) { case ts.SyntaxKind.BreakStatement: return "break"; diff --git a/test/rules/unnecessary-else/test.ts.lint b/test/rules/unnecessary-else/test.ts.lint index f99e7fd7feb..a623baa3398 100644 --- a/test/rules/unnecessary-else/test.ts.lint +++ b/test/rules/unnecessary-else/test.ts.lint @@ -2,31 +2,17 @@ const testReturn = (a) => { if (a===0) { return 0; } else { - ~~~~ [return] + ~ [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] return "odd"; } } @@ -44,11 +30,11 @@ const testReturn = (a) => { if (a<0) { return; } else if (a>0) { - ~~~~ [return] + ~~ [return] if (a%2 === 0) { return ; } else { - ~~~~ [return] + ~ [return] return ; } } @@ -62,7 +48,7 @@ const testReturn = (a) => { if (a===1) { return ; } else { - ~~~~ [return] + ~ [return] return ; } } @@ -72,7 +58,7 @@ const testReturn = (a) => { if (a%3===0) { return; } else { - ~~~~ [return] + ~ [return] console.log(a) } } @@ -85,25 +71,11 @@ const testThrow = (a) => { if ( a===0 ) { throw "error"; } else { - ~~~~ [throw] + ~ [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: @@ -112,7 +84,7 @@ const testThrow = (a) => { if (true) { throw "error"; } else { - ~~~~ [throw] + ~ [throw] break; } default : @@ -126,7 +98,7 @@ const testThrow = (a) => { if (a-i === 0) { throw "error; } else { - ~~~~ [throw] + ~ [throw] console.log(i/a-i); } ++i; @@ -140,40 +112,17 @@ const testThrow = (a) => { 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] + ~ [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) { @@ -183,21 +132,13 @@ const testContinue = () => { } } -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] + ~ [break] i++; } } @@ -206,11 +147,17 @@ const testBreak = (a) => { const testBreak = (a) => { let i = 0; - while(i < 20) { - if (i === a) { - break ; + while(i !== a) { + if (i % 2 === 0) { + if (i % 5 === 0) { + return i; + } else { + ~ [return] + return i/2; + } + } else { + i++; } - i++; } return i-1; } @@ -218,40 +165,14 @@ const testBreak = (a) => { const testBreak = (a) => { let i = 0; while(i < 20) { - if (i === a) + 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` block is unnecessary. [throw]: The preceding `if` block ends with a `throw` statement. This `else` block is unnecessary. [break]: The preceding `if` block ends with a `break` statement. This `else` block is unnecessary. From be2f8b4323b7b534130489a8467a91b6962d745e Mon Sep 17 00:00:00 2001 From: Debsmita Date: Sun, 24 Feb 2019 16:29:44 +0530 Subject: [PATCH 10/14] Incorporated review comments and Added more Testcases --- src/rules/unnecessaryElseRule.ts | 35 +++++-- test/rules/unnecessary-else/test.ts.lint | 125 ++++++++++++++++++----- 2 files changed, 128 insertions(+), 32 deletions(-) diff --git a/src/rules/unnecessaryElseRule.ts b/src/rules/unnecessaryElseRule.ts index 2242692e8c8..bce8053f577 100644 --- a/src/rules/unnecessaryElseRule.ts +++ b/src/rules/unnecessaryElseRule.ts @@ -53,24 +53,41 @@ export class Rule extends Lint.Rules.AbstractRule { function walk(ctx: Lint.WalkContext): void { ts.forEachChild(ctx.sourceFile, function cb(node: ts.Node): void { if (utils.isIfStatement(node)) { - const jumpStatement = getJumpStatement( - getLastStatement(node.thenStatement as ts.Block), - ); - if (jumpStatement !== undefined && node.elseStatement !== undefined) { - ctx.addFailureAtNode( - node.elseStatement.getChildAt(0), - Rule.FAILURE_STRING(jumpStatement), + let jumpStatement; + if ( + node.thenStatement.kind !== ts.SyntaxKind.Block && + !utils.isSameLine(ctx.sourceFile, node.thenStatement.pos, node.thenStatement.end) + ) { + jumpStatement = getJumpStatement( + node.thenStatement.getChildAt(0, ctx.sourceFile).parent, ); - ts.forEachChild(node.elseStatement, cb); + } else if ( + utils.isSameLine(ctx.sourceFile, node.thenStatement.pos, node.thenStatement.end) + ) { + jumpStatement = getJumpStatement(node.thenStatement); + } else { + jumpStatement = getJumpStatement(getLastStatement(node.thenStatement as ts.Block)); + } + if (jumpStatement !== undefined && node.elseStatement !== undefined) { + const elseKeyword = getPositionOfElseKeyword(node, ts.SyntaxKind.ElseKeyword); + ctx.addFailureAtNode(elseKeyword, Rule.FAILURE_STRING(jumpStatement)); } + ts.forEachChild(node.expression, cb); ts.forEachChild(node.thenStatement, cb); + if (node.elseStatement !== undefined) { + ts.forEachChild(node.elseStatement, cb); + } } else { return ts.forEachChild(node, cb); } }); } -function getJumpStatement(node: ts.Statement): string | undefined { +function getPositionOfElseKeyword(node: ts.Node, kind: ts.SyntaxKind) { + return node.getChildren().filter(child => child.kind === kind)[0]; +} + +function getJumpStatement(node: ts.Statement | ts.Node): string | undefined { switch (node.kind) { case ts.SyntaxKind.BreakStatement: return "break"; diff --git a/test/rules/unnecessary-else/test.ts.lint b/test/rules/unnecessary-else/test.ts.lint index a623baa3398..f99e7fd7feb 100644 --- a/test/rules/unnecessary-else/test.ts.lint +++ b/test/rules/unnecessary-else/test.ts.lint @@ -2,17 +2,31 @@ const testReturn = (a) => { if (a===0) { return 0; } else { - ~ [return] + ~~~~ [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] return "odd"; } } @@ -30,11 +44,11 @@ const testReturn = (a) => { if (a<0) { return; } else if (a>0) { - ~~ [return] + ~~~~ [return] if (a%2 === 0) { return ; } else { - ~ [return] + ~~~~ [return] return ; } } @@ -48,7 +62,7 @@ const testReturn = (a) => { if (a===1) { return ; } else { - ~ [return] + ~~~~ [return] return ; } } @@ -58,7 +72,7 @@ const testReturn = (a) => { if (a%3===0) { return; } else { - ~ [return] + ~~~~ [return] console.log(a) } } @@ -71,11 +85,25 @@ const testThrow = (a) => { if ( a===0 ) { throw "error"; } else { - ~ [throw] + ~~~~ [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: @@ -84,7 +112,7 @@ const testThrow = (a) => { if (true) { throw "error"; } else { - ~ [throw] + ~~~~ [throw] break; } default : @@ -98,7 +126,7 @@ const testThrow = (a) => { if (a-i === 0) { throw "error; } else { - ~ [throw] + ~~~~ [throw] console.log(i/a-i); } ++i; @@ -112,17 +140,40 @@ const testThrow = (a) => { 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] + ~~~~ [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) { @@ -132,13 +183,21 @@ const testContinue = () => { } } +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] + ~~~~ [break] i++; } } @@ -147,17 +206,11 @@ const testBreak = (a) => { const testBreak = (a) => { let i = 0; - while(i !== a) { - if (i % 2 === 0) { - if (i % 5 === 0) { - return i; - } else { - ~ [return] - return i/2; - } - } else { - i++; + while(i < 20) { + if (i === a) { + break ; } + i++; } return i-1; } @@ -165,14 +218,40 @@ const testBreak = (a) => { const testBreak = (a) => { let i = 0; while(i < 20) { - if (i === a) { + 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` block is unnecessary. [throw]: The preceding `if` block ends with a `throw` statement. This `else` block is unnecessary. [break]: The preceding `if` block ends with a `break` statement. This `else` block is unnecessary. From f36d4201e2fa8ec3aec60a022db2bf51435aceaf Mon Sep 17 00:00:00 2001 From: Debsmita Date: Mon, 25 Feb 2019 00:37:01 +0530 Subject: [PATCH 11/14] Modified the logic and error message --- src/rules/unnecessaryElseRule.ts | 10 ++-------- test/rules/unnecessary-else/test.ts.lint | 8 ++++---- test/rules/unnecessary-else/tslint.json | 3 +-- 3 files changed, 7 insertions(+), 14 deletions(-) diff --git a/src/rules/unnecessaryElseRule.ts b/src/rules/unnecessaryElseRule.ts index bce8053f577..29fdb08f885 100644 --- a/src/rules/unnecessaryElseRule.ts +++ b/src/rules/unnecessaryElseRule.ts @@ -42,7 +42,7 @@ export class Rule extends Lint.Rules.AbstractRule { /* 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\` block is unnecessary.`; + return `The preceding \`if\` block ends with a \`${name}\` statement. This \`else\` is unnecessary.`; } public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { @@ -72,14 +72,8 @@ function walk(ctx: Lint.WalkContext): void { const elseKeyword = getPositionOfElseKeyword(node, ts.SyntaxKind.ElseKeyword); ctx.addFailureAtNode(elseKeyword, Rule.FAILURE_STRING(jumpStatement)); } - ts.forEachChild(node.expression, cb); - ts.forEachChild(node.thenStatement, cb); - if (node.elseStatement !== undefined) { - ts.forEachChild(node.elseStatement, cb); - } - } else { - return ts.forEachChild(node, cb); } + return ts.forEachChild(node, cb); }); } diff --git a/test/rules/unnecessary-else/test.ts.lint b/test/rules/unnecessary-else/test.ts.lint index f99e7fd7feb..959dd5ce5ff 100644 --- a/test/rules/unnecessary-else/test.ts.lint +++ b/test/rules/unnecessary-else/test.ts.lint @@ -252,7 +252,7 @@ const testNoJump = (a) => { } } -[return]: The preceding `if` block ends with a `return` statement. This `else` block is unnecessary. -[throw]: The preceding `if` block ends with a `throw` statement. This `else` block is unnecessary. -[break]: The preceding `if` block ends with a `break` statement. This `else` block is unnecessary. -[continue]: The preceding `if` block ends with a `continue` statement. This `else` block is unnecessary. +[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 index 959c1d4cb5c..5e3f2360d32 100644 --- a/test/rules/unnecessary-else/tslint.json +++ b/test/rules/unnecessary-else/tslint.json @@ -2,5 +2,4 @@ "rules": { "unnecessary-else": true } - } - \ No newline at end of file +} From a2842a9ad5ba17d0bb597296ad99b7bc0bbe5142 Mon Sep 17 00:00:00 2001 From: Debsmita Date: Mon, 25 Feb 2019 00:44:22 +0530 Subject: [PATCH 12/14] Modified Code Examples --- .../code-examples/unnecessaryElse.examples.ts | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/rules/code-examples/unnecessaryElse.examples.ts b/src/rules/code-examples/unnecessaryElse.examples.ts index ca7a5ea10ce..260b14b596e 100644 --- a/src/rules/code-examples/unnecessaryElse.examples.ts +++ b/src/rules/code-examples/unnecessaryElse.examples.ts @@ -21,55 +21,55 @@ import * as Lint from "../../index"; export const codeExamples = [ { description: - 'Disallows "else" blocks following "if" blocks ending with "return", "break", "continue" or "throw" statement. ', + '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 () { - return ; + if (someCondition()) { + return; } - return; + // some code here - if () { + if (someCondition()) { continue; } // some code here - if () { + if (someCondition()) { throw; } // some code here - if () { + if (someCondition()) { break; } // some code here `, fail: Lint.Utils.dedent` - if () { + if (someCondition()) { return; } else { - + // some code here } - if () { + if (someCondition()) { break; } else { - + // some code here } - if () { + if (someCondition()) { throw; } else { - + // some code here } - if () { + if (someCondition()) { continue; } else { - + // some code here } `, }, From d7425c2449c1b6e34e09fb963fc226a5b8fd9b93 Mon Sep 17 00:00:00 2001 From: Debsmita Date: Mon, 25 Feb 2019 01:44:59 +0530 Subject: [PATCH 13/14] Fixed linting errors --- 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 +- 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/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 ++- 31 files changed, 218 insertions(+), 232 deletions(-) diff --git a/src/configuration.ts b/src/configuration.ts index 52f31f1dd44..95279465418 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 322c358dc6f..7044a5232d8 100644 --- a/src/linter.ts +++ b/src/linter.ts @@ -285,9 +285,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); @@ -325,9 +324,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/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 cfa9d51d9a3..c4364b78771 100644 --- a/src/rules/quotemarkRule.ts +++ b/src/rules/quotemarkRule.ts @@ -148,13 +148,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 cbdf04c3a7f..bfd69655815 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/useDefaultTypeParameterRule.ts b/src/rules/useDefaultTypeParameterRule.ts index 2c1c134e2f4..4c54feeb398 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; } } } From 4c29b4309e698117979d5bd9a8e62b9fbc337b1f Mon Sep 17 00:00:00 2001 From: Debsmita Date: Mon, 25 Feb 2019 02:16:41 +0530 Subject: [PATCH 14/14] Optimized the rule logic --- src/rules/unnecessaryElseRule.ts | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/src/rules/unnecessaryElseRule.ts b/src/rules/unnecessaryElseRule.ts index 29fdb08f885..b7690a0021a 100644 --- a/src/rules/unnecessaryElseRule.ts +++ b/src/rules/unnecessaryElseRule.ts @@ -53,21 +53,10 @@ export class Rule extends Lint.Rules.AbstractRule { function walk(ctx: Lint.WalkContext): void { ts.forEachChild(ctx.sourceFile, function cb(node: ts.Node): void { if (utils.isIfStatement(node)) { - let jumpStatement; - if ( - node.thenStatement.kind !== ts.SyntaxKind.Block && - !utils.isSameLine(ctx.sourceFile, node.thenStatement.pos, node.thenStatement.end) - ) { - jumpStatement = getJumpStatement( - node.thenStatement.getChildAt(0, ctx.sourceFile).parent, - ); - } else if ( - utils.isSameLine(ctx.sourceFile, node.thenStatement.pos, node.thenStatement.end) - ) { - jumpStatement = getJumpStatement(node.thenStatement); - } else { - jumpStatement = getJumpStatement(getLastStatement(node.thenStatement as ts.Block)); - } + 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)); @@ -81,7 +70,7 @@ function getPositionOfElseKeyword(node: ts.Node, kind: ts.SyntaxKind) { return node.getChildren().filter(child => child.kind === kind)[0]; } -function getJumpStatement(node: ts.Statement | ts.Node): string | undefined { +function getJumpStatement(node: ts.Statement): string | undefined { switch (node.kind) { case ts.SyntaxKind.BreakStatement: return "break";