Skip to content

Commit aaa948d

Browse files
ajafffajafff
committed
Simplify fixer logic and reuse for no-else-after-return
1 parent 21bb83e commit aaa948d

File tree

7 files changed

+130
-104
lines changed

7 files changed

+130
-104
lines changed

‎rules/noElseAfterReturnRule.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,11 @@ export class Rule extends Lint.Rules.AbstractRule {
2323
class IfWalker extends AbstractIfStatementWalker<IOptions> {
2424
protected _checkIfStatement(node: ts.IfStatement) {
2525
if (shouldCheckNode(node, this.options.allowElseIf) && endsWithReturnStatement(node.thenStatement))
26-
this.addFailureAtNode(node.getChildAt(5 /*else*/, this.sourceFile), FAIL_MESSAGE);
26+
this._reportUnnecessaryElse(node.elseStatement, FAIL_MESSAGE);
2727
}
2828
}
2929

30-
function shouldCheckNode(node: ts.IfStatement, allowElseIf: boolean): boolean {
30+
function shouldCheckNode(node: ts.IfStatement, allowElseIf: boolean): node is ts.IfStatement & {elseStatement: {}} {
3131
if (node.elseStatement === undefined)
3232
return false;
3333
if (!allowElseIf)

‎rules/noUnnecessaryElseRule.ts‎

Lines changed: 4 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,10 @@
11
import * as ts from 'typescript';
22
import * as Lint from 'tslint';
3-
import * as utils from 'tsutils';
43
import { AbstractIfStatementWalker } from '../src/walker';
54
import { isElseIf } from '../src/utils';
5+
import { endsControlFlow } from 'tsutils';
66

77
const FAIL_MESSAGE = `unnecessary else`;
8-
const FAIL_MESSAGE_BLOCK = `unnecessary else block`;
98

109
export class Rule extends Lint.Rules.AbstractRule {
1110
public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
@@ -15,46 +14,8 @@ export class Rule extends Lint.Rules.AbstractRule {
1514

1615
class IfWalker extends AbstractIfStatementWalker<void> {
1716
protected _checkIfStatement(node: ts.IfStatement): void {
18-
if (isElseIf(node))
19-
return;
20-
21-
const elseStatement = node.elseStatement;
22-
if (isElseStatement(elseStatement) && utils.endsControlFlow(node.thenStatement)) {
23-
if (elseStatement !== undefined && utils.isBlock(elseStatement) && !hasLocals(elseStatement)) {
24-
// remove else scope if safe: keep blocks where local variables change scope when unwrapped
25-
const fixBlock = [
26-
Lint.Replacement.deleteFromTo(node.thenStatement.end, elseStatement.statements.pos), // removes `else {`
27-
Lint.Replacement.deleteText(elseStatement.end - 1, 1), // deletes `}`
28-
];
29-
this.addFailureAtNode(node.getChildAt(5 /*else*/, this.sourceFile), FAIL_MESSAGE_BLOCK, fixBlock);
30-
} else {
31-
// remove else only
32-
const fixElse = Lint.Replacement.deleteFromTo(node.thenStatement.getEnd(), elseStatement.getStart());
33-
this.addFailureAtNode(node.getChildAt(5 /*else*/, this.sourceFile), FAIL_MESSAGE, fixElse);
34-
}
35-
}
36-
}
37-
}
38-
39-
function isElseStatement(node: ts.Statement | undefined): node is ts.Statement {
40-
return node !== undefined;
41-
}
42-
43-
function hasLocals(node: ts.Block): boolean {
44-
for (const statement of node.statements) {
45-
switch (statement.kind) {
46-
case ts.SyntaxKind.VariableStatement:
47-
if (utils.isBlockScopedVariableDeclarationList((<ts.VariableStatement>statement).declarationList))
48-
return true;
49-
break;
50-
51-
case ts.SyntaxKind.ClassDeclaration:
52-
case ts.SyntaxKind.EnumDeclaration:
53-
case ts.SyntaxKind.InterfaceDeclaration:
54-
case ts.SyntaxKind.TypeAliasDeclaration:
55-
return true;
56-
}
17+
const {elseStatement} = node;
18+
if (elseStatement !== undefined && !isElseIf(node) && endsControlFlow(node.thenStatement))
19+
this._reportUnnecessaryElse(elseStatement, FAIL_MESSAGE);
5720
}
58-
59-
return false;
6021
}

‎src/walker.ts‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import * as ts from 'typescript';
22
import * as Lint from 'tslint';
3+
import { isBlock, isBlockScopedVariableDeclarationList } from 'tsutils';
34

45
export abstract class AbstractReturnStatementWalker<T> extends Lint.AbstractWalker<T> {
56
public walk(sourceFile: ts.SourceFile) {
@@ -24,5 +25,32 @@ export abstract class AbstractIfStatementWalker<T> extends Lint.AbstractWalker<T
2425
return ts.forEachChild(sourceFile, cb);
2526
}
2627

28+
protected _reportUnnecessaryElse(elseStatement: ts.Statement, message: string) {
29+
const elseKeyword = elseStatement.parent!.getChildAt(5 /*else*/, this.sourceFile);
30+
if (isBlock(elseStatement) && !elseStatement.statements.some(isBlockScopedDeclaration)) {
31+
// remove else scope if safe: keep blocks where local variables change scope when unwrapped
32+
this.addFailureAtNode(elseKeyword, message, [
33+
Lint.Replacement.deleteFromTo(elseKeyword.end - 4, elseStatement.statements.pos), // removes `else {`
34+
Lint.Replacement.deleteText(elseStatement.end - 1, 1), // removes `}`
35+
]);
36+
} else {
37+
// remove else only
38+
this.addFailureAtNode(elseKeyword, message, Lint.Replacement.deleteText(elseKeyword.end - 4, 4));
39+
}
40+
}
41+
2742
protected abstract _checkIfStatement(node: ts.IfStatement): void;
2843
}
44+
45+
function isBlockScopedDeclaration(statement: ts.Statement): boolean {
46+
switch (statement.kind) {
47+
case ts.SyntaxKind.VariableStatement:
48+
return isBlockScopedVariableDeclarationList((<ts.VariableStatement>statement).declarationList);
49+
case ts.SyntaxKind.ClassDeclaration:
50+
case ts.SyntaxKind.EnumDeclaration:
51+
case ts.SyntaxKind.InterfaceDeclaration:
52+
case ts.SyntaxKind.TypeAliasDeclaration:
53+
return true;
54+
default: return false;
55+
}
56+
}

‎test/rules/no-else-after-return/default/fix.ts.fix‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
if (condition) {
2+
return;
3+
}

‎test/rules/no-else-after-return/default/fix.ts.lint‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
if (condition) {
2+
return;
3+
} else {}
4+
~~~~ [unnecessary else after return]

‎test/rules/no-unnecessary-else/default/test.ts.fix‎

Lines changed: 46 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1,35 +1,48 @@
11
if (condition) {
22
return;
3-
}
3+
}
44

55
if (condition) {
66
return;
7-
}{
7+
} {
88
const foo = "bar";
99
}
1010

1111
if (condition) {
12-
return;
12+
return;
13+
}
14+
var foo = "bar";
15+
16+
17+
if (condition) {
18+
return;
19+
} {
20+
class C {}
1321
}
22+
23+
if (condition) {
24+
return;
25+
}
1426
{ const foo = "bar"; }
1527
return foo;
1628

1729

18-
if (condition) return;;
30+
if (condition) return;
31+
;
1932

2033
if (condition) {
2134
{
2235
return;
2336
}
24-
}
37+
}
2538

2639
if (condition) {
2740
if (condition) {
2841
return;
29-
}
42+
}
3043
return;
3144

32-
}
45+
}
3346

3447
if (condition) {
3548
if (condition) {
@@ -60,7 +73,7 @@ if (condition) {
6073
if (condition) {
6174
if (condition) {
6275
return;
63-
}
76+
}
6477

6578
} else {}
6679

@@ -78,7 +91,7 @@ if (condition) {
7891
default:
7992
return;
8093
}
81-
}
94+
}
8295

8396
if (condition) {
8497
switch (foo) {
@@ -87,7 +100,7 @@ if (condition) {
87100
case 'a':
88101
return;
89102
}
90-
}
103+
}
91104

92105
if (condition) {
93106
switch (foo) {
@@ -101,21 +114,21 @@ if (condition) {
101114
for (;;) {
102115
if (condition) {
103116
continue;
104-
}
117+
}
105118

106119
if (condition) {
107120
break;
108-
}
121+
}
109122

110123
switch (foo) {
111124
case 1:
112125
if (foo) {
113126
continue;
114-
}
127+
}
115128

116129
if (foo) {
117130
break;
118-
}
131+
}
119132
}
120133

121134
if (condition) {
@@ -128,7 +141,7 @@ for (;;) {
128141
case 2:
129142
return;
130143
}
131-
}
144+
}
132145

