[java] Merge different increment/decrement expressions #1890
Conversation
There was a problem hiding this comment.
@oowekyala I've added a samples section here: https://github.com/pmd/pmd/wiki/Java_clean_changes#merge-different-increment-and-decrement-expressions
Could you please review? I'm unsure about how the PMD 7 AST looks like.
Only two things I'd change for now:
- ASTIncrementExpression::getBaseExpression() -> getOperand()
- TestExtension.kt: variableRef -> variableAccess
Let me know, if you agree and whether I should change it.
| * | ||
| * </pre> | ||
| */ | ||
| public final class ASTIncrementExpression extends AbstractJavaExpr implements ASTExpression, LeftRecursiveNode { |
There was a problem hiding this comment.
I'm not so sure about the name. "IncrementExpression" doesn't sound right, since the node can also be a decrement expression...
But I also don't know a better name.
I had a look at how eclipse jdt exposes this: They have a PrefixExpression and a PostfixExpression. So they combine the grammar in a different way....
Is this something we should do as well? Maybe to keep it in mind (not for this PR though).
There was a problem hiding this comment.
IMO PrefixExpr + PostfixExpr are not so great because they share the two operators ++ and --, and I'd rather not duplicate them in separate enums.
AFAIK there's no term for "decrement or increment" expression. Maybe "XcrementExpression"? Just kidding 😆
However there is a term for something that is "either postfix or prefix": "unary". Maybe then, it would make sense to merge IncrementExpression into UnaryExpression and have an attribute for whether it's prefix or postfix instead. That way we have a single node, and also a single enum for unary operators too. Wdyt?
There was a problem hiding this comment.
Not sure if merging IncrementExpr with UnaryExpr is helpful - In the current use cases in the rules, we would then need to check for the operator, right? (At least in AvoidReassigningLoopVariablesRule)
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTIncrementExpression.java
Outdated
Show resolved
Hide resolved
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/IncrementOp.java
Outdated
Show resolved
Hide resolved
...java/net/sourceforge/pmd/lang/java/rule/bestpractices/AvoidReassigningLoopVariablesRule.java
Show resolved
Hide resolved
pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/ast/ASTArrayAccessTest.kt
Show resolved
Hide resolved
pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/ast/TestExtensions.kt
Show resolved
Hide resolved
…mentOp.java Co-Authored-By: Andreas Dangel <[email protected]>
Among the different possible categorisations
of unary expressions, this is probably the
most logical and easiest to document.
Here's a comparison of different possible
categorisations. Note: `_++` is the postfix
increment operator, while `++_` is the prefix
one - idem for decrement. The last one is the
one implemented by this commit.
\## 6.0.x
```
Unary = { -, + }
UnaryNotPlusMinus { !, ~ }
PreIncrement = { ++_ }
PreDecrement { --_ }
Postfix { _++, _++ }
```
* Very assymmetric, splits operators based on parsing
concerns
\## Before pmd#1890:
```
Unary = { -, + , !, ~ }
PreIncrement = { ++_ }
PreDecrement { --_ }
Postfix { _++, _++ }
```
* Minor simplification
\## pmd#1890:
```
Unary = Prefix \ { ++_, --_ })
Increment ( { ++ , -- } x (postfix, prefix) )
```
* Names are weird (IncrementExpr may be decrement, Unary != Increment
even though semantically, Increment \subset Unary)
* No possibility to introduce a supertype (what would it be?)
* But easy to match all increment/decrement expressions
\## JLS (also, Eclipse):
```
Prefix = { !, ~, -, +, ++_, --_ }
Postfix = { _++, _-- }
```
* Both can have super interface UnaryExpr
* This allows matching all increment/decrement expressions easily too
* Easiest to document, JLS like, AST like
* Fits well with `InfixExpr`
The current grammar has PreDecrementExpression, PreIncrementExpression, and PostfixExpression to represent mutation expressions.
This PR makes two contributions:
IncrementOp, which adopts constants from UnaryOp (which didn't really belong there).ASTAssignableExpr.@AccessTypeattribute to check if they're read or written to.