Add new ban-parens rule #3238

calebegg · 2017-09-19T19:01:42Z

Since the definition of “unnecessary” is by its nature very opinionated, I made this rule very open-ended, with a nice set of default “unnecessary” parens that I hope should be relatively uncontroversial.

Let me know if there are defaults you think I should either add or remove.

ajafff · 2017-09-19T20:01:22Z

src/rules/noUnnecessaryParensRule.ts

+            return {
+                message: `the ${whichChild} child of an expression of type ${parentKind}`,
+                test(node: ts.ParenthesizedExpression | ts.ParenthesizedTypeNode) {
+                    if (node.parent == undefined) {


That condition will never be true

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

ajafff · 2017-09-19T20:11:45Z

src/rules/noUnnecessaryParensRule.ts

+                    "VariableDeclaration.initializer",
+                    "ParenthesizedExpression.expression",
+                    "CallExpression.arguments",
+                    "ExpressionStatement.expression",


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)) {}

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

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.

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

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

ajafff

Code looks good to me. Need to think a bit more about the defaults since we cannot simply change them later.

ajafff · 2017-09-26T13:30:34Z

src/rules/noUnnecessaryParensRule.ts

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


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

ajafff · 2017-09-26T13:34:40Z

src/rules/noUnnecessaryParensRule.ts

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


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

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

ajafff · 2017-09-26T13:36:08Z

src/rules/noUnnecessaryParensRule.ts

+        // ex: let x: (string|number) = 3;
+        "VariableDeclaration.type",
+        // ex: function(foo: (number|string)) {}
+        "Parameter.type",


// foo[(bar + "baz")] "ElementAccessExpression.argumentExpression", // `${(foo)}` "TemplateSpan.expression"

ajafff · 2017-09-26T13:37:26Z

src/rules/noUnnecessaryParensRule.ts

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


also "NewExpression.arguments"

ajafff · 2017-09-26T13:41:21Z

src/rules/noUnnecessaryParensRule.ts

+        // ex: foo((a), b)
+        "CallExpression.arguments",
+        // ex: Foo<(string|number), string>
+        "TypeReference.typeArguments",


"*.typeArguments"?

ajafff · 2017-09-26T13:43:04Z

src/rules/noUnnecessaryParensRule.ts

+                    "VariableDeclaration.initializer",
+                    "ParenthesizedExpression.expression",
+                    "CallExpression.arguments",
+                    "ExpressionStatement.expression",


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

ajafff · 2017-09-26T13:52:47Z

@calebegg please merge master to fix the build

* Add link to astexplorer.net * Support *.foo in asChildOf (and add a test) * Add new default rules * Remove erroneous default rule

…rens

ajafff

CI is failing at

tslint/src/rules/noUnnecessaryTypeAssertionRule.ts

Line 77 in 02dd308

(castType.getProperties().some((symbol) => !isNaN(Number(symbol.name))))) {

ajafff · 2017-09-26T18:09:05Z

test/rules/no-unnecessary-parens/test.ts.lint

@@ -0,0 +1,33 @@
+let x = (3);


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

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?

ajafff · 2017-09-26T18:11:42Z

src/rules/noUnnecessaryParensRule.ts

+        // ex: let x: (string|number) = 3;
+        "VariableDeclaration.type",
+        // ex: function(foo: (number|string)) {}
+        "Parameter.type",


I'm still in favor of using "*.type". I can't come up with a case where parens are desired or even required.

* Don’t suggest a fix when the child is a comma operator expresssion * Use *.type instead of Parameter.type

calebegg · 2017-09-26T18:47:00Z

Hey, that's my own commit! :-) Fixed.

ajafff

LGTM
What do you think about renaming the rule to ban-parens? I think that expresses the intention of the rule better: the user needs to specify where to ban parens, the rule does not automagically find all unnecessary parens.

ajafff · 2017-09-26T19:50:52Z

src/rules/noUnnecessaryParensRule.ts

+        // ex: let x = (1 + foo());
+        "VariableDeclaration.initializer",
+        // ex: type x = (string|number);
+        "TypeAliasDeclaration.type",


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

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.

ajafff · 2017-09-26T19:51:02Z

src/rules/noUnnecessaryParensRule.ts

+        // ex: (foo.bar());
+        "ExpressionStatement.expression",
+        // ex: let x: (string|number) = 3;
+        "VariableDeclaration.type",


also redundant

calebegg · 2017-09-26T20:40:22Z

Makes sense. Renamed.

Also `throw(err)` to `throwerr`

calebegg · 2017-09-27T15:48:09Z

FYI, updated to fix a minor issue that came up when running over our codebase -- typeof(x) and throw(y) become typeofx and throwy.

…rens are necessary.

calebegg · 2017-09-27T18:23:51Z

Fixed more issues that cropped up in our codebase -- (0).toString() became 0.toString(), which doesn't compile.

return (
   foo);

becomes

return 
  foo;

which also doesn't compile.

ajafff · 2017-09-28T18:39:06Z

src/rules/banParensRule.ts

+                    // Don't flag `(0).foo()`, because `0.foo()` doesn't work.
+                    (isParenthesizedExpression(node) &&
+                        isNumericLiteral(node.expression) &&
+                        node.parent != undefined &&


node.parent cannot be undefined

Changed to !

For another case, this broke the type narrowing, so I had to add a cast.

ajafff · 2017-09-28T18:39:24Z

src/rules/banParensRule.ts

+                        isPropertyAccessExpression(node.parent)) ||
+                    // Don't flag `return (\nfoo)`, since the parens are necessary.
+                    (isParenthesizedExpression(node) &&
+                        node.parent != undefined &&


ajafff · 2017-09-28T18:40:41Z

src/rules/banParensRule.ts

+                    (isParenthesizedExpression(node) &&
+                        node.parent != undefined &&
+                        isReturnStatement(node.parent) &&
+                        ctx.sourceFile.text[node.getStart() + 1] === "\n")


isSameLine(ctx.sourceFile, node.expression.pos, node.expression.getStart(ctx.sourceFile))

Done (had to tweak it a bit; used node.expression.getStart() and node.getStart())

ajafff · 2017-09-28T18:44:17Z

src/rules/banParensRule.ts

+                    replacement = [];
+                }
+
+                if (!(


Consider extracting this condition to a function and invoke it in the condition in line 199

if (isParenthesizedExpression(node) && !parensAreNecessary(node, ctx.sourceFile) || isParenthesizedType(node))

ajafff · 2017-09-29T15:19:38Z

src/rules/banParensRule.ts

+    }
+}
+
+function isParenthesizedType(node: ts.Node): node is ts.ParenthesizedTypeNode {


note to myself: remove this after the next release of tsutils

ajafff · 2017-09-29T15:24:07Z

src/rules/banParensRule.ts

+            isPropertyAccessExpression(node.parent!)) ||
+        // Don't flag `return (\nfoo)`, since the parens are necessary.
+        (isReturnStatement(node.parent!) &&
+            !isSameLine(sourceFile, node.getStart(), node.expression.getStart())) ||


please pass the sourceFile parameter to .getStart() to avoid unnecesary lookups.
I'm also very confident that you can replace node.getStart() with node.expression.pos. The former gives you the start of the open paren, the latter gives the end of the open paren. But that difference doesn't matter for isSameLine

Done. Sorry, I wrestled with this a bit until I got something working and then didn't pay more attention. I didn't really know the difference between .getStart and .pos (though I think I get it now). Is there documentation for this stuff somewhere?

What's the benefit of passing sourceFile in?

.pos is the position where the parser started to parse the current node (or token). node.pos is the same value as the preceding token's .end.
node.getStart() is the position where the first token of a node starts. The difference between .pos and .getStart() is caused by trivia (whitespace and comments).
Also note that node.pos === node.getFullStart() and node.end === node.getEnd()

Re: passing sourceFile parameter:
I wrote this down in https://palantir.github.io/tslint/develop/custom-rules/performance-tips.html

There are serveral methods that have an optional parameter sourceFile. Don’t omit this parameter if you care for performance. If ommitted, typescript needs to walk up the node’s parent chain until it reaches the SourceFile. This can be quite costly when done frequently on deeply nested nodes.

You also find there why I prefer not to call .getStart():

node.getStart() scans the source to skip all the leading trivia. Although barely noticeable, this operation is not for free. If you need the start position of a node more than once per function, consider caching it.

Got it, makes sense. Thanks for the tips!

ajafff · 2017-09-29T15:27:14Z

src/rules/banParensRule.ts

+            isExpressionStatement(node.parent!)) ||
+        // Don't flag parentheses in an arrow function's body
+        (isArrowFunction(node.parent!) &&
+            (node.parent as ts.ArrowFunction).body === node));


