LlamaLad7 / MixinExtras

Companion library to SpongePowered Mixin with many custom injectors for a more expressive experience.
MIT License
304 stars 17 forks source link

Adds more support for changing 'return' and 'throw' statements #16

Closed Papierkorb2292 closed 7 months ago

Papierkorb2292 commented 1 year ago

Methods for the required bytecode validation are in InjectorUtils. My fork contains possible wiki entries representing the changes.

LlamaLad7 commented 1 year ago

My initial instinct is that @InitVariable and support for wrapping/replacing method exits is too niche and confusing to be added to the main repository right now. The verification logic is also insanely complex, and while it seems you've certainly had a good go at it, I'm hesitant to expose something that finnicky to the masses, especially when as far as I can remember you're the only person I've encountered who's ever needed to wrap a method exit. Modifying the thrown exception may be more useful, and is certainly less complicated, so for the time being my recommendation is that you split that into a separate PR and keep the complex stuff in your own submodule / library for now, though I may re-evaluate at some point in the future.

Papierkorb2292 commented 1 year ago

My initial instinct is that @InitVariable and support for wrapping/replacing method exits is too niche and confusing to be added to the main repository right now. The verification logic is also insanely complex, and while it seems you've certainly had a good go at it, I'm hesitant to expose something that finnicky to the masses, especially when as far as I can remember you're the only person I've encountered who's ever needed to wrap a method exit. Modifying the thrown exception may be more useful, and is certainly less complicated, so for the time being my recommendation is that you split that into a separate PR and keep the complex stuff in your own submodule / library for now, though I may re-evaluate at some point in the future.

Should the BeforeThrow injection point stay in the more complex PR? I only added it in case a local variable is thrown, so it's probably not that useful and the implementation of the registration might be a bit messy.

KorewaLidesu commented 1 year ago

Should the BeforeThrow injection point stay in the more complex PR? I only added it in case a local variable is thrown, so it's probably not that useful and the implementation of the registration might be a bit messy.

Could BeforeThrow be PR? Here is niche case that I encounter :rip:

if (!(pipe.pipe instanceof PipeLogisticsChassis)) {
   throw new TargetNotFoundException("Couldn't find " + clazz.getName() + ", pipe wasn't a chassi pipe", this);
}
Papierkorb2292 commented 1 year ago

Could BeforeThrow be PR? Here is niche case that I encounter :rip:

if (!(pipe.pipe instanceof PipeLogisticsChassis)) {
   throw new TargetNotFoundException("Couldn't find " + clazz.getName() + ", pipe wasn't a chassi pipe", this);
}

In such cases the constructor of the exception can be targeted with a shift of AFTER.

KorewaLidesu commented 1 year ago

In such cases the constructor of the exception can be targeted with a shift of AFTER.

AHHHHHHH! Thanks for the solutions. i'm brainded now Luv u <3

LlamaLad7 commented 7 months ago

Going to close for now. May revisit this in the future but I think it's too finicky atm. Targeting throws will be addressed by an upcoming feature anyway.