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

Merge pull request #3227 from oowekyala:update-OnlyOneReturn

Browse Source 
[java] Update rule OnlyOneReturn #3227
This commit is contained in:
Andreas Dangel
2021-04-23 12:14:38 +02:00
parent 4c9ee04254 52f62698f9
commit 6c9eee83b4
11 changed files with 181 additions and 50 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
.ci/files/all-java.xml
View File

@@ -93,7 +93,7 @@
<rule ref="category/java/codestyle.xml/MethodArgumentCouldBeFinal"/>
<rule ref="category/java/codestyle.xml/MethodNamingConventions"/>
<rule ref="category/java/codestyle.xml/NoPackage"/>
<!-- <rule ref="category/java/codestyle.xml/OnlyOneReturn"/> -->
<rule ref="category/java/codestyle.xml/OnlyOneReturn"/>
<rule ref="category/java/codestyle.xml/PackageCase"/>
<rule ref="category/java/codestyle.xml/PrematureDeclaration"/>
<rule ref="category/java/codestyle.xml/RemoteInterfaceNamingConvention"/>

63
pmd-core/src/main/java/net/sourceforge/pmd/internal/util/IteratorUtil.java
View File

@@ -229,6 +229,69 @@ public final class IteratorUtil {
return list;
}
/**
* Remove the last n elements of the iterator. This uses n elements as a lookahead.
*/
public static <T> Iterator<@NonNull T> dropLast(Iterator<? extends @Nullable T> it, final int n) {
AssertionUtil.requireNonNegative("n", n);
if (n == 0) {
return coerceWildcard(it); // noop
} else if (n == 1) { // i guess this will be common
if (!it.hasNext()) {
return Collections.emptyIterator();
}
return new AbstractIterator<T>() {
T next = it.next();
@Override
protected void computeNext() {
if (it.hasNext()) {
setNext(next);
next = it.next();
} else {
done();
}
}
};
}
// fill a circular lookahead buffer
Object[] ringBuffer = new Object[n];
for (int i = 0; i < n && it.hasNext(); i++) {
ringBuffer[i] = it.next();
}
if (!it.hasNext()) {
// the original iterator has less than n elements
return Collections.emptyIterator();
}
return new AbstractIterator<T>() {
private int idx = 0;
@Override
protected void computeNext() {
if (it.hasNext()) {
setNext((T) ringBuffer[idx]); // yield element X from the buffer
ringBuffer[idx] = it.next(); // overwrite with the element X+n
idx = (idx + 1) % ringBuffer.length; // compute idx of element X+1
} else {
// that's it: our buffer contains the n tail elements
// that we don't want to see.
done();
}
}
};
}
/**
* Coerce an iterator with a wildcard. This is safe because the Iterator
* interface is covariant (not {@link ListIterator} though).
*/
@SuppressWarnings("unchecked")
public static <T> Iterator<T> coerceWildcard(final Iterator<? extends T> it) {
return (Iterator<T>) it;
}
public static <T> Iterable<T> toIterable(final Iterator<T> it) {
return () -> it;
}

16
pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/NodeStream.java
View File

