Restore ctor init merging to previous logic, introduce PRE_BODY enforcer

Closes SpongePowered#658

src/main/java/org/spongepowered/asm/mixin/injection/code/IInsnListEx.java

@@ -48,7 +48,14 @@ public enum SpecialNodeType {
         /**
          * The location for injected initialisers in a constructor 
          */
-        INITIALISER_INJECTION_POINT
+        INITIALISER_INJECTION_POINT,
+
+        /**
+         * The location after field initialisers but before the first
+         * constructor body instruction, requires line numbers to be present in
+         * the target class
+         */
+        CTOR_BODY
 
     }
 

src/main/java/org/spongepowered/asm/mixin/injection/code/InsnListEx.java

@@ -142,6 +142,14 @@ public AbstractInsnNode getSpecialNode(SpecialNodeType type) {
                 }
                 return null;
 
+            case CTOR_BODY:
+                if (this.target instanceof Constructor) {
+                    AbstractInsnNode beforeBody = ((Constructor)this.target).findFirstBodyInsn();
+                    if (this.contains(beforeBody)) {
+                        return beforeBody;
+                    }
+                }
+                return null;
             default:
                 return null;
         }

src/main/java/org/spongepowered/asm/mixin/injection/points/ConstructorHead.java

@@ -37,7 +37,6 @@
 import org.spongepowered.asm.mixin.injection.struct.InjectionPointData;
 import org.spongepowered.asm.mixin.injection.throwables.InvalidInjectionPointException;
 import org.spongepowered.asm.service.MixinService;
-import org.spongepowered.asm.util.asm.MarkerNode;
 
 /**
  * <p>Like {@link MethodHead HEAD}, this injection point can be used to specify
@@ -56,9 +55,15 @@
  * <dl>
  *   <dt>enforce=POST_DELEGATE</dt>
  *   <dd>Select the instruction immediately after the delegate constructor</dd>
- *   <dt>enforce=POST_INIT</dt>
- *   <dd>Select the instruction immediately after field initialisers, this
- *     condition can fail if no initialisers are found.</dd>
+ *   <dt>enforce=POST_MIXIN</dt>
+ *   <dd>Select the instruction immediately after all mixin-initialised field
+ *     initialisers, this is similar to POST_DELEGATE if no applied mixins have
+ *     initialisers for target class fields, except that the injection point
+ *     will be after any mixin-supplied initialisers.</dd>
+ *   <dt>enforce=PRE_BODY</dt>
+ *   <dd>Selects the first instruction in the target method body, as determined
+ *     by the line numbers. If the target method does not have line numbers
+ *     available, the result is equivalent to POST_DELEGATE.</dd>
  * </dl>
  * 
  * <p>Example default behaviour:</p>
@@ -81,7 +86,7 @@ public class ConstructorHead extends MethodHead {
     static enum Enforce {
 
         /**
-         * Use default behaviour
+         * Use default behaviour (POST_INIT)
          */
         DEFAULT,
 
@@ -93,7 +98,12 @@ static enum Enforce {
         /**
          * Enforce selection of post-initialiser insn 
          */
-        POST_INIT;
+        POST_INIT,
+
+        /**
+         * Enforce selection of the first body insn
+         */
+        PRE_BODY;
 
     }
 
@@ -149,30 +159,10 @@ public boolean find(String desc, InsnList insns, Collection<AbstractInsnNode> no
             return true;
         }
 
-        AbstractInsnNode postInit = xinsns.getSpecialNode(SpecialNodeType.INITIALISER_INJECTION_POINT);
+        SpecialNodeType type = this.enforce == Enforce.PRE_BODY ? SpecialNodeType.CTOR_BODY : SpecialNodeType.INITIALISER_INJECTION_POINT;
+        AbstractInsnNode postInit = xinsns.getSpecialNode(type);
 
