Merge pull request #1434 from hcoles/feature/deprecate_unsafe_index_access

deprecate unsafe instruction index access

Author: hcoles

pitest-entry/src/main/java/org/pitest/bytecode/analysis/MethodTree.java

@@ -2,6 +2,7 @@
 
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Optional;
 
 import org.objectweb.asm.Opcodes;
 import org.objectweb.asm.tree.AbstractInsnNode;
@@ -34,16 +35,25 @@ public Location asLocation() {
    * @param index index to work backwards from
    * @return The previous instruction
    */
+  @Deprecated
   public AbstractInsnNode realInstructionBefore(int index) {
     AbstractInsnNode candidate = instructions().get(index - 1);
     if (candidate.getOpcode() == -1) {
       return realInstructionBefore(index - 1);
     }
     return candidate;
   }
-  
+
+  public Optional<AbstractInsnNode> instructionForIndex(int index) {
+    return Optional.ofNullable(instruction(index));
+  }
+
+  @Deprecated
   public AbstractInsnNode instruction(int index) {
-      return instructions().get(index);
+    if (index < 0 || index >= instructions().size() ) {
+      return null;
+    }
+    return instructions().get(index);
   }
 
   public List<AbstractInsnNode> instructions() {

pitest-entry/src/main/java/org/pitest/mutationtest/build/intercept/equivalent/EqualsPerformanceShortcutFilter.java

@@ -112,8 +112,10 @@ private boolean shortCutEquals(MethodTree tree, MutationDetails a, Mutater m) {
   }
 
   private boolean mutatesAConditionalJump(MethodTree tree, int index) {
-    final AbstractInsnNode mutatedInsns = tree.instruction(index);
-    return InstructionMatchers.aConditionalJump().asPredicate().test(mutatedInsns);
+    var mutatedInsns = tree.instructionForIndex(index);
+    return mutatedInsns
+            .map(n -> InstructionMatchers.aConditionalJump().asPredicate().test(n))
+            .orElse(false);
   }
 
   private Predicate<MutationDetails> inEqualsMethod() {

pitest-entry/src/main/java/org/pitest/mutationtest/build/intercept/javafeatures/ForEachLoopFilter.java

@@ -202,11 +202,14 @@ private Predicate<MutationDetails> mutatesIteratorLoopPlumbing() {
         return false;
       }
 
-      final AbstractInsnNode mutatedInstruction = method.instruction(instruction);
+      var mutatedInstruction = method.instructionForIndex(instruction);
+      if (mutatedInstruction.isEmpty()) {
+        return false;
+      }
 
       Set<AbstractInsnNode> toAvoid = cache.computeIfAbsent(method, this::findLoopInstructions);
 
-      return toAvoid.contains(mutatedInstruction);
+      return toAvoid.contains(mutatedInstruction.get());
     };
   }
 

pitest-entry/src/main/java/org/pitest/mutationtest/build/intercept/javafeatures/InlinedFinallyBlockFilter.java

@@ -184,10 +184,13 @@ private boolean isInFinallyBlock(MutationDetails m) {
       return false;
     }
 
-    AbstractInsnNode mutatedInstruction = method.instruction(m.getInstructionIndex());
+    var mutatedInstruction = method.instructionForIndex(m.getInstructionIndex());
+    if (mutatedInstruction.isEmpty()) {
+      return false;
+    }
 
     Context context = Context.start(DEBUG);
-    context = context.store(MUTATED_INSTRUCTION.write(), mutatedInstruction);
+    context = context.store(MUTATED_INSTRUCTION.write(), mutatedInstruction.get());
     context = context.store(HANDLERS.write(), handlers);
     return IS_IN_HANDLER.matches(method.instructions(), context);
   }

pitest-entry/src/main/java/org/pitest/mutationtest/build/intercept/timeout/AvoidForLoopCounterFilter.java

@@ -170,7 +170,12 @@ private Predicate<MutationDetails> mutatesAForLoopCounter() {
         return false;
       }
 
-      final AbstractInsnNode mutatedInstruction = method.instruction(instruction);
+      var maybeIns = method.instructionForIndex(instruction);
+      if (maybeIns.isEmpty()) {
+        return false;
+      }
+
+      var mutatedInstruction = maybeIns.get();
 
       // performance hack
       if (!(mutatedInstruction instanceof IincInsnNode)) {

pitest-entry/src/main/java/org/pitest/mutationtest/build/intercept/timeout/InfiniteForLoopFilter.java

@@ -53,8 +53,10 @@ SequenceMatcher<AbstractInsnNode> infiniteLoopMatcher() {
 
   @Override
   boolean couldCauseInfiniteLoop(MethodTree method, MutationDetails each) {
-    final AbstractInsnNode instruction = method.instruction(each.getInstructionIndex());
-    return instruction.getOpcode() == Opcodes.IINC;
+    var instruction = method.instructionForIndex(each.getInstructionIndex());
+    return instruction
+            .map(i -> i.getOpcode() == Opcodes.IINC)
+            .orElse(false);
   }
 
   private static SequenceQuery<AbstractInsnNode> countingLoopWithoutWriteConditionalAtStart() {

pitest-entry/src/main/java/org/pitest/mutationtest/build/intercept/timeout/InfiniteIteratorLoopFilter.java

@@ -44,8 +44,10 @@ SequenceMatcher<AbstractInsnNode> infiniteLoopMatcher() {
 
   @Override
   boolean couldCauseInfiniteLoop(MethodTree method, MutationDetails each) {
-    final AbstractInsnNode instruction = method.instruction(each.getInstructionIndex());
-    return isIteratorNext(instruction);
+    var instruction = method.instructionForIndex(each.getInstructionIndex());
+    return instruction
+            .map(this::isIteratorNext)
+            .orElse(false);
   }
 
   private static SequenceQuery<AbstractInsnNode> doesNotBreakIteratorLoop() {

pitest-entry/src/test/java/org/pitest/bytecode/analysis/MethodTreeTest.java

@@ -9,6 +9,15 @@ public class MethodTreeTest {
 
     ClassloaderByteArraySource bytes = ClassloaderByteArraySource.fromContext();
 
+    @Test
+    public void retrievesInstructionsWithinMethod() {
+        var clazz = loadClass(MethodTreeTest.class.getName());
+        var method = findMethod(clazz, "retrievesInstructionsWithinMethod");
+        assertThat(method.instructionForIndex(0)).isNotEmpty();
+        assertThat(method.instructionForIndex(-1)).isEmpty();
+        assertThat(method.instructionForIndex(Integer.MAX_VALUE)).isEmpty();
+    }
+
     @Test
     public void recognisesAbstractMethods() {
         var clazz = loadClass(Bar.class.getName());