search

SpongePowered / Mixin

Mixin is a trait/mixin and bytecode weaving framework for Java using ASM
MIT License
1.41k stars 194 forks source link

Allow for shadowed fields to be declared using super types #284

Open FalseHonesty opened 6 years ago

FalseHonesty commented 6 years ago

An issue I'm currently having with mixins is shadowing a field whose type is package private. The field is in SoundManager, and that is the class that I'm mixing into. The type of this field that I'm talking about is SoundManager.SoundSystemStarterThread. This package-private class extends SoundSystem, and that is the class that has all the functionality I want to use in it.

Ideally, I would be able to shadow this field by specifying the super type, SoundSystem, like so:

@Mixin(SoundManager.class)
public class MixinSoundManager {
    @Shadow
    private SoundSystem sndSystem;
}

If it would be possible to check if the type specified in the shadow field is a super type of the mixin class's field at run-time, and allow it if so, that would be optimal. However, if not, another possible solution is to add a signature property to the @Shadow annotation that will specify the actual signature of the field so Mixins can correctly find the field. These might not even be the best solutions, so I'm welcome to feedback or other ideas :)

I am not sure if this functionality is possible, but it would be very useful. Access Transformers are not an option in the environment I'm targeting, and reflection is definitely not the optimal solution, so I'm hoping that this is possible.

Mumfrey commented 6 years ago

I see exactly what you're trying to do and it's absolutely not a bad use-case. Neither of the proposed solutions are awful either so I will address each:

Ideally, I would be able to shadow this field by specifying the super type, SoundSystem, like so

This is probably the cleanest solution, combined with a possible overload of @Coerce to indicate the type variance. I don't see any particular drawbacks to this approach. It also doesn't conflict with the roadmapped feature I mention below.

another possible solution is to add a signature property to the @Shadow annotation that will specify the actual signature of the field so Mixins can correctly find the field

This isn't the worst idea and does make it marginally more reliable to verify the field at runtime. However it's not particularly elegant and doesn't actually really grant any useful functionality and in fact could potentially be another PITA maintenance burden of yet another string-encoded signature.

I am not sure if this functionality is possible, but it would be very useful. Access Transformers are not an option in the environment I'm targeting, and reflection is definitely not the optimal solution, so I'm hoping that this is possible.

I hate access transformers anyway, so I'm with you on that one. Reflection certainly shouldn't be discounted, Java reflection is very good so while it's not necessarily "optimal", for a one-time fetch and assignment of a member field it's only a style concern over a performance once. Sure, reflection might be bad inside a tight loop but once at startup is never going to register.

I've been conscious of this wider issue for some time and have laid a lot of the groundwork for a roadmapped feature that I call Shadow Classes. Shadow Classes work around the visibility issue by allowing surrogates to be created which shadow package-private classes. This has an advantage over coercion in that it allows the shadow to be used in method signatures, as a supertype for mixins themselves and also as a drop-in replacement for soft targets which have some improved functionality.

This is another reason I'd prefer not to have a signature decoration, because ultimately I'd like this situation to be handled by using Shadow Classes instead of any manipulation of the @Shadow itself.

In the short term, all of my focus is on the ModLauncher transition so new features are paused at the moment because I'm tearing the codebase apart and complicating that right now would be quite disruptive. I also can't do new builds at the moment because I'm waiting for my EV certificate renewal from Comodo. I would therefore recommend using reflection in the short term, it's the simplest solution and doesn't carry any performance penalties in this scenario.

Consider this accepted though and I'll put it on the list of pending features to incorporate in 0.8.