From e726b4e55f697ae4d72163056bd72b32a5b058d5 Mon Sep 17 00:00:00 2001 From: Henry Coles Date: Tue, 12 Oct 2021 11:57:47 +0100 Subject: [PATCH] Mutate stream returns to Stream.empty Previously stream returns were mutated to null. This was inconsistent and resulted in less stable mutations. Special handling was also needed to filter out null returns used only in flatMaps, as these acted like stream.empty. Mutating to Stream.empty is simpler and results in more useful mutations that are harder to detect. --- README.md | 1 + .../EquivalentReturnMutationFilter.java | 1 + .../equivalent/NullFlatMapFilter.java | 187 ------------------ .../equivalent/NullFlatMapFilterFactory.java | 26 --- ...ationtest.build.MutationInterceptorFactory | 1 - .../build/MutationDiscoveryTest.java | 10 - .../EquivalentReturnMutationFilterTest.java | 12 ++ .../intercept/equivalent/NullFlatmapTest.java | 162 --------------- .../returns/EmptyObjectReturnValsMutator.java | 17 ++ .../mutators/EmptyObjectReturnValsTest.java | 39 ++-- 10 files changed, 58 insertions(+), 398 deletions(-) delete mode 100644 pitest-entry/src/main/java/org/pitest/mutationtest/build/intercept/equivalent/NullFlatMapFilter.java delete mode 100644 pitest-entry/src/main/java/org/pitest/mutationtest/build/intercept/equivalent/NullFlatMapFilterFactory.java delete mode 100644 pitest-entry/src/test/java/org/pitest/mutationtest/build/intercept/equivalent/NullFlatmapTest.java diff --git a/README.md b/README.md index 990ef6f5d..aae1010b9 100644 --- a/README.md +++ b/README.md @@ -11,6 +11,7 @@ Read all about it at http://pitest.org ### 1.7.2 (unreleased) * #943 Change default mutators - replace negate conditional with remove conditional +* #946 Mutate stream returns to empty stream instead of null ### 1.7.1 diff --git a/pitest-entry/src/main/java/org/pitest/mutationtest/build/intercept/equivalent/EquivalentReturnMutationFilter.java b/pitest-entry/src/main/java/org/pitest/mutationtest/build/intercept/equivalent/EquivalentReturnMutationFilter.java index b4eecedd2..5a424808e 100644 --- a/pitest-entry/src/main/java/org/pitest/mutationtest/build/intercept/equivalent/EquivalentReturnMutationFilter.java +++ b/pitest-entry/src/main/java/org/pitest/mutationtest/build/intercept/equivalent/EquivalentReturnMutationFilter.java @@ -266,6 +266,7 @@ public boolean test(MutationDetails a) { return returnsZeroValue(method, mutatedInstruction) || returnsEmptyString(method, mutatedInstruction) || returns(method, mutatedInstruction, "java/util/Optional","empty") + || returns(method, mutatedInstruction, "java/util/stream/Stream","empty") || returns(method, mutatedInstruction, "java/util/Collections","emptyList") || returns(method, mutatedInstruction, "java/util/Collections","emptySet") || returns(method, mutatedInstruction, "java/util/List","of") diff --git a/pitest-entry/src/main/java/org/pitest/mutationtest/build/intercept/equivalent/NullFlatMapFilter.java b/pitest-entry/src/main/java/org/pitest/mutationtest/build/intercept/equivalent/NullFlatMapFilter.java deleted file mode 100644 index 70f93694a..000000000 --- a/pitest-entry/src/main/java/org/pitest/mutationtest/build/intercept/equivalent/NullFlatMapFilter.java +++ /dev/null @@ -1,187 +0,0 @@ -package org.pitest.mutationtest.build.intercept.equivalent; - -import org.objectweb.asm.Handle; -import org.objectweb.asm.Opcodes; -import org.objectweb.asm.tree.AbstractInsnNode; -import org.objectweb.asm.tree.InvokeDynamicInsnNode; -import org.objectweb.asm.tree.LabelNode; -import org.objectweb.asm.tree.MethodInsnNode; -import org.pitest.bytecode.analysis.ClassTree; -import org.pitest.bytecode.analysis.MethodTree; -import org.pitest.classinfo.ClassName; -import org.pitest.mutationtest.build.InterceptorType; -import org.pitest.mutationtest.build.MutationInterceptor; -import org.pitest.mutationtest.engine.Location; -import org.pitest.mutationtest.engine.Mutater; -import org.pitest.mutationtest.engine.MutationDetails; -import org.pitest.mutationtest.engine.gregor.mutators.returns.NullReturnValsMutator; -import org.pitest.sequence.Context; -import org.pitest.sequence.Match; -import org.pitest.sequence.QueryParams; -import org.pitest.sequence.QueryStart; -import org.pitest.sequence.SequenceMatcher; -import org.pitest.sequence.Slot; -import org.pitest.sequence.SlotRead; - -import java.util.Arrays; -import java.util.Collection; -import java.util.HashMap; -import java.util.Map; -import java.util.function.Predicate; -import java.util.stream.Collectors; -import java.util.stream.Stream; - -import static org.pitest.bytecode.analysis.InstructionMatchers.anyInstruction; -import static org.pitest.bytecode.analysis.InstructionMatchers.isA; -import static org.pitest.bytecode.analysis.InstructionMatchers.isInstruction; -import static org.pitest.bytecode.analysis.InstructionMatchers.methodCallTo; -import static org.pitest.bytecode.analysis.InstructionMatchers.notAnInstruction; -import static org.pitest.bytecode.analysis.InstructionMatchers.opCode; - -/** - * Filters out mutants of the form - * private aMethod() { - * .. clever logic .. - * return Stream.empty() -> return null - * } - * - * Iff the method is only ever called from flatMap. - * - * Flat map treats null and Stream.empty as equivalent, but forcing the programmer - * to return null if they wish to kill the mutant would be controversial at best. - */ -public class NullFlatMapFilter implements MutationInterceptor { - - private static final boolean DEBUG = false; - private static final Slot MUTATED_INSTRUCTION = Slot.create(AbstractInsnNode.class); - static final SequenceMatcher RETURN_EMPTY_STREAM = QueryStart - .any(AbstractInsnNode.class) - .then(methodCallTo(ClassName.fromClass(Stream.class), "empty")) - .then(opCode(Opcodes.ARETURN).and(isInstruction(MUTATED_INSTRUCTION.read()))) - .zeroOrMore(QueryStart.match(anyInstruction())) - .compile(QueryParams.params(AbstractInsnNode.class) - .withIgnores(notAnInstruction()) - .withDebug(DEBUG) - ); - - private static final Slot METHOD_DESC = Slot.create(Location.class); - static final SequenceMatcher HAS_FLAT_MAP_CALL = QueryStart - .any(AbstractInsnNode.class) - .then(dynamicCallTo(METHOD_DESC.read())) - .then(methodCallTo(ClassName.fromClass(Stream.class), "flatMap")) - .zeroOrMore(QueryStart.match(anyInstruction())) - .compile(QueryParams.params(AbstractInsnNode.class) - .withIgnores(notAnInstruction().or(isA(LabelNode.class))) - .withDebug(DEBUG) - ); - - private ClassTree currentClass; - private Map calledOnlyByFlatMap; - - @Override - public InterceptorType type() { - return InterceptorType.FILTER; - } - - @Override - public void begin(ClassTree clazz) { - currentClass = clazz; - calledOnlyByFlatMap = new HashMap<>(); - } - - @Override - public Collection intercept(Collection mutations, Mutater unused) { - return mutations.stream() - .filter(m -> !this.isStreamEmptyMutantWithOnlyFlatMapCalls(m)) - .collect(Collectors.toList()); - } - - private boolean isStreamEmptyMutantWithOnlyFlatMapCalls(MutationDetails mutationDetails) { - if (!mutationDetails.getMutator().equals(NullReturnValsMutator.NULL_RETURNS.getGloballyUniqueId())) { - return false; - } - - MethodTree method = currentClass.method(mutationDetails.getId().getLocation()).get(); - if (!method.isPrivate() || !method.returns(ClassName.fromClass(Stream.class))) { - return false; - } - - final Context context = Context.start(method.instructions(), DEBUG); - context.store(MUTATED_INSTRUCTION.write(), method.instruction(mutationDetails.getInstructionIndex())); - return RETURN_EMPTY_STREAM.matches(method.instructions(), context) - && calledOnlyByFlatMap.computeIfAbsent(mutationDetails.getId().getLocation(), this::calledOnlyFromFlatMap); - } - - private boolean calledOnlyFromFlatMap(Location location) { - MethodTree mutated = currentClass.method(location).get(); - - boolean flatMapCallFound = false; - for (MethodTree each : currentClass.methods()) { - if (callsTarget(mutated, each)) { - flatMapCallFound = isFlatMapCall(mutated, each); - if (!flatMapCallFound) { - return false; - } - } - } - - return flatMapCallFound; - - } - - private boolean callsTarget(MethodTree target, MethodTree method) { - return method.instructions().stream() - .anyMatch(callTo(target.asLocation().getClassName(), target.asLocation().getMethodName(), target.asLocation().getMethodDesc()) - .or(dynamicCallTo(target.asLocation()))); - } - - private Predicate callTo(ClassName className, String name, String desc) { - return n -> { - if (n instanceof MethodInsnNode) { - MethodInsnNode call = (MethodInsnNode) n; - return call.owner.equals(className.asInternalName()) && call.name.equals(name) && call.desc.equals(desc); - } - return false; - }; - } - - private boolean isFlatMapCall(MethodTree mutated, MethodTree each) { - final Context context = Context.start(each.instructions(), DEBUG); - context.store(METHOD_DESC.write(), mutated.asLocation()); - return HAS_FLAT_MAP_CALL.matches(each.instructions(), context); - } - - - private static Match dynamicCallTo(SlotRead desc) { - return (c, t) -> dynamicCallTo(c.retrieve(desc).get()).test(t); - } - - private static Predicate dynamicCallTo(Location desc) { - return (t) -> { - if ( t instanceof InvokeDynamicInsnNode) { - InvokeDynamicInsnNode call = (InvokeDynamicInsnNode) t; - return Arrays.stream(call.bsmArgs) - .anyMatch(isHandle(desc.getClassName(), desc.getMethodName(), desc.getMethodDesc())); - } - return false; - }; - } - - private static Predicate isHandle(ClassName owner, String name, String desc) { - return o -> { - if (o instanceof Handle) { - Handle handle = (Handle) o; - return handle.getOwner().equals(owner.asInternalName()) - && handle.getName().equals(name) - && handle.getDesc().equals(desc); - } - return false; - }; - } - - @Override - public void end() { - currentClass = null; - calledOnlyByFlatMap = null; - } -} diff --git a/pitest-entry/src/main/java/org/pitest/mutationtest/build/intercept/equivalent/NullFlatMapFilterFactory.java b/pitest-entry/src/main/java/org/pitest/mutationtest/build/intercept/equivalent/NullFlatMapFilterFactory.java deleted file mode 100644 index ac767dea9..000000000 --- a/pitest-entry/src/main/java/org/pitest/mutationtest/build/intercept/equivalent/NullFlatMapFilterFactory.java +++ /dev/null @@ -1,26 +0,0 @@ -package org.pitest.mutationtest.build.intercept.equivalent; - -import org.pitest.mutationtest.build.InterceptorParameters; -import org.pitest.mutationtest.build.MutationInterceptor; -import org.pitest.mutationtest.build.MutationInterceptorFactory; -import org.pitest.plugin.Feature; - -public class NullFlatMapFilterFactory implements MutationInterceptorFactory { - - @Override - public MutationInterceptor createInterceptor(InterceptorParameters params) { - return new NullFlatMapFilter(); - } - - @Override - public Feature provides() { - return Feature.named("FNULLSTREAM") - .withOnByDefault(true) - .withDescription("Filters equivalent mutations where null is passed to flatmap in place of Stream.empty"); - } - - @Override - public String description() { - return "Null stream flatmap filter"; - } -} diff --git a/pitest-entry/src/main/resources/META-INF/services/org.pitest.mutationtest.build.MutationInterceptorFactory b/pitest-entry/src/main/resources/META-INF/services/org.pitest.mutationtest.build.MutationInterceptorFactory index 6c9182e42..93a0e7178 100755 --- a/pitest-entry/src/main/resources/META-INF/services/org.pitest.mutationtest.build.MutationInterceptorFactory +++ b/pitest-entry/src/main/resources/META-INF/services/org.pitest.mutationtest.build.MutationInterceptorFactory @@ -17,6 +17,5 @@ org.pitest.mutationtest.build.intercept.kotlin.KotlinFilterFactory org.pitest.mutationtest.filter.LimitNumberOfMutationsPerClassFilterFactory org.pitest.mutationtest.build.intercept.equivalent.EqualsPerformanceShortcutFilterFactory org.pitest.mutationtest.build.intercept.equivalent.EquivalentReturnMutationFilter -org.pitest.mutationtest.build.intercept.equivalent.NullFlatMapFilterFactory org.pitest.plugin.export.MutantExportFactory diff --git a/pitest-entry/src/test/java/org/pitest/mutationtest/build/MutationDiscoveryTest.java b/pitest-entry/src/test/java/org/pitest/mutationtest/build/MutationDiscoveryTest.java index 7c0189854..063458e5b 100755 --- a/pitest-entry/src/test/java/org/pitest/mutationtest/build/MutationDiscoveryTest.java +++ b/pitest-entry/src/test/java/org/pitest/mutationtest/build/MutationDiscoveryTest.java @@ -212,16 +212,6 @@ public void filtersEquivalentReturnValsMutants() { assertThat(actual).isEmpty(); } - @Test - public void filtersEquivalentNullStreamMutantsUsedInFlatMapCalls() { - final Collection actual = findMutants(HasPrivateStreamMethodUsedOnlyInSingleFlatMap.class); - - this.data.setFeatures(Collections.singletonList("-FNULLSTREAM")); - final Collection actualWithoutFilter = findMutants(HasPrivateStreamMethodUsedOnlyInSingleFlatMap.class); - - assertThat(actual.size()).isLessThan(actualWithoutFilter.size()); - } - @Test public void filterMutantsInJavaRecords() { this.data.setDetectInlinedCode(true); diff --git a/pitest-entry/src/test/java/org/pitest/mutationtest/build/intercept/equivalent/EquivalentReturnMutationFilterTest.java b/pitest-entry/src/test/java/org/pitest/mutationtest/build/intercept/equivalent/EquivalentReturnMutationFilterTest.java index 54ee93d37..224a43d72 100644 --- a/pitest-entry/src/test/java/org/pitest/mutationtest/build/intercept/equivalent/EquivalentReturnMutationFilterTest.java +++ b/pitest-entry/src/test/java/org/pitest/mutationtest/build/intercept/equivalent/EquivalentReturnMutationFilterTest.java @@ -13,6 +13,7 @@ import java.util.List; import java.util.Optional; import java.util.Set; +import java.util.stream.Stream; import static org.assertj.core.api.Assertions.assertThat; import static org.pitest.mutationtest.engine.gregor.mutators.returns.BooleanFalseReturnValsMutator.FALSE_RETURNS; @@ -157,6 +158,11 @@ public void filtersEquivalentSetMutants() { public void filtersEquivalentOptionalMutants() { verifier.assertFiltersNMutationFromClass(1, AlreadyReturnsEmptyOptional.class); } + + @Test + public void filtersEquivalentStreamMutants() { + verifier.assertFiltersNMutationFromClass(1, AlreadyReturnsEmptyStream.class); + } } class Widget{} @@ -334,3 +340,9 @@ public Optional a() { return Optional.empty(); } } + +class AlreadyReturnsEmptyStream { + public Stream a() { + return Stream.empty(); + } +} \ No newline at end of file diff --git a/pitest-entry/src/test/java/org/pitest/mutationtest/build/intercept/equivalent/NullFlatmapTest.java b/pitest-entry/src/test/java/org/pitest/mutationtest/build/intercept/equivalent/NullFlatmapTest.java deleted file mode 100644 index 9859ee833..000000000 --- a/pitest-entry/src/test/java/org/pitest/mutationtest/build/intercept/equivalent/NullFlatmapTest.java +++ /dev/null @@ -1,162 +0,0 @@ -package org.pitest.mutationtest.build.intercept.equivalent; - -import org.junit.Test; -import org.pitest.mutationtest.build.InterceptorType; -import org.pitest.mutationtest.build.MutationInterceptor; -import org.pitest.mutationtest.build.intercept.javafeatures.FilterTester; -import org.pitest.mutationtest.engine.gregor.mutators.NullMutateEverything; -import org.pitest.mutationtest.engine.gregor.mutators.returns.NullReturnValsMutator; - -import java.util.List; -import java.util.stream.Stream; - -import static org.assertj.core.api.Assertions.assertThat; - -public class NullFlatmapTest { - - MutationInterceptor testee = new NullFlatMapFilterFactory().createInterceptor(null); - - FilterTester verifier = new FilterTester("", this.testee, NullReturnValsMutator.NULL_RETURNS, - new NullMutateEverything()); - - @Test - public void declaresTypeAsFilter() { - assertThat(this.testee.type()).isEqualTo(InterceptorType.FILTER); - } - - @Test - public void filtersNullReturnMutantWhenMethodUsedOnlyInFlatMap() { // - verifier.assertFiltersNMutationFromClass(1, HasPrivateStreamMethodUsedOnlyInSingleFlatMap.class); - } - - @Test - public void doesNotFilterNullReturnsInPublicStreamMethods() { - verifier.assertFiltersNMutationFromClass(0, HasPublicStreamMethodUsedOnlyInSingleFlatMap.class); - } - - @Test - public void doesNotFilterOtherMutantsInNullReturnsStreamMethods() { - verifier.assertFiltersNMutationFromClass(1, HasPrivateStreamMethodUsedOnlyInSingleFlatMapThatHasOtherMutableCode.class); - } - - @Test - public void doesNotFilterNullReturnsWhenOriginalNotEmptyStream() { - verifier.assertFiltersNMutationFromClass(0, HasPrivateStreamMethodThatDoesNotReturnEmpty.class); - } - - @Test - public void doesNotFilterNullReturnsWhenNoFlatMapCallsExist() { - verifier.assertFiltersNMutationFromClass(0, NotCalledFromFlatMap.class); - } - - @Test - public void doesNotFilterNullReturnsWhenNonFlatMapCallsExist() { - verifier.assertFiltersNMutationFromClass(0, HasPrivateStreamMethodCalledNotFromFlatMap.class); - } - - @Test - public void doesNotFilterEligibleMutantsInMethodsWithSameName() { - verifier.assertFiltersNMutationFromClass(1, HasMutableMethodWithSameNameButDifferentSignature.class); - } -} - -class HasPrivateStreamMethodUsedOnlyInSingleFlatMap { - public Stream makesCall(List l) { - return l.stream() - .flatMap(this::aStream); - - } - - private Stream aStream(String l) { - return Stream.empty(); - } -} - -class HasPrivateStreamMethodUsedOnlyInSingleFlatMapThatHasOtherMutableCode { - public Stream makesCall(List l) { - return l.stream() - .flatMap(this::aStream); - - } - - private Stream aStream(String l) { - System.out.println("Keep mutating me"); - return Stream.empty(); - } -} - -class NotCalledFromFlatMap { - public Stream makesCall() { - return aStream("") - .map(s -> s + "boo"); - } - - private Stream aStream(String l) { - return Stream.empty(); - } -} - -class HasPrivateStreamMethodCalledNotFromFlatMap { - public Stream makesCall(List l) { - return l.stream() - .flatMap(this::aStream); - - } - - public Stream alsoMakesCall() { - return aStream("") - .map(s -> s + "boo"); - } - - private Stream aStream(String l) { - return Stream.empty(); - } -} - -class HasPrivateStreamMethodThatDoesNotReturnEmpty { - public Stream makesCall(List l) { - return l.stream() - .flatMap(this::aStream); - - } - - private Stream aStream(String l) { - return Stream.of(""); - } -} - - -class HasPublicStreamMethodUsedOnlyInSingleFlatMap { - public Stream makesCall(List l) { - return l.stream() - .flatMap(this::aStream); - - } - - public Stream aStream(String l) { - return Stream.empty(); - } -} - -class HasMutableMethodWithSameNameButDifferentSignature { - public Stream makesCall(List l) { - return l.stream() - .flatMap(this::aStream); - - } - - public Stream makesCall(List l, int unused) { - return l.stream() - .map(this::aStream); - - } - - private Stream aStream(String l) { - return Stream.empty(); - } - - // can mutate this one - private Stream aStream(int i) { - return Stream.empty(); - } -} \ No newline at end of file diff --git a/pitest/src/main/java/org/pitest/mutationtest/engine/gregor/mutators/returns/EmptyObjectReturnValsMutator.java b/pitest/src/main/java/org/pitest/mutationtest/engine/gregor/mutators/returns/EmptyObjectReturnValsMutator.java index 227ef4808..2dba32f02 100644 --- a/pitest/src/main/java/org/pitest/mutationtest/engine/gregor/mutators/returns/EmptyObjectReturnValsMutator.java +++ b/pitest/src/main/java/org/pitest/mutationtest/engine/gregor/mutators/returns/EmptyObjectReturnValsMutator.java @@ -67,6 +67,7 @@ class AReturnMethodVisitor extends AbstractInsnMutator { NON_NULL_MUTATIONS.put("java.lang.Double", returnDoubleZero()); NON_NULL_MUTATIONS.put("java.lang.String", returnEmptyString()); NON_NULL_MUTATIONS.put("java.util.Optional", returnEmptyOptional()); + NON_NULL_MUTATIONS.put("java.util.stream.Stream", returnEmptyStream()); NON_NULL_MUTATIONS.put("java.util.List", returnEmptyList()); NON_NULL_MUTATIONS.put("java.util.Set", returnEmptySet()); NON_NULL_MUTATIONS.put("java.util.Collection", returnEmptyList()); @@ -234,4 +235,20 @@ public String describe(final int opCode, final MethodInfo methodInfo) { } }; } + + private static ZeroOperandMutation returnEmptyStream() { + return new ZeroOperandMutation() { + @Override + public void apply(final int opCode, final MethodVisitor mv) { + mv.visitInsn(Opcodes.POP); + mv.visitMethodInsn(Opcodes.INVOKESTATIC, "java/util/stream/Stream", "empty", "()Ljava/util/stream/Stream;", true); + mv.visitInsn(Opcodes.ARETURN); + } + + @Override + public String describe(final int opCode, final MethodInfo methodInfo) { + return "replaced return value with Stream.empty for " + methodInfo.getDescription(); + } + }; + } } diff --git a/pitest/src/test/java/org/pitest/mutationtest/engine/gregor/mutators/EmptyObjectReturnValsTest.java b/pitest/src/test/java/org/pitest/mutationtest/engine/gregor/mutators/EmptyObjectReturnValsTest.java index 7dc073a93..06e5104bd 100644 --- a/pitest/src/test/java/org/pitest/mutationtest/engine/gregor/mutators/EmptyObjectReturnValsTest.java +++ b/pitest/src/test/java/org/pitest/mutationtest/engine/gregor/mutators/EmptyObjectReturnValsTest.java @@ -9,8 +9,10 @@ import java.util.Collection; import java.util.Collections; import java.util.List; +import java.util.Optional; import java.util.Set; import java.util.concurrent.Callable; +import java.util.stream.Stream; import static org.assertj.core.api.Assertions.assertThat; @@ -141,12 +143,19 @@ public void mutatesCollectionsToEmptyList() throws Exception { createFirstMutant(ACollection.class), Collections.emptyList()); } -// must build on java 7 -// @Test -// public void mutatesToOptionalEmpty() throws Exception { -// assertMutantCallableReturns(new AnOptional(), -// createFirstMutant(AnOptional.class), Optional.empty()); -// } + + @Test + public void mutatesToOptionalEmpty() throws Exception { + assertMutantCallableReturns(new AnOptional(), + createFirstMutant(AnOptional.class), Optional.empty()); + } + + @Test + public void mutatesToEmptyStream() { + Stream actual = mutateAndCall(new AStream(), + createFirstMutant(AStream.class)); + assertThat(actual).isEmpty(); + } private static class ObjectReturn implements Callable { @Override @@ -240,12 +249,18 @@ public Collection call() throws Exception { } } -// private static class AnOptional implements Callable> { -// @Override -// public Optional call() throws Exception { -// return Optional.of("hello"); -// } -// } + private static class AnOptional implements Callable> { + @Override + public Optional call() throws Exception { + return Optional.of("hello"); + } + } + private static class AStream implements Callable> { + @Override + public Stream call() throws Exception { + return Stream.of("hello"); + } + } } \ No newline at end of file