-        if (postInit instanceof MarkerNode) {
-            AbstractInsnNode postPostInit = postInit.getNext();
-            if (postDelegate == postInit || postDelegate == postPostInit) {
-                // If the marker is immedately after the delegate call, then we haven't actually
-                // found any initialiser insns. This is fine as long as we're not Enforce.POST_INIT
-                // which we assume will only be set by the user when they want to be sure that the
-                // initialisers are properly detected. Set this null to trigger the enforcement or
-                // fallback behaviour below.
-                postInit = null;
-            } else {
-                postInit = postPostInit;
-            }
-        }
-
-        if (this.enforce == Enforce.POST_INIT || postInit != null) {
-            if (postInit == null) {
-                if (this.verbose) {
-                    this.logger.warn("@At(\"{}\") on {}{} targetting {} failed for enforce=POST_INIT because no initialiser was found",
-                            this.getAtCode(), this.method.name, this.method.desc, xinsns);
-                }
-                return false;
-            }
+        if (postInit != null) {
             nodes.add(postInit);
             return true;
         }

src/main/java/org/spongepowered/asm/mixin/injection/struct/Constructor.java

@@ -24,18 +24,23 @@
  */
 package org.spongepowered.asm.mixin.injection.struct;
 
+import java.util.Deque;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.Set;
 
+import org.objectweb.asm.Label;
 import org.objectweb.asm.Opcodes;
 import org.objectweb.asm.tree.AbstractInsnNode;
 import org.objectweb.asm.tree.ClassNode;
 import org.objectweb.asm.tree.FieldInsnNode;
+import org.objectweb.asm.tree.LabelNode;
+import org.objectweb.asm.tree.LineNumberNode;
 import org.objectweb.asm.tree.MethodInsnNode;
 import org.objectweb.asm.tree.MethodNode;
 import org.spongepowered.asm.mixin.transformer.ClassInfo;
 import org.spongepowered.asm.mixin.transformer.struct.Initialiser;
+import org.spongepowered.asm.mixin.transformer.struct.InsnRange;
 import org.spongepowered.asm.util.Bytecode;
 import org.spongepowered.asm.util.Constants;
 import org.spongepowered.asm.util.asm.MarkerNode;
@@ -52,9 +57,18 @@ public class Constructor extends Target {
     private DelegateInitialiser delegateInitialiser;
 
     private MarkerNode initialiserInjectionPoint;
+    private MarkerNode bodyStart;
+
+    private final String targetName; 
+    private final String targetSuperName;
+
+    private Set<String> mixinInitialisedFields = new HashSet<String>();
 
     public Constructor(ClassInfo classInfo, ClassNode classNode, MethodNode method) {
         super(classInfo, classNode, method);
+
+        this.targetName = this.classInfo.getName(); 
+        this.targetSuperName = this.classInfo.getSuperName();
     }
 
     /**
@@ -73,6 +87,40 @@ public DelegateInitialiser findDelegateInitNode() {
 
         return this.delegateInitialiser;
     }
+
+    /**
+     * Get whether this constructor should have mixin initialisers injected into
+     * it based on whether a delegate initialiser call is absent or is a call to
+     * super()
+     */
+    public boolean isInjectable() {
+        DelegateInitialiser delegateInit = this.findDelegateInitNode();
+        return !delegateInit.isPresent || delegateInit.isSuper;
+    }
+
+    /**
+     * Inspect an incoming initialiser to determine which fields are touched by
+     * the mixin. This is then used in {@link #findInitialiserInjectionPoint} to
+     * determine the instruction after which the mixin initialisers will be
+     * placed
+     * 
+     * @param initialiser the initialiser to inspect
+     */
+    public void inspect(Initialiser initialiser) {
+        if (this.initialiserInjectionPoint != null) {
+            throw new IllegalStateException("Attempted to inspect an incoming initialiser after the injection point was already determined");
+        }
+
+        for (AbstractInsnNode initialiserInsn : initialiser.getInsns()) {
+            if (initialiserInsn.getOpcode() == Opcodes.PUTFIELD) {
+                FieldInsnNode fieldInsn = (FieldInsnNode)initialiserInsn;
+                if (!fieldInsn.owner.equals(this.targetName)) {
+                    continue;
+                }
+                this.mixinInitialisedFields.add(Constructor.fieldKey((FieldInsnNode)initialiserInsn));
+            }
+        }
+    }
 
     /**
      * Find the injection point for injected initialiser insns in the target
@@ -90,34 +138,20 @@ public AbstractInsnNode findInitialiserInjectionPoint(Initialiser.InjectionMode
             return this.initialiserInjectionPoint;
         }
 
-        String targetName = this.classInfo.getName(); 
-        String targetSuperName = this.classInfo.getSuperName();
-
-        Set<String> initialisedFields = new HashSet<String>();
-        for (AbstractInsnNode initialiserInsn : this.insns) {
-            if (initialiserInsn.getOpcode() == Opcodes.PUTFIELD) {
-                FieldInsnNode fieldInsn = (FieldInsnNode)initialiserInsn;
-                if (!fieldInsn.owner.equals(targetName)) {
-                    continue;
-                }
-                initialisedFields.add(Constructor.fieldKey((FieldInsnNode)initialiserInsn)); 
-            }
-        }
-
         AbstractInsnNode lastInsn = null;
         for (Iterator<AbstractInsnNode> iter = this.insns.iterator(); iter.hasNext();) {
             AbstractInsnNode insn = iter.next();
             if (insn.getOpcode() == Opcodes.INVOKESPECIAL && Constants.CTOR.equals(((MethodInsnNode)insn).name)) {
                 String owner = ((MethodInsnNode)insn).owner;
-                if (owner.equals(targetName) || owner.equals(targetSuperName)) {
+                if (owner.equals(this.targetName) || owner.equals(targetSuperName)) {
                     lastInsn = insn;
                     if (mode == Initialiser.InjectionMode.SAFE) {
                         break;
                     }
                 }
             } else if (insn.getOpcode() == Opcodes.PUTFIELD && mode == Initialiser.InjectionMode.DEFAULT) {
                 String key = Constructor.fieldKey((FieldInsnNode)insn);
-                if (initialisedFields.contains(key)) {
+                if (this.mixinInitialisedFields.contains(key)) {
                     lastInsn = insn;
                 }
             }            
@@ -132,8 +166,70 @@ public AbstractInsnNode findInitialiserInjectionPoint(Initialiser.InjectionMode
         return this.initialiserInjectionPoint;
     }
 
+    /**
+     * Find the injection point 
+     */
+    public AbstractInsnNode findFirstBodyInsn() {
+        if (this.bodyStart == null) {
+            this.bodyStart = new MarkerNode(MarkerNode.BODY_START);
+            InsnRange range = Constructor.getRange(this.method);
+            if (range.isValid()) {
+                Deque<AbstractInsnNode> body = range.apply(this.insns, true);
+                this.insertBefore(body.pop(), this.bodyStart);
+            } else if (range.marker > -1) {
+                this.insert(this.insns.get(range.marker), this.bodyStart);
+            } else {
+                this.bodyStart = null;
+            }
+        }
+        return this.bodyStart;
+    }
+
     private static String fieldKey(FieldInsnNode fieldNode) {
         return String.format("%s:%s", fieldNode.desc, fieldNode.name);
     }
 
+    /**
+     * Identifies line numbers in the supplied ctor which correspond to the
+     * start and end of the method body.
+     * 
+     * @param ctor constructor to scan
+     * @return range indicating the line numbers of the specified constructor
+     *      and the position of the superclass ctor invocation
+     */
+    public static InsnRange getRange(MethodNode ctor) {
+        boolean lineNumberIsValid = false;
+        AbstractInsnNode endReturn = null;
+
+        int line = 0, start = 0, end = 0, delegateIndex = -1;
+        for (Iterator<AbstractInsnNode> iter = ctor.instructions.iterator(); iter.hasNext();) {
+            AbstractInsnNode insn = iter.next();
+            if (insn instanceof LineNumberNode) {
+                line = ((LineNumberNode)insn).line;
+                lineNumberIsValid = true;
+            } else if (insn instanceof MethodInsnNode) {
+                if (insn.getOpcode() == Opcodes.INVOKESPECIAL && Constants.CTOR.equals(((MethodInsnNode)insn).name) && delegateIndex == -1) {
+                    delegateIndex = ctor.instructions.indexOf(insn);
+                    start = line;
+                }
+            } else if (insn.getOpcode() == Opcodes.PUTFIELD) {
+                lineNumberIsValid = false;
+            } else if (insn.getOpcode() == Opcodes.RETURN) {
+                if (lineNumberIsValid) {
+                    end = line;
+                } else {
+                    end = start;
+                    endReturn = insn;
+                }
+            }
+        }
+
+        if (endReturn != null) {
+            LabelNode label = new LabelNode(new Label());
+            ctor.instructions.insertBefore(endReturn, label);
+            ctor.instructions.insertBefore(endReturn, new LineNumberNode(start, label));
+        }
+
+        return new InsnRange(start, end, delegateIndex);
+    }
 }

src/main/java/org/spongepowered/asm/mixin/transformer/MixinApplicatorStandard.java

@@ -61,7 +61,6 @@
 import org.spongepowered.asm.service.MixinService;
 import org.spongepowered.asm.util.Annotations;
 import org.spongepowered.asm.util.Bytecode;
-import org.spongepowered.asm.util.Bytecode.DelegateInitialiser;
 import org.spongepowered.asm.util.Constants;
 import org.spongepowered.asm.util.ConstraintParser;
 import org.spongepowered.asm.util.ConstraintParser.Constraint;
@@ -675,8 +674,7 @@ protected void applyInitialisers(MixinTargetContext mixin) {
 
         // Patch the initialiser into the target class ctors
         for (Constructor ctor : this.context.getConstructors()) {
-            DelegateInitialiser superCall = ctor.findDelegateInitNode();
-            if (!superCall.isPresent || superCall.isSuper) {
+            if (ctor.isInjectable()) {
                 int extraStack = initialiser.getMaxStack() - ctor.getMaxStack();
                 if (extraStack > 0) {
                     ctor.extendStack().add(extraStack);