Propagate MixinErrors as MixinApplicatorExceptions with context#118
Open
comp500 wants to merge 1 commit intoFabricMC:mainfrom
Open
Propagate MixinErrors as MixinApplicatorExceptions with context#118comp500 wants to merge 1 commit intoFabricMC:mainfrom
comp500 wants to merge 1 commit intoFabricMC:mainfrom
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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. TheseMixinErrors don't currently get handled byIMixinErrorHandlerhandlers, 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 existingIMixinErrorHandlerimplementations 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.MixinApplicatorStandardseems like the most appropriate place to insert context of the current mixin and activity stack similarly to the existingExceptionhandling, and doesn't requireMixinErrorto change how it is constructed and used by internal Mixin code.(upstream equivalent PR: SpongePowered#640)