Support for Mutable on setter accessors for final fields, closes Spon…

…gePowered#378
AresClient · Jan 4, 2020 · 8cdc32d · 8cdc32d
1 parent cf5a440
commit 8cdc32d
Show file tree

Hide file tree

Showing 8 changed files with 92 additions and 12 deletions.
diff --git a/src/main/java/org/spongepowered/asm/mixin/Mutable.java b/src/main/java/org/spongepowered/asm/mixin/Mutable.java
@@ -29,11 +29,20 @@
 import java.lang.annotation.RetentionPolicy;
 import java.lang.annotation.Target;
 
+import org.spongepowered.asm.mixin.gen.Accessor;
+
 /**
- * Use in conjunction with {@link Final} to indicate that whilst a field is
- * final in the target class, mutation within the mixin is intentional.
+ * Use in conjunction with {@link Final} or {@link Accessor} to indicate that
+ * whilst a target field is final in the target class, mutation within the mixin
+ * is intentional.
+ * 
+ * <p>Applying this annotation has the effect of removing the <tt>final</tt>
+ * modifier from the target field in order to avoid illegal access errors when a
+ * shadow visible outside the mixin class (eg. to derived or package-local
+ * mixins) is mutated. It also permits the mutation on Java runtimes which also
+ * check for mutation of <tt>private</tt> final variables (JRE 9 and later).</p>
  */
