Add mixin member to Dynamic, verbosify error messages accordingly

src/main/java/org/spongepowered/asm/mixin/Dynamic.java

@@ -65,25 +65,43 @@
  *   <li><tt>"All calls to Foo.bar are replaced at runtime by mod Y with a calls
  *     to Baz.flop"</tt></li>
  *   <li><tt>"Added by SomeOtherUpstreamMixin"</tt></li>
- *   <li><tt>"com.foo.bar.SomeOtherUpstreamMixin"</tt></li>
  * </ul>
  * 
  * <p>In other words, the contents of the <tt>&#064;Dynamic</tt> should try to
  * provide as much useful context for the decorated method without being overly
- * verbose. In the case of the last example, where the contents of the
- * annotation are the fully-qualified coordinates of an upstream mixin, it may
- * be possible for tooling to enable navigation or other enhanced functionality,
- * so deciding for a project </p>
+ * verbose.</p>
+ * 
+ * <p>In the case where the target method or field is added by an upstream
+ * mixin, and the mixin in question is on the classpath of the project, it is
+ * also possible to use the {@link #mixin} value to specify the mixin which
+ * contributes the target member. This provides both a useful navigable link in
+ * IDEs, and useful context for mixin tooling such as IDE plugins.</p>
+ * 
+ * <p>If the target member is contributed by an upstream mixin but the mixin is
+ * <em>not</em> on the classpath of the current project, using the fully-
+ * qualified name of the upstream mixin as the {@link #value} is a useful
+ * fallback, since developers reading the source code can still use the string
+ * value as a search term in their IDE or on Github.</p>
  */
 @Target({ ElementType.METHOD, ElementType.FIELD })
 @Retention(RetentionPolicy.CLASS)
 public @interface Dynamic {
 
     /**
      * Description of this <tt>&#064;Dynamic</tt> member. See the notes above.
+     * Can be omitted if {@link #mixin} is specified instead.
      * 
      * @return description of the member
      */
-    public String value();
+    public String value() default "";
+
+    /**
+     * If the target member is added by an upstream mixin, and the mixin in
+     * question is on the classpath, specify the mixin class here in order to
+     * provide useful context for the annotated member.
+     * 
+     * @return upstream mixin reference
+     */
+    public Class<?> mixin() default void.class;
 
 }

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

@@ -36,6 +36,7 @@
 import java.util.Set;
 
 import org.spongepowered.asm.lib.Opcodes;
+import org.spongepowered.asm.lib.Type;
 import org.spongepowered.asm.lib.tree.AnnotationNode;
 import org.spongepowered.asm.lib.tree.MethodNode;
 import org.spongepowered.asm.mixin.Dynamic;
@@ -177,21 +178,22 @@ protected void readAnnotation() {
     protected Set<MemberInfo> parseTargets(String type) {
         List<String> methods = Annotations.<String>getValue(this.annotation, "method", false);
         if (methods == null) {
-            throw new InvalidInjectionException(this, type + " annotation on " + this.method.name + " is missing method name");
+            throw new InvalidInjectionException(this, String.format("%s annotation on %s is missing method name", type, this.method.name));
         }
 
         Set<MemberInfo> members = new LinkedHashSet<MemberInfo>();
         for (String method : methods) {
             try {
                 MemberInfo targetMember = MemberInfo.parseAndValidate(method, this.mixin);
                 if (targetMember.owner != null && !targetMember.owner.equals(this.mixin.getTargetClassRef())) {
-                    throw new InvalidInjectionException(this, type + " annotation on " + this.method.name + " specifies a target class '"
-                            + targetMember.owner + "', which is not supported");
+                    throw new InvalidInjectionException(this,
+                            String.format("%s annotation on %s specifies a target class '%s', which is not supported", type, this.method.name,
+                                    targetMember.owner));
                 }
                 members.add(targetMember);
             } catch (InvalidMemberDescriptorException ex) {
-                throw new InvalidInjectionException(this, type + " annotation on " + this.method.name + ", has invalid target descriptor: \""
-                        + method + "\". " + this.mixin.getReferenceMapper().getStatus());
+                throw new InvalidInjectionException(this, String.format("%s annotation on %s, has invalid target descriptor: \"%s\". %s", type,
+                        this.method.name, method, this.mixin.getReferenceMapper().getStatus()));
             }
         }
         return members;
@@ -200,7 +202,8 @@ protected Set<MemberInfo> parseTargets(String type) {
     protected List<AnnotationNode> readInjectionPoints(String type) {
         List<AnnotationNode> ats = Annotations.<AnnotationNode>getValue(this.annotation, this.atKey, false);
         if (ats == null) {
-            throw new InvalidInjectionException(this, type + " annotation on " + this.method.name + " is missing '" + this.atKey + "' value(s)");
+            throw new InvalidInjectionException(this, String.format("%s annotation on %s is missing '%s' value(s)", type, this.method.name,
+                    this.atKey));
         }
         return ats;
     }
@@ -414,8 +417,10 @@ private void findMethods(Set<MemberInfo> searchFor, String type) {
         }
 
         if (this.targets.size() == 0) {
-            throw new InvalidInjectionException(this, type + " annotation on " + this.method.name + " could not find any targets matching "
-                    + InjectionInfo.namesOf(searchFor) + ". " + this.mixin.getReferenceMapper().getStatus() + this.getDynamicInfo());
+            throw new InvalidInjectionException(this,
+                    String.format("%s annotation on %s could not find any targets matching %s in the target class %s. %s%s", 
+                            type, this.method.name, InjectionInfo.namesOf(searchFor), this.mixin.getTarget(),
+                            this.mixin.getReferenceMapper().getStatus(), this.getDynamicInfo()));
         }
     }
 
@@ -429,13 +434,13 @@ private void checkTarget(MethodNode target) {
         int priority = Annotations.<Integer>getValue(merged, "priority");
 
         if (priority >= this.mixin.getPriority() && !owner.equals(this.mixin.getClassName())) {
-            throw new InvalidInjectionException(this, this + " cannot inject into " + this.classNode.name + "::" + target.name + target.desc
-                    + " merged by " + owner + " with priority " + priority);
+            throw new InvalidInjectionException(this, String.format("%s cannot inject into %s::%s%s merged by %s with priority %d", this,
+                    this.classNode.name, target.name, target.desc, owner, priority));
         }
 
         if (Annotations.getVisible(target, Final.class) != null) {
-            throw new InvalidInjectionException(this, this + " cannot inject into @Final method " + this.classNode.name + "::" + target.name
-                    + target.desc + " merged by " + owner);
+            throw new InvalidInjectionException(this, String.format("%s cannot inject into @Final method %s::%s%s merged by %s", this,
+                    this.classNode.name, target.name, target.desc, owner));
         }
     }
 
@@ -446,8 +451,13 @@ private void checkTarget(MethodNode target) {
      * string is returned.
      */
     protected String getDynamicInfo() {
-        String dynamic = Annotations.<String>getValue(Annotations.getInvisible(this.method, Dynamic.class));
-        return Strings.isNullOrEmpty(dynamic) ? "" : String.format(" Method is @Dynamic(%s)", dynamic);
+        AnnotationNode annotation = Annotations.getInvisible(this.method, Dynamic.class);
+        String description = Strings.nullToEmpty(Annotations.<String>getValue(annotation));
+        Type upstream = Annotations.<Type>getValue(annotation, "mixin");
+        if (upstream != null) {
+            description = String.format("{%s} %s", upstream.getClassName(), description).trim();
+        }
+        return description.length() > 0 ? String.format(" Method is @Dynamic(%s)", description) : "";
     }
 
     /**
@@ -505,8 +515,8 @@ public static AnnotationNode getInjectorAnnotation(IMixinInfo mixin, MethodNode
                 ModifyConstant.class
             );
         } catch (IllegalArgumentException ex) {
-            throw new InvalidMixinException(mixin, "Error parsing annotations on " + method.name + " in " + mixin.getClassName() + ": "
-                    + ex.getMessage());
+            throw new InvalidMixinException(mixin, String.format("Error parsing annotations on %s in %s: %s", method.name, mixin.getClassName(),
+                    ex.getMessage()));
         }
         return annotation;
     }

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

@@ -30,6 +30,7 @@
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.spongepowered.asm.lib.Opcodes;
+import org.spongepowered.asm.lib.Type;
 import org.spongepowered.asm.lib.tree.AbstractInsnNode;
 import org.spongepowered.asm.lib.tree.AnnotationNode;
 import org.spongepowered.asm.lib.tree.FieldInsnNode;
@@ -382,20 +383,21 @@ protected boolean attachSpecialMethod(MixinTargetContext context, MixinMethodNod
             }
             target = context.findRemappedMethod(mixinMethod);
             if (target == null) {
-                throw new InvalidMixinException(this.mixin, type + " method " + mixinMethod.name + " in " + this.mixin
-                        + " was not located in the target class. " + context.getReferenceMapper().getStatus()
-                        + MixinPreProcessorStandard.getDynamicInfo(mixinMethod));
+                throw new InvalidMixinException(this.mixin,
+                        String.format("%s method %s in %s was not located in the target class %s. %s%s", type, mixinMethod.name, this.mixin,
+                                context.getTarget(), context.getReferenceMapper().getStatus(),
+                                MixinPreProcessorStandard.getDynamicInfo(mixinMethod)));
             }
             mixinMethod.name = method.renameTo(target.name);
         }
 
         if (Constants.CTOR.equals(target.name)) {
-            throw new InvalidMixinException(this.mixin, "Nice try! " + mixinMethod.name + " in " + this.mixin + " cannot alias a constructor!");
+            throw new InvalidMixinException(this.mixin, String.format("Nice try! %s in %s cannot alias a constructor", mixinMethod.name, this.mixin));
         }
 
         if (!Bytecode.compareFlags(mixinMethod, target, Opcodes.ACC_STATIC)) {
-            throw new InvalidMixinException(this.mixin, "STATIC modifier of " + type + " method " + mixinMethod.name + " in " + this.mixin
-                    + " does not match the target");
+            throw new InvalidMixinException(this.mixin, String.format("STATIC modifier of %s method %s in %s does not match the target", type,
+                    mixinMethod.name, this.mixin));
         }
 
         this.conformVisibility(context, mixinMethod, type, target);
@@ -445,13 +447,14 @@ protected Method getSpecialMethod(MixinMethodNode mixinMethod, SpecialMethod typ
 
     protected void checkMethodNotUnique(Method method, SpecialMethod type) {
         if (method.isUnique()) {
-            throw new InvalidMixinException(this.mixin, type + " method " + method.getName() + " cannot be @Unique");
+            throw new InvalidMixinException(this.mixin, String.format("%s method %s in %s cannot be @Unique", type, method.getName(), this.mixin));
         }
     }
 
     protected void checkMixinNotUnique(MixinMethodNode mixinMethod, SpecialMethod type) {
         if (this.mixin.isUnique()) {
-            throw new InvalidMixinException(this.mixin, type + " method " + mixinMethod.name + " found in a @Unique mixin");
+            throw new InvalidMixinException(this.mixin, String.format("%s method %s found in a @Unique mixin %s", type, mixinMethod.name,
+                    this.mixin));
         }
     }
 
@@ -482,8 +485,8 @@ protected boolean attachUniqueMethod(MixinTargetContext context, MixinMethodNode
         }
 
         if (this.strictUnique) {
-            throw new InvalidMixinException(this.mixin, "Method conflict, " + type + " method " + mixinMethod.name + " in " + this.mixin
-                    + " cannot overwrite " + target.name + target.desc + " in " + context.getTarget());
+            throw new InvalidMixinException(this.mixin, String.format("Method conflict, %s method %s in %s cannot overwrite %s%s in %s",
+                    type, mixinMethod.name, this.mixin, target.name, target.desc, context.getTarget()));
         }
 
         AnnotationNode unique = Annotations.getVisible(mixinMethod, Unique.class);
@@ -546,7 +549,7 @@ protected void attachFields(MixinTargetContext context) {
             field.remapTo(mixinField.desc);
 
             if (field.isUnique() && isShadow) {
-                throw new InvalidMixinException(this.mixin, "@Shadow field " + mixinField.name + " cannot be @Unique");
+                throw new InvalidMixinException(this.mixin, String.format("@Shadow field %s cannot be @Unique", mixinField.name));
             }
 
             FieldNode target = context.findField(mixinField, shadow);
@@ -557,16 +560,16 @@ protected void attachFields(MixinTargetContext context) {
                 target = context.findRemappedField(mixinField);
                 if (target == null) {
                     // If this field is a shadow field but is NOT found in the target class, that's bad, mmkay
-                    throw new InvalidMixinException(this.mixin, "Shadow field " + mixinField.name + " was not located in the target class"
-                            + context.getReferenceMapper().getStatus()
-                            + MixinPreProcessorStandard.getDynamicInfo(mixinField));
+                    throw new InvalidMixinException(this.mixin, String.format("Shadow field %s was not located in the target class %s. %s%s",
+                            mixinField.name, context.getTarget(), context.getReferenceMapper().getStatus(),
+                            MixinPreProcessorStandard.getDynamicInfo(mixinField)));
                 }
                 mixinField.name = field.renameTo(target.name);
             }
 
             if (!Bytecode.compareFlags(mixinField, target, Opcodes.ACC_STATIC)) {
-                throw new InvalidMixinException(this.mixin, "STATIC modifier of @Shadow field " + mixinField.name + " in " + this.mixin
-                        + " does not match the target");
+                throw new InvalidMixinException(this.mixin, String.format("STATIC modifier of @Shadow field %s in %s does not match the target",
+                        mixinField.name, this.mixin));
             }
 
             if (field.isUnique()) {
@@ -579,8 +582,8 @@ protected void attachFields(MixinTargetContext context) {
                 }
 
                 if (this.strictUnique) {
-                    throw new InvalidMixinException(this.mixin, "Field conflict, @Unique field " + mixinField.name + " in " + this.mixin
-                            + " cannot overwrite " + target.name + target.desc + " in " + context.getTarget());
+                    throw new InvalidMixinException(this.mixin, String.format("Field conflict, @Unique field %s in %s cannot overwrite %s%s in %s",
+                            mixinField.name, this.mixin, target.name, target.desc, context.getTarget()));
                 }
 
                 MixinPreProcessorStandard.logger.warn("Discarding @Unique public field {} in {} because it already exists in {}. "
@@ -592,7 +595,8 @@ protected void attachFields(MixinTargetContext context) {
 
             // Check that the shadow field has a matching descriptor
             if (!target.desc.equals(mixinField.desc)) {
-                throw new InvalidMixinException(this.mixin, "The field " + mixinField.name + " in the target class has a conflicting signature");
+                throw new InvalidMixinException(this.mixin, String.format("The field %s in the target class has a conflicting signature",
+                        mixinField.name));
             }
 
             if (!target.name.equals(mixinField.name)) {
@@ -640,12 +644,13 @@ protected boolean validateField(MixinTargetContext context, FieldNode field, Ann
         // Imaginary super fields get stripped from the class, but first we validate them
         if (Constants.IMAGINARY_SUPER.equals(field.name)) {
             if (field.access != Opcodes.ACC_PRIVATE) {
-                throw new InvalidMixinException(this.mixin, "Imaginary super field " + context + "." + field.name
-                        + " must be private and non-final");
+                throw new InvalidMixinException(this.mixin, String.format("Imaginary super field %s.%s must be private and non-final", context,
+                        field.name));
             }
             if (!field.desc.equals("L" + this.mixin.getClassRef() + ";")) {
-                throw new InvalidMixinException(this.mixin, "Imaginary super field " + context + "." + field.name
-                        + " must have the same type as the parent mixin");
+                throw new InvalidMixinException(this.mixin,
+                        String.format("Imaginary super field %s.%s must have the same type as the parent mixin (%s)", context, field.name,
+                                this.mixin.getClassName()));
             }
             return false;
         }
@@ -727,8 +732,12 @@ protected static String getDynamicInfo(FieldNode method) {
     }
 
     private static String getDynamicInfo(String targetType, AnnotationNode annotation) {
-        String dynamic = Annotations.<String>getValue(annotation);
-        return Strings.isNullOrEmpty(dynamic) ? "" : String.format(" %s is @Dynamic(%s)", targetType, dynamic);
+        String description = Strings.nullToEmpty(Annotations.<String>getValue(annotation));
+        Type upstream = Annotations.<Type>getValue(annotation, "mixin");
+        if (upstream != null) {
+            description = String.format("{%s} %s", upstream.getClassName(), description).trim();
+        }
+        return description.length() > 0 ? String.format(" %s is @Dynamic(%s)", targetType, description) : "";
     }
 
 }