statementsAfter(ASTLocalVariableDeclaration node) {
+ return node.asStream().followingSiblings().filterIs(ASTStatement.class);
}
}
diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/internal/JavaRuleUtil.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/internal/JavaRuleUtil.java
index 260d94a6b2..ea346cbbb8 100644
--- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/internal/JavaRuleUtil.java
+++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/internal/JavaRuleUtil.java
@@ -28,6 +28,8 @@ import net.sourceforge.pmd.lang.ast.NodeStream;
import net.sourceforge.pmd.lang.ast.impl.javacc.JavaccToken;
import net.sourceforge.pmd.lang.java.ast.ASTAnyTypeDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTArgumentList;
+import net.sourceforge.pmd.lang.java.ast.ASTArrayAccess;
+import net.sourceforge.pmd.lang.java.ast.ASTAssignableExpr;
import net.sourceforge.pmd.lang.java.ast.ASTAssignableExpr.ASTNamedReferenceExpr;
import net.sourceforge.pmd.lang.java.ast.ASTAssignableExpr.AccessType;
import net.sourceforge.pmd.lang.java.ast.ASTAssignmentExpression;
@@ -55,6 +57,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTNumericLiteral;
import net.sourceforge.pmd.lang.java.ast.ASTStatement;
import net.sourceforge.pmd.lang.java.ast.ASTSuperExpression;
import net.sourceforge.pmd.lang.java.ast.ASTThisExpression;
+import net.sourceforge.pmd.lang.java.ast.ASTThrowStatement;
import net.sourceforge.pmd.lang.java.ast.ASTUnaryExpression;
import net.sourceforge.pmd.lang.java.ast.ASTVariableAccess;
import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId;
@@ -68,6 +71,8 @@ import net.sourceforge.pmd.lang.java.ast.TypeNode;
import net.sourceforge.pmd.lang.java.ast.UnaryOp;
import net.sourceforge.pmd.lang.java.symbols.JFieldSymbol;
import net.sourceforge.pmd.lang.java.symbols.JVariableSymbol;
+import net.sourceforge.pmd.lang.java.types.InvocationMatcher;
+import net.sourceforge.pmd.lang.java.types.InvocationMatcher.CompoundInvocationMatcher;
import net.sourceforge.pmd.lang.java.types.JPrimitiveType.PrimitiveTypeKind;
import net.sourceforge.pmd.lang.java.types.JTypeMirror;
import net.sourceforge.pmd.lang.java.types.TypeTestUtil;
@@ -78,6 +83,22 @@ import net.sourceforge.pmd.util.CollectionUtil;
*/
public final class JavaRuleUtil {
+ // this is a hacky way to do it, but let's see where this goes
+ private static final CompoundInvocationMatcher KNOWN_PURE_METHODS = InvocationMatcher.parseAll(
+ "_#toString()",
+ "_#hashCode()",
+ "_#equals(java.lang.Object)",
+ "java.lang.String#_(_*)",
+ // actually not all of them, probs only stream of some type
+ // arg which doesn't implement Closeable...
+ "java.util.stream.Stream#_(_*)",
+ "java.util.Collection#size()",
+ "java.util.List#get(int)",
+ "java.util.Map#get(_)",
+ "java.lang.Iterable#iterator()",
+ "java.lang.Comparable#compareTo(_)"
+ );
+
private JavaRuleUtil() {
// utility class
}
@@ -648,6 +669,17 @@ public final class JavaRuleUtil {
return e instanceof ASTThisExpression && ((ASTThisExpression) e).getQualifier() == null;
}
+ /**
+ * Returns true if the expression is a {@link ASTNamedReferenceExpr}
+ * that references any of the symbol in the set.
+ */
+ public static boolean isReferenceToVar(@Nullable ASTExpression expression, @NonNull Set symbols) {
+ if (expression instanceof ASTNamedReferenceExpr) {
+ return symbols.contains(((ASTNamedReferenceExpr) expression).getReferencedSym());
+ }
+ return false;
+ }
+
/**
* Returns true if both expressions refer to the same variable.
* A "variable" here can also means a field path, eg, {@code this.field.a}.
@@ -688,6 +720,17 @@ public final class JavaRuleUtil {
return false;
}
+ /**
+ * Returns true if the expression is a reference to a local variable.
+ */
+ public static boolean isReferenceToLocal(ASTExpression expr) {
+ if (expr instanceof ASTVariableAccess) {
+ JVariableSymbol sym = ((ASTVariableAccess) expr).getReferencedSym();
+ return sym != null && !sym.isField();
+ }
+ return false;
+ }
+
/**
* Returns true if the expression has the form `field`, or `this.field`,
* where `field` is a field declared in the enclosing class.
@@ -750,4 +793,59 @@ public final class JavaRuleUtil {
Node parent = it.getParent();
return parent == null || it.getIndexInParent() == parent.getNumChildren() - 1;
}
+
+
+ /**
+ * Whether the node or one of its descendants is an expression with
+ * side effects. Conservatively, any method call is a potential side-effect,
+ * as well as assignments to fields or array elements. We could relax
+ * this assumption with (much) more data-flow logic, including a memory model.
+ *
+ * By default assignments to locals are not counted as side-effects,
+ * unless the lhs is in the given set of symbols.
+ *
+ * @param node A node
+ * @param localVarsToTrack Local variables to track
+ */
+ public static boolean hasSideEffect(@Nullable JavaNode node, Set localVarsToTrack) {
+ return node != null && node.descendantsOrSelf()
+ .filterIs(ASTExpression.class)
+ .any(e -> hasSideEffectNonRecursive(e, localVarsToTrack));
+ }
+
+ /**
+ * Returns true if the expression has side effects we don't track.
+ * Does not recurse into sub-expressions.
+ */
+ private static boolean hasSideEffectNonRecursive(ASTExpression e, Set localVarsToTrack) {
+ if (e instanceof ASTAssignmentExpression) {
+ ASTAssignableExpr lhs = ((ASTAssignmentExpression) e).getLeftOperand();
+ return isNonLocalLhs(lhs) || isReferenceToVar(lhs, localVarsToTrack);
+ } else if (e instanceof ASTUnaryExpression) {
+ ASTUnaryExpression unary = (ASTUnaryExpression) e;
+ ASTExpression lhs = unary.getOperand();
+ return !unary.getOperator().isPure()
+ && (isNonLocalLhs(lhs) || isReferenceToVar(lhs, localVarsToTrack));
+ }
+
+ if (e.ancestors(ASTThrowStatement.class).nonEmpty()) {
+ // then this side effect can never be observed in containing code,
+ // because control flow jumps out of the method
+ return false;
+ }
+
+ return e instanceof ASTMethodCall && !isPure((ASTMethodCall) e)
+ || e instanceof ASTConstructorCall;
+ }
+
+ private static boolean isNonLocalLhs(ASTExpression lhs) {
+ return lhs instanceof ASTArrayAccess || !isReferenceToLocal(lhs);
+ }
+
+ /**
+ * Whether the invocation has no side-effects. Very conservative.
+ */
+ private static boolean isPure(ASTMethodCall call) {
+ return isGetterCall(call) || KNOWN_PURE_METHODS.anyMatch(call);
+ }
}
diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/xpath/internal/MatchesSignatureFunction.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/xpath/internal/MatchesSignatureFunction.java
index a2a27f0224..187d100612 100644
--- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/xpath/internal/MatchesSignatureFunction.java
+++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/xpath/internal/MatchesSignatureFunction.java
@@ -5,7 +5,7 @@
package net.sourceforge.pmd.lang.java.rule.xpath.internal;
import net.sourceforge.pmd.lang.java.ast.InvocationNode;
-import net.sourceforge.pmd.lang.java.types.TypeTestUtil.InvocationMatcher;
+import net.sourceforge.pmd.lang.java.types.InvocationMatcher;
import net.sf.saxon.trans.XPathException;
diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/InvocationMatcher.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/InvocationMatcher.java
new file mode 100644
index 0000000000..0c52fe4b15
--- /dev/null
+++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/InvocationMatcher.java
@@ -0,0 +1,334 @@
+/*
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+package net.sourceforge.pmd.lang.java.types;
+
+import static net.sourceforge.pmd.util.CollectionUtil.listOf;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+import org.apache.commons.lang3.StringUtils;
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+import net.sourceforge.pmd.internal.util.AssertionUtil;
+import net.sourceforge.pmd.lang.java.ast.ASTConstructorCall;
+import net.sourceforge.pmd.lang.java.ast.ASTExpression;
+import net.sourceforge.pmd.lang.java.ast.ASTList;
+import net.sourceforge.pmd.lang.java.ast.InvocationNode;
+import net.sourceforge.pmd.lang.java.ast.JavaNode;
+import net.sourceforge.pmd.lang.java.ast.QualifiableExpression;
+import net.sourceforge.pmd.util.CollectionUtil;
+import net.sourceforge.pmd.util.OptionalBool;
+import net.sourceforge.pmd.util.StringUtil;
+
+/**
+ * Matches a method or constructor call against a particular overload.
+ * Use {@link #parse(String)} to create one. For example,
+ *
+ *
+ * java.lang.String#toString() // match calls to toString on String instances
+ * _#toString() // match calls to toString on any receiver
+ * _#_() // match all calls to a method with no parameters
+ * _#toString(_*) // match calls to a "toString" method with any number of parameters
+ * _#eq(_, _) // match calls to an "eq" method that has 2 parameters of unspecified type
+ * _#eq(java.lang.String, _) // like the previous, but the first parameter must be String
+ * java.util.ArrayList#new(int) // match constructor calls of this overload of the ArrayList constructor
+ *
+ *
+ * The receiver matcher (first half) is matched against the
+ * static type of the receiver of the call, and not the
+ * declaration site of the method, unless the called method is
+ * static, or a constructor.
+ *
+ *
The parameters are matched against the declared parameters
+ * types of the called overload, and not the actual argument types.
+ * In particular, for vararg methods, the signature should mention
+ * a single parameter, with an array type.
+ *
+ *
For example {@code Integer.valueOf('0')} will be matched by
+ * {@code _#valueOf(int)} but not {@code _#valueOf(char)}, which is
+ * an overload that does not exist (the char is widened to an int,
+ * so the int overload is selected).
+ *
+ *
Full EBNF grammar
+ *
+ * (no whitespace is tolerated anywhere):
+ *
{@code
+ * sig ::= type '#' method_name param_list
+ * type ::= qname ( '[]' )* | '_'
+ * method_name ::= '_' | ident | 'new'
+ * param_list ::= '(_*)' | '(' type (',' type )* ')'
+ * qname ::= java binary name
+ * }
+ */
+public final class InvocationMatcher {
+
+ final @Nullable String expectedName;
+ final @Nullable List argMatchers;
+ final TypeMatcher qualifierMatcher;
+
+ InvocationMatcher(TypeMatcher qualifierMatcher, String expectedName, @Nullable List argMatchers) {
+ this.expectedName = "_".equals(expectedName) ? null : expectedName;
+ this.argMatchers = argMatchers;
+ this.qualifierMatcher = qualifierMatcher;
+ }
+
+ /**
+ * See {@link #matchesCall(InvocationNode)}.
+ */
+ public boolean matchesCall(@Nullable JavaNode node) {
+ if (node instanceof InvocationNode) {
+ return matchesCall((InvocationNode) node);
+ }
+ return false;
+ }
+
+ /**
+ * Returns true if the call matches this matcher. This means,
+ * the called overload is the one identified by the argument
+ * matchers, and the actual qualifier type is a subtype of the
+ * one mentioned by the qualifier matcher.
+ */
+ public boolean matchesCall(@Nullable InvocationNode node) {
+ if (node == null) {
+ return false;
+ }
+ if (expectedName != null && !node.getMethodName().equals(expectedName)
+ || argMatchers != null && ASTList.sizeOrZero(node.getArguments()) != argMatchers.size()) {
+ return false;
+ }
+ OverloadSelectionResult info = node.getOverloadSelectionInfo();
+ if (info.isFailed() || !matchQualifier(node)) {
+ return false;
+ }
+ return argsMatchOverload(info.getMethodType());
+ }
+
+ private boolean matchQualifier(InvocationNode node) {
+ if (qualifierMatcher == TypeMatcher.ANY) { // NOPMD CompareObjectsWithEquals
+ return true;
+ }
+ if (node instanceof ASTConstructorCall) {
+ JTypeMirror newType = ((ASTConstructorCall) node).getTypeNode().getTypeMirror();
+ return qualifierMatcher.matches(newType, true);
+ }
+ JMethodSig m = node.getMethodType();
+ JTypeMirror qualType;
+ if (node instanceof QualifiableExpression) {
+ ASTExpression qualifier = ((QualifiableExpression) node).getQualifier();
+ if (qualifier != null) {
+ qualType = qualifier.getTypeMirror();
+ } else {
+ // todo: if qualifier == null, then we should take the type of the
+ // implicit receiver, ie `this` or `SomeOuter.this`
+ qualType = m.getDeclaringType();
+ }
+ } else {
+ qualType = m.getDeclaringType();
+ }
+
+ return qualifierMatcher.matches(qualType, m.isStatic());
+ }
+
+ private boolean argsMatchOverload(JMethodSig invoc) {
+ if (argMatchers == null) {
+ return true;
+ }
+ List formals = invoc.getFormalParameters();
+ if (invoc.getArity() != argMatchers.size()) {
+ return false;
+ }
+ for (int i = 0; i < formals.size(); i++) {
+ if (!argMatchers.get(i).matches(formals.get(i), true)) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+
+ /**
+ * Parses a {@link CompoundInvocationMatcher} which matches any of
+ * the provided matchers.
+ *
+ * @param first First signature, in the format described on this class
+ * @param rest Other signatures, in the format described on this class
+ *
+ * @return A sig matcher
+ *
+ * @throws IllegalArgumentException If any signature is malformed (see EBNF)
+ * @throws NullPointerException If any signature is null
+ * @see #parse(String)
+ */
+ public static CompoundInvocationMatcher parseAll(String first, String... rest) {
+ List matchers = CollectionUtil.map(listOf(first, rest), InvocationMatcher::parse);
+ return new CompoundInvocationMatcher(matchers);
+ }
+
+ /**
+ * Parses an {@link InvocationMatcher}.
+ *
+ * @param sig A signature in the format described on this class
+ *
+ * @return A sig matcher
+ *
+ * @throws IllegalArgumentException If the signature is malformed (see EBNF)
+ * @throws NullPointerException If the signature is null
+ * @see #parseAll(String, String...)
+ */
+ public static InvocationMatcher parse(String sig) {
+ int i = parseType(sig, 0);
+ final TypeMatcher qualifierMatcher = newMatcher(sig.substring(0, i));
+ i = consumeChar(sig, i, '#');
+ final int nameStart = i;
+ i = parseSimpleName(sig, i);
+ final String methodName = sig.substring(nameStart, i);
+ i = consumeChar(sig, i, '(');
+ if (isChar(sig, i, ')')) {
+ return new InvocationMatcher(qualifierMatcher, methodName, Collections.emptyList());
+ }
+ // (_*) matches any argument list
+ List argMatchers;
+ if (isChar(sig, i, '_')
+ && isChar(sig, i + 1, '*')
+ && isChar(sig, i + 2, ')')) {
+ argMatchers = null;
+ i = i + 3;
+ } else {
+ argMatchers = new ArrayList<>();
+ i = parseArgList(sig, i, argMatchers);
+ }
+ if (i != sig.length()) {
+ throw new IllegalArgumentException("Not a valid signature " + sig);
+ }
+ return new InvocationMatcher(qualifierMatcher, methodName, argMatchers);
+ }
+
+ private static int parseSimpleName(String sig, final int start) {
+ int i = start;
+ while (i < sig.length() && Character.isJavaIdentifierPart(sig.charAt(i))) {
+ i++;
+ }
+ if (i == start) {
+ throw new IllegalArgumentException("Not a valid signature " + sig);
+ }
+ return i;
+ }
+
+ private static int parseArgList(String sig, int i, List argMatchers) {
+ while (i < sig.length()) {
+ i = parseType(sig, i, argMatchers);
+ if (isChar(sig, i, ')')) {
+ return i + 1;
+ }
+ i = consumeChar(sig, i, ',');
+ }
+ throw new IllegalArgumentException("Not a valid signature " + sig);
+ }
+
+ private static int consumeChar(String source, int i, char c) {
+ if (isChar(source, i, c)) {
+ return i + 1;
+ }
+ throw newParseException(source, i, "character '" + c + "'");
+ }
+
+ private static RuntimeException newParseException(String source, int i, String expectedWhat) {
+ final String indent = " ";
+ String message = "Expected " + expectedWhat + " at index " + i + ":\n";
+ message += indent + "\"" + StringUtil.escapeJava(source) + "\"\n";
+ message += indent + StringUtils.repeat(' ', i + 1) + '^' + "\n";
+ return new IllegalArgumentException(message);
+ }
+
+ private static boolean isChar(String source, int i, char c) {
+ return i < source.length() && source.charAt(i) == c;
+ }
+
+ private static int parseType(String source, int i, List result) {
+ final int start = i;
+ i = parseType(source, i);
+ result.add(newMatcher(source.substring(start, i)));
+ return i;
+ }
+
+ private static int parseType(String source, int i) {
+ final int start = i;
+ while (i < source.length() && (Character.isJavaIdentifierPart(source.charAt(i))
+ || source.charAt(i) == '.')) {
+ i++;
+ }
+ if (i == start) {
+ throw newParseException(source, i, "type");
+ }
+
+ AssertionUtil.assertValidJavaBinaryName(source.substring(start, i));
+ // array dimensions
+ while (isChar(source, i, '[')) {
+ i = consumeChar(source, i + 1, ']');
+ }
+ return i;
+ }
+
+ private static TypeMatcher newMatcher(String name) {
+ return "_".equals(name) ? TypeMatcher.ANY : new TypeMatcher(name);
+ }
+
+ private static final class TypeMatcher {
+
+ /** Matches any type. */
+ public static final TypeMatcher ANY = new TypeMatcher(null);
+
+ final @Nullable String name;
+
+ private TypeMatcher(@Nullable String name) {
+ this.name = name;
+ }
+
+ boolean matches(JTypeMirror type, boolean exact) {
+ if (name == null) {
+ return true;
+ }
+ return exact ? TypeTestUtil.isExactlyAOrAnon(name, type) == OptionalBool.YES
+ : TypeTestUtil.isA(name, type);
+ }
+ }
+
+ /**
+ * A compound of several matchers (logical OR). Get one from
+ * {@link InvocationMatcher#parseAll(String, String...)};
+ */
+ public static final class CompoundInvocationMatcher {
+
+ private final List matchers;
+
+ private CompoundInvocationMatcher(List matchers) {
+ this.matchers = matchers;
+ }
+
+ // todo make this smarter. Like collecting all possible names
+ // into a set to do a quick pre-screening before we test
+ // everything linearly
+
+ /**
+ * Returns true if any of the matchers match the node.
+ *
+ * @see #matchesCall(JavaNode)
+ */
+ public boolean anyMatch(InvocationNode node) {
+ return CollectionUtil.any(matchers, it -> it.matchesCall(node));
+ }
+
+ /**
+ * Returns true if any of the matchers match the node.
+ *
+ * @see #matchesCall(JavaNode)
+ */
+ public boolean anyMatch(JavaNode node) {
+ return CollectionUtil.any(matchers, it -> it.matchesCall(node));
+ }
+ }
+}
diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/TypeTestUtil.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/TypeTestUtil.java
index e33284a5d5..b7b243bc62 100644
--- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/TypeTestUtil.java
+++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/TypeTestUtil.java
@@ -5,32 +5,24 @@
package net.sourceforge.pmd.lang.java.types;
import java.lang.reflect.Modifier;
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.List;
import org.apache.commons.lang3.StringUtils;
import org.checkerframework.checker.nullness.qual.NonNull;
import org.checkerframework.checker.nullness.qual.Nullable;
import net.sourceforge.pmd.internal.util.AssertionUtil;
-import net.sourceforge.pmd.lang.java.ast.ASTConstructorCall;
-import net.sourceforge.pmd.lang.java.ast.ASTExpression;
-import net.sourceforge.pmd.lang.java.ast.ASTList;
import net.sourceforge.pmd.lang.java.ast.InternalApiBridge;
-import net.sourceforge.pmd.lang.java.ast.InvocationNode;
-import net.sourceforge.pmd.lang.java.ast.JavaNode;
-import net.sourceforge.pmd.lang.java.ast.QualifiableExpression;
import net.sourceforge.pmd.lang.java.ast.TypeNode;
import net.sourceforge.pmd.lang.java.symbols.JClassSymbol;
import net.sourceforge.pmd.lang.java.symbols.JTypeDeclSymbol;
import net.sourceforge.pmd.lang.java.symbols.JTypeParameterSymbol;
import net.sourceforge.pmd.lang.java.symbols.internal.UnresolvedClassStore;
import net.sourceforge.pmd.util.OptionalBool;
-import net.sourceforge.pmd.util.StringUtil;
/**
* Public utilities to test the type of nodes.
+ *
+ * @see InvocationMatcher
*/
public final class TypeTestUtil {
@@ -269,7 +261,7 @@ public final class TypeTestUtil {
return isExactlyAOrAnon(canonicalName, node.getTypeMirror()) == OptionalBool.YES;
}
- private static OptionalBool isExactlyAOrAnon(@NonNull String canonicalName, final @NonNull JTypeMirror node) {
+ static OptionalBool isExactlyAOrAnon(@NonNull String canonicalName, final @NonNull JTypeMirror node) {
AssertionUtil.requireParamNotNull("canonicalName", canonicalName);
JTypeDeclSymbol sym = node.getSymbol();
@@ -296,259 +288,4 @@ public final class TypeTestUtil {
}
- /**
- * Matches a method or constructor call against a particular overload.
- * Use {@link #parse(String)} to create one. For example,
- *
- *
- * java.lang.String#toString() // match calls to toString on String instances
- * _#toString() // match calls to toString on any receiver
- * _#_() // match all calls to a method with no parameters
- * _#toString(_*) // match calls to a "toString" method with any number of parameters
- * _#eq(_, _) // match calls to an "eq" method that has 2 parameters of unspecified type
- * _#eq(java.lang.String, _) // like the previous, but the first parameter must be String
- * java.util.ArrayList#new(int) // match constructor calls of this overload of the ArrayList constructor
- *
- *
- * The receiver matcher (first half) is matched against the
- * static type of the receiver of the call, and not the
- * declaration site of the method, unless the called method is
- * static, or a constructor.
- *
- *
The parameters are matched against the declared parameters
- * types of the called overload, and not the actual argument types.
- * In particular, for vararg methods, the signature should mention
- * a single parameter, with an array type.
- *
- *
For example {@code Integer.valueOf('0')} will be matched by
- * {@code _#valueOf(int)} but not {@code _#valueOf(char)}, which is
- * an overload that does not exist (the char is widened to an int,
- * so the int overload is selected).
- *
- *
Full EBNF grammar
- *
- * (no whitespace is tolerated anywhere):
- *
{@code
- * sig ::= type '#' method_name param_list
- * type ::= qname ( '[]' )* | '_'
- * method_name ::= '_' | ident | 'new'
- * param_list ::= '(_*)' | '(' type (',' type )* ')'
- * qname ::= java binary name
- * }
- */
- public static final class InvocationMatcher {
-
- final @Nullable String expectedName;
- final @Nullable List argMatchers;
- final TypeMatcher qualifierMatcher;
-
- InvocationMatcher(TypeMatcher qualifierMatcher, String expectedName, @Nullable List argMatchers) {
- this.expectedName = "_".equals(expectedName) ? null : expectedName;
- this.argMatchers = argMatchers;
- this.qualifierMatcher = qualifierMatcher;
- }
-
- /**
- * See {@link #matchesCall(InvocationNode)}.
- */
- public boolean matchesCall(@Nullable JavaNode node) {
- if (node instanceof InvocationNode) {
- return matchesCall((InvocationNode) node);
- }
- return false;
- }
-
- /**
- * Returns true if the call matches this matcher. This means,
- * the called overload is the one identified by the argument
- * matchers, and the actual qualifier type is a subtype of the
- * one mentioned by the qualifier matcher.
- */
- public boolean matchesCall(@Nullable InvocationNode node) {
- if (node == null) {
- return false;
- }
- if (expectedName != null && !node.getMethodName().equals(expectedName)
- || argMatchers != null && ASTList.sizeOrZero(node.getArguments()) != argMatchers.size()) {
- return false;
- }
- OverloadSelectionResult info = node.getOverloadSelectionInfo();
- if (info.isFailed() || !matchQualifier(node)) {
- return false;
- }
- return argsMatchOverload(info.getMethodType());
- }
-
- private boolean matchQualifier(InvocationNode node) {
- if (qualifierMatcher == TypeMatcher.ANY) {
- return true;
- }
- if (node instanceof ASTConstructorCall) {
- JTypeMirror newType = ((ASTConstructorCall) node).getTypeNode().getTypeMirror();
- return qualifierMatcher.matches(newType, true);
- }
- JMethodSig m = node.getMethodType();
- JTypeMirror qualType;
- if (node instanceof QualifiableExpression) {
- ASTExpression qualifier = ((QualifiableExpression) node).getQualifier();
- if (qualifier != null) {
- qualType = qualifier.getTypeMirror();
- } else {
- // todo: if qualifier == null, then we should take the type of the
- // implicit receiver, ie `this` or `SomeOuter.this`
- qualType = m.getDeclaringType();
- }
- } else {
- qualType = m.getDeclaringType();
- }
-
- return qualifierMatcher.matches(qualType, m.isStatic());
- }
-
- private boolean argsMatchOverload(JMethodSig invoc) {
- if (argMatchers == null) {
- return true;
- }
- List formals = invoc.getFormalParameters();
- if (invoc.getArity() != argMatchers.size()) {
- return false;
- }
- for (int i = 0; i < formals.size(); i++) {
- if (!argMatchers.get(i).matches(formals.get(i), true)) {
- return false;
- }
- }
- return true;
- }
-
-
- /**
- * Parses an {@link InvocationMatcher}.
- *
- * @param sig A signature in the format described on this class
- *
- * @return A sig matcher
- *
- * @throws IllegalArgumentException If the signature is malformed (see EBNF)
- * @throws NullPointerException If the signature is null
- */
- public static InvocationMatcher parse(String sig) {
- int i = parseType(sig, 0);
- final TypeMatcher qualifierMatcher = newMatcher(sig.substring(0, i));
- i = consumeChar(sig, i, '#');
- final int nameStart = i;
- i = parseSimpleName(sig, i);
- final String methodName = sig.substring(nameStart, i);
- i = consumeChar(sig, i, '(');
- if (isChar(sig, i, ')')) {
- return new InvocationMatcher(qualifierMatcher, methodName, Collections.emptyList());
- }
- // (_*) matches any argument list
- List argMatchers;
- if (isChar(sig, i, '_')
- && isChar(sig, i + 1, '*')
- && isChar(sig, i + 2, ')')) {
- argMatchers = null;
- i = i + 3;
- } else {
- argMatchers = new ArrayList<>();
- i = parseArgList(sig, i, argMatchers);
- }
- if (i != sig.length()) {
- throw new IllegalArgumentException("Not a valid signature " + sig);
- }
- return new InvocationMatcher(qualifierMatcher, methodName, argMatchers);
- }
-
- private static int parseSimpleName(String sig, final int start) {
- int i = start;
- while (i < sig.length() && Character.isJavaIdentifierPart(sig.charAt(i))) {
- i++;
- }
- if (i == start) {
- throw new IllegalArgumentException("Not a valid signature " + sig);
- }
- return i;
- }
-
- private static int parseArgList(String sig, int i, List argMatchers) {
- while (i < sig.length()) {
- i = parseType(sig, i, argMatchers);
- if (isChar(sig, i, ')')) {
- return i + 1;
- }
- i = consumeChar(sig, i, ',');
- }
- throw new IllegalArgumentException("Not a valid signature " + sig);
- }
-
- private static int consumeChar(String source, int i, char c) {
- if (isChar(source, i, c)) {
- return i + 1;
- }
- throw newParseException(source, i, "character '" + c + "'");
- }
-
- private static RuntimeException newParseException(String source, int i, String expectedWhat) {
- final String indent = " ";
- String message = "Expected " + expectedWhat + " at index " + i + ":\n";
- message += indent + "\"" + StringUtil.escapeJava(source) + "\"\n";
- message += indent + StringUtils.repeat(' ', i + 1) + '^' + "\n";
- return new IllegalArgumentException(message);
- }
-
- private static boolean isChar(String source, int i, char c) {
- return i < source.length() && source.charAt(i) == c;
- }
-
- private static int parseType(String source, int i, List result) {
- final int start = i;
- i = parseType(source, i);
- result.add(newMatcher(source.substring(start, i)));
- return i;
- }
-
- private static int parseType(String source, int i) {
- final int start = i;
- while (i < source.length() && (Character.isJavaIdentifierPart(source.charAt(i))
- || source.charAt(i) == '.')) {
- i++;
- }
- if (i == start) {
- throw newParseException(source, i, "type");
- }
-
- AssertionUtil.assertValidJavaBinaryName(source.substring(start, i));
- // array dimensions
- while (isChar(source, i, '[')) {
- i = consumeChar(source, i + 1, ']');
- }
- return i;
- }
-
- private static TypeMatcher newMatcher(String name) {
- return "_".equals(name) ? TypeMatcher.ANY : new TypeMatcher(name);
- }
-
- private static final class TypeMatcher {
-
- /** Matches any type. */
- public static final TypeMatcher ANY = new TypeMatcher(null);
-
- final @Nullable String name;
-
- private TypeMatcher(@Nullable String name) {
- this.name = name;
- }
-
- boolean matches(JTypeMirror type, boolean exact) {
- if (name == null) {
- return true;
- }
- return exact ? TypeTestUtil.isExactlyAOrAnon(name, type) == OptionalBool.YES
- : TypeTestUtil.isA(name, type);
- }
- }
- }
-
}
diff --git a/pmd-java/src/main/resources/category/java/codestyle.xml b/pmd-java/src/main/resources/category/java/codestyle.xml
index a0b0bbd29a..b758d9df32 100644
--- a/pmd-java/src/main/resources/category/java/codestyle.xml
+++ b/pmd-java/src/main/resources/category/java/codestyle.xml
@@ -1204,18 +1204,28 @@ public class SomeClass {
-Checks for variables that are defined before they might be used. A reference is deemed to be premature if it is created right before a block of code that doesn't use it that also has the ability to return or throw an exception.
+Checks for variables that are defined before they might be used. A declaration is
+deemed to be premature if there are some statements that may return or throw an
+exception between the time the variable is declared and the time it is first read.
+
+Some variables cannot be declared close to their first usage because of side-effects
+occurring before they're first used. We try to avoid reporting those by considering
+most method and constructor invocations to be impure. See the second example.
+
+Note that this rule is meant to improve code readability but is not an optimization.
+A smart JIT will not care whether the variable is declared prematurely or not, as it
+can reorder code.
3
+
+
+
diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/codestyle/PrematureDeclarationTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/codestyle/PrematureDeclarationTest.java
index 62dbb60691..c73a9f9781 100644
--- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/codestyle/PrematureDeclarationTest.java
+++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/codestyle/PrematureDeclarationTest.java
@@ -6,7 +6,6 @@ package net.sourceforge.pmd.lang.java.rule.codestyle;
import net.sourceforge.pmd.testframework.PmdRuleTst;
-@org.junit.Ignore("Rule has not been updated yet")
public class PrematureDeclarationTest extends PmdRuleTst {
// no additional unit tests
}
diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/types/InvocationMatcherTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/types/InvocationMatcherTest.java
index d6f72183d5..626a0f6206 100644
--- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/types/InvocationMatcherTest.java
+++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/types/InvocationMatcherTest.java
@@ -4,7 +4,7 @@
package net.sourceforge.pmd.lang.java.types;
-import static net.sourceforge.pmd.lang.java.types.TypeTestUtil.InvocationMatcher.parse;
+import static net.sourceforge.pmd.lang.java.types.InvocationMatcher.parse;
import static org.hamcrest.Matchers.equalTo;
import org.hamcrest.MatcherAssert;
diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/PrematureDeclaration.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/PrematureDeclaration.xml
index 7b6a70b47b..c9eff3cef4 100644
--- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/PrematureDeclaration.xml
+++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/PrematureDeclaration.xml
@@ -1,8 +1,8 @@
+ xmlns="http://pmd.sourceforge.net/rule-tests"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://pmd.sourceforge.net/rule-tests http://pmd.sourceforge.net/rule-tests_1_0_0.xsd">
premature declaration before unrelated test
@@ -11,18 +11,18 @@
public class Bar {
public int lengthSumOf(String[] strings) {
- int sum = 0; // wasted cycles if strings have problems
+ int sum = 0; // premature
if (strings == null || strings.length == 0) return 0;
- for (int i=0; i
+ ]]>
@@ -30,6 +30,7 @@ public class Bar {
0
#1067 PrematureDeclaration lambda closure false positive
0
#1108 PrematureDeclaration lambda false positive
0
Optional ofRunnable(Supplier sup) {return Optional.of(sup.get());}
+ private static String sign(String input) {return input.toLowerCase();}
+
+ public String lengthSumOf(String a, String b) {
+ String signingInput = Stream.of(a, b)
+ .filter(Objects::nonNull)
+ .map(String::valueOf)
+ .collect(Collectors.joining());
+
+ return ofRunnable(() -> sign(signingInput))
+ .orElse(null);
+ }
+}
+ ]]>
+
+
+ PrematureDeclaration with captured lambda var
+ 0
+ Optional ofRunnable(Supplier sup) {return Optional.of(sup.get());}
+ private static String sign(String input) {return input.toLowerCase();}
+
+ public String lengthSumOf(String a, String b) {
+ String signingInput = Stream.of(a, b)
+ .filter(Objects::nonNull)
+ .map(String::valueOf)
+ .collect(Collectors.joining());
+
+ return ofRunnable(() -> {
+ return sign(signingInput);
+ }).orElse(null);
+ }
+}
+ ]]>
+
+
+
+ PrematureDeclaration with captured lambda var (2)
+ 0
+ Optional ofRunnable(Supplier sup) {return Optional.of(sup);}
+ private static String sign(String input) {return input.toLowerCase();}
+
+ public boolean lengthSumOf(boolean sign) {
String signingInput = Stream.of(a, b)
.filter(Objects::nonNull)
.map(String::valueOf)
.collect(Collectors.joining(EMPTY));
+ if (sign)
+ return ofRunnable(() -> {
+ return sign(signingInput); // this should be recognized as a usage
+ }).orElse(false);
- return Try.of(() -> sign(signingInput))
- .getOrElse(() -> null);
+ return signingInput;
}
}
- ]]>
+ ]]>
PrematureDeclaration should also check inside lambdas
1
{
+public class PrematureDeclarationLambda {
+ private static Optional ofRunnable(Supplier sup) {return Optional.of(sup);}
+ public boolean lengthSumOf(String[] strings) {
+
+ return ofRunnable(() -> {
// Inside that lambda *is* a premature declaration of `sum`
int sum = 0;
@@ -176,9 +247,252 @@ public class PrematureDeclarationLambda {
}
return sum;
- });
+ }).isPresent();
}
}
]]>
+
+ #1429 PrematureDeclaration should try to recognize side-effects
+ 0
+ cache = new HashMap<>();
+ final AtomicInteger ref = new AtomicInteger();
+
+ public String doSomeThing(String uri, String cachedPath, String ref) {
+ var prior = cache.put(uri, cachedPath); // may side-effect
+
+ if (ref == null) {
+ return createRef(cachedPath, 1); // may side-effect
+ }
+ if (prior == null) {
+ return ref.substring(1);
+ }
+ return ref;
+ }
+
+ static String createRef(String cachedPath, int refCount) {
+ return "" + cachedPath + refCount;
+ }
+ }
+ ]]>
+
+
+ #1429 PrematureDeclaration side-effect of time methods
+ 0
+ scriptClass;
+ Object cachedResult;
+
+ private void doScript(Class klass) {}
+
+ public Object getScriptedObject() {
+ synchronized (this) {
+ Class scriptClassToExecute; // here
+
+ if (this.cachedResult != null) {
+ Object result = this.cachedResult;
+ this.cachedResult = null;
+ return result;
+ }
+
+ scriptClassToExecute = this.scriptClass; // here
+ return doScript(scriptClassToExecute);
+ }
+ }
+ }
+ ]]>
+
+
+ FN with exception ctor flagged as side-effect
+
+ 1
+
+ Declaration of 'file2' can be moved closer to its usages
+
+ scriptClass;
+ Object cachedResult;
+
+ private void doScript(Class klass) {}
+
+
+ public String combine(String pattern1, String pattern2) {
+ String ext1 = pattern1.substring(starDotPos1 + 1);
+ int dotPos2 = pattern2.indexOf('.');
+ String file2 = dotPos2 == -1 ? pattern2 : pattern2.substring(0, dotPos2);
+ String ext2 = dotPos2 == -1 ? "" : pattern2.substring(dotPos2);
+ boolean ext1All = ext1.equals(".*") || ext1.equals("");
+ boolean ext2All = ext2.equals(".*") || ext2.equals("");
+ if (!ext1All && !ext2All) {
+ throw new IllegalArgumentException("Cannot combine patterns: " + pattern1 + " vs " + pattern2);
+ }
+ String ext = ext1All ? ext2 : ext1;
+ return file2 + ext;
+ }
+ }
+ ]]>
+
+
+ FN with trivial initializers
+
+ 1
+
+ Declaration of 'isReal' can be moved closer to its usages
+
+ parse(String value) throws IllegalArgumentException {
+ if (!value.isEmpty()) {
+ return Collections.emptySet();
+ }
+ // obviously this cannot be reordered
+ // We should also track side effects on locals
+ String originalValue = value;
+ value = value.trim();
+ if ("ALL".equalsIgnoreCase(value)) {
+ return EnumSet.allOf(TestGroup.class);
+ }
+ if (value.toUpperCase().startsWith("ALL-")) {
+ Set groups = EnumSet.allOf(TestGroup.class);
+ groups.remove(TestGroup.A);
+ return groups;
+ }
+ return parseGroups(originalValue, value);
+ }
+ }
+ ]]>
+