-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Fix #5590: [java] LiteralsFirstInComparisons with constant field #5595
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Compared to main: |
I changed the return type of a protected method which should have been package-private. The covariant overrides were also not needed.
adangel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
|
|
||
| @Override | ||
| public boolean isCompileTimeConstant() { | ||
| return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isCompileTimeConstant can also be removed here and the default from ASTExpression can be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, this change broke the (treedump) tests, that expect that NullLiterals are not compile-time constants. See also
pmd/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/AbstractLiteral.java
Lines 58 to 60 in 120c2bb
| public boolean isCompileTimeConstant() { | |
| return true; // note: NullLiteral overrides this to false | |
| } |
@oowekyala I would have expected, that all literals are compile time constants. But this doesn't seem to be the case for null? Is this correct? Do you know, why?
If I understand it correctly, compile-time constants can only be created by Constant Expressions which is of a primitive type or String. But null is neither, therefore null is not a compile-time constant...
Describe the PR
To fix this i had to refactor a bit the constant folding logic. It wasn't actually implementing the definition of the JLS for a "compile-time constant". The logic now makes
ASTExpression#isCompileTimeConstant()and#getConstValue()respect the JLS definition, that is, the only variables referenced must be compile-time constant fields (ie static final with CT constant initializer, and cannot have blank initializer). However this is very restrictive, while some rules want to know if the value is known at compile time, even if it uses non-static final fields, or final local variables for instance. I extended the logic of the constant folder to track whether the resolved constant value is a CT constant according to the strict definition or to the loose definition.Related issues
Ready?
./mvnw clean verifypasses (checked automatically by github actions)