Skip to content

lint: Forbid ++ and -- operators in many positions#5878

Closed
weswigham wants to merge 3 commits into
microsoft:masterfrom
weswigham:forbid-++
Closed

lint: Forbid ++ and -- operators in many positions#5878
weswigham wants to merge 3 commits into
microsoft:masterfrom
weswigham:forbid-++

Conversation

@weswigham
Copy link
Copy Markdown
Member

Always forbid prefix versions, forbid postfix versions when not used in a for statement, an expression statement, or an element access expression.

@DanielRosenwasser is still on the fence about this rule, since we violate it so many times already and isn't sure if its the right call to add it.

@weswigham weswigham changed the title Forbid ++ and -- operators in many positions lint: Forbid ++ and -- operators in many positions Dec 2, 2015
@weswigham weswigham added the Infrastructure Issue relates to TypeScript team infrastructure label Dec 2, 2015
@sandersn
Copy link
Copy Markdown
Member

sandersn commented Dec 2, 2015

I looked at the failures in the checker, scanner and emitter. All the checker ones can be trivially split into two expressions -- they are all of the form if (!x.id) x.id = counter++;

The ones in the scanner all look like this:

return pos++, token = SyntaxKind.PercentToken;

(there are a lot of the form return pos += 2, ... as well that should be changed -- see below about a no-comma rule)
Changing this would be a improvement. I think a valid transformation is

pos++;
return token = SyntaxKind.PercentToken;

But that's based on my understanding of , as equivalent to begin/progn/>>. In other words, the pos++ is gratuitously nested inside an expression for no good reason except to save vertical space. (Note that if we had a no-comma rule already, then the scanner wouldn't have any hits. Adding a no-comma rule would also be an improvement in my opinion.)

The emitter uses prefix ++ quite a bit (though by no means exclusively). I suppose we could argue about which one to allow if ++i outnumbers i++.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is not an override and won't be called by the RuleWalker's walk of the tree, right? I don't know what the usual style for lint rules is, but in the past I found it less confusing to give these non-recursive checker methods a different prefix, something like checkIncrementDecrement.

@sandersn
Copy link
Copy Markdown
Member

sandersn commented Dec 2, 2015

👍

@GoToLoop
Copy link
Copy Markdown

TS shouldn't get in the way of what is completely traditionally valid JS/Java/C code.
I use ++ & -- operators in many places for casual coding.

@DanielRosenwasser
Copy link
Copy Markdown
Member

This is for our own codebase, not for users.

@GoToLoop
Copy link
Copy Markdown

Whew! Sorry! O_o
Although I think contributors shouldn't have so many rules to follow and have some reasonable freedom.

@sandersn sandersn mentioned this pull request Dec 23, 2015
@weswigham
Copy link
Copy Markdown
Member Author

This is superceeded by #6212 now.

@weswigham weswigham closed this Jan 3, 2016
@weswigham weswigham deleted the forbid-++ branch October 12, 2017 08:05
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Infrastructure Issue relates to TypeScript team infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants