phoedos/pmd
1
1
Fork 1
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[java] Fix ImmutableField with conditionally assignment in ctors

Browse Source 
Fixes #3850
This commit is contained in:
Andreas Dangel
2022-03-17 11:56:27 +01:00
parent 59b36b6185
commit b22990a668
3 changed files with 37 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
docs/pages/release_notes.md
View File

@ -57,6 +57,8 @@ The CLI itself remains compatible, if you run PMD via command-line, no action is
* [#3299](https://github.com/pmd/pmd/issues/3299): \[core] Deprecate system properties of PMDCommandLineInterface
* doc
* [#3812](https://github.com/pmd/pmd/issues/3812): \[doc] Documentation website table of contents broken on pages with many subheadings
* java-design
* [#3850](https://github.com/pmd/pmd/issues/3850): \[java] ImmutableField - false negative when field assigned in constructor conditionally
### API Changes

27
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/ImmutableFieldRule.java
View File

@ -16,9 +16,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTConstructorDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTDoStatement;
import net.sourceforge.pmd.lang.java.ast.ASTForStatement;
import net.sourceforge.pmd.lang.java.ast.ASTIfStatement;
import net.sourceforge.pmd.lang.java.ast.ASTLambdaExpression;
import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTTryStatement;
import net.sourceforge.pmd.lang.java.ast.ASTVariableInitializer;
import net.sourceforge.pmd.lang.java.ast.ASTWhileStatement;
@ -60,11 +58,15 @@ public class ImmutableFieldRule extends AbstractLombokAwareRule {
continue;
}
FieldImmutabilityType type = initializedInConstructor(field, entry.getValue(), new HashSet<>(constructors));
List<NameOccurrence> usages = entry.getValue();
FieldImmutabilityType type = initializedInConstructor(field, usages, new HashSet<>(constructors));
if (type == FieldImmutabilityType.MUTABLE) {
continue;
}
if (type == FieldImmutabilityType.IMMUTABLE || type == FieldImmutabilityType.CHECKDECL && initializedWhenDeclared(field)) {
if (initializedWhenDeclared(field) && usages.isEmpty()) {
addViolation(data, field.getNode(), field.getImage());
}
if (type == FieldImmutabilityType.IMMUTABLE || type == FieldImmutabilityType.CHECKDECL && !initializedWhenDeclared(field)) {
addViolation(data, field.getNode(), field.getImage());
}
}
@ -90,14 +92,7 @@ public class ImmutableFieldRule extends AbstractLombokAwareRule {
methodInitCount++;
continue;
}
// Check for assigns in if-statements, which can depend on
// constructor
// args or other runtime knowledge and can be a valid reason
// to instantiate
// in one constructor only
if (node.getFirstParentOfType(ASTIfStatement.class) != null) {
methodInitCount++;
}
if (inAnonymousInnerClass(node)) {
methodInitCount++;
} else if (node.getFirstParentOfType(ASTLambdaExpression.class) != null) {
@ -106,15 +101,15 @@ public class ImmutableFieldRule extends AbstractLombokAwareRule {
consSet.add(constructor);
}
} else {
if (node.getFirstParentOfType(ASTMethodDeclaration.class) != null) {
methodInitCount++;
} else if (node.getFirstParentOfType(ASTLambdaExpression.class) != null) {
if (node.getFirstParentOfType(ASTLambdaExpression.class) != null) {
lambdaUsage++;
} else {
methodInitCount++;
}
}
}
}
if (usages.isEmpty() || methodInitCount == 0 && lambdaUsage == 0 && consSet.isEmpty()) {
if (usages.isEmpty() || methodInitCount == 0 && lambdaUsage == 0 && allConstructors.equals(consSet)) {
result = FieldImmutabilityType.CHECKDECL;
} else {
allConstructors.removeAll(consSet);

27
pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/ImmutableField.xml
View File

@ -294,7 +294,7 @@ public final class Foo {
<expected-problems>0</expected-problems>
<code><![CDATA[
public class MyClass {
private int positive = 1;
private int positive = 1; // cannot be final, is modified in ctor
public MyClass(int value) {
// if negative keep default
@ -308,10 +308,11 @@ public class MyClass {
<test-code>
<description>Bug 1740480, assignment in single constructor based on constructor argument check</description>
<expected-problems>0</expected-problems>
<expected-problems>1</expected-problems>
<expected-linenumbers>2</expected-linenumbers>
<code><![CDATA[
public class MyClass {
private int positive;
private int positive; // can be final
public MyClass(int value) {
if (value > 0) {
@ -548,6 +549,26 @@ public class TestFinal {
while (random.nextBoolean())
e = e.add(BigInteger.ONE);
}
}
]]></code>
</test-code>
<test-code>
<description>[java] ImmutableField - false negative when field assigned in constructor conditionally #3850</description>
<expected-problems>1</expected-problems>
<expected-linenumbers>3</expected-linenumbers>
<code><![CDATA[
public class ExampleImmutableField {
private String str; // false negative here, could be final
public ExampleImmutableField(String strLocal, boolean flag) {
if (flag){
this.str = strLocal;
} else {
this.str = strLocal+"123";
}
}
}
]]></code>
</test-code>