better definition and messges

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/internal/PrettyPrintingUtil.java

@@ -11,17 +11,21 @@ import org.checkerframework.checker.nullness.qual.NonNull;
 import net.sourceforge.pmd.lang.java.ast.ASTAmbiguousName;
 import net.sourceforge.pmd.lang.java.ast.ASTAnnotationTypeDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTAnyTypeDeclaration;
+import net.sourceforge.pmd.lang.java.ast.ASTArrayAccess;
 import net.sourceforge.pmd.lang.java.ast.ASTArrayType;
+import net.sourceforge.pmd.lang.java.ast.ASTClassLiteral;
 import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceType;
 import net.sourceforge.pmd.lang.java.ast.ASTConstructorDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTEnumDeclaration;
+import net.sourceforge.pmd.lang.java.ast.ASTFieldAccess;
 import net.sourceforge.pmd.lang.java.ast.ASTFieldDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTFormalParameter;
 import net.sourceforge.pmd.lang.java.ast.ASTFormalParameters;
 import net.sourceforge.pmd.lang.java.ast.ASTImportDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTIntersectionType;
 import net.sourceforge.pmd.lang.java.ast.ASTList;
+import net.sourceforge.pmd.lang.java.ast.ASTLiteral;
 import net.sourceforge.pmd.lang.java.ast.ASTMethodCall;
 import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTMethodOrConstructorDeclaration;
@@ -29,13 +33,18 @@ import net.sourceforge.pmd.lang.java.ast.ASTPrimitiveType;
 import net.sourceforge.pmd.lang.java.ast.ASTRecordDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTReferenceType;
 import net.sourceforge.pmd.lang.java.ast.ASTResource;
+import net.sourceforge.pmd.lang.java.ast.ASTSuperExpression;
+import net.sourceforge.pmd.lang.java.ast.ASTThisExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTType;
 import net.sourceforge.pmd.lang.java.ast.ASTTypeArguments;
+import net.sourceforge.pmd.lang.java.ast.ASTTypeExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTUnionType;
+import net.sourceforge.pmd.lang.java.ast.ASTVariableAccess;
 import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId;
 import net.sourceforge.pmd.lang.java.ast.ASTVoidType;
 import net.sourceforge.pmd.lang.java.ast.ASTWildcardType;
 import net.sourceforge.pmd.lang.java.ast.JavaNode;
+import net.sourceforge.pmd.lang.java.ast.JavaVisitorBase;
 import net.sourceforge.pmd.lang.java.symbols.JMethodSymbol;
 import net.sourceforge.pmd.lang.java.types.JMethodSig;
 import net.sourceforge.pmd.lang.java.types.TypePrettyPrint;
