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

Add new ban-parens rule #3238

Closed
wants to merge 23 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
cffa68e
Merge remote-tracking branch 'palantir/master'
calebegg Sep 18, 2017
6465c76
Add no-unnecessary-parens rule
calebegg Sep 19, 2017
10702fe
Added a test that uses .type and a few more rules to the defaults.
calebegg Sep 19, 2017
4dd6f05
Remove null check for node.parent
calebegg Sep 19, 2017
bb5c967
Address review comments.
calebegg Sep 26, 2017
f58f608
Merge remote-tracking branch 'palantir/master' into no-unnecessary-pa…
calebegg Sep 26, 2017
d23b6bb
Address review comments
calebegg Sep 26, 2017
9d5ff22
Remove unnecessary parentheses
calebegg Sep 26, 2017
005beda
Rename to ban-parens
calebegg Sep 26, 2017
b748e96
Remove premature return.
calebegg Sep 26, 2017
c981ad1
Fix an issue with `typeof(x)` being corrected to `typeofx`
calebegg Sep 27, 2017
efae3d3
Fix issue with (0).foo() being corrected to 0.foo()
calebegg Sep 27, 2017
ad649af
Fix issue with `return (\nfoo);` getting flagged even though those pa…
calebegg Sep 27, 2017
930b733
Fix issue with destructuring assignment parentheses
calebegg Sep 27, 2017
5a38167
Sometimes parentheses are necessary in arrow functions
calebegg Sep 27, 2017
9664596
Respond to review comments.
calebegg Sep 28, 2017
9a1454d
Respond to review comments.
calebegg Sep 29, 2017
de72308
Merge branch 'master' into no-unnecessary-parens
calebegg Sep 29, 2017
2c0af41
Add tsx tests
calebegg Oct 20, 2017
49dcea1
Respond to review comments
calebegg Oct 25, 2017
29d536c
Respond to review comments
calebegg Nov 3, 2017
73de3c3
Merge remote-tracking branch 'palantir/master' into no-unnecessary-pa…
calebegg Nov 3, 2017
e282a3f
Fix ban-parens violations
calebegg Nov 6, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/configs/all.ts
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ export const rules = {
"no-unnecessary-initializer": true,
"no-unnecessary-qualifier": true,
"no-unnecessary-type-assertion": true,
"no-unnecessary-parens": [true, { "default": true }],
"number-literal-format": true,
"object-literal-key-quotes": [true, "consistent-as-needed"],
"object-literal-shorthand": true,
Expand Down
190 changes: 190 additions & 0 deletions src/rules/noUnnecessaryParensRule.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,190 @@
/**
* @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 { isLiteralExpression, isParenthesizedExpression } from "tsutils";
import * as ts from "typescript";

import * as Lint from "../index";

interface Options {
withChild?: string[];
asChildOf?: string[];
default?: boolean;
}

const syntaxKindMapping = ts.SyntaxKind as {} as { [k: string]: ts.SyntaxKind };

export class Rule extends Lint.Rules.AbstractRule {
/* tslint:disable:object-literal-sort-keys */
public static metadata: Lint.IRuleMetadata = {
ruleName: "no-unnecessary-parens",
description: Lint.Utils.dedent`
Warns when parentheses are used that are unnecessary`,
options: {
type: "object",
properties: {
withChild: {
type: "list",
listType: "string",
},
asChildOf: {
type: "list",
listType: "string",
},
default: { type: "boolean" },
},
additionalProperties: false,
},
optionsDescription: Lint.Utils.dedent`
withChild: A list of token kinds around which to flag parentheses.
For example, \`{"withChild": ["Identifier"]}\` would flag
\`(foo)\` as having unnecessary parentheses around it.

asChildOf: A list of token kinds and properties on those tokens
such that if the parenthesized expression is the appropriate
child of a token of that kind, it will be flagged. For example,
\`{"asChildOf": ["VariableDeclaration.initializer"]}\` would
flag the parentheses in \`let x = (1 + 2)\`, regardless of the
kind of the parenthesized expression.

default: Whether to default the set of bans to a set of hopefully
uncontroversial bans picked by tslint.
`,
optionExamples: [
[{
withChild: [
"Identifier",
"LiteralExpression",
],
asChildOf: [
"VariableDeclaration.initializer",
"ParenthesizedExpression.expression",
"CallExpression.arguments",
"ExpressionStatement.expression",
Copy link
Contributor

Choose a reason for hiding this comment

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

All of these options require knowledge of typescript's AST. I don't think that's practicle for most TSLint users.

Maybe the rule should just find all unnecessary parens by default and provide options to disable some of them. Something like "allow-typeof" to allow (typeof foo)==="string" for example. These can be added on demand.

Also with these options, there's currently no way to ban parens on "*.type", e.g. let foo: (string) and foo(param: (1)) {}

Copy link
Contributor

Choose a reason for hiding this comment

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

There's currently no way to ban (1 + 2) + 3 without banning (1 + 2) * 3, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the rule should just find all unnecessary parens

So, I didn't want to get into litigating what's "necessary". I don't want to tell people to not do (true && false) || true or something, when I think that's probably preferable to leaving out the parentheses since it's hard to remember boolean prescience.

Instead, I want to ban them where they're clearly unnecessary, and leave the edge cases for human reviewers (or the judgement of the author).

I feel like having a list of allowances would get out of hand.

If you think this approach is fundamentally unsuitable, that's fine, just let me know. I'll just use it on my project.

Also with these options, there's currently no way to ban parens on "*.type", e.g. let foo: (string) and foo(param: (1)) {}

Why not? I added some examples and tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the documentation could point to http://astexplorer.net. That's how I find these AST names.

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a link to astexplorer.net sounds like a good idea.

],
}],
[{ default: true }],
],
type: "typescript",
typescriptOnly: true,
};
/* tslint:enable:object-literal-sort-keys */

public static FAILURE_STRING_FACTORY(expressionTypeName: string) {
return `Don't include unnecessary parentheses around ${expressionTypeName}`;
}

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

function isNodeOfKind(node: ts.Node, kindName: string) {
switch (kindName) {
case "LiteralExpression":
Copy link
Contributor

Choose a reason for hiding this comment

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

consider adding case "*": return true; to allow for *.type

return isLiteralExpression(node);
case "Keyword":
return node.kind > ts.SyntaxKind.FirstKeyword && node.kind < ts.SyntaxKind.LastKeyword;
default:
return node.kind === syntaxKindMapping[kindName];
}
}

function isParenthesizedType(node: ts.Node): node is ts.ParenthesizedTypeNode {
return node.kind === ts.SyntaxKind.ParenthesizedType;
}

function walk(ctx: Lint.WalkContext<Options>) {
if (ctx.options.withChild == undefined) {
ctx.options.withChild = [];
}
if (ctx.options.asChildOf == undefined) {
ctx.options.asChildOf = [];
}
const withChild = ctx.options.default ? [
...ctx.options.withChild,
// ex: (foo).bar()
"Identifier",
// ex: ("abc") + "def"
"LiteralExpression",
// ex: let x: ('a') = 'a';
"LiteralType",
// ex: let x: (string) = 'b'; let x = (undefined);
"Keyword",
// ex: (new Foo())
"NewExpression",
] : ctx.options.withChild;
const asChildOf = ctx.options.default ? [
// ex: let x = (1 + foo());
"VariableDeclaration.initializer",
// ex: type x = (string|number);
"TypeAliasDeclaration.type",
Copy link
Contributor

Choose a reason for hiding this comment

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

this is redundant now, but I'm fine with leaving it in for documentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's true. I guess it does give a better error message, kinda, to have the more specific ones.

I'd be fine either way. Up to you.

// ex: ((1 + 1)) + 2
"ParenthesizedExpression.expression",
// ex: foo((a), b)
"CallExpression.arguments",
Copy link
Contributor

Choose a reason for hiding this comment

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

also "NewExpression.arguments"

// ex: Foo<(string|number), string>
"TypeReference.typeArguments",
Copy link
Contributor

Choose a reason for hiding this comment

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

"*.typeArguments"?

// ex: (foo.bar());
"ExpressionStatement.expression",
// ex: function foo((a: string), b: number) {}
"SignatureDeclaration.parameters",
Copy link
Contributor

Choose a reason for hiding this comment

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

SignatureDeclaration is not a SyntaxKind. It's the base type for FunctionDeclaration, FunctionExpression, ConstructorDeclaration, IndexSignature, ...
This option will not work as intended.
Also note that a parameter cannot be wrapped in parens

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good call on both counts. I was being overenthusiastic. :-)

...ctx.options.asChildOf,
] : ctx.options.asChildOf;

const restrictions = withChild.map((name: string) => ({
message: `an expression of type ${name}`,
test(node: ts.ParenthesizedExpression | ts.ParenthesizedTypeNode) {
return isNodeOfKind(isParenthesizedExpression(node) ? node.expression : node.type, name);
},
})).concat(
asChildOf.map((name: string) => {
const [parentKind, whichChild] = name.split(".");
return {
message: `the ${whichChild} child of an expression of type ${parentKind}`,
test(node: ts.ParenthesizedExpression | ts.ParenthesizedTypeNode) {
if (node.parent == undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

That condition will never be true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, but I thought the check would be preferable to the !. Switched to the !.

return false;
}
if (!isNodeOfKind(node.parent, parentKind)) {
return false;
}
const parentMapping = node.parent as {} as { [k: string]: ts.Node | ts.Node[] };
const childOrChildren = parentMapping[whichChild];
return Array.isArray(childOrChildren) ?
childOrChildren.indexOf(node) !== -1 :
childOrChildren === node;
},
};
}));

return ts.forEachChild(ctx.sourceFile, function cb(node: ts.Node): void {
if (isParenthesizedExpression(node) || isParenthesizedType(node)) {
const restriction = restrictions.find((r) => r.test(node));
if (restriction != undefined) {
ctx.addFailureAtNode(
node,
Rule.FAILURE_STRING_FACTORY(restriction.message),
[
Lint.Replacement.deleteFromTo(node.getStart(), node.getStart() + 1),
Lint.Replacement.deleteFromTo(node.getEnd() - 1, node.getEnd()),
]);
}
}
return ts.forEachChild(node, cb);
});
}
4 changes: 2 additions & 2 deletions src/verify/lines.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ export function parseLine(text: string): Line {
if (endErrorMatch !== null) {
const [, squiggles, message] = endErrorMatch;
const startErrorCol = text.indexOf("~");
const zeroLengthError = (squiggles === ZERO_LENGTH_ERROR);
const zeroLengthError = squiggles === ZERO_LENGTH_ERROR;
const endErrorCol = zeroLengthError ? startErrorCol : text.lastIndexOf("~") + 1;
return new EndErrorLine(startErrorCol, endErrorCol, message);
}
Expand All @@ -75,7 +75,7 @@ export function parseLine(text: string): Line {
export function printLine(line: Line, code?: string): string | undefined {
if (line instanceof ErrorLine) {
if (code === undefined) {
throw new Error("Must supply argument for code parameter when line is an ErrorLine");
throw new Error("Must supply argument for code parameter when line is an ErrorLine");
}

const leadingSpaces = " ".repeat(line.startCol);
Expand Down
17 changes: 17 additions & 0 deletions test/rules/no-unnecessary-parens/test.ts.fix
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
let x = 3;
let y = x;
function foo() {
return 1;
}
x ;

let x = 1 + 1;
foo(1 + 1, 2 + 2);
console.log((1 + 1) * 2);
let x: string = 'b';
let x = false;

// No issues with
let z = (x) => x + 1;
let x = (1 + 1) * 2;
(f());
27 changes: 27 additions & 0 deletions test/rules/no-unnecessary-parens/test.ts.lint
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
let x = (3);
Copy link
Contributor

Choose a reason for hiding this comment

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

There's one edge case that would result in invalid code:

let y, z;
let x = (y = 1, z = 2);

I hope nobody writes such code.
Nontheless there should probably be an exception for ts.SyntaxKind.BinaryExpression where operatorToken.kind === ts.SyntaxKind.CommaToken

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, the comma operator is the worst. On my shortlist of new tslint rules is one to ban it entirely.

I made it so that it doesn't suggest a fix if the child uses a comma operator, but it still flags it as an issue. How does that sound?

~~~ [Don't include unnecessary parentheses around an expression of type LiteralExpression]
let y = (x);
~~~ [Don't include unnecessary parentheses around an expression of type Identifier]
function foo() {
return (1);
~~~ [Don't include unnecessary parentheses around an expression of type LiteralExpression]
}
( x );
~~~~~~~~~~~~ [Don't include unnecessary parentheses around an expression of type Identifier]

let x = (1 + 1);
~~~~~~~ [Don't include unnecessary parentheses around the initializer child of an expression of type VariableDeclaration]
foo((1 + 1), (2 + 2));
~~~~~~~ [Don't include unnecessary parentheses around the arguments child of an expression of type CallExpression]
~~~~~~~ [Don't include unnecessary parentheses around the arguments child of an expression of type CallExpression]
console.log(((1 + 1)) * 2);
~~~~~~~ [Don't include unnecessary parentheses around the expression child of an expression of type ParenthesizedExpression]
let x: (string) = 'b';
~~~~~~~~ [Don't include unnecessary parentheses around an expression of type Keyword]
let x = (false);
~~~~~~~ [Don't include unnecessary parentheses around an expression of type Keyword]

// No issues with
let z = (x) => x + 1;
let x = (1 + 1) * 2;
(f());
18 changes: 18 additions & 0 deletions test/rules/no-unnecessary-parens/tslint.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
{
"rules": {
"no-unnecessary-parens": [true,
{
"withChild": [
"Identifier",
"LiteralExpression",
"Keyword"
],
"asChildOf": [
"VariableDeclaration.initializer",
"ParenthesizedExpression.expression",
"CallExpression.arguments"
]
}
]
}
}