133146
if (condition) {
134147
switch (foo) {
@@ -143,10 +156,11 @@ for (;;) {
143156

144157
if (condition) {
145158
throw 'foo';
146-
}
159+
}
147160

148161
if (condition)
149-
throw 'foo';;
162+
throw 'foo';
163+
;
150164

151165
if (condition) {
152166
}else if (condition) {
@@ -157,7 +171,7 @@ if (condition) {
157171
}else{
158172
if (condition) {
159173
return;
160-
}
174+
}
161175
}
162176

163177
if (condition) {
@@ -180,7 +194,7 @@ if (condition) {
180194
} finally {
181195
return;
182196
}
183-
}
197+
}
184198

185199
if (condition) {
186200
try {
@@ -195,14 +209,14 @@ if (condition) {
195209
} finally {
196210
return;
197211
}
198-
}
212+
}
199213

200214
if (condition) {
201215
try {
202216
return;
203217
} finally {
204218
}
205-
}
219+
}
206220

207221
if (condition) {
208222
for (;;) {
@@ -234,7 +248,7 @@ if (condition) {
234248
return;
235249
}
236250
}
237-
}
251+
}
238252

239253
if (condition) {
240254
for (;;) {
@@ -250,7 +264,7 @@ if (condition) {
250264
foo: for (;;) {
251265
if (condition) {
252266
continue;
253-
}
267+
}
254268
break;
255269

256270
}
@@ -262,10 +276,11 @@ if (condition) {
262276
while (true) {
263277
if (condition) {
264278
continue foo;
265-
}
279+
}
266280
break foo;
267281

268282
}
283+
269284
}
270285
} else {}
271286

@@ -277,7 +292,7 @@ if (condition) {
277292
continue;
278293
if (condition) {
279294
continue foo;
280-
}
295+
}
281296
break foo;
282297

283298
}
@@ -291,7 +306,7 @@ foo: for (;;) {
291306
case 1:
292307
if (condition) {
293308
break;
294-
}
309+
}
295310
default:
296311
continue foo;
297312
}
@@ -330,30 +345,32 @@ if (condition) {
330345
} catch {
331346
throw 'foo';
332347
}
333-
}
348+
}
334349

335350
if (condition) {
336351
try {
337352
return foo();
338353
} finally {
339354
}
340-
}
355+
}
341356

342357
if (condition) {
343358
try {
344359
} finally {
345360
return;
346361
}
347-
}
362+
}
348363

349364
foo: bar: for (;;) {
350365
if (condition)
351366
for (;;)
352367
continue foo;
368+
353369

354370
if (condition)
355371
for (;;)
356372
break bar;
373+
357374

358375
if (condition)
359376
baz: bas: for (;;)

0 commit comments

Comments
 (0)