Add the ban-comma-operator rule #3250
Changes from 2 commits
cffa68e
7304abb
e7ac243
0a5f386
93f7e25
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
/** | ||
* @license | ||
* Copyright 2017 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 { isBinaryExpression } from "tsutils"; | ||
import * as ts from "typescript"; | ||
|
||
import * as Lint from "../index"; | ||
|
||
export class Rule extends Lint.Rules.AbstractRule { | ||
/* tslint:disable:object-literal-sort-keys */ | ||
public static metadata: Lint.IRuleMetadata = { | ||
ruleName: "ban-comma-operator", | ||
description: "Bans the comma operator.", | ||
options: null, | ||
optionsDescription: "", | ||
optionExamples: [true], | ||
type: "typescript", | ||
typescriptOnly: true, | ||
}; | ||
/* tslint:enable:object-literal-sort-keys */ | ||
|
||
public static FAILURE_STRING = "Don't use the comma operator."; | ||
|
||
public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { | ||
return this.applyWithFunction(sourceFile, walk); | ||
} | ||
} | ||
|
||
function walk(ctx: Lint.WalkContext<void>) { | ||
return ts.forEachChild(ctx.sourceFile, function cb(node: ts.Node): void { | ||
if (isBinaryExpression(node) && node.operatorToken.kind === ts.SyntaxKind.CommaToken) { | ||
ctx.addFailureAtNode(node, Rule.FAILURE_STRING); | ||
} | ||
return ts.forEachChild(node, cb); | ||
}); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,7 +26,7 @@ export class Rule extends Lint.Rules.AbstractRule { | |
ruleName: "no-conditional-assignment", | ||
description: "Disallows any type of assignment in conditionals.", | ||
descriptionDetails: "This applies to `do-while`, `for`, `if`, and `while` statements and conditional (ternary) expressions.", | ||
rationale: Lint.Utils.dedent ` | ||
rationale: Lint.Utils.dedent` | ||
Assignments in conditionals are often typos: | ||
for example \`if (var1 = var2)\` instead of \`if (var1 == var2)\`. | ||
They also can be an indicator of overly clever code which decreases maintainability.`, | ||
|
@@ -56,39 +56,48 @@ function walk(ctx: Lint.WalkContext<void>) { | |
} | ||
switch (kind) { | ||
case ts.SyntaxKind.ConditionalExpression: | ||
return check((node as ts.ConditionalExpression).condition), | ||
cb((node as ts.ConditionalExpression).whenTrue), | ||
cb((node as ts.ConditionalExpression).whenFalse); | ||
check((node as ts.ConditionalExpression).condition); | ||
cb((node as ts.ConditionalExpression).whenTrue); | ||
cb((node as ts.ConditionalExpression).whenFalse); | ||
return; | ||
case ts.SyntaxKind.IfStatement: | ||
return check((node as ts.IfStatement).expression), | ||
cb((node as ts.IfStatement).thenStatement), | ||
maybeCallback(cb, (node as ts.IfStatement).elseStatement); | ||
check((node as ts.IfStatement).expression); | ||
cb((node as ts.IfStatement).thenStatement); | ||
maybeCallback(cb, (node as ts.IfStatement).elseStatement); | ||
return; | ||
case ts.SyntaxKind.DoStatement: | ||
case ts.SyntaxKind.WhileStatement: | ||
return check((node as ts.DoStatement | ts.WhileStatement).expression), | ||
cb((node as ts.IterationStatement).statement); | ||
check((node as ts.DoStatement | ts.WhileStatement).expression); | ||
cb((node as ts.IterationStatement).statement); | ||
return; | ||
case ts.SyntaxKind.ForStatement: | ||
return maybeCallback(cb, (node as ts.ForStatement).initializer), | ||
maybeCallback(check, (node as ts.ForStatement).condition), | ||
maybeCallback(cb, (node as ts.ForStatement).incrementor), | ||
cb((node as ts.ForStatement).statement); | ||
maybeCallback(cb, (node as ts.ForStatement).initializer); | ||
maybeCallback(check, (node as ts.ForStatement).condition); | ||
maybeCallback(cb, (node as ts.ForStatement).incrementor); | ||
cb((node as ts.ForStatement).statement); | ||
return; | ||
} | ||
if (checking !== 0) { | ||
switch (kind) { | ||
case ts.SyntaxKind.BinaryExpression: | ||
if (isAssignmentKind((node as ts.BinaryExpression).operatorToken.kind)) { | ||
ctx.addFailureAtNode(node, Rule.FAILURE_STRING); | ||
} | ||
return cb((node as ts.BinaryExpression).left), cb((node as ts.BinaryExpression).right); | ||
cb((node as ts.BinaryExpression).left); | ||
cb((node as ts.BinaryExpression).right); | ||
return; | ||
case ts.SyntaxKind.ParenthesizedExpression: | ||
case ts.SyntaxKind.NonNullExpression: | ||
case ts.SyntaxKind.AsExpression: | ||
case ts.SyntaxKind.TypeAssertionExpression: | ||
return cb((node as ts.AssertionExpression | ts.NonNullExpression | ts.ParenthesizedExpression).expression); | ||
cb((node as ts.AssertionExpression | ts.NonNullExpression | ts.ParenthesizedExpression).expression); | ||
return; | ||
case ts.SyntaxKind.PrefixUnaryExpression: | ||
return cb((node as ts.PrefixUnaryExpression).operand); | ||
cb((node as ts.PrefixUnaryExpression).operand); | ||
return; | ||
default: | ||
return noCheck(node); | ||
noCheck(node); | ||
return; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's not necessary to change the 3 returns above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's true. Changed back. I changed them for consistency with the others; I don't like that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I introduced that pattern back then thinking this might benefit from tail call optimization. I haven't measured if it actually makes a difference though. |
||
} | ||
} | ||
return ts.forEachChild(node, cb); | ||
|
@@ -109,6 +118,7 @@ function walk(ctx: Lint.WalkContext<void>) { | |
|
||
function maybeCallback(cb: (node: ts.Node) => void, node?: ts.Node) { | ||
if (node !== undefined) { | ||
return cb(node); | ||
cb(node); | ||
return; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the return here is unnecessary There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
let x = (y = 1, z = 2); | ||
~~~~~~~~~~~~ [Don't use the comma operator.] | ||
|
||
// Error prone: forgot to add parens around arguments. | ||
(x, y => x + y)(a, b); | ||
~~~~~~~~~~~~~ [Don't use the comma operator.] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
{ | ||
"rules": { | ||
"ban-comma-operator": true | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this is also a good candidate for
latest.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.