you can remove the second part of the condition. A ParenthesizedExpression can only occur in the body of an ArrowFunction

ajafff · 2017-09-29T15:36:10Z

src/rules/banParensRule.ts

+        // Don't flag parentheses in an arrow function's body
+        (isArrowFunction(node.parent!) &&
+            (node.parent as ts.ArrowFunction).body === node));
+}


We should stop adding more exceptions or it will get out of hand. Users should not ban parens around certain Expressions, for example BinaryExpression and ConditionalExpression. (maybe add this to the docs?)
Otherwise you would need to allow BinaryExpression and ConditionalExpression when it is (New|Call|PropertyAccess|ElementAccess|Void|Await|Delete)Expression.expression, e.g. (foo.bar || foo.baz)(), (foo ? bar : baz)(), ...

... and probably many more

Added documentation.

I agree it's getting complex. If you think there are any default syntax kinds that aren't worth the exceptions, let me know. It's running cleanly over our codebase now with defaults, so I am at least somewhat confident that we've found most of the strange cases.

calebegg · 2017-10-10T17:47:03Z

Anything else you need from me for this?

ajafff · 2017-10-10T18:59:29Z

@calebegg nope, I just want @adidahiya to sign this off before landing, but it seems like he's busy lately.

adidahiya · 2017-10-20T15:25:17Z