@@ -222,4 +231,91 @@ public final class PrettyPrintingUtil {
     private static TypePrettyPrinter overloadPrinter() {
         return new TypePrettyPrinter().useSimpleNames(true).printMethodResult(false);
     }
+
+
+    public static CharSequence prettyPrint(JavaNode node) {
+        StringBuilder sb = new StringBuilder();
+        node.acceptVisitor(new ExprPrinter(), sb);
+        return sb;
+    }
+
+    static class ExprPrinter extends JavaVisitorBase<StringBuilder, Void> {
+
+        @Override
+        public Void visit(ASTTypeExpression node, StringBuilder data) {
+            data.append(prettyPrintType(node.getTypeNode()));
+            return null;
+        }
+        @Override
+        public Void visit(ASTClassLiteral node, StringBuilder data) {
+            data.append(prettyPrintType(node.getTypeNode()));
+            data.append(".class");
+            return null;
+        }
+
+        @Override
+        public Void visitLiteral(ASTLiteral node, StringBuilder data) {
+            data.append(node.getText());
+            return null;
+        }
+
+        @Override
+        public Void visit(ASTFieldAccess node, StringBuilder data) {
+            node.getQualifier().acceptVisitor(this, data);
+            data.append('.').append(node.getName());
+            return null;
+        }
+
+        @Override
+        public Void visit(ASTVariableAccess node, StringBuilder data) {
+            data.append(node.getName());
+            return null;
+        }
+
+        @Override
+        public Void visit(ASTThisExpression node, StringBuilder data) {
+            if (node.getQualifier() != null) {
+                node.getQualifier().acceptVisitor(this, data);
+                data.append('.');
+            }
+            data.append("this");
+            return null;
+        }
+
+        @Override
+        public Void visit(ASTSuperExpression node, StringBuilder data) {
+            if (node.getQualifier() != null) {
+                node.getQualifier().acceptVisitor(this, data);
+                data.append('.');
+            }
+            data.append("super");
+            return null;
+        }
+
+        @Override
+        public Void visit(ASTArrayAccess node, StringBuilder data) {
+            node.getQualifier().acceptVisitor(this, data);
+            data.append('[');
+            node.getIndexExpression().acceptVisitor(this, data);
+            data.append(']');
+            return null;
+        }
+
+        @Override
+        public Void visit(ASTMethodCall node, StringBuilder data) {
+            if (node.getQualifier() != null) {
+                node.getQualifier().acceptVisitor(this, data);
+                data.append('.');
+            }
+            data.append(node.getMethodName());
+            if (node.getArguments().isEmpty()) {
+                data.append("()");
+            } else {
+                data.append("(...)");
+            }
+            return null;
+        }
+    }
+
+
 }

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/LawOfDemeterRule.java

@@ -4,6 +4,11 @@
 
 package net.sourceforge.pmd.lang.java.rule.design;
 
+import static net.sourceforge.pmd.lang.java.rule.internal.JavaRuleUtil.isArrayLengthFieldAccess;
+import static net.sourceforge.pmd.lang.java.rule.internal.JavaRuleUtil.isCallOnThisInstance;
+import static net.sourceforge.pmd.lang.java.rule.internal.JavaRuleUtil.isGetterCall;
+import static net.sourceforge.pmd.lang.java.rule.internal.JavaRuleUtil.isRefToFieldOfThisInstance;
+import static net.sourceforge.pmd.lang.java.rule.internal.JavaRuleUtil.isUnqualifiedThisOrSuper;
 import static net.sourceforge.pmd.util.CollectionUtil.listOf;
 
 import java.util.Collection;
@@ -18,9 +23,12 @@ import net.sourceforge.pmd.RuleContext;
 import net.sourceforge.pmd.lang.java.ast.ASTArrayAccess;
 import net.sourceforge.pmd.lang.java.ast.ASTAssignableExpr.ASTNamedReferenceExpr;
 import net.sourceforge.pmd.lang.java.ast.ASTExpression;
+import net.sourceforge.pmd.lang.java.ast.ASTExpressionStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTFieldAccess;
 import net.sourceforge.pmd.lang.java.ast.ASTForeachStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTMethodCall;
+import net.sourceforge.pmd.lang.java.ast.ASTVariableAccess;
+import net.sourceforge.pmd.lang.java.ast.internal.PrettyPrintingUtil;
 import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule;
 import net.sourceforge.pmd.lang.java.rule.internal.DataflowPass;
 import net.sourceforge.pmd.lang.java.rule.internal.DataflowPass.AssignmentEntry;
@@ -30,6 +38,7 @@ import net.sourceforge.pmd.lang.java.rule.internal.JavaRuleUtil;
 import net.sourceforge.pmd.lang.java.symbols.JFieldSymbol;
 import net.sourceforge.pmd.lang.java.types.InvocationMatcher;
 import net.sourceforge.pmd.lang.java.types.JClassType;
+import net.sourceforge.pmd.lang.java.types.JTypeMirror;
 import net.sourceforge.pmd.lang.java.types.TypeTestUtil;
 import net.sourceforge.pmd.properties.PropertyDescriptor;
 import net.sourceforge.pmd.properties.PropertyFactory;
