From 6465c764aac408b12e8e1aa5df92099d3fbf6a83 Mon Sep 17 00:00:00 2001 From: Caleb Eggensperger Date: Tue, 19 Sep 2017 15:00:08 -0400 Subject: [PATCH 01/19] Add no-unnecessary-parens rule --- src/configs/all.ts | 1 + src/rules/noUnnecessaryParensRule.ts | 190 ++++++++++++++++++ src/verify/lines.ts | 4 +- test/rules/no-unnecessary-parens/test.ts.fix | 17 ++ test/rules/no-unnecessary-parens/test.ts.lint | 27 +++ test/rules/no-unnecessary-parens/tslint.json | 18 ++ 6 files changed, 255 insertions(+), 2 deletions(-) create mode 100644 src/rules/noUnnecessaryParensRule.ts create mode 100644 test/rules/no-unnecessary-parens/test.ts.fix create mode 100644 test/rules/no-unnecessary-parens/test.ts.lint create mode 100644 test/rules/no-unnecessary-parens/tslint.json diff --git a/src/configs/all.ts b/src/configs/all.ts index f04e635c3e1..a0c2409f816 100644 --- a/src/configs/all.ts +++ b/src/configs/all.ts @@ -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, diff --git a/src/rules/noUnnecessaryParensRule.ts b/src/rules/noUnnecessaryParensRule.ts new file mode 100644 index 00000000000..a76ef340e59 --- /dev/null +++ b/src/rules/noUnnecessaryParensRule.ts @@ -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", + ], + }], + [{ 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": + 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) { + 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", + // ex: ((1 + 1)) + 2 + "ParenthesizedExpression.expression", + // ex: foo((a), b) + "CallExpression.arguments", + // ex: Foo<(string|number), string> + "TypeReference.typeArguments", + // ex: (foo.bar()); + "ExpressionStatement.expression", + // ex: function foo((a: string), b: number) {} + "SignatureDeclaration.parameters", + ...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) { + 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); + }); +} diff --git a/src/verify/lines.ts b/src/verify/lines.ts index 788d0bcca96..4499e8b6fa1 100644 --- a/src/verify/lines.ts +++ b/src/verify/lines.ts @@ -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); } @@ -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); diff --git a/test/rules/no-unnecessary-parens/test.ts.fix b/test/rules/no-unnecessary-parens/test.ts.fix new file mode 100644 index 00000000000..bf58f51f7bd --- /dev/null +++ b/test/rules/no-unnecessary-parens/test.ts.fix @@ -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()); diff --git a/test/rules/no-unnecessary-parens/test.ts.lint b/test/rules/no-unnecessary-parens/test.ts.lint new file mode 100644 index 00000000000..65e6509f15e --- /dev/null +++ b/test/rules/no-unnecessary-parens/test.ts.lint @@ -0,0 +1,27 @@ +let x = (3); + ~~~ [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()); diff --git a/test/rules/no-unnecessary-parens/tslint.json b/test/rules/no-unnecessary-parens/tslint.json new file mode 100644 index 00000000000..68bf13769ee --- /dev/null +++ b/test/rules/no-unnecessary-parens/tslint.json @@ -0,0 +1,18 @@ +{ + "rules": { + "no-unnecessary-parens": [true, + { + "withChild": [ + "Identifier", + "LiteralExpression", + "Keyword" + ], + "asChildOf": [ + "VariableDeclaration.initializer", + "ParenthesizedExpression.expression", + "CallExpression.arguments" + ] + } + ] + } +} From 10702fe439de79aaf3c84479628c351f60f44be7 Mon Sep 17 00:00:00 2001 From: Caleb Eggensperger Date: Tue, 19 Sep 2017 16:32:12 -0400 Subject: [PATCH 02/19] Added a test that uses .type and a few more rules to the defaults. --- src/rules/noUnnecessaryParensRule.ts | 10 ++++++++++ test/rules/no-unnecessary-parens/test.ts.fix | 1 + test/rules/no-unnecessary-parens/test.ts.lint | 2 ++ test/rules/no-unnecessary-parens/tslint.json | 3 ++- 4 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/rules/noUnnecessaryParensRule.ts b/src/rules/noUnnecessaryParensRule.ts index a76ef340e59..602e6cdce6b 100644 --- a/src/rules/noUnnecessaryParensRule.ts +++ b/src/rules/noUnnecessaryParensRule.ts @@ -127,6 +127,12 @@ function walk(ctx: Lint.WalkContext) { "Keyword", // ex: (new Foo()) "NewExpression", + // ex (options[0]).foo + "ElementAccessExpression", + // ex (x.a).b + "PropertyAccessExpression", + // ex (f()); + "CallExpression", ] : ctx.options.withChild; const asChildOf = ctx.options.default ? [ // ex: let x = (1 + foo()); @@ -143,6 +149,10 @@ function walk(ctx: Lint.WalkContext) { "ExpressionStatement.expression", // ex: function foo((a: string), b: number) {} "SignatureDeclaration.parameters", + // ex: let x: (string|number) = 3; + "VariableDeclaration.type", + // ex: function(foo: (number|string)) {} + "Parameter.type", ...ctx.options.asChildOf, ] : ctx.options.asChildOf; diff --git a/test/rules/no-unnecessary-parens/test.ts.fix b/test/rules/no-unnecessary-parens/test.ts.fix index bf58f51f7bd..b6eeed44018 100644 --- a/test/rules/no-unnecessary-parens/test.ts.fix +++ b/test/rules/no-unnecessary-parens/test.ts.fix @@ -10,6 +10,7 @@ foo(1 + 1, 2 + 2); console.log((1 + 1) * 2); let x: string = 'b'; let x = false; +let x: number|string = 1; // No issues with let z = (x) => x + 1; diff --git a/test/rules/no-unnecessary-parens/test.ts.lint b/test/rules/no-unnecessary-parens/test.ts.lint index 65e6509f15e..b6d63bed60e 100644 --- a/test/rules/no-unnecessary-parens/test.ts.lint +++ b/test/rules/no-unnecessary-parens/test.ts.lint @@ -20,6 +20,8 @@ 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] +let x: (number|string) = 1; + ~~~~~~~~~~~~~~~ [Don't include unnecessary parentheses around the type child of an expression of type VariableDeclaration] // No issues with let z = (x) => x + 1; diff --git a/test/rules/no-unnecessary-parens/tslint.json b/test/rules/no-unnecessary-parens/tslint.json index 68bf13769ee..b0075543be0 100644 --- a/test/rules/no-unnecessary-parens/tslint.json +++ b/test/rules/no-unnecessary-parens/tslint.json @@ -10,7 +10,8 @@ "asChildOf": [ "VariableDeclaration.initializer", "ParenthesizedExpression.expression", - "CallExpression.arguments" + "CallExpression.arguments", + "VariableDeclaration.type" ] } ] From 4dd6f0593508319706bd7b861d11b12bc4ff6b90 Mon Sep 17 00:00:00 2001 From: Caleb Eggensperger Date: Tue, 19 Sep 2017 16:34:32 -0400 Subject: [PATCH 03/19] Remove null check for node.parent --- src/rules/noUnnecessaryParensRule.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/rules/noUnnecessaryParensRule.ts b/src/rules/noUnnecessaryParensRule.ts index 602e6cdce6b..7e769eee73e 100644 --- a/src/rules/noUnnecessaryParensRule.ts +++ b/src/rules/noUnnecessaryParensRule.ts @@ -167,10 +167,7 @@ function walk(ctx: Lint.WalkContext) { return { message: `the ${whichChild} child of an expression of type ${parentKind}`, test(node: ts.ParenthesizedExpression | ts.ParenthesizedTypeNode) { - if (node.parent == undefined) { - return false; - } - if (!isNodeOfKind(node.parent, parentKind)) { + if (!isNodeOfKind(node.parent!, parentKind)) { return false; } const parentMapping = node.parent as {} as { [k: string]: ts.Node | ts.Node[] }; From bb5c967b2a605f093e38e46ae19f49eae60862d9 Mon Sep 17 00:00:00 2001 From: Caleb Eggensperger Date: Tue, 26 Sep 2017 12:59:43 -0400 Subject: [PATCH 04/19] Address review comments. * Add link to astexplorer.net * Support *.foo in asChildOf (and add a test) * Add new default rules * Remove erroneous default rule --- src/rules/noUnnecessaryParensRule.ts | 22 +++++++++++++------ test/rules/no-unnecessary-parens/test.ts.fix | 2 ++ test/rules/no-unnecessary-parens/test.ts.lint | 4 ++++ test/rules/no-unnecessary-parens/tslint.json | 3 ++- 4 files changed, 23 insertions(+), 8 deletions(-) diff --git a/src/rules/noUnnecessaryParensRule.ts b/src/rules/noUnnecessaryParensRule.ts index 7e769eee73e..25864b2a367 100644 --- a/src/rules/noUnnecessaryParensRule.ts +++ b/src/rules/noUnnecessaryParensRule.ts @@ -33,7 +33,9 @@ export class Rule extends Lint.Rules.AbstractRule { public static metadata: Lint.IRuleMetadata = { ruleName: "no-unnecessary-parens", description: Lint.Utils.dedent` - Warns when parentheses are used that are unnecessary`, + Warns when parentheses are used that are unnecessary. Tip: Use + astexplorer.net with the TypeScript parser to determine the token + types you want to avoid parentheses around.`, options: { type: "object", properties: { @@ -75,6 +77,7 @@ export class Rule extends Lint.Rules.AbstractRule { "ParenthesizedExpression.expression", "CallExpression.arguments", "ExpressionStatement.expression", + "*.type", ], }], [{ default: true }], @@ -99,6 +102,8 @@ function isNodeOfKind(node: ts.Node, kindName: string) { return isLiteralExpression(node); case "Keyword": return node.kind > ts.SyntaxKind.FirstKeyword && node.kind < ts.SyntaxKind.LastKeyword; + case "*": + return true; default: return node.kind === syntaxKindMapping[kindName]; } @@ -141,18 +146,21 @@ function walk(ctx: Lint.WalkContext) { "TypeAliasDeclaration.type", // ex: ((1 + 1)) + 2 "ParenthesizedExpression.expression", - // ex: foo((a), b) + // ex: foo((a), b); new Foo((a)); "CallExpression.arguments", - // ex: Foo<(string|number), string> - "TypeReference.typeArguments", + "NewExpression.arguments", + // ex: Foo<(a|b), c>; foo<(a)>(); + "*.typeArguments", // ex: (foo.bar()); "ExpressionStatement.expression", - // ex: function foo((a: string), b: number) {} - "SignatureDeclaration.parameters", // ex: let x: (string|number) = 3; "VariableDeclaration.type", // ex: function(foo: (number|string)) {} "Parameter.type", + // foo[(bar + "baz")] + "ElementAccessExpression.argumentExpression", + // `${(foo)}` + "TemplateSpan.expression", ...ctx.options.asChildOf, ] : ctx.options.asChildOf; @@ -165,7 +173,7 @@ function walk(ctx: Lint.WalkContext) { asChildOf.map((name: string) => { const [parentKind, whichChild] = name.split("."); return { - message: `the ${whichChild} child of an expression of type ${parentKind}`, + message: `the ${whichChild} child of an expression${parentKind === "*" ? "" : ` of type ${parentKind}`}`, test(node: ts.ParenthesizedExpression | ts.ParenthesizedTypeNode) { if (!isNodeOfKind(node.parent!, parentKind)) { return false; diff --git a/test/rules/no-unnecessary-parens/test.ts.fix b/test/rules/no-unnecessary-parens/test.ts.fix index b6eeed44018..742c6b87ae9 100644 --- a/test/rules/no-unnecessary-parens/test.ts.fix +++ b/test/rules/no-unnecessary-parens/test.ts.fix @@ -11,6 +11,8 @@ console.log((1 + 1) * 2); let x: string = 'b'; let x = false; let x: number|string = 1; +let x: Foo; +let x = foo(); // No issues with let z = (x) => x + 1; diff --git a/test/rules/no-unnecessary-parens/test.ts.lint b/test/rules/no-unnecessary-parens/test.ts.lint index b6d63bed60e..6e8fb975a40 100644 --- a/test/rules/no-unnecessary-parens/test.ts.lint +++ b/test/rules/no-unnecessary-parens/test.ts.lint @@ -22,6 +22,10 @@ let x = (false); ~~~~~~~ [Don't include unnecessary parentheses around an expression of type Keyword] let x: (number|string) = 1; ~~~~~~~~~~~~~~~ [Don't include unnecessary parentheses around the type child of an expression of type VariableDeclaration] +let x: Foo<(string|number)>; + ~~~~~~~~~~~~~~~ [Don't include unnecessary parentheses around the typeArguments child of an expression] +let x = foo<(string|number)>(); + ~~~~~~~~~~~~~~~ [Don't include unnecessary parentheses around the typeArguments child of an expression] // No issues with let z = (x) => x + 1; diff --git a/test/rules/no-unnecessary-parens/tslint.json b/test/rules/no-unnecessary-parens/tslint.json index b0075543be0..400610baf5f 100644 --- a/test/rules/no-unnecessary-parens/tslint.json +++ b/test/rules/no-unnecessary-parens/tslint.json @@ -11,7 +11,8 @@ "VariableDeclaration.initializer", "ParenthesizedExpression.expression", "CallExpression.arguments", - "VariableDeclaration.type" + "VariableDeclaration.type", + "*.typeArguments" ] } ] From d23b6bbaf3abd9f357922d790ff1fbf74665ee9d Mon Sep 17 00:00:00 2001 From: Caleb Eggensperger Date: Tue, 26 Sep 2017 14:46:27 -0400 Subject: [PATCH 05/19] Address review comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Don’t suggest a fix when the child is a comma operator expresssion * Use *.type instead of Parameter.type --- src/rules/noUnnecessaryParensRule.ts | 15 +++++++++++++-- test/rules/no-unnecessary-parens/test.ts.fix | 1 + test/rules/no-unnecessary-parens/test.ts.lint | 2 ++ 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/rules/noUnnecessaryParensRule.ts b/src/rules/noUnnecessaryParensRule.ts index 25864b2a367..35ce15535b7 100644 --- a/src/rules/noUnnecessaryParensRule.ts +++ b/src/rules/noUnnecessaryParensRule.ts @@ -15,7 +15,7 @@ * limitations under the License. */ -import { isLiteralExpression, isParenthesizedExpression } from "tsutils"; +import { isBinaryExpression, isLiteralExpression, isParenthesizedExpression } from "tsutils"; import * as ts from "typescript"; import * as Lint from "../index"; @@ -156,7 +156,7 @@ function walk(ctx: Lint.WalkContext) { // ex: let x: (string|number) = 3; "VariableDeclaration.type", // ex: function(foo: (number|string)) {} - "Parameter.type", + "*.type", // foo[(bar + "baz")] "ElementAccessExpression.argumentExpression", // `${(foo)}` @@ -191,6 +191,17 @@ function walk(ctx: Lint.WalkContext) { if (isParenthesizedExpression(node) || isParenthesizedType(node)) { const restriction = restrictions.find((r) => r.test(node)); if (restriction != undefined) { + // Don't suggest a fix for a (hopefully rare) pattern where + // removing the parentheses would almost always be bad, e.g. + // let x = (y = 1, z = 2); + if (isParenthesizedExpression(node) && + isBinaryExpression(node.expression) && + node.expression.operatorToken.kind === ts.SyntaxKind.CommaToken) { + ctx.addFailureAtNode( + node, + Rule.FAILURE_STRING_FACTORY(restriction.message)); + return; + } ctx.addFailureAtNode( node, Rule.FAILURE_STRING_FACTORY(restriction.message), diff --git a/test/rules/no-unnecessary-parens/test.ts.fix b/test/rules/no-unnecessary-parens/test.ts.fix index 742c6b87ae9..bcd2c3925e6 100644 --- a/test/rules/no-unnecessary-parens/test.ts.fix +++ b/test/rules/no-unnecessary-parens/test.ts.fix @@ -13,6 +13,7 @@ let x = false; let x: number|string = 1; let x: Foo; let x = foo(); +let x = (y = 1, z = 2); // No issues with let z = (x) => x + 1; diff --git a/test/rules/no-unnecessary-parens/test.ts.lint b/test/rules/no-unnecessary-parens/test.ts.lint index 6e8fb975a40..0214daae1d5 100644 --- a/test/rules/no-unnecessary-parens/test.ts.lint +++ b/test/rules/no-unnecessary-parens/test.ts.lint @@ -26,6 +26,8 @@ let x: Foo<(string|number)>; ~~~~~~~~~~~~~~~ [Don't include unnecessary parentheses around the typeArguments child of an expression] let x = foo<(string|number)>(); ~~~~~~~~~~~~~~~ [Don't include unnecessary parentheses around the typeArguments child of an expression] +let x = (y = 1, z = 2); + ~~~~~~~~~~~~~~ [Don't include unnecessary parentheses around the initializer child of an expression of type VariableDeclaration] // No issues with let z = (x) => x + 1; From 9d5ff22fb0a76fde685c1d642fcb7b7518da8728 Mon Sep 17 00:00:00 2001 From: Caleb Eggensperger Date: Tue, 26 Sep 2017 14:46:38 -0400 Subject: [PATCH 06/19] Remove unnecessary parentheses --- src/rules/noUnnecessaryTypeAssertionRule.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rules/noUnnecessaryTypeAssertionRule.ts b/src/rules/noUnnecessaryTypeAssertionRule.ts index 812718c8d33..29abf903269 100644 --- a/src/rules/noUnnecessaryTypeAssertionRule.ts +++ b/src/rules/noUnnecessaryTypeAssertionRule.ts @@ -74,7 +74,7 @@ class Walker extends Lint.AbstractWalker { // they're being matched against an inferred type. So, in addition, // check if any properties are numbers, which implies that this is // likely a tuple type. - (castType.getProperties().some((symbol) => !isNaN(Number(symbol.name))))) { + castType.getProperties().some((symbol) => !isNaN(Number(symbol.name)))) { // It's not always safe to remove a cast to a literal type or tuple // type, as those types are sometimes widened without the cast. From 005bedae0086c7f08d03ec7793e29badc543e76a Mon Sep 17 00:00:00 2001 From: Caleb Eggensperger Date: Tue, 26 Sep 2017 16:40:14 -0400 Subject: [PATCH 07/19] Rename to ban-parens --- src/configs/all.ts | 2 +- ...ecessaryParensRule.ts => banParensRule.ts} | 21 +++++------ .../test.ts.fix | 0 test/rules/ban-parens/test.ts.lint | 35 +++++++++++++++++++ .../tslint.json | 2 +- test/rules/no-unnecessary-parens/test.ts.lint | 35 ------------------- 6 files changed, 48 insertions(+), 47 deletions(-) rename src/rules/{noUnnecessaryParensRule.ts => banParensRule.ts} (92%) rename test/rules/{no-unnecessary-parens => ban-parens}/test.ts.fix (100%) create mode 100644 test/rules/ban-parens/test.ts.lint rename test/rules/{no-unnecessary-parens => ban-parens}/tslint.json (92%) delete mode 100644 test/rules/no-unnecessary-parens/test.ts.lint diff --git a/src/configs/all.ts b/src/configs/all.ts index b643c3586b9..178259ef2e1 100644 --- a/src/configs/all.ts +++ b/src/configs/all.ts @@ -88,6 +88,7 @@ export const rules = { // Functionality "await-promise": true, // "ban": no sensible default + "ban-parens": [true, { "default": true }], "curly": true, "forin": true, // "import-blacklist": no sensible default @@ -201,7 +202,6 @@ 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, diff --git a/src/rules/noUnnecessaryParensRule.ts b/src/rules/banParensRule.ts similarity index 92% rename from src/rules/noUnnecessaryParensRule.ts rename to src/rules/banParensRule.ts index 35ce15535b7..9443f79aa35 100644 --- a/src/rules/noUnnecessaryParensRule.ts +++ b/src/rules/banParensRule.ts @@ -31,11 +31,12 @@ 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", + ruleName: "ban-parens", description: Lint.Utils.dedent` - Warns when parentheses are used that are unnecessary. Tip: Use - astexplorer.net with the TypeScript parser to determine the token - types you want to avoid parentheses around.`, + Warns when parentheses are used around or as a child of certain + expression types. Tip: Use astexplorer.net with the TypeScript + parser to determine the token types you want to ban parentheses + around.`, options: { type: "object", properties: { @@ -52,15 +53,15 @@ export class Rule extends Lint.Rules.AbstractRule { 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. + withChild: A list of token kinds around which to ban parentheses. + For example, \`{"withChild": ["Identifier"]}\` would ban + \`(foo)\`. 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, + child of a token of that kind, it will be banned. For example, \`{"asChildOf": ["VariableDeclaration.initializer"]}\` would - flag the parentheses in \`let x = (1 + 2)\`, regardless of the + ban 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 @@ -88,7 +89,7 @@ export class Rule extends Lint.Rules.AbstractRule { /* tslint:enable:object-literal-sort-keys */ public static FAILURE_STRING_FACTORY(expressionTypeName: string) { - return `Don't include unnecessary parentheses around ${expressionTypeName}`; + return `Don't include parentheses around ${expressionTypeName}`; } public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { diff --git a/test/rules/no-unnecessary-parens/test.ts.fix b/test/rules/ban-parens/test.ts.fix similarity index 100% rename from test/rules/no-unnecessary-parens/test.ts.fix rename to test/rules/ban-parens/test.ts.fix diff --git a/test/rules/ban-parens/test.ts.lint b/test/rules/ban-parens/test.ts.lint new file mode 100644 index 00000000000..f50c587636a --- /dev/null +++ b/test/rules/ban-parens/test.ts.lint @@ -0,0 +1,35 @@ +let x = (3); + ~~~ [Don't include parentheses around an expression of type LiteralExpression] +let y = (x); + ~~~ [Don't include parentheses around an expression of type Identifier] +function foo() { + return (1); + ~~~ [Don't include parentheses around an expression of type LiteralExpression] +} +( x ); +~~~~~~~~~~~~ [Don't include parentheses around an expression of type Identifier] + +let x = (1 + 1); + ~~~~~~~ [Don't include parentheses around the initializer child of an expression of type VariableDeclaration] +foo((1 + 1), (2 + 2)); + ~~~~~~~ [Don't include parentheses around the arguments child of an expression of type CallExpression] + ~~~~~~~ [Don't include parentheses around the arguments child of an expression of type CallExpression] +console.log(((1 + 1)) * 2); + ~~~~~~~ [Don't include parentheses around the expression child of an expression of type ParenthesizedExpression] +let x: (string) = 'b'; + ~~~~~~~~ [Don't include parentheses around an expression of type Keyword] +let x = (false); + ~~~~~~~ [Don't include parentheses around an expression of type Keyword] +let x: (number|string) = 1; + ~~~~~~~~~~~~~~~ [Don't include parentheses around the type child of an expression of type VariableDeclaration] +let x: Foo<(string|number)>; + ~~~~~~~~~~~~~~~ [Don't include parentheses around the typeArguments child of an expression] +let x = foo<(string|number)>(); + ~~~~~~~~~~~~~~~ [Don't include parentheses around the typeArguments child of an expression] +let x = (y = 1, z = 2); + ~~~~~~~~~~~~~~ [Don't include parentheses around the initializer child of an expression of type VariableDeclaration] + +// No issues with +let z = (x) => x + 1; +let x = (1 + 1) * 2; +(f()); diff --git a/test/rules/no-unnecessary-parens/tslint.json b/test/rules/ban-parens/tslint.json similarity index 92% rename from test/rules/no-unnecessary-parens/tslint.json rename to test/rules/ban-parens/tslint.json index 400610baf5f..c636e9063d0 100644 --- a/test/rules/no-unnecessary-parens/tslint.json +++ b/test/rules/ban-parens/tslint.json @@ -1,6 +1,6 @@ { "rules": { - "no-unnecessary-parens": [true, + "ban-parens": [true, { "withChild": [ "Identifier", diff --git a/test/rules/no-unnecessary-parens/test.ts.lint b/test/rules/no-unnecessary-parens/test.ts.lint deleted file mode 100644 index 0214daae1d5..00000000000 --- a/test/rules/no-unnecessary-parens/test.ts.lint +++ /dev/null @@ -1,35 +0,0 @@ -let x = (3); - ~~~ [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] -let x: (number|string) = 1; - ~~~~~~~~~~~~~~~ [Don't include unnecessary parentheses around the type child of an expression of type VariableDeclaration] -let x: Foo<(string|number)>; - ~~~~~~~~~~~~~~~ [Don't include unnecessary parentheses around the typeArguments child of an expression] -let x = foo<(string|number)>(); - ~~~~~~~~~~~~~~~ [Don't include unnecessary parentheses around the typeArguments child of an expression] -let x = (y = 1, z = 2); - ~~~~~~~~~~~~~~ [Don't include unnecessary parentheses around the initializer child of an expression of type VariableDeclaration] - -// No issues with -let z = (x) => x + 1; -let x = (1 + 1) * 2; -(f()); From b748e9668cd739683acd92129957b6dc772ce6e7 Mon Sep 17 00:00:00 2001 From: Caleb Eggensperger Date: Tue, 26 Sep 2017 17:19:12 -0400 Subject: [PATCH 08/19] Remove premature return. --- src/rules/banParensRule.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/rules/banParensRule.ts b/src/rules/banParensRule.ts index 9443f79aa35..d80d10e342c 100644 --- a/src/rules/banParensRule.ts +++ b/src/rules/banParensRule.ts @@ -201,15 +201,15 @@ function walk(ctx: Lint.WalkContext) { ctx.addFailureAtNode( node, Rule.FAILURE_STRING_FACTORY(restriction.message)); - return; + } else { + 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()), + ]); } - 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); From c981ad1bafa742ac4034675ae827f6b06fde0e93 Mon Sep 17 00:00:00 2001 From: Caleb Eggensperger Date: Wed, 27 Sep 2017 11:46:19 -0400 Subject: [PATCH 09/19] Fix an issue with `typeof(x)` being corrected to `typeofx` Also `throw(err)` to `throwerr` --- src/rules/banParensRule.ts | 25 ++++++++++++++----------- test/rules/ban-parens/test.ts.fix | 1 + test/rules/ban-parens/test.ts.lint | 2 ++ 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/src/rules/banParensRule.ts b/src/rules/banParensRule.ts index d80d10e342c..3d396f7dd73 100644 --- a/src/rules/banParensRule.ts +++ b/src/rules/banParensRule.ts @@ -192,24 +192,27 @@ function walk(ctx: Lint.WalkContext) { if (isParenthesizedExpression(node) || isParenthesizedType(node)) { const restriction = restrictions.find((r) => r.test(node)); if (restriction != undefined) { + let replacement = [ + Lint.Replacement.deleteFromTo(node.getStart(), node.getStart() + 1), + Lint.Replacement.deleteFromTo(node.getEnd() - 1, node.getEnd()), + ]; + const charBeforeParens = ctx.sourceFile.text[node.getStart() - 1]; + // Prevent correcting typeof(x) to typeofx, throw(err) to throwerr + if (charBeforeParens.match(/\w/) !== null) { + replacement.push(Lint.Replacement.appendText(node.getStart(), " ")); + } // Don't suggest a fix for a (hopefully rare) pattern where // removing the parentheses would almost always be bad, e.g. // let x = (y = 1, z = 2); if (isParenthesizedExpression(node) && isBinaryExpression(node.expression) && node.expression.operatorToken.kind === ts.SyntaxKind.CommaToken) { - ctx.addFailureAtNode( - node, - Rule.FAILURE_STRING_FACTORY(restriction.message)); - } else { - 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()), - ]); + replacement = []; } + ctx.addFailureAtNode( + node, + Rule.FAILURE_STRING_FACTORY(restriction.message), + replacement); } } return ts.forEachChild(node, cb); diff --git a/test/rules/ban-parens/test.ts.fix b/test/rules/ban-parens/test.ts.fix index bcd2c3925e6..63c486eb487 100644 --- a/test/rules/ban-parens/test.ts.fix +++ b/test/rules/ban-parens/test.ts.fix @@ -14,6 +14,7 @@ let x: number|string = 1; let x: Foo; let x = foo(); let x = (y = 1, z = 2); +typeof x; // No issues with let z = (x) => x + 1; diff --git a/test/rules/ban-parens/test.ts.lint b/test/rules/ban-parens/test.ts.lint index f50c587636a..7fd7cf0d0bf 100644 --- a/test/rules/ban-parens/test.ts.lint +++ b/test/rules/ban-parens/test.ts.lint @@ -28,6 +28,8 @@ let x = foo<(string|number)>(); ~~~~~~~~~~~~~~~ [Don't include parentheses around the typeArguments child of an expression] let x = (y = 1, z = 2); ~~~~~~~~~~~~~~ [Don't include parentheses around the initializer child of an expression of type VariableDeclaration] +typeof(x); + ~~~ [Don't include parentheses around an expression of type Identifier] // No issues with let z = (x) => x + 1; From efae3d309317b2a44d61c1655342964aa26d4b3f Mon Sep 17 00:00:00 2001 From: Caleb Eggensperger Date: Wed, 27 Sep 2017 14:13:12 -0400 Subject: [PATCH 10/19] Fix issue with (0).foo() being corrected to 0.foo() --- src/rules/banParensRule.ts | 16 +++++++++++----- test/rules/ban-parens/test.ts.fix | 1 + test/rules/ban-parens/test.ts.lint | 1 + 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/rules/banParensRule.ts b/src/rules/banParensRule.ts index 3d396f7dd73..fab09b4b020 100644 --- a/src/rules/banParensRule.ts +++ b/src/rules/banParensRule.ts @@ -15,7 +15,7 @@ * limitations under the License. */ -import { isBinaryExpression, isLiteralExpression, isParenthesizedExpression } from "tsutils"; +import { isBinaryExpression, isLiteralExpression, isNumericLiteral, isParenthesizedExpression, isPropertyAccessExpression } from "tsutils"; import * as ts from "typescript"; import * as Lint from "../index"; @@ -209,10 +209,16 @@ function walk(ctx: Lint.WalkContext) { node.expression.operatorToken.kind === ts.SyntaxKind.CommaToken) { replacement = []; } - ctx.addFailureAtNode( - node, - Rule.FAILURE_STRING_FACTORY(restriction.message), - replacement); + // Don't replace (0).foo() with 0.foo() + if (!(isParenthesizedExpression(node) && + isNumericLiteral(node.expression) && + node.parent != undefined && + isPropertyAccessExpression(node.parent))) { + ctx.addFailureAtNode( + node, + Rule.FAILURE_STRING_FACTORY(restriction.message), + replacement); + } } } return ts.forEachChild(node, cb); diff --git a/test/rules/ban-parens/test.ts.fix b/test/rules/ban-parens/test.ts.fix index 63c486eb487..e694f9e83db 100644 --- a/test/rules/ban-parens/test.ts.fix +++ b/test/rules/ban-parens/test.ts.fix @@ -15,6 +15,7 @@ let x: Foo; let x = foo(); let x = (y = 1, z = 2); typeof x; +(0).toLocaleString('en-US', {style: 'percent'}); // No issues with let z = (x) => x + 1; diff --git a/test/rules/ban-parens/test.ts.lint b/test/rules/ban-parens/test.ts.lint index 7fd7cf0d0bf..9b62a4d33dc 100644 --- a/test/rules/ban-parens/test.ts.lint +++ b/test/rules/ban-parens/test.ts.lint @@ -30,6 +30,7 @@ let x = (y = 1, z = 2); ~~~~~~~~~~~~~~ [Don't include parentheses around the initializer child of an expression of type VariableDeclaration] typeof(x); ~~~ [Don't include parentheses around an expression of type Identifier] +(0).toLocaleString('en-US', {style: 'percent'}); // No issues with let z = (x) => x + 1; From ad649afc517845231347998d80e8adbd790a663d Mon Sep 17 00:00:00 2001 From: Caleb Eggensperger Date: Wed, 27 Sep 2017 14:23:37 -0400 Subject: [PATCH 11/19] Fix issue with `return (\nfoo);` getting flagged even though those parens are necessary. --- src/rules/banParensRule.ts | 27 +++++++++++++++++++++------ test/rules/ban-parens/test.ts.fix | 2 ++ test/rules/ban-parens/test.ts.lint | 2 ++ 3 files changed, 25 insertions(+), 6 deletions(-) diff --git a/src/rules/banParensRule.ts b/src/rules/banParensRule.ts index fab09b4b020..9ecae534828 100644 --- a/src/rules/banParensRule.ts +++ b/src/rules/banParensRule.ts @@ -15,7 +15,14 @@ * limitations under the License. */ -import { isBinaryExpression, isLiteralExpression, isNumericLiteral, isParenthesizedExpression, isPropertyAccessExpression } from "tsutils"; +import { + isBinaryExpression, + isLiteralExpression, + isNumericLiteral, + isParenthesizedExpression, + isPropertyAccessExpression, + isReturnStatement, +} from "tsutils"; import * as ts from "typescript"; import * as Lint from "../index"; @@ -209,11 +216,19 @@ function walk(ctx: Lint.WalkContext) { node.expression.operatorToken.kind === ts.SyntaxKind.CommaToken) { replacement = []; } - // Don't replace (0).foo() with 0.foo() - if (!(isParenthesizedExpression(node) && - isNumericLiteral(node.expression) && - node.parent != undefined && - isPropertyAccessExpression(node.parent))) { + + if (!( + // Don't flag `(0).foo()`, because `0.foo()` doesn't work. + (isParenthesizedExpression(node) && + isNumericLiteral(node.expression) && + node.parent != undefined && + isPropertyAccessExpression(node.parent)) || + // Don't flag `return (\nfoo)`, since the parens are necessary. + (isParenthesizedExpression(node) && + node.parent != undefined && + isReturnStatement(node.parent) && + ctx.sourceFile.text[node.getStart() + 1] === "\n") + )) { ctx.addFailureAtNode( node, Rule.FAILURE_STRING_FACTORY(restriction.message), diff --git a/test/rules/ban-parens/test.ts.fix b/test/rules/ban-parens/test.ts.fix index e694f9e83db..323a90cf858 100644 --- a/test/rules/ban-parens/test.ts.fix +++ b/test/rules/ban-parens/test.ts.fix @@ -16,6 +16,8 @@ let x = foo(); let x = (y = 1, z = 2); typeof x; (0).toLocaleString('en-US', {style: 'percent'}); +return ( + 0); // No issues with let z = (x) => x + 1; diff --git a/test/rules/ban-parens/test.ts.lint b/test/rules/ban-parens/test.ts.lint index 9b62a4d33dc..06543cccbd6 100644 --- a/test/rules/ban-parens/test.ts.lint +++ b/test/rules/ban-parens/test.ts.lint @@ -31,6 +31,8 @@ let x = (y = 1, z = 2); typeof(x); ~~~ [Don't include parentheses around an expression of type Identifier] (0).toLocaleString('en-US', {style: 'percent'}); +return ( + 0); // No issues with let z = (x) => x + 1; From 930b733fd132c9f2c4ff188ef0f0689e9a013113 Mon Sep 17 00:00:00 2001 From: Caleb Eggensperger Date: Wed, 27 Sep 2017 15:40:43 -0400 Subject: [PATCH 12/19] Fix issue with destructuring assignment parentheses --- src/rules/banParensRule.ts | 11 ++++++++++- test/rules/ban-parens/test.ts.fix | 4 +++- test/rules/ban-parens/test.ts.lint | 5 ++++- test/rules/ban-parens/tslint.json | 1 + 4 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/rules/banParensRule.ts b/src/rules/banParensRule.ts index 9ecae534828..86c4ac70d2d 100644 --- a/src/rules/banParensRule.ts +++ b/src/rules/banParensRule.ts @@ -17,8 +17,10 @@ import { isBinaryExpression, + isExpressionStatement, isLiteralExpression, isNumericLiteral, + isObjectLiteralExpression, isParenthesizedExpression, isPropertyAccessExpression, isReturnStatement, @@ -227,7 +229,14 @@ function walk(ctx: Lint.WalkContext) { (isParenthesizedExpression(node) && node.parent != undefined && isReturnStatement(node.parent) && - ctx.sourceFile.text[node.getStart() + 1] === "\n") + ctx.sourceFile.text[node.getStart() + 1] === "\n") || + // Don't flag parens around destructuring assignment + (isParenthesizedExpression(node) && + isBinaryExpression(node.expression) && + node.expression.operatorToken.kind === ts.SyntaxKind.EqualsToken && + isObjectLiteralExpression(node.expression.left) && + node.parent != undefined && + isExpressionStatement(node.parent)) )) { ctx.addFailureAtNode( node, diff --git a/test/rules/ban-parens/test.ts.fix b/test/rules/ban-parens/test.ts.fix index 323a90cf858..7bf611af203 100644 --- a/test/rules/ban-parens/test.ts.fix +++ b/test/rules/ban-parens/test.ts.fix @@ -18,8 +18,10 @@ typeof x; (0).toLocaleString('en-US', {style: 'percent'}); return ( 0); +({a, b} = obj); +obj = {a, b}; // No issues with let z = (x) => x + 1; let x = (1 + 1) * 2; -(f()); +(f()) + 1; diff --git a/test/rules/ban-parens/test.ts.lint b/test/rules/ban-parens/test.ts.lint index 06543cccbd6..011cc88b3a2 100644 --- a/test/rules/ban-parens/test.ts.lint +++ b/test/rules/ban-parens/test.ts.lint @@ -33,8 +33,11 @@ typeof(x); (0).toLocaleString('en-US', {style: 'percent'}); return ( 0); +({a, b} = obj); +(obj = {a, b}); +~~~~~~~~~~~~~~ [Don't include parentheses around the expression child of an expression of type ExpressionStatement] // No issues with let z = (x) => x + 1; let x = (1 + 1) * 2; -(f()); +(f()) + 1; diff --git a/test/rules/ban-parens/tslint.json b/test/rules/ban-parens/tslint.json index c636e9063d0..c5787f34b73 100644 --- a/test/rules/ban-parens/tslint.json +++ b/test/rules/ban-parens/tslint.json @@ -12,6 +12,7 @@ "ParenthesizedExpression.expression", "CallExpression.arguments", "VariableDeclaration.type", + "ExpressionStatement.expression", "*.typeArguments" ] } From 5a38167bc123559cb684724fe138a0ce0e782a0f Mon Sep 17 00:00:00 2001 From: Caleb Eggensperger Date: Wed, 27 Sep 2017 16:29:26 -0400 Subject: [PATCH 13/19] Sometimes parentheses are necessary in arrow functions --- src/rules/banParensRule.ts | 8 +++++++- test/rules/ban-parens/test.ts.fix | 5 ++++- test/rules/ban-parens/test.ts.lint | 7 +++++-- test/rules/ban-parens/tslint.json | 3 ++- 4 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/rules/banParensRule.ts b/src/rules/banParensRule.ts index 86c4ac70d2d..6ac6d9cd5ac 100644 --- a/src/rules/banParensRule.ts +++ b/src/rules/banParensRule.ts @@ -16,6 +16,7 @@ */ import { + isArrowFunction, isBinaryExpression, isExpressionStatement, isLiteralExpression, @@ -236,7 +237,12 @@ function walk(ctx: Lint.WalkContext) { node.expression.operatorToken.kind === ts.SyntaxKind.EqualsToken && isObjectLiteralExpression(node.expression.left) && node.parent != undefined && - isExpressionStatement(node.parent)) + isExpressionStatement(node.parent)) || + // Don't flag parentheses in an arrow function's body + (isParenthesizedExpression(node) && + node.parent != undefined && + isArrowFunction(node.parent) && + node.parent.body === node) )) { ctx.addFailureAtNode( node, diff --git a/test/rules/ban-parens/test.ts.fix b/test/rules/ban-parens/test.ts.fix index 7bf611af203..361f0be539b 100644 --- a/test/rules/ban-parens/test.ts.fix +++ b/test/rules/ban-parens/test.ts.fix @@ -15,11 +15,14 @@ let x: Foo; let x = foo(); let x = (y = 1, z = 2); typeof x; +obj = {a, b}; + +// Exceptions (0).toLocaleString('en-US', {style: 'percent'}); return ( 0); ({a, b} = obj); -obj = {a, b}; +let foo = () => ({a: 1}.a); // No issues with let z = (x) => x + 1; diff --git a/test/rules/ban-parens/test.ts.lint b/test/rules/ban-parens/test.ts.lint index 011cc88b3a2..4c30d1ed4f8 100644 --- a/test/rules/ban-parens/test.ts.lint +++ b/test/rules/ban-parens/test.ts.lint @@ -30,12 +30,15 @@ let x = (y = 1, z = 2); ~~~~~~~~~~~~~~ [Don't include parentheses around the initializer child of an expression of type VariableDeclaration] typeof(x); ~~~ [Don't include parentheses around an expression of type Identifier] +(obj = {a, b}); +~~~~~~~~~~~~~~ [Don't include parentheses around the expression child of an expression of type ExpressionStatement] + +// Exceptions (0).toLocaleString('en-US', {style: 'percent'}); return ( 0); ({a, b} = obj); -(obj = {a, b}); -~~~~~~~~~~~~~~ [Don't include parentheses around the expression child of an expression of type ExpressionStatement] +let foo = () => ({a: 1}.a); // No issues with let z = (x) => x + 1; diff --git a/test/rules/ban-parens/tslint.json b/test/rules/ban-parens/tslint.json index c5787f34b73..1c757676367 100644 --- a/test/rules/ban-parens/tslint.json +++ b/test/rules/ban-parens/tslint.json @@ -5,7 +5,8 @@ "withChild": [ "Identifier", "LiteralExpression", - "Keyword" + "Keyword", + "PropertyAccessExpression" ], "asChildOf": [ "VariableDeclaration.initializer", From 9664596153c6173b47cf4e18d6000293b6d7a193 Mon Sep 17 00:00:00 2001 From: Caleb Eggensperger Date: Thu, 28 Sep 2017 15:18:52 -0400 Subject: [PATCH 14/19] Respond to review comments. --- src/rules/banParensRule.ts | 58 ++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 31 deletions(-) diff --git a/src/rules/banParensRule.ts b/src/rules/banParensRule.ts index 6ac6d9cd5ac..998d76bef6f 100644 --- a/src/rules/banParensRule.ts +++ b/src/rules/banParensRule.ts @@ -25,6 +25,7 @@ import { isParenthesizedExpression, isPropertyAccessExpression, isReturnStatement, + isSameLine, } from "tsutils"; import * as ts from "typescript"; @@ -199,7 +200,7 @@ function walk(ctx: Lint.WalkContext) { })); return ts.forEachChild(ctx.sourceFile, function cb(node: ts.Node): void { - if (isParenthesizedExpression(node) || isParenthesizedType(node)) { + if ((isParenthesizedExpression(node) && !parensAreNecessary(node, ctx.sourceFile)) || isParenthesizedType(node)) { const restriction = restrictions.find((r) => r.test(node)); if (restriction != undefined) { let replacement = [ @@ -219,38 +220,33 @@ function walk(ctx: Lint.WalkContext) { node.expression.operatorToken.kind === ts.SyntaxKind.CommaToken) { replacement = []; } - - if (!( - // Don't flag `(0).foo()`, because `0.foo()` doesn't work. - (isParenthesizedExpression(node) && - isNumericLiteral(node.expression) && - node.parent != undefined && - isPropertyAccessExpression(node.parent)) || - // Don't flag `return (\nfoo)`, since the parens are necessary. - (isParenthesizedExpression(node) && - node.parent != undefined && - isReturnStatement(node.parent) && - ctx.sourceFile.text[node.getStart() + 1] === "\n") || - // Don't flag parens around destructuring assignment - (isParenthesizedExpression(node) && - isBinaryExpression(node.expression) && - node.expression.operatorToken.kind === ts.SyntaxKind.EqualsToken && - isObjectLiteralExpression(node.expression.left) && - node.parent != undefined && - isExpressionStatement(node.parent)) || - // Don't flag parentheses in an arrow function's body - (isParenthesizedExpression(node) && - node.parent != undefined && - isArrowFunction(node.parent) && - node.parent.body === node) - )) { - ctx.addFailureAtNode( - node, - Rule.FAILURE_STRING_FACTORY(restriction.message), - replacement); - } + ctx.addFailureAtNode( + node, + Rule.FAILURE_STRING_FACTORY(restriction.message), + replacement); } } return ts.forEachChild(node, cb); }); } + +/** + * Checks some exceptional cases where the parentheses likely are still required. + */ +function parensAreNecessary(node: ts.ParenthesizedExpression, sourceFile: ts.SourceFile) { + return ( + // Don't flag `(0).foo()`, because `0.foo()` doesn't work. + (isNumericLiteral(node.expression) && + isPropertyAccessExpression(node.parent!)) || + // Don't flag `return (\nfoo)`, since the parens are necessary. + (isReturnStatement(node.parent!) && + !isSameLine(sourceFile, node.getStart(), node.expression.getStart())) || + // Don't flag parens around destructuring assignment + (isBinaryExpression(node.expression) && + node.expression.operatorToken.kind === ts.SyntaxKind.EqualsToken && + isObjectLiteralExpression(node.expression.left) && + isExpressionStatement(node.parent!)) || + // Don't flag parentheses in an arrow function's body + (isArrowFunction(node.parent!) && + (node.parent as ts.ArrowFunction).body === node)); +} From 9a1454d4dba0a46d59c08a1ca6a816fc1f7609bd Mon Sep 17 00:00:00 2001 From: Caleb Eggensperger Date: Fri, 29 Sep 2017 15:58:14 -0400 Subject: [PATCH 15/19] Respond to review comments. --- src/rules/banParensRule.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/rules/banParensRule.ts b/src/rules/banParensRule.ts index 998d76bef6f..cef2ae9a3ad 100644 --- a/src/rules/banParensRule.ts +++ b/src/rules/banParensRule.ts @@ -68,6 +68,11 @@ export class Rule extends Lint.Rules.AbstractRule { For example, \`{"withChild": ["Identifier"]}\` would ban \`(foo)\`. + Some token types shouldn't be used here, since the fixer (which) + just removes the parens) would break the code. For example, + BinaryExpression and ConditionalExpression both have many cases + where removing the parens can break code. + 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 banned. For example, @@ -240,13 +245,12 @@ function parensAreNecessary(node: ts.ParenthesizedExpression, sourceFile: ts.Sou isPropertyAccessExpression(node.parent!)) || // Don't flag `return (\nfoo)`, since the parens are necessary. (isReturnStatement(node.parent!) && - !isSameLine(sourceFile, node.getStart(), node.expression.getStart())) || + !isSameLine(sourceFile, node.expression.pos, node.expression.getStart(sourceFile))) || // Don't flag parens around destructuring assignment (isBinaryExpression(node.expression) && node.expression.operatorToken.kind === ts.SyntaxKind.EqualsToken && isObjectLiteralExpression(node.expression.left) && isExpressionStatement(node.parent!)) || // Don't flag parentheses in an arrow function's body - (isArrowFunction(node.parent!) && - (node.parent as ts.ArrowFunction).body === node)); + isArrowFunction(node.parent!)); } From 2c0af4194c62125abae2eded322d8e511a9de0e4 Mon Sep 17 00:00:00 2001 From: Caleb Eggensperger Date: Fri, 20 Oct 2017 13:43:38 -0400 Subject: [PATCH 16/19] Add tsx tests --- test/rules/ban-parens/test.tsx.fix | 5 +++++ test/rules/ban-parens/test.tsx.lint | 9 +++++++++ 2 files changed, 14 insertions(+) create mode 100644 test/rules/ban-parens/test.tsx.fix create mode 100644 test/rules/ban-parens/test.tsx.lint diff --git a/test/rules/ban-parens/test.tsx.fix b/test/rules/ban-parens/test.tsx.fix new file mode 100644 index 00000000000..d2d45a3b225 --- /dev/null +++ b/test/rules/ban-parens/test.tsx.fix @@ -0,0 +1,5 @@ +const foo = +
text
+; + +