test/rules/ban-parens/test.ts.lint

+// No issues with
+let z = (x) => x + 1;
+let x = (1 + 1) * 2;
+(f()) + 1;


can you also add some tests for common JSX syntax?

const foo = ( <div>text</div> ); <button ref={(el => this.buttonEl = el)} /> ~ ~ <button ref={el => (this.buttonEl = el)} /> ~ ~

Added two tests. I don't really know what I'm doing with JSX :-) so let me know if they could be improved. (The 2nd and 3rd you gave aren't errors with my current test settings)

Ok, the 3rd example I gave should not be an error, but I think the 2nd one should so that we can align more closely with prettier. Would you be able to update your rule implementation so that my 2nd snippet does cause a lint failure?

adidahiya

sorry for the delay. I have some minor comments, see above

adidahiya · 2017-10-20T18:27:38Z

test/rules/ban-parens/test.tsx.lint

+            ~
+    <div>text</div>
+~~~~~~~~~~~~~~~~~~~
+);


these should not be banned by default. parens around multiline jsx statements are enforced in the default tslint-react config.

this is why I wanted you to add these tests -- the default config should not conflict with tslint-react.

I see -- I wish you'd just said that.

I don't know if it'll be easy to do this, if jsx expressions can occur anywhere. That eliminates most of the rules. I can just exclude jsx files entirely from this check, maybe?

Sorry if I wasn't clear upfront. Excluding .tsx files completely in this rule is not a good option because it will feel pretty inconsistent for TSX users -- there's lots of regular TS code in .tsx files. I haven't really dug into the internals of this rule implementation here (deferring to @ajafff) but I am concerned about the overall UX of this rule for tslint-react users, so I'd like you to update it before merging.