-@Target(ElementType.FIELD)
+@Target({ ElementType.METHOD, ElementType.FIELD })
 @Retention(RetentionPolicy.RUNTIME)
 public @interface Mutable {
 

diff --git a/src/main/java/org/spongepowered/asm/mixin/gen/AccessorGenerator.java b/src/main/java/org/spongepowered/asm/mixin/gen/AccessorGenerator.java
@@ -78,6 +78,14 @@ protected final MethodNode createMethod(int maxLocals, int maxStack) {
         accessor.maxStack = maxStack;
         return accessor;
     }
+
+    /**
+     * Perform pre-flight checks on the accessor, to ensure it can be generated
+     * sanely 
+     */
+    public void validate() {
+        // Stub for subclasses
+    }
 
     /**
      * Generate the accessor method

diff --git a/src/main/java/org/spongepowered/asm/mixin/gen/AccessorGeneratorFieldSetter.java b/src/main/java/org/spongepowered/asm/mixin/gen/AccessorGeneratorFieldSetter.java
@@ -24,26 +24,57 @@
  */
 package org.spongepowered.asm.mixin.gen;
 
+import org.apache.logging.log4j.LogManager;
 import org.objectweb.asm.Opcodes;
 import org.objectweb.asm.tree.FieldInsnNode;
 import org.objectweb.asm.tree.InsnNode;
 import org.objectweb.asm.tree.MethodNode;
 import org.objectweb.asm.tree.VarInsnNode;
+import org.spongepowered.asm.mixin.MixinEnvironment.Option;
+import org.spongepowered.asm.mixin.Mutable;
+import org.spongepowered.asm.mixin.transformer.ClassInfo.Method;
+import org.spongepowered.asm.mixin.transformer.MixinTargetContext;
+import org.spongepowered.asm.util.Bytecode;
 
 /**
  * Generator for field setters
  */
 public class AccessorGeneratorFieldSetter extends AccessorGeneratorField {
 
+    /**
+     * True if the accessor method is decorated with {@link Mutable} 
+     */
+    private boolean mutable;
+
     public AccessorGeneratorFieldSetter(AccessorInfo info) {
         super(info);
     }
+
+    @Override
+    public void validate() {
+        super.validate();
+
+        Method method = this.info.getClassInfo().findMethod(this.info.getMethod());
+        this.mutable = method.isDecoratedMutable();
+        if (this.mutable || !Bytecode.hasFlag(this.targetField, Opcodes.ACC_FINAL)) {
+            return;
+        }
+
+        if (this.info.getContext().getOption(Option.DEBUG_VERBOSE)) {
+            LogManager.getLogger("mixin").warn("{} for final field {}::{} is not @Mutable", this.info,
+                    ((MixinTargetContext)this.info.getContext()).getTarget(), this.targetField.name);
+        }                    
+    }
 
     /* (non-Javadoc)
      * @see org.spongepowered.asm.mixin.gen.AccessorGenerator#generate()
      */
     @Override
     public MethodNode generate() {
+        if (this.mutable) {
+            this.targetField.access &= ~Opcodes.ACC_FINAL;
+        }
+
         int stackSpace = this.targetIsStatic ? 0 : 1; // Stack space for "this"
         int maxLocals = stackSpace + this.targetType.getSize();
         int maxStack = stackSpace + this.targetType.getSize();

diff --git a/src/main/java/org/spongepowered/asm/mixin/gen/AccessorInfo.java b/src/main/java/org/spongepowered/asm/mixin/gen/AccessorInfo.java
@@ -282,6 +282,12 @@ private static String getPrefixList() {
      */
     protected MethodNode targetMethod;
 
+    /**
+     * Generator which will be responsible for actually generating the accessor
+     * method body 
+     */
+    protected AccessorGenerator generator;
+
     public AccessorInfo(MixinTargetContext mixin, MethodNode method) {
         this(mixin, method, Accessor.class);
     }
@@ -401,6 +407,8 @@ public static String inflectTarget(AccessorName name, AccessorType type, String
         }
         return null;
     }
+
+
 
     /**
      * Get the inflected/specified target member for this accessor
@@ -466,6 +474,15 @@ public String toString() {
     public void locate() {
         this.targetField = this.findTargetField();
     }
+
+    /**
+     * Called immediately after locate, initialises the generator for this
+     * accessor and runs validation. 
+     */
+    public void validate() {
+        this.generator = this.type.getGenerator(this);
+        this.generator.validate();
+    }
 
     /**
      * Second pass, generate the actual accessor method for this accessor. The
@@ -475,7 +492,7 @@ public void locate() {
      * @return generated accessor method
      */
     public MethodNode generate() {
-        MethodNode generatedAccessor = this.type.getGenerator(this).generate();
+        MethodNode generatedAccessor = this.generator.generate();
         Annotations.merge(this.method, generatedAccessor);
         return generatedAccessor;
     }

diff --git a/src/main/java/org/spongepowered/asm/mixin/gen/InvokerInfo.java b/src/main/java/org/spongepowered/asm/mixin/gen/InvokerInfo.java
@@ -42,7 +42,7 @@
  */
 class InvokerInfo extends AccessorInfo {
 
-    public InvokerInfo(MixinTargetContext mixin, MethodNode method) {
+    InvokerInfo(MixinTargetContext mixin, MethodNode method) {
         super(mixin, method, Invoker.class);
     }
 

diff --git a/src/main/java/org/spongepowered/asm/mixin/struct/SpecialMethodInfo.java b/src/main/java/org/spongepowered/asm/mixin/struct/SpecialMethodInfo.java
@@ -29,6 +29,7 @@
 import org.objectweb.asm.tree.MethodNode;
 import org.spongepowered.asm.mixin.injection.IInjectionPointContext;
 import org.spongepowered.asm.mixin.refmap.IMixinContext;
+import org.spongepowered.asm.mixin.transformer.ClassInfo;
 import org.spongepowered.asm.mixin.transformer.MixinTargetContext;
 import org.spongepowered.asm.util.Bytecode;
 import org.spongepowered.asm.util.asm.MethodNodeEx;
@@ -105,6 +106,13 @@ public final AnnotationNode getAnnotation() {
     public final ClassNode getClassNode() {
         return this.classNode;
     }
+
+    /**
+     * Get the class metadata for the mixin
+     */
+    public final ClassInfo getClassInfo() {
+        return this.mixin.getClassInfo();
+    }
 
     /**
      * Get method being called

diff --git a/src/main/java/org/spongepowered/asm/mixin/transformer/ClassInfo.java b/src/main/java/org/spongepowered/asm/mixin/transformer/ClassInfo.java
@@ -403,7 +403,7 @@ public boolean isDecoratedMutable() {
             return this.decoratedMutable;
         }
 
-        public void setDecoratedFinal(boolean decoratedFinal, boolean decoratedMutable) {
+        protected void setDecoratedFinal(boolean decoratedFinal, boolean decoratedMutable) {
             this.decoratedFinal = decoratedFinal;
             this.decoratedMutable = decoratedMutable;
         }
@@ -486,11 +486,8 @@ public Method(Member member) {
             this.frames = member instanceof Method ? ((Method)member).frames : null;
         }
 
-        @SuppressWarnings("unchecked")
         public Method(MethodNode method) {
             this(method, false);
-            this.setUnique(Annotations.getVisible(method, Unique.class) != null);
-            this.isAccessor = Annotations.getSingleVisible(method, Accessor.class, Invoker.class) != null;
         }
 
         @SuppressWarnings("unchecked")
@@ -499,6 +496,9 @@ public Method(MethodNode method, boolean injected) {
             this.frames = this.gatherFrames(method);
             this.setUnique(Annotations.getVisible(method, Unique.class) != null);
             this.isAccessor = Annotations.getSingleVisible(method, Accessor.class, Invoker.class) != null;
+            boolean decoratedFinal = Annotations.getVisible(method, Final.class) != null;
+            boolean decoratedMutable = Annotations.getVisible(method, Mutable.class) != null;
+            this.setDecoratedFinal(decoratedFinal, decoratedMutable);
         }
 
         public Method(String name, String desc) {
@@ -606,7 +606,7 @@ public ClassInfo getImplementor() {
     /**
      * A field
      */
-    class Field extends Member {
+    public class Field extends Member {
 
         public Field(Member member) {
             super(member);

diff --git a/src/main/java/org/spongepowered/asm/mixin/transformer/MixinTargetContext.java b/src/main/java/org/spongepowered/asm/mixin/transformer/MixinTargetContext.java
@@ -354,7 +354,7 @@ public ClassInfo getTargetClassInfo() {
      * @return the local class info
      */
     @Override
-    ClassInfo getClassInfo() {
+    public ClassInfo getClassInfo() {
         return this.mixin.getClassInfo();
     }
 
@@ -559,7 +559,7 @@ private void transformMethodRef(MethodNode method, Iterator<AbstractInsnNode> it
         if (methodRef.getOwner().equals(this.getClassRef())) {
             methodRef.setOwner(this.getTarget().getClassRef());
             Method md = this.getClassInfo().findMethod(methodRef.getName(), methodRef.getDesc(), ClassInfo.INCLUDE_ALL);
-            if (md != null && md.isRenamed() && md.getOriginalName().equals(methodRef.getName()) && md.isSynthetic()) {
+            if (md != null && md.isRenamed() && md.getOriginalName().equals(methodRef.getName()) && (md.isSynthetic() || md.isConformed())) {
                 methodRef.setName(md.getName());
             }
             this.upgradeMethodRef(method, methodRef, md);
@@ -1307,6 +1307,13 @@ List<MethodNode> generateAccessors() {
                 accessor.locate();
             }
 
+            accessorActivity.next("Validate"); 
+            Activity validateActivity = this.activities.begin("?");
+            for (AccessorInfo accessor : this.accessors) {
+                validateActivity.next(accessor.toString());
+                accessor.validate();
+            }
+
             accessorActivity.next("Generate"); 
             Activity generateActivity = this.activities.begin("?");
             for (AccessorInfo accessor : this.accessors) {