From 6697597278e21365aacc3b25648459d706cbd738 Mon Sep 17 00:00:00 2001 From: Henry Coles Date: Thu, 9 Jun 2022 13:08:37 +0100 Subject: [PATCH] move assert filtering to interceptor --- .../intercept/javafeatures/AssertFilter.java | 63 +++++++++ .../javafeatures/AssertionsFilterFactory.java | 27 ++++ ...ationtest.build.MutationInterceptorFactory | 1 + .../javafeatures/AssertionsTest.java | 132 ++++++++++++++++++ .../verifier/interceptors/Verifier.java | 15 +- .../gregor/AvoidAssertsMethodAdapter.java | 87 ------------ .../engine/gregor/MutatingClassVisitor.java | 7 +- .../gregor/AvoidAssertsMethodAdapterTest.java | 123 ---------------- .../engine/gregor/TestGregorMutater.java | 30 ---- 9 files changed, 237 insertions(+), 248 deletions(-) create mode 100644 pitest-entry/src/main/java/org/pitest/mutationtest/build/intercept/javafeatures/AssertFilter.java create mode 100644 pitest-entry/src/main/java/org/pitest/mutationtest/build/intercept/javafeatures/AssertionsFilterFactory.java create mode 100644 pitest-entry/src/test/java/org/pitest/mutationtest/build/intercept/javafeatures/AssertionsTest.java delete mode 100644 pitest/src/main/java/org/pitest/mutationtest/engine/gregor/AvoidAssertsMethodAdapter.java delete mode 100644 pitest/src/test/java/org/pitest/mutationtest/engine/gregor/AvoidAssertsMethodAdapterTest.java diff --git a/pitest-entry/src/main/java/org/pitest/mutationtest/build/intercept/javafeatures/AssertFilter.java b/pitest-entry/src/main/java/org/pitest/mutationtest/build/intercept/javafeatures/AssertFilter.java new file mode 100644 index 000000000..63a6b404a --- /dev/null +++ b/pitest-entry/src/main/java/org/pitest/mutationtest/build/intercept/javafeatures/AssertFilter.java @@ -0,0 +1,63 @@ +package org.pitest.mutationtest.build.intercept.javafeatures; + +import org.objectweb.asm.tree.AbstractInsnNode; +import org.objectweb.asm.tree.FieldInsnNode; +import org.objectweb.asm.tree.LabelNode; +import org.pitest.bytecode.analysis.MethodTree; +import org.pitest.mutationtest.build.intercept.Region; +import org.pitest.mutationtest.build.intercept.RegionInterceptor; +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.SlotWrite; + +import java.util.List; +import java.util.stream.Collectors; + +import static org.pitest.bytecode.analysis.InstructionMatchers.anyInstruction; +import static org.pitest.bytecode.analysis.InstructionMatchers.jumpsTo; +import static org.pitest.bytecode.analysis.InstructionMatchers.notAnInstruction; +import static org.pitest.bytecode.analysis.OpcodeMatchers.IFNE; +import static org.pitest.sequence.Result.result; + + +public class AssertFilter extends RegionInterceptor { + + static final Slot START = Slot.create(AbstractInsnNode.class); + static final Slot END = Slot.create(LabelNode.class); + + static final SequenceMatcher ASSERT_GET = QueryStart + .any(AbstractInsnNode.class) + .then(getStatic("$assertionsDisabled").and(store(START.write()))) + .then(IFNE.and(jumpsTo(END.write()))) + .zeroOrMore(QueryStart.match(anyInstruction())) + .compile(QueryParams.params(AbstractInsnNode.class) + .withIgnores(notAnInstruction()) + ); + + private static Match getStatic(String name) { + return (c,n) -> { + if (n instanceof FieldInsnNode) { +return result(((FieldInsnNode) n).name.equals(name), c); + } + return result(false,c); + }; + } + + + private static Match store(SlotWrite slot) { + return (c,n) -> result(true, c.store(slot, n)); + } + + protected List computeRegions(MethodTree method) { + Context context = Context.start(); + List regions = ASSERT_GET.contextMatches(method.instructions(), context).stream() + .map(c -> new Region(c.retrieve(START.read()).get(), c.retrieve(END.read()).get())) + .collect(Collectors.toList()); + return regions; + } + +} diff --git a/pitest-entry/src/main/java/org/pitest/mutationtest/build/intercept/javafeatures/AssertionsFilterFactory.java b/pitest-entry/src/main/java/org/pitest/mutationtest/build/intercept/javafeatures/AssertionsFilterFactory.java new file mode 100644 index 000000000..826896dfe --- /dev/null +++ b/pitest-entry/src/main/java/org/pitest/mutationtest/build/intercept/javafeatures/AssertionsFilterFactory.java @@ -0,0 +1,27 @@ +package org.pitest.mutationtest.build.intercept.javafeatures; + +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 AssertionsFilterFactory implements MutationInterceptorFactory { + + @Override + public String description() { + return "Assertions filter"; + } + + @Override + public MutationInterceptor createInterceptor(InterceptorParameters params) { + return new AssertFilter(); + } + + @Override + public Feature provides() { + return Feature.named("FASSERT") + .withOnByDefault(true) + .withDescription("Filters mutations in compiler generated code for assertions"); + } + +} 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 21d9928a4..64827def5 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 @@ -10,6 +10,7 @@ org.pitest.mutationtest.build.intercept.javafeatures.ForEachLoopFilterFactory org.pitest.mutationtest.build.intercept.javafeatures.EnumConstructorFilterFactory org.pitest.mutationtest.build.intercept.javafeatures.RecordFilterFactory org.pitest.mutationtest.build.intercept.javafeatures.StringSwitchFilterFactory +org.pitest.mutationtest.build.intercept.javafeatures.AssertionsFilterFactory org.pitest.mutationtest.build.intercept.logging.LoggingCallsFilterFactory org.pitest.mutationtest.build.intercept.timeout.InfiniteForLoopFilterFactory org.pitest.mutationtest.build.intercept.timeout.InfiniteIteratorLoopFilterFactory diff --git a/pitest-entry/src/test/java/org/pitest/mutationtest/build/intercept/javafeatures/AssertionsTest.java b/pitest-entry/src/test/java/org/pitest/mutationtest/build/intercept/javafeatures/AssertionsTest.java new file mode 100644 index 000000000..b91bc460d --- /dev/null +++ b/pitest-entry/src/test/java/org/pitest/mutationtest/build/intercept/javafeatures/AssertionsTest.java @@ -0,0 +1,132 @@ +package org.pitest.mutationtest.build.intercept.javafeatures; + +import org.junit.Test; +import org.objectweb.asm.tree.AbstractInsnNode; +import org.objectweb.asm.tree.FieldInsnNode; +import org.objectweb.asm.tree.JumpInsnNode; +import org.pitest.mutationtest.engine.gregor.mutators.NullMutateEverything; +import org.pitest.verifier.interceptors.InterceptorVerifier; +import org.pitest.verifier.interceptors.VerifierStart; + +import java.util.function.Predicate; + +import static org.pitest.bytecode.analysis.InstructionMatchers.isA; +import static org.pitest.bytecode.analysis.OpcodeMatchers.ATHROW; + +public class AssertionsTest { + AssertionsFilterFactory underTest = new AssertionsFilterFactory(); + InterceptorVerifier v = VerifierStart.forInterceptorFactory(underTest) + .usingMutator(new NullMutateEverything()); + + @Test + public void doesNotFilterCodeWithNoAsserts() { + v.forClass(NoAsserts.class) + .forAnyCode() + .mutantsAreGenerated() + .noMutantsAreFiltered() + .verify(); + } + + @Test + public void filtersAssertCode() { + v.forClass(HasAssertStatement.class) + .forMethod("foo") + .forCodeMatching(assertInstructions()) + .mutantsAreGenerated() + .allMutantsAreFiltered() + .verify(); + } + + @Test + public void filtersAssertCodeWhenOtherStatementsPresent() { + v.forClass(HasAssertStatementAndOtherStatements.class) + .forMethod("foo") + .forCodeMatching(sysoutFieldReference()) + .mutantsAreGenerated() + .noMutantsAreFiltered() + .verify(); + } + + @Test + public void leavesCodeNotInAssert() { + v.forClass(HasAssertStatementAndOtherStatements.class) + .forMethod("foo") + .forCodeMatching(sysoutFieldReference()) + .mutantsAreGenerated() + .noMutantsAreFiltered() + .verify(); + } + + @Test + public void filtersMultipleAsserts() { + v.forClass(MultipleAsserts.class) + .forMethod("foo") + .forCodeMatching(assertInstructions()) + .mutantsAreGenerated() + .allMutantsAreFiltered() + .verify(); + } + + @Test + public void leavesCodeNotInAssertWhenMultipleAsserts() { + v.forClass(MultipleAsserts.class) + .forMethod("foo") + .forCodeMatching(assertInstructions()) + .mutantsAreGenerated() + .allMutantsAreFiltered() + .verify(); + } + + private Predicate assertInstructions() { + return isA(JumpInsnNode.class).or(ATHROW).asPredicate().or(fieldAccess()); + } + + private Predicate fieldAccess() { + return isA(FieldInsnNode.class).asPredicate().and(sysoutFieldReference().negate()); + } + + private Predicate sysoutFieldReference() { + return n -> n instanceof FieldInsnNode && ((FieldInsnNode) n).owner.equals("java/lang/System"); + } + + static class HasAssertStatement { + public void foo(final int i) { + assert ((i + 20) > 10); + } + } + + static class HasAssertStatementAndOtherStatements { + public int state; + + public void foo(final int i) { + System.out.println("before"); + assert ((i + 20) > 10); + System.out.println("after"); + } + } + + static class NoAsserts { + static boolean aField = true; + public void foo(final int i) { + if (aField) { + throw new AssertionError(); + } + if (i > 10) { + throw new AssertionError(); + } + } + } + + static class MultipleAsserts { + public int state; + + public void foo(final int i) { + assert (i != 11); + System.out.println("before"); + assert ((i + 20) > 10); + System.out.println("middle"); + assert (i != 10); + System.out.println("after"); + } + } +} diff --git a/pitest-entry/src/test/java/org/pitest/verifier/interceptors/Verifier.java b/pitest-entry/src/test/java/org/pitest/verifier/interceptors/Verifier.java index 127cb3f0c..2b79a458c 100644 --- a/pitest-entry/src/test/java/org/pitest/verifier/interceptors/Verifier.java +++ b/pitest-entry/src/test/java/org/pitest/verifier/interceptors/Verifier.java @@ -17,11 +17,22 @@ public class Verifier { private final Sample sample; private final MutationInterceptor interceptor; private final Mutater mutator; + private final Predicate match; + public Verifier(Sample sample, MutationInterceptor interceptor, Mutater m) { + this(sample,interceptor, m, mut -> true); + } + + public Verifier(Sample sample, MutationInterceptor interceptor, Mutater m, Predicate match) { this.sample = sample; this.interceptor = interceptor; this.mutator = m; + this.match = match; + } + + public Verifier forMethod(String name) { + return new Verifier(sample, interceptor, mutator, match.and(m -> m.getMethod().equals(name))); } public MutantVerifier forAnyCode() { @@ -29,7 +40,7 @@ public MutantVerifier forAnyCode() { } public MutantVerifier forCodeMatching(Predicate match) { - return forMutantsMatching(mutates(match, sample)); + return forMutantsMatching(mutates(match, sample).and(this.match)); } public Collection findMutants() { @@ -43,7 +54,7 @@ public MutantVerifier forMutantsMatching(Predicate match) { List filtered = new ArrayList<>(mutations); filtered.removeAll(afterFiltering); - return new MutantVerifier(sample, match, mutations, afterFiltering, filtered); + return new MutantVerifier(sample, this.match.and(match), mutations, afterFiltering, filtered); } private Collection filter(ClassTree clazz, diff --git a/pitest/src/main/java/org/pitest/mutationtest/engine/gregor/AvoidAssertsMethodAdapter.java b/pitest/src/main/java/org/pitest/mutationtest/engine/gregor/AvoidAssertsMethodAdapter.java deleted file mode 100644 index c6b2ec389..000000000 --- a/pitest/src/main/java/org/pitest/mutationtest/engine/gregor/AvoidAssertsMethodAdapter.java +++ /dev/null @@ -1,87 +0,0 @@ -/* - * Copyright 2012 Henry Coles - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and limitations under the License. - */ -package org.pitest.mutationtest.engine.gregor; - -import org.objectweb.asm.Label; -import org.objectweb.asm.MethodVisitor; -import org.objectweb.asm.Opcodes; -import org.pitest.bytecode.ASMVersion; - -/** - * Disables mutations within java assertions - * - */ -public class AvoidAssertsMethodAdapter extends MethodVisitor { - - private static final String DISABLE_REASON = "ASSERTS"; - - private final MutationContext context; - private boolean assertBlockStarted; - private Label destination; - - public AvoidAssertsMethodAdapter(final MutationContext context, - final MethodVisitor delegateMethodVisitor) { - super(ASMVersion.ASM_VERSION, delegateMethodVisitor); - this.context = context; - } - - @Override - public void visitMethodInsn(final int opcode, final String owner, - final String name, final String desc, boolean itf) { - - if ((opcode == Opcodes.INVOKEVIRTUAL) && "java/lang/Class".equals(owner) - && "desiredAssertionStatus".equals(name)) { - this.context.disableMutations(DISABLE_REASON); - } - super.visitMethodInsn(opcode, owner, name, desc, itf); - } - - @Override - public void visitFieldInsn(final int opcode, final String owner, - final String name, final String desc) { - - if ("$assertionsDisabled".equals(name)) { - if (opcode == Opcodes.GETSTATIC) { - this.context.disableMutations(DISABLE_REASON); - this.assertBlockStarted = true; - } else if (opcode == Opcodes.PUTSTATIC) { - this.context.enableMutatations(DISABLE_REASON); - } - } - super.visitFieldInsn(opcode, owner, name, desc); - - } - - @Override - public void visitJumpInsn(final int opcode, final Label destination) { - if ((opcode == Opcodes.IFNE) && this.assertBlockStarted) { - this.destination = destination; - this.assertBlockStarted = false; - } - super.visitJumpInsn(opcode, destination); - - } - - @Override - public void visitLabel(final Label label) { - // delegate to child first to ensure visitLabel not in scope for mutation - super.visitLabel(label); - if (this.destination == label) { - this.context.enableMutatations(DISABLE_REASON); - this.destination = null; - } - } - -} diff --git a/pitest/src/main/java/org/pitest/mutationtest/engine/gregor/MutatingClassVisitor.java b/pitest/src/main/java/org/pitest/mutationtest/engine/gregor/MutatingClassVisitor.java index 75dd77a90..c61f14aff 100644 --- a/pitest/src/main/java/org/pitest/mutationtest/engine/gregor/MutatingClassVisitor.java +++ b/pitest/src/main/java/org/pitest/mutationtest/engine/gregor/MutatingClassVisitor.java @@ -105,7 +105,7 @@ private MethodVisitor visitMethodForMutation( next = each.create(methodContext, methodInfo, next); } - return wrapWithDecorators(methodContext, wrapWithAssertFilter(methodContext, next), methodInfo); + return wrapWithDecorators(methodContext, next, methodInfo); } private static MethodVisitor wrapWithLineTracker( @@ -113,10 +113,5 @@ private static MethodVisitor wrapWithLineTracker( return new LineTrackingMethodVisitor(methodContext, mv); } - private static MethodVisitor wrapWithAssertFilter( - MethodMutationContext methodContext, - final MethodVisitor wrappedMethodVisitor) { - return new AvoidAssertsMethodAdapter(methodContext, wrappedMethodVisitor); - } } diff --git a/pitest/src/test/java/org/pitest/mutationtest/engine/gregor/AvoidAssertsMethodAdapterTest.java b/pitest/src/test/java/org/pitest/mutationtest/engine/gregor/AvoidAssertsMethodAdapterTest.java deleted file mode 100644 index 03cab3b65..000000000 --- a/pitest/src/test/java/org/pitest/mutationtest/engine/gregor/AvoidAssertsMethodAdapterTest.java +++ /dev/null @@ -1,123 +0,0 @@ -package org.pitest.mutationtest.engine.gregor; - -import static org.mockito.ArgumentMatchers.anyString; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.verify; - -import org.junit.Before; -import org.junit.Test; -import org.mockito.Mock; -import org.mockito.Mockito; -import org.objectweb.asm.Label; -import org.objectweb.asm.MethodVisitor; -import org.objectweb.asm.Opcodes; -import org.pitest.bytecode.MethodDecoratorTest; - -public class AvoidAssertsMethodAdapterTest extends MethodDecoratorTest { - - @Mock - private MethodMutationContext context; - - @Mock - private Label label; - - private AvoidAssertsMethodAdapter testee; - - @Override - @Before - public void setUp() { - super.setUp(); - this.testee = new AvoidAssertsMethodAdapter(this.context, this.mv); - this.testee.visitCode(); - } - - @Test - public void shouldDisableMutationsWhenAssertionDisabledFlagIsChecked() { - this.testee.visitFieldInsn(Opcodes.GETSTATIC, "foo", "$assertionsDisabled", - "Z"); - verify(this.context).disableMutations(anyString()); - } - - @Test - public void shouldEnableMutationsWhenReachLabelOfFirstIFNEAfterCheckingAssertionDisabledFlag() { - this.testee.visitFieldInsn(Opcodes.GETSTATIC, "foo", "$assertionsDisabled", - "Z"); - this.testee.visitJumpInsn(Opcodes.IFNE, this.label); - this.testee.visitLabel(this.label); - verify(this.context).enableMutatations(anyString()); - } - - @Test - public void shouldNotEnableMutationsWhenNonAssertionCheckLabelReached() { - final Label anotherLabel = Mockito.mock(Label.class); - this.testee.visitFieldInsn(Opcodes.GETSTATIC, "foo", "$assertionsDisabled", - "Z"); - this.testee.visitJumpInsn(Opcodes.IFNE, this.label); - this.testee.visitLabel(anotherLabel); - verify(this.context, never()).enableMutatations(anyString()); - } - - @Test - public void shouldNotTryToEnableMutationsWhenIFNEInstructionEncounteredWithoutCheckingAssertionDisabledFlag() { - this.testee.visitJumpInsn(Opcodes.IFNE, this.label); - this.testee.visitLabel(this.label); - verify(this.context, never()).enableMutatations(anyString()); - } - - @Test - public void shouldOnlyCaptureFirstIFNEEncountered() { - final Label anotherLabel = Mockito.mock(Label.class); - this.testee.visitFieldInsn(Opcodes.GETSTATIC, "foo", "$assertionsDisabled", - "Z"); - this.testee.visitJumpInsn(Opcodes.IFNE, this.label); - this.testee.visitJumpInsn(Opcodes.IFNE, anotherLabel); - this.testee.visitLabel(this.label); - verify(this.context).enableMutatations(anyString()); - } - - @Test - public void shouldDisableMutationsForCodeSettingWhenAssertionDisabledFlagIsSetInStaticInitializer() { - this.testee.visitMethodInsn(Opcodes.INVOKEVIRTUAL, "java/lang/Class", - "desiredAssertionStatus", "()Z", true); - verify(this.context).disableMutations(anyString()); - this.testee - .visitFieldInsn( - Opcodes.PUTSTATIC, - "org/pitest/mutationtest/engine/gregor/TestGregorMutater$HasAssertStatement", - "$assertionsDisabled", "Z"); - verify(this.context).enableMutatations(anyString()); - } - - @Test - public void shouldForwardInterceptedFieldInstructionsToChild() { - this.testee.visitFieldInsn(Opcodes.GETSTATIC, "foo", "$assertionsDisabled", - "Z"); - verify(this.mv).visitFieldInsn(Opcodes.GETSTATIC, "foo", - "$assertionsDisabled", "Z"); - } - - @Test - public void shouldForwardInterceptedVisitLabelInstructionsToChild() { - this.testee.visitLabel(this.label); - verify(this.mv).visitLabel(this.label); - } - - @Test - public void shouldForwardInterceptedVisitJumpInstructionsToChild() { - this.testee.visitJumpInsn(Opcodes.IFEQ, this.label); - verify(this.mv).visitJumpInsn(Opcodes.IFEQ, this.label); - } - - @Override - @Test - public void shouldForwardVisitMethodInsnToChild() { - this.testee.visitMethodInsn(1, "foo", "bar", "far", true); - verify(this.mv).visitMethodInsn(1, "foo", "bar", "far", true); - } - - @Override - protected MethodVisitor getTesteeVisitor() { - return this.testee; - } - -} diff --git a/pitest/src/test/java/org/pitest/mutationtest/engine/gregor/TestGregorMutater.java b/pitest/src/test/java/org/pitest/mutationtest/engine/gregor/TestGregorMutater.java index 5503582ee..f6b7d18f7 100644 --- a/pitest/src/test/java/org/pitest/mutationtest/engine/gregor/TestGregorMutater.java +++ b/pitest/src/test/java/org/pitest/mutationtest/engine/gregor/TestGregorMutater.java @@ -89,19 +89,6 @@ public void shouldMutateCustomConstructorsAddedToEnums() { assertThat(actualDetails).isNotEmpty(); } - @Test - public void shouldNotMutateAssertStatements() { - MutatorVerifierStart.forMutator(NegateConditionalsMutator.NEGATE_CONDITIONALS) - .forClass(HasAssertStatement.class) - .noMutantsCreated(); - } - - @Test - public void shouldMutateOtherStatementsWhenAssertIsPresent() { - MutatorVerifierStart.forMutator(NegateConditionalsMutator.NEGATE_CONDITIONALS) - .forClass(HasAssertStatementAndOtherStatements.class) - .createsNMutants(1); - } @Test public void shouldNotMutateGroovyClasses() { @@ -218,23 +205,6 @@ public int mutable() { } - public static class HasAssertStatement { - public void foo(final int i) { - assert ((i + 20) > 10); - } - } - - public static class HasAssertStatementAndOtherStatements { - public int state; - - public void foo(final int i) { - assert ((i + 20) > 10); - if (i > 1) { - this.state = 1; - } - } - } - public static class OneStraightThroughMethod { public void straightThrough(int i) { i++;