From a4353bb3669779df8bb763cd03b2d0ca86c78ba7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Sun, 4 Apr 2021 17:22:40 +0200 Subject: [PATCH 1/3] Add dropLast to NodeStream --- .../pmd/internal/util/IteratorUtil.java | 63 +++++++++++++++++++ .../sourceforge/pmd/lang/ast/NodeStream.java | 16 +++++ .../pmd/lang/ast/internal/AxisStream.java | 9 +++ .../ast/internal/IteratorBasedNStream.java | 6 ++ .../pmd/internal/util/IteratorUtilTest.java | 48 +++++++++++++- .../ast/internal/NodeStreamBlanketTest.java | 15 +++++ .../rule/codestyle/OnlyOneReturnRule.java | 17 +++-- .../table/internal/SymbolTableResolver.java | 2 +- 8 files changed, 164 insertions(+), 12 deletions(-) diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/internal/util/IteratorUtil.java b/pmd-core/src/main/java/net/sourceforge/pmd/internal/util/IteratorUtil.java index 4bc1bf877b..352ff36d16 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/internal/util/IteratorUtil.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/internal/util/IteratorUtil.java @@ -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 Iterator<@NonNull T> dropLast(Iterator 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 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() { + 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 Iterator coerceWildcard(final Iterator it) { + return (Iterator) it; + } + public static Iterable toIterable(final Iterator it) { return () -> it; } diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/NodeStream.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/NodeStream.java index 931536b857..d12c91f76c 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/NodeStream.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/NodeStream.java @@ -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 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 dropLast(int n); + /** * Returns the longest prefix of elements that satisfy the given predicate. diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/internal/AxisStream.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/internal/AxisStream.java index 4ebdc30c51..31ef6efd37 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/internal/AxisStream.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/internal/AxisStream.java @@ -471,6 +471,15 @@ abstract class AxisStream extends IteratorBasedNStream { return StreamImpl.sliceChildren(node, filter, newLow, newLen); } + @Override + public NodeStream 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; diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/internal/IteratorBasedNStream.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/internal/IteratorBasedNStream.java index 6eac5312de..f854f37356 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/internal/IteratorBasedNStream.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/internal/IteratorBasedNStream.java @@ -122,6 +122,12 @@ abstract class IteratorBasedNStream implements NodeStream { return maxSize == 0 ? NodeStream.empty() : mapIter(iter -> IteratorUtil.take(iter, maxSize)); } + @Override + public NodeStream dropLast(int n) { + AssertionUtil.requireNonNegative("n", n); + return n == 0 ? this : mapIter(iter -> IteratorUtil.dropLast(iter, n)); + } + @Override public NodeStream takeWhile(Predicate predicate) { return mapIter(iter -> IteratorUtil.takeWhile(iter, predicate)); diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/internal/util/IteratorUtilTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/internal/util/IteratorUtilTest.java index b91d05bdc1..aca14a1cfd 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/internal/util/IteratorUtilTest.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/internal/util/IteratorUtilTest.java @@ -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 iter = iterOf("ab", "cdde", "e", "", "f", "fe"); + + Iterator dropped = IteratorUtil.dropLast(iter, 2); + + assertEquals(listOf("ab", "cdde", "e", ""), IteratorUtil.toList(dropped)); + } + + @Test + public void testDropLastOne() { + Iterator iter = iterOf("ab", "cdde", "e", "", "f", "fe"); + + Iterator dropped = IteratorUtil.dropLast(iter, 1); + + assertEquals(listOf("ab", "cdde", "e", "", "f"), IteratorUtil.toList(dropped)); + } + + @Test + public void testDropMoreThanSize() { + Iterator iter = iterOf("ab", "c"); + + Iterator dropped = IteratorUtil.dropLast(iter, 4); + + assertEquals(emptyList(), IteratorUtil.toList(dropped)); + } + + @Test + public void testDropLastZero() { + Iterator iter = iterOf("ab", "c"); + + Iterator dropped = IteratorUtil.dropLast(iter, 0); + + assertEquals(listOf("ab", "c"), IteratorUtil.toList(dropped)); + } + + @Test + public void testDropLastNegative() { + Iterator iter = iterOf("ab", "c"); + + Assert.assertThrows(IllegalArgumentException.class, () -> IteratorUtil.dropLast(iter, -3)); + } + private void assertExhausted(Iterator mapped) { assertFalse(mapped.hasNext()); exception.expect(NoSuchElementException.class); diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/lang/ast/internal/NodeStreamBlanketTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/lang/ast/internal/NodeStreamBlanketTest.java index 706ad89042..72fb54767e 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/lang/ast/internal/NodeStreamBlanketTest.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/lang/ast/internal/NodeStreamBlanketTest.java @@ -129,6 +129,17 @@ public class NodeStreamBlanketTest { ); } + @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 { return ts.subList(1, ts.size()); } + static List init(List ts) { + return ts.subList(0, ts.size() - 1); + } + static class Prop { final Predicate property; diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/OnlyOneReturnRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/OnlyOneReturnRule.java index 3421f616ce..94035235d7 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/OnlyOneReturnRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/OnlyOneReturnRule.java @@ -8,27 +8,24 @@ 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.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()) { + if (node.getBody() == null) { return data; } + node.getBody().descendants(ASTReturnStatement.class) + List returnNodes = node.findDescendantsOfType(ASTReturnStatement.class); if (returnNodes.size() > 1) { diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/table/internal/SymbolTableResolver.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/table/internal/SymbolTableResolver.java index 9b7d09cefa..51a62119f2 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/table/internal/SymbolTableResolver.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/table/internal/SymbolTableResolver.java @@ -219,7 +219,7 @@ public final class SymbolTableResolver { int pushed = pushOnStack(f.selfType(top(), node.getTypeMirror())); pushed += pushOnStack(f.typeHeader(top(), node.getSymbol())); - NodeStream notBody = node.children().drop(1).take(node.getNumChildren() - 2); + NodeStream notBody = node.children().drop(1).dropLast(1); for (JavaNode it : notBody) { setTopSymbolTable(it); } From 79ccde716a30e3b3ff7ced953097bd32dbb9db2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Sun, 4 Apr 2021 17:49:45 +0200 Subject: [PATCH 2/3] Update OnlyOneReturn rule --- .../rule/codestyle/OnlyOneReturnRule.java | 25 +++++---------- .../rule/codestyle/OnlyOneReturnTest.java | 1 - .../java/rule/codestyle/xml/OnlyOneReturn.xml | 31 +++++++------------ 3 files changed, 18 insertions(+), 39 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/OnlyOneReturnRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/OnlyOneReturnRule.java index 94035235d7..8aca730dd1 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/OnlyOneReturnRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/OnlyOneReturnRule.java @@ -4,10 +4,7 @@ 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.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.AbstractJavaRulechainRule; @@ -21,23 +18,15 @@ public class OnlyOneReturnRule extends AbstractJavaRulechainRule { @Override public Object visit(ASTMethodDeclaration node, Object data) { if (node.getBody() == null) { - return data; + return null; } - node.getBody().descendants(ASTReturnStatement.class) + NodeStream returnsExceptLast = + node.getBody().descendants(ASTReturnStatement.class).dropLast(1); - List returnNodes = node.findDescendantsOfType(ASTReturnStatement.class); - - if (returnNodes.size() > 1) { - for (Iterator 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; } } diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/codestyle/OnlyOneReturnTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/codestyle/OnlyOneReturnTest.java index 216bc3a12c..d656015f72 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/codestyle/OnlyOneReturnTest.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/codestyle/OnlyOneReturnTest.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 OnlyOneReturnTest extends PmdRuleTst { // no additional unit tests } diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/OnlyOneReturn.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/OnlyOneReturn.xml index 41aa08a7ee..f9cf2afe7b 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/OnlyOneReturn.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/OnlyOneReturn.xml @@ -80,9 +80,10 @@ public class Foo { 0 - #1353 False positive "Only One Return" with lambda + #1353 return inside lambda 0 search(final String indexName, final SearchRequest searchRequest) { - final SearchHit[] empty = new SearchHit[0]; - final Optional searchDefinition = settingsService.getSearchDefinition(indexName); - return searchDefinition.>map( - i -> { - final List> 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; } } ]]> From 52f62698f94a0f8eda8d961592578ef04359d257 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Mon, 19 Apr 2021 17:33:43 +0200 Subject: [PATCH 3/3] Update ci file --- .ci/files/all-java.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.ci/files/all-java.xml b/.ci/files/all-java.xml index 9a2ae34ab4..2e60e8c6ae 100644 --- a/.ci/files/all-java.xml +++ b/.ci/files/all-java.xml @@ -96,7 +96,7 @@ - +