Add support for adding enum constants

[Earthcomputer]

... To end the reflection hacks and improper ASM once and for all! :P

Rationale

• Sometimes adding enum constants is the only way to do something you want, at least that gives maximum compatibility to other mods.
• Even when the above is not the case, it would sometimes be the cleanest solution.
• It stops modders from doing it wrong with ASM, or at least in an incompatible way.
• It will stop people doing those god awful reflection hacks that break everything.

Myths

Q: This will break switch statements A: Not according to JLS 13.4.26:

Adding or reordering constants in an enum will not break compatibility with pre-existing binaries. If a pre-existing binary attempts to access an enum constant that no longer exists, the client will fail at run time with a NoSuchFieldError. Therefore such a change is not recommended for widely distributed enums. In all other respects, the binary compatibility rules for enums are identical to those for classes.

This clause in the JLS is not new, and has existed since enums were introduced.

Q: What if compilers are non-compliant to the JLS? A: I tested all the following compilers, and all of them conform to the JLS:

• javac 1.8.0_242
• javac 1.6.0_45
• javac 1.5.0
• ECJ 3.20.0 (compatibility level 5)
• ECJ 3.5.0 (compatibility level 5)

The full output of each of these compilations can be found here (files annoyingly appear in reverse order for me). Note that there is an unrelated bug in ECJ related to the thread-safety of enum switch statements.

Thus, there are no compilers in common use which are non-compliant to this clause in the JLS. If a modder is using a compiler that happens to be non-compliant, then a) I would like to know what compiler that is, and b) it's a bug in that compiler, it wouldn't be Mixin's fault.

Q: What other problems could adding enum constants cause? The rest of these are related to direct use of ordinal() by client code, especially when used to serialize an enum constant. In terms of network serialization, this would have only been a problem if the same mod wasn't installed on the client and the server; if serializing to the disk, well, it was the client code's fault for using ordinal() in this way anyway. If Mixin were to additionally impose the constraint that enum constants should only be added at the end of the enum, it would mitigate this problem.

Challenges

There are three challenges (aside from deciding on the frontend) to implementing this feature.

values and valueOf values() is typically implemented by storing a static field called $VALUES (javac) or ENUM$VALUES (ECJ), but the name of this field can change during obfuscation. This field could still be found by looking for synthetic static fields of the type of the array of the containing enum. Now the tricky bit, changing <clinit> to add the enum constants to the array. You would need to find the putstatic instruction which sets the field you found, then either replace it with another sequence of instructions or trace the array local variable back from there. A far easier, and in my opinion better, alternative would be to simply overwrite values() with a new implementation incorporating the new values (which would include creating our own array field and injecting into the head of <clinit> to initialize it). This is a better solution because it is compiler-independent and isn't affected by obfuscation.

Both javac and ECJ implement valueOf() by calling Enum.valueOf, which would already work with the existing modifications we have made, but this is not required by the spec, so we should probably also overwrite valueOf() to do this just to be safe.

