hiifong/pmd
1
0
Fork 0
forked from phoedos/pmd
Code Pull Requests Activity

Merge pull request #4101 from adangel:issue-4099-constructorcallsoverridable

Browse Source 
[java] ConstructorCallsOverridableMethod should consider method calls… #4101
This commit is contained in:
Andreas Dangel 2022-09-30 11:22:57 +02:00
commit 7d58fc8551
No known key found for this signature in database
GPG Key ID: 93450DF2DF9A3FA3
9 changed files with 460 additions and 111 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
docs/pages/release_notes.md
View File

@ -48,6 +48,11 @@ from Lua. This means, that the Lua language in PMD can now parse both Lua and Lu
* java-design
* [#4090](https://github.com/pmd/pmd/issues/4090): \[java] FinalFieldCouldBeStatic false positive with non-static synchronized block (regression in 6.48, worked with 6.47)
* java-errorprone
* [#1718](https://github.com/pmd/pmd/issues/1718): \[java] ConstructorCallsOverridableMethod false positive when calling super method
* [#2348](https://github.com/pmd/pmd/issues/2348): \[java] ConstructorCallsOverridableMethod occurs when unused overloaded method is defined
* [#4099](https://github.com/pmd/pmd/issues/4099): \[java] ConstructorCallsOverridableMethod should consider method calls with var access
### API Changes
#### CPD CLI

236
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/ConstructorCallsOverridableMethodRule.java
View File

File diff suppressed because it is too large Load Diff

4
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/typeinference/TypeInferenceResolver.java
View File

@ -234,7 +234,9 @@ public final class TypeInferenceResolver {
for (Variable var : variablesToResolve) {
List<JavaTypeDefinition> lowerBounds = getLowerBoundsOf(var, bounds);
// TODO: should call incorporation
instantiations.put(var, lub(lowerBounds));
if (!lowerBounds.isEmpty()) {
instantiations.put(var, lub(lowerBounds));
}
}
uninstantiatedVariables.removeAll(variablesToResolve);

2
pmd-java/src/main/resources/category/java/errorprone.xml
View File

@ -1170,7 +1170,7 @@ boolean x = (y == Double.NaN);
<rule name="ConstructorCallsOverridableMethod"
language="java"
since="1.04"
message="Overridable {0} called during object construction"
message="Overridable {0} called during object construction{1}"
class="net.sourceforge.pmd.lang.java.rule.errorprone.ConstructorCallsOverridableMethodRule"
externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_errorprone.html#constructorcallsoverridablemethod">
<description>

19
pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/constructorcallsoverridablemethod/AbstractThing.java Normal file
View File

@ -0,0 +1,19 @@
/*
* BSD-style license; for more info see http://pmd.sourceforge.net/license.html
*/
package net.sourceforge.pmd.lang.java.rule.errorprone.constructorcallsoverridablemethod;
public abstract class AbstractThing implements Thing {
protected AbstractThing(Thing original) {
setName(original.getName());
}
@Override
public void setName(String name) { }
@Override
public String getName() {
return "";
}
}

11
pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/constructorcallsoverridablemethod/Thing.java Normal file
View File

@ -0,0 +1,11 @@
/*
* BSD-style license; for more info see http://pmd.sourceforge.net/license.html
*/
package net.sourceforge.pmd.lang.java.rule.errorprone.constructorcallsoverridablemethod;
public interface Thing {
String getName();
void setName(String name);
}

15
pmd-java/src/test/java/net/sourceforge/pmd/typeresolution/ClassTypeResolverTest.java
View File

@ -19,6 +19,7 @@ import static org.junit.Assert.assertSame;
import java.lang.reflect.Method;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Comparator;
import java.util.HashSet;
@ -47,6 +48,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTFormalParameter;
import net.sourceforge.pmd.lang.java.ast.ASTImportDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTLiteral;
import net.sourceforge.pmd.lang.java.ast.ASTLocalVariableDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTName;
import net.sourceforge.pmd.lang.java.ast.ASTNullLiteral;
import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression;
@ -1873,9 +1875,20 @@ public class ClassTypeResolverTest {
@Test
public void testMethodCallExpressionTypes() throws Exception {
ASTCompilationUnit cu = java11.parseClass(MethodCallExpressionTypes.class);
ASTPrimaryExpression expr = cu.getFirstDescendantOfType(ASTPrimaryExpression.class);
List<ASTMethodDeclaration> methods = cu.findDescendantsOfType(ASTMethodDeclaration.class);
// objectsToString
ASTPrimaryExpression expr = methods.get(0).getFirstDescendantOfType(ASTPrimaryExpression.class);
assertEquals(forClass(String.class), expr.getTypeDefinition());
assertEquals(forClass(Objects.class), expr.getFirstChildOfType(ASTPrimaryPrefix.class).getTypeDefinition());
// arraysAsList
expr = methods.get(1).getFirstDescendantOfType(ASTPrimaryExpression.class);
// note: the type parameter is not correct - it should be bound to String and not object
// but that's close enough - import is, that we correctly identified the method call
// and the result of the method call is a List.
assertEquals(forClass(List.class, forClass(Object.class)), expr.getTypeDefinition());
assertEquals(forClass(Arrays.class), expr.getFirstChildOfType(ASTPrimaryPrefix.class).getTypeDefinition());
}
private JavaTypeDefinition getChildTypeDef(Node node, int childIndex) {

8
pmd-java/src/test/java/net/sourceforge/pmd/typeresolution/testdata/MethodCallExpressionTypes.java vendored
View File

@ -4,10 +4,16 @@
package net.sourceforge.pmd.typeresolution.testdata;
import java.util.Arrays;
import java.util.Collection;
import java.util.Objects;
public class MethodCallExpressionTypes {
public void bar() {
public void objectsToString() {
Objects.toString(null);
}
public void arraysAsList() {
Collection<String> l = Arrays.asList("a");
}
}

271
pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/ConstructorCallsOverridableMethod.xml
View File

@ -320,4 +320,275 @@ import java.lang.annotation.*;
}
]]></code>
</test-code>
<test-code>
<description>[java] ConstructorCallsOverridableMethod should consider method calls with var access #4099</description>
<expected-problems>9</expected-problems>
<expected-linenumbers>7,14,21,28,35,42,49,56,63</expected-linenumbers>
<code><![CDATA[
import java.util.Collection;
import java.util.Set;
class Foo1 {
final boolean tag = true;
public Foo1() {
bar(tag); // should report a warning at this line
}
public void bar(boolean b) {}
}
class Foo2 {
public Foo2() {
bar(true); // should report a warning at this line
}
public void bar(boolean b) {}
}
class Foo3 {
public Foo3(String arg) {
bar(arg); // should report a warning at this line
}
public void bar(String s) {}
}
class Foo4 {
public Foo4(String[] arg) {
bar(arg); // should report a warning at this line
}
public void bar(String[] s) {}
}
class Foo5 {
public Foo5(int[] arg) {
bar(arg); // should report a warning at this line
}
public void bar(int[] i) {}
}
class Foo6 {
public Foo6(int[] arg) {
bar(arg); // should report a warning at this line
}
public void bar(int i[]) {} // note, the different array notation!
}
class Foo7 {
public Foo7(String[] arg) {
bar(arg); // should report a warning at this line
}
public void bar(String s[]) {} // note, the different array notation!
}
class Foo8 {
public Foo8(String[] arg) {
bar(arg); // should report a warning at this line
}
public void bar(String... s) {} // vararg
}
class Foo9<E> {
public Foo9(Set<E> arg) {
bar(arg); // should report a warning at this line
}
public void bar(Collection<E> s) {} // base type
}
]]></code>
</test-code>
<test-code>
<description>False positive with public method call on new instance</description>
<expected-problems>2</expected-problems>
<expected-linenumbers>4,5</expected-linenumbers>
<code><![CDATA[
class Foo {
public Foo() {
privateInitOk(); // false positive
privateInitBad1(); // warning expected here
privateInitBad2(); // warning expected here
}
private void privateInitOk() {
new Foo().publicMethod();
}
private void privateInitBad1() {
publicMethod();
}
private void privateInitBad2() {
this.publicMethod();
}
public void publicMethod() { }
}
]]></code>
</test-code>
<test-code>
<description>NPE when trying to find method name of method call</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
class Foo {
void bar(Object o) {
((String) o).length();
}
}
]]></code>
</test-code>
<test-code>
<description>False negative with method call as argument</description>
<expected-problems>1</expected-problems>
<expected-linenumbers>5</expected-linenumbers>
<code><![CDATA[
package net.sourceforge.pmd.lang.java.rule.errorprone.constructorcallsoverridablemethod;
public abstract class AbstractThing implements Thing {
protected AbstractThing(Thing original) {
setName(original.getName());
}
@Override
public void setName(String name) { }
@Override
public String getName() { return ""; }
}
]]></code>
</test-code>
<test-code>
<description>Clone and finalize overridden #1718</description>
<expected-problems>6</expected-problems>
<expected-linenumbers>3,4,5,7,8,9</expected-linenumbers>
<code><![CDATA[
class Foo {
public Foo() throws Throwable {
this.equals(new String());
this.clone();
this.finalize();
equals(new String());
clone();
finalize();
// #1718 - super calls should be ignored
super.equals(new String());
super.clone();
super.finalize();
}
public boolean equals(Object o) { return true; }
public Object clone() throws CloneNotSupportedException {
return super.clone();
}
public void finalize() throws Throwable {
super.finalize();
}
}
]]></code>
</test-code>
<test-code>
<description>False negative with var args and Arrays.asList</description>
<expected-problems>1</expected-problems>
<expected-linenumbers>6</expected-linenumbers>
<code><![CDATA[
import java.util.Arrays;
import java.util.Collection;
class Foo {
public Foo(String ... names) {
setNames(Arrays.asList(names));
}
public void setNames(Collection<String> names) { }
}
]]></code>
</test-code>
<test-code>
<description>Misleading message for method call chain</description>
<expected-problems>3</expected-problems>
<expected-linenumbers>3,4,5</expected-linenumbers>
<expected-messages>
<message>Overridable method 'overridableMethod' called during object construction (call stack: [intermediatePrivateMethod, otherMethod1, otherMethod2, overridableMethod])</message>
<message>Overridable method 'overridableMethod' called during object construction (call stack: [shorterChain, overridableMethod])</message>
<message>Overridable method 'otherOverridableMethod' called during object construction (call stack: [differentChain, otherOverridableMethod])</message>
</expected-messages>
<code><![CDATA[
public class Foo {
public Foo() {
intermediatePrivateMethod();
shorterChain();
differentChain();
}
private void intermediatePrivateMethod() {
otherMethod1();
}
private final void otherMethod1() {
otherMethod2();
}
private final void otherMethod2() {
overridableMethod();
}
private void shorterChain() {
overridableMethod();
}
void overridableMethod();
private void differentChain() {
otherOverridableMethod();
}
void otherOverridableMethod();
}
]]></code>
</test-code>
<test-code>
<description>[java] ConstructorCallsOverridableMethod occurs when unused overloaded method is defined #2348</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class Scratch {
private Long member = 0L;
public Scratch(final Long v) {
setMember(v);
}
public final void setMember(final Long v) {
this.member = v;
}
// Renaming this or marking it as final prevents the
// 'ConstructorCallsOverridableMethod' rule from being broken.
public void setMember(final String n) {
setMember(Long.valueOf(n));
}
}
]]></code>
</test-code>
<test-code>
<description>[java] ConstructorCallsOverridableMethod occurs when unused overloaded method is defined #2348 - sample 2</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
import java.util.Date;
public class MyTimestamp {
private final long t;
public MyTimestamp() {
t = new Date().getTime(); // <-- false-positive violation here
}
public long getTime() {
return t;
}
}
]]></code>
</test-code>
</test-data>