@@ -285,9 +285,25 @@ public interface NodeStream<@NonNull T extends Node> extends Iterable<@NonNull T
* @throws IllegalArgumentException if n is negative
* @see Stream#skip(long)
* @see #take(int)
* @see #dropLast(int)
*/
NodeStream<T> drop(int n);
/**
* Returns a stream consisting of the elements of this stream except
* the n tail elements. If n is greater than the number of elements
* of this stream, returns an empty stream. This requires a lookahead
* buffer in general.
*
* @param n the number of trailing elements to skip
*
* @return A new node stream
*
* @throws IllegalArgumentException if n is negative
* @see #drop(int)
*/
NodeStream<T> dropLast(int n);
/**
* Returns the longest prefix of elements that satisfy the given predicate.

9
pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/internal/AxisStream.java
View File

@@ -471,6 +471,15 @@ abstract class AxisStream<T extends Node> extends IteratorBasedNStream<T> {
return StreamImpl.sliceChildren(node, filter, newLow, newLen);
}
@Override
public NodeStream<Node> dropLast(int n) {
AssertionUtil.requireNonNegative("n", n);
if (n == 0) {
return this;
}
return take(max(len - n, 0));
}
@Override
public boolean nonEmpty() {
return len > 0;

6
pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/internal/IteratorBasedNStream.java
View File

@@ -122,6 +122,12 @@ abstract class IteratorBasedNStream<T extends Node> implements NodeStream<T> {
return maxSize == 0 ? NodeStream.empty() : mapIter(iter -> IteratorUtil.take(iter, maxSize));
}
@Override
public NodeStream<T> dropLast(int n) {
AssertionUtil.requireNonNegative("n", n);
return n == 0 ? this : mapIter(iter -> IteratorUtil.dropLast(iter, n));
}
@Override
public NodeStream<T> takeWhile(Predicate<? super T> predicate) {
return mapIter(iter -> IteratorUtil.takeWhile(iter, predicate));

48
pmd-core/src/test/java/net/sourceforge/pmd/internal/util/IteratorUtilTest.java
View File

@@ -6,11 +6,12 @@ package net.sourceforge.pmd.internal.util;
import static java.util.Collections.emptyIterator;
import static java.util.Collections.emptyList;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.collection.IsIterableContainingInOrder.contains;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;
import java.util.ArrayList;
@@ -23,6 +24,7 @@ import java.util.function.Consumer;
import java.util.function.Function;
import java.util.function.Predicate;
import org.junit.Assert;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
@@ -459,6 +461,50 @@ public class IteratorUtilTest {
assertThat(mapped, contains("c", "b", "a"));
}
@Test
public void testDropLast() {
Iterator<String> iter = iterOf("ab", "cdde", "e", "", "f", "fe");
Iterator<String> dropped = IteratorUtil.dropLast(iter, 2);
assertEquals(listOf("ab", "cdde", "e", ""), IteratorUtil.toList(dropped));
}
@Test
public void testDropLastOne() {
Iterator<String> iter = iterOf("ab", "cdde", "e", "", "f", "fe");
Iterator<String> dropped = IteratorUtil.dropLast(iter, 1);
assertEquals(listOf("ab", "cdde", "e", "", "f"), IteratorUtil.toList(dropped));
}
@Test
public void testDropMoreThanSize() {
Iterator<String> iter = iterOf("ab", "c");
Iterator<String> dropped = IteratorUtil.dropLast(iter, 4);
assertEquals(emptyList(), IteratorUtil.toList(dropped));
}
@Test
public void testDropLastZero() {
Iterator<String> iter = iterOf("ab", "c");
Iterator<String> dropped = IteratorUtil.dropLast(iter, 0);
assertEquals(listOf("ab", "c"), IteratorUtil.toList(dropped));
}
@Test
public void testDropLastNegative() {
Iterator<String> iter = iterOf("ab", "c");
Assert.assertThrows(IllegalArgumentException.class, () -> IteratorUtil.dropLast(iter, -3));
}
private void assertExhausted(Iterator<?> mapped) {
assertFalse(mapped.hasNext());
exception.expect(NoSuchElementException.class);

15
pmd-core/src/test/java/net/sourceforge/pmd/lang/ast/internal/NodeStreamBlanketTest.java
View File

@@ -129,6 +129,17 @@ public class NodeStreamBlanketTest<T extends Node> {
);
}
@Test
public void testDropLast() {
assertImplication(
stream,
prop("nonEmpty", NodeStream::nonEmpty),
prop("dropLast(0) == this", it -> it.dropLast(0) == it),
prop("dropLast(1).count() == count() - 1", it -> it.dropLast(1).count() == it.count() - 1),
prop("dropLast(1).toList() == toList().init()", it -> it.dropLast(1).toList().equals(init(it.toList())))
);
}
@Test
public void testDropMoreThan1() {
assertImplication(
@@ -270,6 +281,10 @@ public class NodeStreamBlanketTest<T extends Node> {
return ts.subList(1, ts.size());
}
static <T> List<T> init(List<T> ts) {
return ts.subList(0, ts.size() - 1);
}
static class Prop<T> {
final Predicate<? super T> property;

38
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/OnlyOneReturnRule.java
View File

@@ -4,43 +4,29 @@
package net.sourceforge.pmd.lang.java.rule.codestyle;
import java.util.Iterator;
import java.util.List;
import net.sourceforge.pmd.lang.ast.Node;
import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration;
import net.sourceforge.pmd.lang.ast.NodeStream;
import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTReturnStatement;
import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule;
public class OnlyOneReturnRule extends AbstractJavaRule {
public class OnlyOneReturnRule extends AbstractJavaRulechainRule {
@Override
public Object visit(ASTClassOrInterfaceDeclaration node, Object data) {
if (node.isInterface()) {
return data;
}
return super.visit(node, data);
public OnlyOneReturnRule() {
super(ASTMethodDeclaration.class);
}
@Override
public Object visit(ASTMethodDeclaration node, Object data) {
if (node.isAbstract()) {
return data;
if (node.getBody() == null) {
return null;
}
List<ASTReturnStatement> returnNodes = node.findDescendantsOfType(ASTReturnStatement.class);
NodeStream<ASTReturnStatement> returnsExceptLast =
node.getBody().descendants(ASTReturnStatement.class).dropLast(1);
if (returnNodes.size() > 1) {
for (Iterator<ASTReturnStatement> i = returnNodes.iterator(); i.hasNext();) {
Node problem = i.next();
// skip the last one, it's OK
if (!i.hasNext()) {
continue;
}
addViolation(data, problem);
}
for (ASTReturnStatement returnStmt : returnsExceptLast) {
addViolation(data, returnStmt);
}
return data;
return null;
}
}

2
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/table/internal/SymbolTableResolver.java
View File

@@ -219,7 +219,7 @@ public final class SymbolTableResolver {
int pushed = pushOnStack(f.selfType(top(), node.getTypeMirror()));
pushed += pushOnStack(f.typeHeader(top(), node.getSymbol()));
NodeStream<? extends JavaNode> notBody = node.children().drop(1).take(node.getNumChildren() - 2);
NodeStream<? extends JavaNode> notBody = node.children().drop(1).dropLast(1);
for (JavaNode it : notBody) {
setTopSymbolTable(it);
}

1
pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/codestyle/OnlyOneReturnTest.java
View File

@@ -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 OnlyOneReturnTest extends PmdRuleTst {
// no additional unit tests
}

31
pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/OnlyOneReturn.xml
View File

@@ -80,9 +80,10 @@ public class Foo {
<expected-problems>0</expected-problems>
<code><![CDATA[
public class Foo {
interface Filter { boolean accept(Object o); }
public int foo() {
FileFilter f = new FileFilter() {
public boolean accept(File file) {
Filter f = new Filter() {
public boolean accept(Object o) {
return false;
}
};
@@ -93,26 +94,16 @@ public class Foo {
</test-code>
<test-code>
<description>#1353 False positive "Only One Return" with lambda</description>
<description>#1353 return inside lambda</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class OnlyOneReturn {
public Try<SearchHit[]> search(final String indexName, final SearchRequest searchRequest) {
final SearchHit[] empty = new SearchHit[0];
final Optional<SearchDefinition> searchDefinition = settingsService.getSearchDefinition(indexName);
return searchDefinition.<Try<SearchHit[]>>map(
i -> {
final List<Try<ProviderSearchHit[]>> res = i.getSearchMapping().stream()
.peek(m -> LOGGER.debug("Treating backend \"{}\"", m.getProviderRef()))
.map(m -> invokeAdapter(getProviderSearchService(m.getProviderRef()), m, searchRequest))
.collect(Collectors.toList());
return TryCollections.pull(res).map(l -> sortReturning(l.stream().collect(ArrayCollectors.arrayMerging(
SearchServiceImpl::toSearchHit,
SearchHit::getInternalId,
Function.identity(),
SearchServiceImpl::merge)).orElse(Collections.emptyList()), SearchServiceImpl.searchHitComparator()))
.map(list -> list.toArray(empty));
}).orElse(Try.success(empty));
public class Foo {
interface Filter { boolean accept(Object o); }
public int foo() {
Filter f = o -> {
return false;
};
return 2;
}
}
]]></code>