LlamaLad7 / MixinExtras

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

Add ability to add exception handlers (try/catch/finally) #8

Closed Earthcomputer closed 2 years ago

Earthcomputer commented 2 years ago

This is kind of like WrapWithCondition but for exception handlers. Currently you need to Overwrite or Redirect these in, which is silly.

LlamaLad7 commented 2 years ago

Good idea, however I feel it may be more useful to be able to wrap a larger section of the method, so I'm thinking of perhaps not using an At at all for this, and just letting people use a Slice to determine what to wrap, the whole method being the default.

LlamaLad7 commented 2 years ago

The best implementation I can think of for this would be simply passing the exception to the handler method inside the catch block, however that would become a lot more complicated with finally as well. Is there a use-case for finally you can think of which wouldn't just be doable with an Inject (and then doing the same thing in your catch handler, e.g. by calling the inject handler or another method with the shared logic)?

Earthcomputer commented 2 years ago

I don't understand, what makes finally difficult to implement? Just duplicate the call to the handler method.

LlamaLad7 commented 2 years ago

My thought was the handler method would specifically represent the catch block, and as such would be given the caught exception as a parameter. What would it be given if it were also to be called at the end of the try block? Passing null and relying on the handler to differentiate between the try and the catch feels kind of strange imo.

However, a larger problem that I forgot about initially is the stack being cleared when going to a catch block. In the case where a smaller portion of code is being wrapped instead of the whole method, this would either require limiting this feature's use to when there is an empty stack, or doing static analysis and storing the whole stack in locals. Neither of these options is particularly great. For that reason I feel this may be better represented by the general chainable redirects idea, in which you receive a reference to the call which was there before you.

Earthcomputer commented 2 years ago

My thought was the handler method would specifically represent the catch block, and as such would be given the caught exception as a parameter. What would it be given if it were also to be called at the end of the try block? Passing null and relying on the handler to differentiate between the try and the catch feels kind of strange imo.

Finally blocks don't have access to the exception either, so there's no need to pass it as a parameter.

However, a larger problem that I forgot about initially is the stack being cleared when going to a catch block [...]

That's fair.

LlamaLad7 commented 2 years ago

Closing for now as out of scope / far too fiddly.

LlamaLad7 commented 2 years ago

At least for individual calls this is possible via WrapOperation.