Skip to content

Safe Transformer Types #102

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

Merged
merged 3 commits into from
Sep 24, 2023
Merged

Conversation

Su5eD
Copy link
Member

@Su5eD Su5eD commented Mar 4, 2022

The issue

Currently, modlauncher determines the type of ITransformers at runtime by getting their generic T type. However, this fails when the interface is implemented in a parent class.

See also:

Trouble with generics

To address the generics issue, we'll use even more generics.
We can't use bounded types because ASM Nodes don't share a common parent, and enums aren't an option either since they can't have generic parameters.

The solution

So, we'll introduce a transformer type class whose generic T type must correspond to the transformer's T type. And by making it a "fake enum" with predefined instances we can effectively limit ITransformer to those types.
This type is then returned from the transformer in a new getTargetType method.

Important disclaimer: This PR introduces BREAKING changes

@cpw
Copy link
Member

cpw commented Mar 6, 2022

This will probably have to wait for the 1.19 release cycle, because it's a deeply breaking change.

@marchermans marchermans added the for next mc release Indicates issues or pull requests which can only be merged or closed in the next MC release window. label Mar 21, 2022
@marchermans marchermans self-assigned this Mar 21, 2022
@marchermans marchermans added this to the MC 1.19 milestone Mar 21, 2022
@marchermans marchermans marked this pull request as draft March 21, 2022 08:35
@marchermans marchermans added the breaking-change Indicates that the Pull Request or Issue is a breaking change label Mar 21, 2022
@cpw
Copy link
Member

cpw commented Jun 14, 2023

This is a substantial change. What does it give us? Does it address any key bugs?

@cpw
Copy link
Member

cpw commented Jun 14, 2023

This will be modlauncher 11. new major number. We'll need to update the entire stack.

@cpw
Copy link
Member

cpw commented Jun 14, 2023

We might need to make sure we have backward compatibility due to problems with tech like mixins, which will require significant refactors to land on this.

@Su5eD Su5eD force-pushed the feature/transformer-types branch from 6c73b73 to f9a5356 Compare June 15, 2023 12:54
@Su5eD Su5eD marked this pull request as ready for review June 15, 2023 12:54
@cpw
Copy link
Member

cpw commented Sep 24, 2023

letsroll

@cpw cpw self-requested a review September 24, 2023 18:07
Copy link
Member

@cpw cpw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ye

@cpw cpw merged commit a032080 into McModLauncher:main Sep 24, 2023
@Su5eD Su5eD deleted the feature/transformer-types branch September 24, 2023 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Indicates that the Pull Request or Issue is a breaking change enhancement for next mc release Indicates issues or pull requests which can only be merged or closed in the next MC release window.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants