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

Better rationalize what to do with escaping objects

Browse Source
This commit is contained in:
Clément Fournier committed 2022-02-18 18:45:17 +01:00
1 parent 47942277fb
commit c0041a715e
2 files changed
+99 -46
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/LawOfDemeterRule.java
+55 -31
View File
@@ -35,6 +35,8 @@ import net.sourceforge.pmd.lang.java.ast.ASTReturnStatement;
import net.sourceforge.pmd.lang.java.ast.ASTThrowStatement;
import net.sourceforge.pmd.lang.java.ast.ASTTypeExpression;
import net.sourceforge.pmd.lang.java.ast.ASTVariableAccess;
import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclarator;
import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId;
import net.sourceforge.pmd.lang.java.ast.QualifiableExpression;
import net.sourceforge.pmd.lang.java.ast.internal.PrettyPrintingUtil;
import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule;
@@ -45,6 +47,7 @@ import net.sourceforge.pmd.lang.java.rule.internal.DataflowPass.ReachingDefiniti
import net.sourceforge.pmd.lang.java.symbols.JClassSymbol;
import net.sourceforge.pmd.lang.java.symbols.JFieldSymbol;
import net.sourceforge.pmd.lang.java.symbols.JTypeDeclSymbol;
import net.sourceforge.pmd.lang.java.symbols.JVariableSymbol;
import net.sourceforge.pmd.lang.java.types.JClassType;
import net.sourceforge.pmd.lang.java.types.JTypeMirror;
import net.sourceforge.pmd.lang.java.types.TypeOps;
@@ -111,40 +114,64 @@ public class LawOfDemeterRule extends AbstractJavaRulechainRule {
@Override
public Object visit(ASTFieldAccess node, Object data) {
int degree = foreignDegree(node);
if (isReportedDegree(degree)) {
if (shouldReport(node)) {
addViolationWithMessage(
data, node,
FIELD_ACCESS_ON_FOREIGN_VALUE,
new Object[] {
node.getName(),
PrettyPrintingUtil.prettyPrint(node.getQualifier()),
degree,
}
);
foreignDegree(node.getQualifier()),
});
}
return null;
}
@Override
public Object visit(ASTMethodCall node, Object data) {
ASTExpression qualifier = node.getQualifier();
if (qualifier != null) {
int degree = foreignDegree(node);
if (isReportedDegree(degree)) {
addViolationWithMessage(
data, node,
METHOD_CALL_ON_FOREIGN_VALUE,
new Object[] {
node.getMethodName(),
PrettyPrintingUtil.prettyPrint(node.getQualifier()),
degree,
});
}
if (shouldReport(node)) {
addViolationWithMessage(
data, node,
METHOD_CALL_ON_FOREIGN_VALUE,
new Object[] {
node.getMethodName(),
PrettyPrintingUtil.prettyPrint(node.getQualifier()),
foreignDegree(node.getQualifier()),
});
}
return null;
}
private boolean shouldReport(QualifiableExpression expr) {
ASTExpression qualifier = expr.getQualifier();
if (qualifier == null) {
return false;
}
int degree = foreignDegree(expr);
if (isReportedDegree(degree) && isUsedAsGetter(expr)) {
if (expr.getParent() instanceof ASTVariableDeclarator) { // NOPMD #3786
// Stored in local var, don't report if some usages escape.
// In that case, usage sites with non-escaping usage will be reported.
return isAllowedStore(((ASTVariableDeclarator) expr.getParent()).getVarId());
} else {
return true;
}
}
// Reported degree may be higher if LHS is a local var with the reported degree.
// If some usages of that local escape, the local hasn't been reported. Those usages
// that don't escape need to be reported.
if (qualifier instanceof ASTVariableAccess
&& isReportedDegree(foreignDegree(qualifier))) {
JVariableSymbol sym = ((ASTVariableAccess) qualifier).getReferencedSym();
return sym != null && !isAllowedStore(sym.tryGetNode());
}
return false;
}
private boolean isAllowedStore(ASTVariableDeclaratorId varId) {
return varId != null && varId.getLocalUsages().stream().noneMatch(this::escapesMethod);
}
private int foreignDegree(@Nullable ASTExpression expr) {
if (expr == null) {
return 0;
@@ -192,11 +219,11 @@ public class LawOfDemeterRule extends AbstractJavaRulechainRule {
|| call.getQualifier() == null // either static or call on this. Prevents NPE when unresolved
|| isFactoryMethod(call)
|| isBuilderPattern(call.getQualifier())
|| !isDangerousGetter(call)
|| isPureData(call)) {
return ACCESSIBLE;
} else if (isPureDataContainer(call.getMethodType().getDeclaringType())
|| isPureDataContainer(call.getTypeMirror())
|| !isGetterCall(call)
|| isTransformationMethod(call)) {
return asForeignAsQualifier(call);
}
@@ -237,16 +264,15 @@ public class LawOfDemeterRule extends AbstractJavaRulechainRule {
return false;
}
// a dangerous getter is one that may be used later to call another getter
private boolean isDangerousGetter(ASTMethodCall expr) {
return isGetterCall(expr) && isUsedInThisMethod(expr);
private boolean escapesMethod(ASTExpression expr) {
return expr.getParent() instanceof ASTArgumentList
|| expr.getParent() instanceof ASTReturnStatement
|| expr.getParent() instanceof ASTThrowStatement;
}
private boolean isUsedInThisMethod(ASTExpression expr) {
return !(expr.getParent() instanceof ASTExpressionStatement)
&& !(expr.getParent() instanceof ASTArgumentList)
&& !(expr.getParent() instanceof ASTReturnStatement)
&& !(expr.getParent() instanceof ASTThrowStatement);
private boolean isUsedAsGetter(ASTExpression expr) {
return !escapesMethod(expr) && !(expr.getParent() instanceof ASTExpressionStatement);
}
@@ -266,9 +292,7 @@ public class LawOfDemeterRule extends AbstractJavaRulechainRule {
}
private int fieldAccessDegree(ASTNamedReferenceExpr expr) {
if (isRefToFieldOfThisClass(expr)
|| isPureData(expr)
|| !isUsedInThisMethod(expr)) {
if (isRefToFieldOfThisClass(expr) || isPureData(expr)) {
return ACCESSIBLE;
} else if (isArrayLengthFieldAccess(expr)) {
return asForeignAsQualifier((ASTFieldAccess) expr);
@@ -344,7 +368,7 @@ public class LawOfDemeterRule extends AbstractJavaRulechainRule {
* ctors, etc).
* </ul>
* You can use any method, but you can't use yourself the result of
* a getter, or a field (though you can pass it as an argument to a method).
* a getter, or field (though you can let it escape).
*/
private static final int ACCESSIBLE = 1;
pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/LawOfDemeter.xml
+44 -15
View File
@@ -56,7 +56,7 @@ class Bar { C getC() {} }
<description>Simple Method calls with chaining</description>
<expected-problems>1</expected-problems>
<expected-messages>
<message>Call to `getC` on foreign value `b` (degree 2)</message>
<message>Call to `getC` on foreign value `b` (degree 1)</message>
</expected-messages>
<code><![CDATA[
public class Foo {
@@ -204,10 +204,10 @@ public class Foo {
<expected-problems>4</expected-problems>
<expected-linenumbers>18,19,24,25</expected-linenumbers>
<expected-messages>
<message>Access to field `a` on foreign value `b` (degree 2)</message>
<message>Access to field `a` on foreign value `this.b` (degree 2)</message>
<message>Access to field `a` on foreign value `b` (degree 2)</message>
<message>Access to field `a` on foreign value `this.b` (degree 2)</message>
<message>Access to field `a` on foreign value `b` (degree 1)</message>
<message>Access to field `a` on foreign value `this.b` (degree 1)</message>
<message>Access to field `a` on foreign value `b` (degree 1)</message>
<message>Access to field `a` on foreign value `this.b` (degree 1)</message>
</expected-messages>
<code><![CDATA[
public class B {
@@ -414,7 +414,7 @@ class BarBuilder {
<expected-problems>1</expected-problems>
<expected-linenumbers>5</expected-linenumbers>
<expected-messages>
<message>Call to `getB` on foreign value `this.getA()` (degree 2)</message>
<message>Call to `getB` on foreign value `this.getA()` (degree 1)</message>
</expected-messages>
<code><![CDATA[
public class Test {
@@ -438,7 +438,7 @@ interface B { A getA(); }
<expected-problems>1</expected-problems>
<expected-linenumbers>4</expected-linenumbers>
<expected-messages>
<message>Call to `getA` on foreign value `this.getA().getB()` (degree 3)</message>
<message>Call to `getA` on foreign value `this.getA().getB()` (degree 2)</message>
</expected-messages>
<code><![CDATA[
public class Test {
@@ -680,8 +680,8 @@ public final class ControlEvent {
<expected-problems>2</expected-problems>
<expected-linenumbers>10,12</expected-linenumbers>
<expected-messages>
<message>Access to field `attributes` on foreign value `h` (degree 2)</message>
<message>Access to field `attributes` on foreign value `h` (degree 2)</message>
<message>Access to field `attributes` on foreign value `h` (degree 1)</message>
<message>Access to field `attributes` on foreign value `h` (degree 1)</message>
</expected-messages>
<code><![CDATA[
import java.nio.ByteBuffer;
@@ -760,7 +760,7 @@ public final class ControlEvent {
<description>Field access on call</description>
<expected-problems>1</expected-problems>
<expected-messages>
<message>Access to field `manager` on foreign value `t.getLastToken()` (degree 2)</message>
<message>Access to field `manager` on foreign value `t.getLastToken()` (degree 1)</message>
</expected-messages>
<code><![CDATA[
import java.util.*;
@@ -777,7 +777,8 @@ public final class ControlEvent {
}
class Q {
void foo(Token t) {
Manager checkMessage = t.getLastToken().manager;
Manager m = t.getLastToken().manager;
m.doSomethn();
}
}
]]></code>
@@ -963,7 +964,7 @@ public final class ControlEvent {
<description>Foreign stream</description>
<expected-problems>1</expected-problems>
<expected-messages>
<message>Call to `getSomething` on foreign value `a` (degree 2)</message>
<message>Call to `getSomething` on foreign value `a` (degree 1)</message>
</expected-messages>
<code><![CDATA[
import java.util.*;
@@ -1067,11 +1068,11 @@ public final class ControlEvent {
]]></code>
</test-code>
<test-code>
<description>Return and throw mean not used here</description>
<description>Return and throw escape</description>
<expected-problems>1</expected-problems>
<expected-linenumbers>3</expected-linenumbers>
<expected-messages>
<message>Call to `getLanguage` on foreign value `rule` (degree 2)</message>
<message>Call to `getLanguage` on foreign value `rule` (degree 1)</message>
</expected-messages>
<code><![CDATA[
class Foo {
@@ -1079,7 +1080,7 @@ public final class ControlEvent {
rule.getLanguage().doSomething(); // warn
println(rule.getLanguage()); // no warning
if (foo)
if (foo())
return rule.getLanguage(); // no warning
else
throw rule.getLanguage(); // no warning
@@ -1094,4 +1095,32 @@ public final class ControlEvent {
}
]]></code>
</test-code>
<test-code>
<description>Local var is fine until used, if some of them escape</description>
<expected-problems>1</expected-problems>
<expected-linenumbers>4</expected-linenumbers>
<expected-messages>
<message>Call to `doSomething` on foreign value `l` (degree 2)</message>
</expected-messages>
<code><![CDATA[
class Foo {
public String toString(Rule rule) {
Language l = rule.getLanguage(); // no warning
l.doSomething(); // warn: degree 3
println(l); // no warning
if (foo())
return l; // no warning
else
throw l; // no warning
}
}
interface Language {
String getName();
void doSomething();
}
interface Rule {
Language getLanguage();
}
]]></code>
</test-code>
</test-data>