ajafff · 2017-10-22T13:36:52Z

@calebegg I would avoid any hard coded special handling for JsxElements or .jsx files.
Instead you could add a config option for exceptions that override the blacklisted locations.

Consider this code snippet from the tests:

const foo = (
    <div>text</div>
);

The blacklist disallows parens in VariableDeclaration.initializer while the whitelist requires parens around JsxElement. Therefore no error.

@adidahiya I'd rather not distinguish between single-line and multi-line JsxElements.
Do we also need to whitelist JsxSelfClosingElement? It will mostly be on a single line but can also span multiple lines when there are many attributes.

In the near future TypeScript will add support for JsxFragment. We should whitelist it now to be prepared for that feature.

calebegg · 2017-10-25T21:29:01Z

Makes sense. Done. (I can easily add JsxSelfClosingElement if we need it).

ajafff

Sorry for the delay. This should be good to go once my 2 comments are addressed

ajafff · 2017-11-02T23:28:23Z

src/rules/banParensRule.ts

+    return ts.forEachChild(ctx.sourceFile, function cb(node: ts.Node): void {
+        if ((isParenthesizedExpression(node) && !parensAreNecessary(node, ctx.sourceFile)) || isParenthesizedType(node)) {
+            const restriction = restrictions.find((r) => r.test(node));
+            if (restriction != undefined) {


consider moving the check for exceptions to this condition. that avoid checking exceptions for every blacklist entry.

also: prefer !== over !=

Oh, that makes much more sense!

I actually moved it to above restrictions.find so that it can just skip the testing entirely for exception nodes.

ajafff · 2017-11-02T23:29:02Z

src/rules/banParensRule.ts

+        ...ctx.options.asChildOf,
+    ] : ctx.options.asChildOf;
+    const exceptions = ctx.options.default ? [
+        "JsxElement",


I think it's best to also whitelist JsxSelfClosingElement

ajafff · 2017-11-02T23:31:39Z

You need to merge latest master to fix the CI failures

* Add JsxSelfClosingElement to default exceptions * Refactor exception logic to one place.

…rens

ajafff · 2017-11-03T19:55:58Z

@adidahiya PTAL

@calebegg merging master caused some new lint failures:

/home/ubuntu/tslint/src/rules/completed-docs/tagExclusion.ts
ERROR: 78:43   ban-parens  Don't include parentheses around an expression of type LiteralExpression

/home/ubuntu/tslint/src/rules/maxLineLengthRule.ts
ERROR: 99:28   ban-parens  Don't include parentheses around an expression of type Identifier

/home/ubuntu/tslint/src/rules/noUnsafeAnyRule.ts
ERROR: 102:45  ban-parens  Don't include parentheses around the initializer child of an expression of type VariableDeclaration

adidahiya · 2017-11-07T20:24:02Z

Ok, I have some more feedback but I apologize if this is poorly timed. I haven't been able to keep up with all the external PRs submitted to this repo recently.

I don't think this rule in its current form belongs in the core rule set. The configuration options are esoteric and unapproachable for most users. This is mainly because they expose internals of the AST semantics to the end user -- I'm drawing a line there when it comes to developer experience. I think we need to limit the scope of possible configuration because otherwise users will get confused by the error messages it produces and will have a hard time figuring out how to configure the rule's options to make their linter pass. Alternatively, it would be fine to keep this in a separate ruleset package for power users of TSLint.

calebegg · 2017-11-15T17:33:54Z

Closing. It would have been great to get this feedback earlier on in the process.

calebegg added 2 commits September 18, 2017 19:31

Merge remote-tracking branch 'palantir/master'

cffa68e

Add no-unnecessary-parens rule

6465c76

ajafff reviewed Sep 19, 2017

View reviewed changes

calebegg added 2 commits September 19, 2017 16:32

Added a test that uses .type and a few more rules to the defaults.

10702fe

Remove null check for node.parent

4dd6f05

brandf mentioned this pull request Sep 22, 2017

noUnusedVariableRule should apply the ignorePattern to imports #3187

Merged

4 tasks

ajafff reviewed Sep 26, 2017

View reviewed changes

calebegg added 2 commits September 26, 2017 12:59

Address review comments.

bb5c967

* Add link to astexplorer.net * Support *.foo in asChildOf (and add a test) * Add new default rules * Remove erroneous default rule

Merge remote-tracking branch 'palantir/master' into no-unnecessary-pa…

f58f608

…rens

ajafff reviewed Sep 26, 2017

View reviewed changes

calebegg added 2 commits September 26, 2017 14:46

Address review comments

d23b6bb

* Don’t suggest a fix when the child is a comma operator expresssion * Use *.type instead of Parameter.type

Remove unnecessary parentheses

9d5ff22

ajafff approved these changes Sep 26, 2017

View reviewed changes

ajafff requested a review from adidahiya September 26, 2017 19:54

Rename to ban-parens

005beda

calebegg changed the title ~~Add new no-unnecessary-parens rule~~ Add new ban-parens rule Sep 26, 2017

calebegg added 2 commits September 26, 2017 17:19

Remove premature return.

b748e96

Fix an issue with typeof(x) being corrected to typeofx

c981ad1

Also `throw(err)` to `throwerr`

calebegg added 2 commits September 27, 2017 14:13

Fix issue with (0).foo() being corrected to 0.foo()

efae3d3

Fix issue with return (\nfoo); getting flagged even though those pa…

ad649af

…rens are necessary.

calebegg added 2 commits September 27, 2017 16:31

Fix issue with destructuring assignment parentheses

930b733

Sometimes parentheses are necessary in arrow functions

5a38167

ajafff reviewed Sep 28, 2017

View reviewed changes

Respond to review comments.

9664596

ajafff reviewed Sep 29, 2017

View reviewed changes

calebegg and others added 2 commits September 29, 2017 15:58

Respond to review comments.

9a1454d

Merge branch 'master' into no-unnecessary-parens

de72308

adidahiya approved these changes Oct 20, 2017

View reviewed changes

adidahiya reviewed Oct 20, 2017

View reviewed changes

adidahiya suggested changes Oct 20, 2017

View reviewed changes

Add tsx tests

2c0af41

adidahiya suggested changes Oct 20, 2017

View reviewed changes

Respond to review comments

49dcea1

ajafff reviewed Nov 2, 2017

View reviewed changes

calebegg added 2 commits November 3, 2017 15:43

Respond to review comments

29d536c

* Add JsxSelfClosingElement to default exceptions * Refactor exception logic to one place.

Merge remote-tracking branch 'palantir/master' into no-unnecessary-pa…

73de3c3

…rens

ajafff approved these changes Nov 3, 2017

View reviewed changes

Fix ban-parens violations

e282a3f

calebegg closed this Nov 15, 2017

ajafff mentioned this pull request Dec 11, 2017

[Rule request] Flag unnecessary parenthesis #3567

Closed

ajafff mentioned this pull request Dec 19, 2017

[rule suggestion] ban-syntax #3591

Closed

Add new ban-parens rule #3238

Add new ban-parens rule #3238

Conversation

calebegg commented Sep 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajafff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajafff commented Sep 26, 2017

ajafff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebegg commented Sep 26, 2017

ajafff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebegg commented Sep 26, 2017

calebegg commented Sep 27, 2017

calebegg commented Sep 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebegg commented Oct 10, 2017

ajafff commented Oct 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adidahiya Oct 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adidahiya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajafff commented Oct 22, 2017

calebegg commented Oct 25, 2017 • edited Loading

ajafff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajafff commented Nov 2, 2017

ajafff commented Nov 3, 2017

adidahiya commented Nov 7, 2017

calebegg commented Nov 15, 2017

adidahiya Oct 20, 2017 •

edited

Loading

calebegg commented Oct 25, 2017 •

edited

Loading