Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Propagate MixinErrors as MixinApplicatorExceptions with context #118

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

comp500
Copy link

@comp500 comp500 commented Aug 14, 2023

Many internal Mixin errors, particularly InjectionErrors, are raised as the result of invalid mixins (e.g. failed injection check as the mixin was compiled for a different Minecraft version) or mixin conflicts that are not issues in Mixin itself. These MixinErrors don't currently get handled by IMixinErrorHandler handlers, and when propagated to more generic error handling code don't have much machine-readable context.

Wrapping these errors in MixinApplicatorExceptions allows them to be handled by existing IMixinErrorHandler implementations in the apply error phase, and attaching this context allows Mixin callers to reason about the cause of mixin errors and present them in a more user-friendly way.

MixinApplicatorStandard seems like the most appropriate place to insert context of the current mixin and activity stack similarly to the existing Exception handling, and doesn't require MixinError to change how it is constructed and used by internal Mixin code.

(upstream equivalent PR: SpongePowered#640)

Many internal Mixin errors, particularly InjectionErrors, are raised as
the result of invalid mixins (e.g. failed injection check as the mixin
was compiled for a different Minecraft version) or mixin conflicts that
are not issues in Mixin itself. These MixinErrors don't currently get
handled by IMixinErrorHandler handlers, and when propagated to more
generic error handling code don't have much machine-readable context.

Wrapping these errors in MixinApplicatorExceptions allows them to be
handled by existing IMixinErrorHandler implementations in the apply
error phase, and attaching this context allows Mixin callers to reason
about the cause of mixin errors and present them in a more user-friendly
way.

MixinApplicatorStandard seems like the most appropriate place to insert
context of the current mixin and activity stack similarly to the
existing Exception handling, and doesn't require MixinError to change
how it is constructed and used by internal Mixin code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant