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

Add the ban-comma-operator rule #3250

Merged
merged 5 commits into from Sep 29, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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 @@ -88,6 +88,7 @@ export const rules = {
// Functionality
"await-promise": true,
// "ban": no sensible default
"ban-comma-operator": true,
Copy link
Contributor

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

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.

"curly": true,
"forin": true,
// "import-blacklist": no sensible default
Expand Down
50 changes: 50 additions & 0 deletions src/rules/banCommaOperatorRule.ts
@@ -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);
});
}
46 changes: 28 additions & 18 deletions src/rules/noConditionalAssignmentRule.ts
Expand Up @@ -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.`,
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

it's not necessary to change the 3 returns above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 return foo() implies that foo() actually returns a value, rather than has only side effects.

Copy link
Contributor

Choose a reason for hiding this comment

The 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);
Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

the return here is unnecessary

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.

}
}
6 changes: 6 additions & 0 deletions test/rules/ban-comma-operator/test.ts.lint
@@ -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.]
5 changes: 5 additions & 0 deletions test/rules/ban-comma-operator/tslint.json
@@ -0,0 +1,5 @@
{
"rules": {
"ban-comma-operator": true
}
}