@@ -66,47 +75,59 @@ public class LawOfDemeterRule extends AbstractJavaRulechainRule {
         definePropertyDescriptor(ALLOWED_STATIC_CONTAINERS);
     }
 
+    /**
+     * This cache is there to prevent recursion in case of cycles. It
+     * also avoids recomputing the degree of too many nodes, as the degree
+     * of a call chain depends on the degree of the qualifier. {@link #visit(ASTMethodCall, Object)}
+     * is called on every part of the chain, so without memoization we
+     * would run in O(n2).
+     */
     private final Map<ASTExpression, Integer> degreeCache = new LinkedHashMap<>();
 
     @Override
     public void end(RuleContext ctx) {
-        degreeCache.clear();
+        degreeCache.clear(); // avoid memory leak
     }
 
     @Override
     public Object visit(ASTFieldAccess node, Object data) {
-        if (isTooHighDegree(foreignDegree(node.getQualifier()))) {
-            addViolationWithMessage(data, node, "Field access on foreign value");
+        int degree = foreignDegree(node);
+        if (isTooHighDegree(degree)) {
+            addViolationWithMessage(
+                data, node,
+                "Access to field `{0}` on foreign value `{1}` (degree {2})",
+                new Object[] {
+                    node.getName(),
+                    PrettyPrintingUtil.prettyPrint(node.getQualifier()),
+                    degree
+                }
+            );
         }
         return null;
     }
 
     private boolean isTooHighDegree(int degree) {
-        return degree > 1;
+        return degree > 1; // todo make that configurable
     }
 
     @Override
     public Object visit(ASTMethodCall node, Object data) {
-        String reason = getViolationReason(node);
-        if (reason != null) {
-            addViolation(data, node, reason);
-        }
-
-        return null;
-    }
-
-    private @Nullable String getViolationReason(ASTMethodCall call) {
-        ASTExpression qualifier = call.getQualifier();
-        if (qualifier == null || isBuilderPattern(qualifier)) {
-            return null;
-        }
-        if (isTooHighDegree(foreignDegree(call))) {
-            if (isTooHighDegree(foreignDegree(call.getQualifier()))) {
+        ASTExpression qualifier = node.getQualifier();
+        if (qualifier != null) {
+            int degree = foreignDegree(node);
+            if (isTooHighDegree(degree)) {
                 // qualifier will be reported
-                return null;
+                if (!isTooHighDegree(foreignDegree(node.getQualifier()))) {
+                    addViolationWithMessage(
+                        data, node,
+                        "Call to `{0}` on foreign value `{1}` (degree {2})",
+                        new Object[] {
+                            node.getMethodName(),
+                            PrettyPrintingUtil.prettyPrint(node.getQualifier()),
+                            degree
+                        });
+                }
             }
-
-            return "call on foreign value";
         }
         return null;
     }
@@ -141,7 +162,7 @@ public class LawOfDemeterRule extends AbstractJavaRulechainRule {
     }
 
     private boolean isLocalFieldAccess(ASTFieldAccess access) {
-        return JavaRuleUtil.isUnqualifiedThisOrSuper(access) // field of this instance
+        return isUnqualifiedThisOrSuper(access) // field of this instance
             || isAllowedStaticFieldAccess(access);
     }
 
@@ -166,7 +187,7 @@ public class LawOfDemeterRule extends AbstractJavaRulechainRule {
         }
         // formal parameters are not foreign otherwise we couldn't call any methods on them
         if (def.getVarId().isFormalParameter()) {
-            return 0;
+            return 1;
         }
         return foreignDegree(def.getRhsAsExpression());
     }
@@ -191,39 +212,46 @@ public class LawOfDemeterRule extends AbstractJavaRulechainRule {
     private int foreignDegreeImpl(ASTExpression expr) {
         if (expr instanceof ASTMethodCall) {
             return methodForeignDegreeImpl((ASTMethodCall) expr);
-        } else if (expr instanceof ASTNamedReferenceExpr) {
-            if (expr instanceof ASTFieldAccess) {
-                return fieldForeignDegreeImpl(expr);
-            }
-            // a variable access
-
-            DataflowResult dataflow = DataflowPass.getDataflowResult(expr.getRoot());
-            ReachingDefinitionSet reaching = dataflow.getReachingDefinitions((ASTNamedReferenceExpr) expr);
-            if (reaching.isNotFullyKnown()) {
-                return 0;
-            }
-
-            // note this max could be changed to min to get a more conservative
-            // strategy, trading recall for precision.
-            return reaching.getReaching().stream().mapToInt(this::foreignDegree).max().orElse(0);
+        } else if (expr instanceof ASTFieldAccess) {
+            return fieldForeignDegreeImpl((ASTFieldAccess) expr);
+        } else if (expr instanceof ASTVariableAccess) {
+            return variableAccessDegree((ASTVariableAccess) expr);
         } else if (expr instanceof ASTArrayAccess) {
             return foreignDegree(((ASTArrayAccess) expr).getQualifier());
         }
         return 0;
     }
 
-    private int fieldForeignDegreeImpl(ASTExpression expr) {
-        if (isLocalFieldAccess((ASTFieldAccess) expr)) {
-            return 0;
-        } else if (JavaRuleUtil.isArrayLengthFieldAccess(expr)) {
-            return foreignDegree(((ASTFieldAccess) expr).getQualifier());
+    private int variableAccessDegree(ASTVariableAccess expr) {
+        if (JavaRuleUtil.isRefToFieldOfThisInstance(expr)) {
+            return 1;
         }
-        return 1 + foreignDegree(((ASTFieldAccess) expr).getQualifier());
+
+        DataflowResult dataflow = DataflowPass.getDataflowResult(expr.getRoot());
+        ReachingDefinitionSet reaching = dataflow.getReachingDefinitions(expr);
+        if (reaching.isNotFullyKnown()) {
+            return 0; // should never happen
+        }
+
+        // note this max could be changed to min to get a more conservative
+        // strategy, trading recall for precision. maybe make that configurable
+        return reaching.getReaching().stream().mapToInt(this::foreignDegree).max().orElse(0);
+    }
+
+    private int fieldForeignDegreeImpl(ASTFieldAccess expr) {
+        if (isRefToFieldOfThisInstance(expr)) {
+            return 1;
+        } else if (isArrayLengthFieldAccess(expr)) {
+            // as foreign as the array
+            return foreignDegree(expr.getQualifier());
+        }
+        // more foreign
+        return 1 + foreignDegree(expr.getQualifier());
     }
 
     private int methodForeignDegreeImpl(ASTMethodCall expr) {
-        if (isLocalMethod(expr)) {
-            return 0;
+        if (producesTrustedData(expr)) {
+            return 1;
         } else if (increasesDegree(expr)) {
             return 1 + foreignDegree(expr.getQualifier());
         }
@@ -233,24 +261,46 @@ public class LawOfDemeterRule extends AbstractJavaRulechainRule {
     /**
      * Method that produces trusted data.
      */
-    private boolean isLocalMethod(ASTMethodCall expr) {
+    private boolean producesTrustedData(ASTMethodCall expr) {
+        if (expr.getOverloadSelectionInfo().isFailed()) {
+            return true; // be conservative
+        }
         // static methods are taken to be construction methods.
         return expr.getMethodType().isStatic()
-            || JavaRuleUtil.isCallOnThisInstance(expr)
-            || isFactoryMethod(expr);
+            || isCallOnThisInstance(expr)
+            || isFactoryMethod(expr)
+            || isBuilderPattern(expr.getQualifier())
+            || !isGetterLike(expr) // action methods are not dangerous
+            || isNeverForeignMethod(expr)
+            || isPureData(expr.getTypeMirror())
+            || isPureDataContainer(expr.getMethodType().getDeclaringType());
+    }
+
+    private boolean isPureData(JTypeMirror type) {
+        return TypeTestUtil.isA(String.class, type)
+            || TypeTestUtil.isA(StringBuilder.class, type)
+            || TypeTestUtil.isA(StringBuffer.class, type)
+            || type.isPrimitive()
+            || type.isBoxedPrimitive();
+    }
+
+    private boolean isPureDataContainer(JTypeMirror type) {
+        return TypeTestUtil.isA(Collection.class, type)
+            || type.isArray();
     }
 
     /**
-     * Method that produces untrusted data.
+     * Method that reaches across a boundary. This method assumes
+     * {@link #producesTrustedData(ASTMethodCall)} returned false.
      */
     private boolean increasesDegree(ASTMethodCall expr) {
-        return isGetterLike(expr)
-            && !isNeverForeignMethod(expr);
+        return isGetterLike(expr);
     }
 
     private boolean isGetterLike(ASTMethodCall expr) {
-        return JavaRuleUtil.isGetterCall(expr)
-            || expr.getArguments().isEmpty();
+        return (isGetterCall(expr)
+            || expr.getArguments().isEmpty())
+            && !(expr.getParent() instanceof ASTExpressionStatement);
     }
 
     /**
@@ -261,6 +311,7 @@ public class LawOfDemeterRule extends AbstractJavaRulechainRule {
             || TypeTestUtil.isA(Collection.class, expr.getQualifier())
             || TypeTestUtil.isA(StringBuilder.class, expr.getQualifier())
             || TypeTestUtil.isA(StringBuffer.class, expr.getQualifier())
+            || TypeTestUtil.isA(String.class, expr.getQualifier())
             || isBuilderPattern(expr)
             || isFactoryMethod(expr);
     }

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/internal/JavaRuleUtil.java

@@ -817,25 +817,22 @@ public final class JavaRuleUtil {
 
     /**
      * Returns true if the expression has the form `field`, or `this.field`,
-     * where `field` is a field declared in the enclosing class.
-     * Assumes we're not in a static context.
+     * where `field` is a field declared in the enclosing class. Considers
+     * inherited fields. Assumes we're not in a static context.
      */
     public static boolean isRefToFieldOfThisInstance(ASTExpression usage) {
         if (!(usage instanceof ASTNamedReferenceExpr)) {
             return false;
         }
         JVariableSymbol symbol = ((ASTNamedReferenceExpr) usage).getReferencedSym();
-        if (!(symbol instanceof JFieldSymbol)
-            || !((JFieldSymbol) symbol).getEnclosingClass().equals(usage.getEnclosingType().getSymbol())
-            || Modifier.isStatic(((JFieldSymbol) symbol).getModifiers())) {
+        if (!(symbol instanceof JFieldSymbol)) {
             return false;
         }
 
         if (usage instanceof ASTVariableAccess) {
-            return true;
+            return !Modifier.isStatic(((JFieldSymbol) symbol).getModifiers());
         } else if (usage instanceof ASTFieldAccess) {
-            ASTExpression qualifier = ((ASTFieldAccess) usage).getQualifier();
-            return getThisOrSuperQualifier(qualifier) == null;
+            return isUnqualifiedThisOrSuper(((ASTFieldAccess) usage).getQualifier());
         }
         return false;
     }

pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/LawOfDemeter.xml

@@ -184,8 +184,13 @@ public class Foo {
     </test-code>
     <test-code>
         <description>Instance methods on fields</description>
-        <expected-problems>2</expected-problems>
-        <expected-linenumbers>18,21</expected-linenumbers>
+        <expected-problems>3</expected-problems>
+        <expected-linenumbers>18,20,21</expected-linenumbers>
+        <expected-messages>
+            <message>Access to field `a` on foreign value `b` (degree 2)</message>
+            <message>Access to field `a` on foreign value `b` (degree 2)</message>
+            <message>Access to field `a` on foreign value `b` (degree 2)</message>
+        </expected-messages>
         <code><![CDATA[
 public class B {
     public A a = new A();
@@ -219,8 +224,7 @@ public class Foo {
 
     <test-code>
         <description>Exclude iterator and list elements</description>
-        <expected-problems>1</expected-problems>
-        <expected-linenumbers>24</expected-linenumbers>
+        <expected-problems>0</expected-problems>
         <code><![CDATA[
 import java.util.List;
 import java.util.ArrayList;
@@ -245,11 +249,14 @@ public class Foo {
 
         List<String> anotherList = calcList();
         for (String s : anotherList) {
-            if (!s.isEmpty()) { // here
+            if (!s.isEmpty()) {
                 System.out.println(s);
             }
         }
     }
+    List<String> calcList() {
+        return null;
+    }
 }
         ]]></code>
     </test-code>
@@ -370,6 +377,34 @@ public class Test {
     }
 }
 
+class Bar {
+    static BarBuilder newBuilder() { }
+}
+class BarBuilder {
+    BarBuilder withFoo(String s) {}
+    Bar build() {}
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Method chain is reset when someone produces trusted data</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Test {
+    A getA() {}
+    public void bar() {
+
+        // Inner Builder pattern chained
+        final Bar bar = Bar.newBuilder()
+                .withFoo("foo")
+                .build();
+    }
+}
+interface A {
+    B getB();
+    String foo();
+}
 class Bar {
     static BarBuilder newBuilder() { }
 }
@@ -479,18 +514,39 @@ class TaskManager {
         <expected-problems>0</expected-problems>
         <code><![CDATA[
 public final class Util {
-    public static boolean check(String passwd, String hashed) {
+    public static boolean check(String hashed) { // degree 1
         try {
-            final String[] parts = hashed.split("\\$");
+            final String[] parts = hashed.split("\\$"); // degree 1: string is pure data
 
-            if (parts.length != 5
-                    || parts.isEmpty()               // wrong violation - method chain calls
+            if (parts.length != 5                    // array length is ok
+                    || parts.clone()                 // wrong violation - method chain calls
                     || parts[1].isEmpty()            // wrong violation - method chain calls
                     || parts[1].equals("s0")) {      // wrong violation - method chain calls
                 throw new IllegalArgumentException("Invalid hashed value");
             }
         } catch (Exception e) { }
     }
+}
+        ]]></code>
+    </test-code>
+    <test-code>
+        <description>List access</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+import java.util.List;
+public final class Util {
+    public static boolean check(String hashed) { // degree 1
+        try {
+            final List<String> parts = hashed.split("\\$"); // degree 1: string is pure data
+
+            if (parts.size() != 5
+                    || parts.isEmpty()
+                    || parts.get(1).isEmpty()
+                    || parts.get(1).equals("s0")) {
+                throw new IllegalArgumentException("Invalid hashed value");
+            }
+        } catch (Exception e) { }
+    }
 }
         ]]></code>
     </test-code>
@@ -581,7 +637,11 @@ public final class ControlEvent {
     <test-code>
         <description>Stackoverflow with cyclic data flow</description>
         <expected-problems>2</expected-problems>
-        <expected-linenumbers>11,15</expected-linenumbers>
+        <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>
+        </expected-messages>
         <code><![CDATA[
             import java.nio.ByteBuffer;
             import java.util.Iterator;
@@ -592,16 +652,19 @@ public final class ControlEvent {
 
                 void writeAttributes(int ctype, Attribute.Holder h) throws IOException {
                     ByteBuffer buf, out, bufOut;
-                    for (Attribute a : h.attributes) {
-                        if (a.layout() == h.attributes) { // warn
+                    for (Attribute a : h.attributes) { // warn(h.attributes)
+                        if (a.layout()
+                                == h.attributes) { // warn(h.attributes)
                             DataOutputStream savedOut = out;
                             out = savedOut;
                         } else {
-                            out.write(a.bytes()); // warn
+                            out.write(a.bytes());
                         }
                     }
                 }
 
+                byte[] bytes() {}
+
                 class Holder {
 
                     Object attributes;