AkashiiKun / MoreAxolotlVariantsAPI-Common

Axolotl Variants Extender API!
Other
3 stars 4 forks source link

Enum extension after class load is dangerous #1

Closed RaymondBlaze closed 1 year ago

RaymondBlaze commented 1 year ago

Enum.values() and Enum.valueOf(String) are based on the value of synthetic field Enum.$VALUES.

Currently, the api is populating new enum instances at runtime and not adding them the synthetic field in <clinit>.

This will break the mentioned methods, which could be dangerous and confusing for other mods.

This could be solved by regretting to the old <clinit> mixin, and use Service to fetch other mods request on extending the enum at such an early timing.

AkashiiKun commented 1 year ago

Other mods will already break due to the renderer's mixin that changes the textures location to a defined namespace. More Axolotls break this API because they also modify the renderer. This API is made to be an API anyone can use with ease.

RaymondBlaze commented 1 year ago

I'm pretty sure after testing

@SubscribeEvent
public static void onPlayerLogin(PlayerEvent.PlayerLoggedInEvent event) {
    Axolotl.Variant[] variants = Axolotl.Variant.values();
    LOGGER.debug("Axolotl Variants: " + Arrays.toString(variants));
}

would result in Axolotl Variants: [LUCY, WILD, GOLD, CYAN, BLUE], with mavapi and mavm installed.

I have no idea why mavm itself would even work with the values() method.

An ASM based safe enum extension would be like this one in Fabric-ASM, nothing should go wrong as the whole bytecode is modified under expected format. This is just for referencing the realization, mavapi is xplatform so we can't really introduce Fabric-ASM here.

AkashiiKun commented 1 year ago

https://github.com/AkashiiKun/MoreAxolotlVariantsAPI-Common/blob/870909d79e21a8f9b1461b08eb2f8ecd32a6acf3/Common/src/main/java/io/github/akashiikun/mavapi/v1/impl/mixin/VariantWidener.java#L28

Is literally adding them to the enum field. That's why it works.

RaymondBlaze commented 1 year ago

https://github.com/AkashiiKun/MoreAxolotlVariantsAPI-Common/blob/870909d79e21a8f9b1461b08eb2f8ecd32a6acf3/Common/src/main/java/io/github/akashiikun/mavapi/v1/impl/mixin/VariantWidener.java#L28

Is literally adding them to the enum field. That's why it works.

What I'm suggesting is that mutating a final synthetic field after class load may not work properly, and the test code and result already showed an unexpected behavior: missing all the added variants from the method call Axolotl.Variant.values().

I have assumption that your code about serializing, deserializing, adding to textures would work is because they are all mixined to Minecraft's class, while the test code is in a separate class and loaded in a different module (FML loads mods into different modules with the name of their modid).

As enum classes are a part of Java's builtin types, there could exist JVM magic which hackly handles related fields/methods, which could possibly explain unexpected behavior in different modules. They might even fetch the enum constants from the java.lang.Class, which actually caches the enum constants on method call (See Class.getEnumConstantsShared() and Class.enumConstantDirectory()).