Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Added unnecessary-else Rule #4502

Merged
merged 20 commits into from Mar 2, 2019
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
5bd58d2
Added no-unnecessary-else Rule
debsmita1 Feb 4, 2019
021b97c
* Updated the branch with upstream/master
debsmita1 Feb 5, 2019
90e9d7a
Added 'unnecessary-else rule' under latest.ts instead of recommended.ts
debsmita1 Feb 8, 2019
ee45076
Changed year to 2019 under License
debsmita1 Feb 9, 2019
8d4f035
Changed name of the rule to 'unnecessary-else'
debsmita1 Feb 9, 2019
c2db576
Removed comments like valid from test.ts.lint file
debsmita1 Feb 9, 2019
a1ec00c
Modified the description, rationale and the FAILURE_STRING
debsmita1 Feb 9, 2019
b46d538
Added codeExamples
debsmita1 Feb 9, 2019
89c814e
Changed the Logic and Added new testcases
debsmita1 Feb 12, 2019
0f7d798
Merge branch 'master' of github.com:palantir/tslint into no-unnecessa…
debsmita1 Feb 16, 2019
2e0825f
Incorporated review comments and Added more testcases
debsmita1 Feb 24, 2019
7578c43
Revert "Incorporated review comments and Added more testcases"
debsmita1 Feb 24, 2019
b281653
Merge branch 'master' of github.com:palantir/tslint into no-unnecessa…
debsmita1 Feb 24, 2019
be2f8b4
Incorporated review comments and Added more Testcases
debsmita1 Feb 24, 2019
f36d420
Modified the logic and error message
debsmita1 Feb 24, 2019
a2842a9
Modified Code Examples
debsmita1 Feb 24, 2019
d7425c2
Fixed linting errors
debsmita1 Feb 24, 2019
717baef
Merge branch 'master' of github.com:palantir/tslint into no-unnecessa…
debsmita1 Feb 24, 2019
4c29b43
Optimized the rule logic
debsmita1 Feb 24, 2019
418c234
Merge branch 'master' of github.com:palantir/tslint into no-unnecessa…
debsmita1 Feb 28, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions src/configs/all.ts
Expand Up @@ -152,6 +152,7 @@ export const rules = {
"unnecessary-constructor": true,
"use-default-type-parameter": true,
"use-isnan": true,
"unnecessary-else": true,

// Maintainability

Expand Down
1 change: 1 addition & 0 deletions src/configs/latest.ts
Expand Up @@ -67,6 +67,7 @@ export const rules = {
// added in v5.12
"function-constructor": true,
"unnecessary-bind": true,
"unnecessary-else": true,
};
// tslint:enable object-literal-sort-keys

Expand Down
76 changes: 76 additions & 0 deletions 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 () {
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
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 {

}
`,
},
];
117 changes: 117 additions & 0 deletions src/rules/unnecessaryElseRule.ts
@@ -0,0 +1,117 @@
/**
* @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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a purely stylistic rule; is there any functional difference?

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\` block is unnecessary.`;
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
}

public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
return this.applyWithFunction(sourceFile, walk);
}
}

function walk(ctx: Lint.WalkContext<void>): 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)
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
) {
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));
}
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you have a few different ts.forEachChilds: individual ones for the node's elseStatement and thenStatement if it's an IfStatement, but just a regular recursion otherwise? You should be able to simplify this a bit by only having one call to ts.forEachChild that's always called.

Also, if jumpStatement is undefined but node.elseStatement isn't, this code will skip it. That seems like a potential bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done be2f8b4

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @debsmita1 I think I wasn't very clear here. I'm suggesting you don't need to have ts.forEachChild statements in the big if (utils.isIfStatement block:

if (utils.isIfStatement(node)) {
    // all your glorious rule logic here
    // no ts.forEachChild in this area
}

ts.forEachChild(node, cb);

Note the lack of else statement: a single ts.forEachChild should recurse on all the node's children no matter what. Could you please switch to that? It's a bit less code and better practice to minimize recursive calls (since they can get tricky in larger rules).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done f36d420

}
});
}

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";
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<T>(arr: ReadonlyArray<T>): T {
return arr[arr.length - 1];
}