Another thing to note is that some enum classes may call their own values() method later in the <clinit> method, so to yield correct results, our own values array has to be fully created at the head of <clinit>. This would be complicated if were to use the naive approach of storing the enum constants in a static field in the mixin (which may or may not end up in the target class depending on whether it's an accessor mixin), which are initialized inline (i.e. in the <clinit> method in the mixin). It would be hard to distinguish this enum constant initialization, which would need to go in the head of the target <clinit> from other things which may be in the mixin's <clinit>. Furthermore, you could break the semantics of <clinit> by reordering these operations. So ya, better not use that approach.

The constructor This is definitely the bigger problem. ECJ and javac add two synthetic parameters, String name and int ordinal (in that order), to every constructor in the enum. It is logical that these will form part of the constructors in any compilation of an enum, as they are required by the Enum constructor. However I could not find this specified anywhere, which makes this the biggest assumption we would have to make; compilers could swap these parameters or put them at the end of the parameter list instead, although I can't see why any compiler would do either of these things.

Obviously, these constructors are private, but this can easily be solved with creating static shadow constructors which only work inside <clinit>. These shadow constructors would also not include the name or ordinal.

Another less obvious issue is that it's possible that the JVM may enforce that enum constructors can only be called in <clinit> in that enum. This may be mitigated by inlining a shadow constructor.

Abstract enum classes That is, enums containing abstract methods. Yeah... I don't want to think about these right now. It would be easier to just not support them for now and possibly add support for them in the future.

Proposal for the API

With these challenges in mind, here's my proposal for how the API would look like for adding enum constants. This is only a suggestion and is up for discussion.

public class SomeEnumExtensions {
   public static final String MY_NEW_CONSTANT_NAME = "MY_NEW_CONSTANT";
   public static final SomeEnum MY_NEW_CONSTANT = SomeEnum.valueOf(MY_NEW_CONSTANT_NAME);
}

@Mixin(SomeEnum.class)
public class MixinSomeEnum {
   // Creating this in a method here allows us to copy the contents of the method to the head of <clinit> rather than the tail.
   @EnumConstant(SomeEnumExtensions.MY_NEW_CONSTANT_NAME)
   private static SomeEnum createMyNewConstant() {
      return constructor("custom constructor arguments", 1.5f);
   }

   // The behaviour of @Shadow would be modified to allow for constructors if the target class is an enum, and it would ignore the name and ordinal in this case
   @Shadow
   private static SomeEnum constructor(String foo, float bar) {
      throw new UnsupportedOperationException();
   }
}

I couldn't think of a more idiomatic way of binding the enum constant to a field, both because of the aforementioned <clinit> reasons, and because such a field wouldn't be accessible outside the mixin anyway, and not easy to access with a duck interface because it would be static (and useless if there are no instances of the enum by default). However, this valueOf solution is clean enough, comparably to how duck interfaces were used instead of accessor mixins before they existed.

[gabizou]

Having given some thought and a brief discussion in discord, I'm just going to copy paste some points I've made and propositions of how this could look dependent on some more discussion with Mumfrey.

Concerns

Adding a new Enum value at the byte code level is fine with the following highlighted understandings:

• Any precompiled default switch cases will 100% be reached if the new enum value is used for those methods
• AIOOB is avoided since the switch table arrays are constructed based on the values() array which will already be populated with any and all mixin'ed enum values
• Removing existing enum values is not at all within scope
• First come first serve enum additions are a moot point, in general it's not at all valid to rely on any ordinal() values of custom enum additions
• We need some way to target some specific custom enum classes that may have method overrides
• Object values used in constructors need to be statically defined and cannot have static recursive initialization loops (stack overflows at runtime)

Proposal for how this could look

While I appreciate the usage of some familiar annotations, I don't believe for a single second that @Shadow is an appropriate usage since there's a few things that would need to be addressed, and I'll mention them a bit later.

First, let's start with addressing the question "How should we encourage accessing these new custom enum values?" Answer is potentially as simple as "How do we access static fields in accessor mixins?"

@Mixin(ItemTier.class)
public interface ItemTierAccessor {
  @EnumCreator(name = "SUPER_DUPER_DIAMONDS_AND_STARS_TIER")
  public static ItemTier getSuperDiamondsAndStarsTier() {
    throw new UnsupportedOperationException("Untransformed mixing!");
  }
}

In the end, this would look relatively simple, we can easily access the enum value with ItemTierAccessor.getSuperDiamondsAndStarsTier() which would give us a valid ItemTier, as expected! And, we don't introduce any sort of class referencing rewrites, nor do we explicitly create artificial constructors, we just recycle the existing ItemTier(String name, int ordinal) constructor, add that to the class initializer, add a custom final field maybe Lnet/minecraft/item/ItemTier;enumcreator$SUPER_DUPER_DIAMONDS_AND_STARS_TIER$sessionId:Lnet/minecraft/item/ItemTier; as an example, since we don't have to worry about actually accessing the field outside the now-transformed interface static method.

BUT WAIT! What about different constructors? What about constructors that take objects?!

So, I had a brief thought about this, and I think it's very well possible to do some fanciful constructor writing here. Just like how we have @Constant to detect what constant value is actually filled in, we can somewhat recycle @Constant in this case where we have the following:

@Mixin(ItemTier.class)
public interface ItemTierAccessor {
  @EnumCreator(name = "SUPER_DUPER_DIAMONDS_AND_STARS_TIER",
    args = {
      @Constant(intValue = 4), 
      @Constant(intValue = 2048), 
      @Constant(floatValue=3.5F), 
      @Constant(objectValue = "Lcom/example/mymod/CustomEnumValues;MY_CONSTANT_INGREDIENT_SUPPLIER:Ljava/util/function/Supplier;")
    }
  )  
  public static ItemTier getSuperDiamondsAndStarsTier() {
    throw new UnsupportedOperationException("Untransformed mixing!");
  }
}

And with this, we have Mixin being able to verify that the field value MY_CONSTANT_INGREDIENT_SUPPLIER is actually static, but we could also further enhance the access by saying it's a method instead! Because ultimately, as long as the access is static, we're fine realistically. And what this gives us is something very close to matching the required arguments to match against a specific constructor (with the added name obviously).

But what if I want to use a particular enum type as a base? Like if one enum already exists where it overrides a method?

Well, for this I'd go as far as suggesting we might be able to do an extensionTarget = ItemTier.DIAMOND on the @EnumCreator variation, and if we needed to explicitly override/soft-implement a method for our custom type, then we'd have issues and that I don't have a solution for.

[LudoCrypt]

In one of my Minecraft mods i have something like this which adds onto mixins

@Mixin(NoteBlockInstrument.class)
@Unique
public abstract class NoteblockInstrumentMixin {

	@Shadow
	@Final
	@Mutable
	private static NoteBlockInstrument[] $VALUES;

	private static final NoteBlockInstrument MUSIC_BOX = noteblockExpansion$addVariant("MUSIC_BOX", "music_box", NoteblockSounds.MUSIC_BOX);

	@Invoker("<init>")
	public static NoteBlockInstrument noteblockExpansion$invokeInit(String internalName, int internalId, String name, SoundEvent sound) {
		throw new AssertionError();
	}

	private static NoteBlockInstrument noteblockExpansion$addVariant(String internalName, String name, SoundEvent sound) {
		ArrayList<NoteBlockInstrument> variants = new ArrayList<NoteBlockInstrument>(Arrays.asList(NoteblockInstrumentMixin.$VALUES));
		NoteBlockInstrument instrument = noteblockExpansion$invokeInit(internalName, variants.get(variants.size() - 1).ordinal() + 1, name, sound);
		variants.add(instrument);
		NoteblockInstrumentMixin.$VALUES = variants.toArray(new NoteBlockInstrument[0]);
		return instrument;
	}
}

https://github.com/LudoCrypt/Noteblock-Expansion-Forge/blob/main/src/main/java/net/ludocrypt/nbexpand/mixin/NoteblockInstrumentMixin.java and this seems to work exactly fine!