[lukebemish]

In light of the recent discussion on discord -- this seems like a useful thing to have in-source support for, right?

Like interface injection, JST has to generate stubs occasionally here -- for the EnumProxy fields or parameter providing methods that enum extensions can use.

This PR is marked as a draft for now. It is blocked by:

• Matching MDG PR
• Matching NG PR
• Matching FML work (Strip JST-Injected Enum Extensions FancyModLoader#412)
• Implementing IndexedEnum and company support
• Handle ambiguity with multiple ctors and make sure to pick the right one when grabbing indices for getParameter

In particular -- FML needs to handle cases where enum entries to be injected exist already gracefully, including handling ordering so that "dev env with source-injected plus runtime-injected-from-a-dependency enum entries" and "prod server being tested against" get the same enum ordering.

This system validates that the targeted class extends a provided interface (presumably, you'd pass it IExtensibleEnum for neoforge-mode, and nothing for "vanilla" mode).

One question I have -- I currently hard-code the EnumProxy class used in stubs. Should this be un-hard-coded as an argument or the like?

[neoforged-pr-publishing]

• Publish PR to GitHub Packages

Last commit published: f6b881712d335b77985df7f8127e0061db9a1c23 - version: 2.0.17-pr-62-enum-extension

PR Publishing

The artifacts published by this PR:

• 📦 net.neoforged.jst:jst-api:2.0.17-pr-62-enum-extension
• 📦 net.neoforged.jst:jst-cli:2.0.17-pr-62-enum-extension
• 📦 net.neoforged.jst:jst-cli-bundle:2.0.17-pr-62-enum-extension

Repository Declaration

In order to use the artifacts published by the PR, add the following repository to your buildscript:

repositories {
    maven {
        name = "Maven for PR #62" // https://github.com/neoforged/JavaSourceTransformer/pull/62
        url = uri("https://prmaven.neoforged.net/JavaSourceTransformer/pr62")
        content {
            includeModule("net.neoforged.jst", "jst-api")
            includeModule("net.neoforged.jst", "jst-cli")
            includeModule("net.neoforged.jst", "jst-cli-bundle")
        }
    }
}

[lukebemish]

Whoops, just realized I think there's some indexed-enum stuff or the like this needs to support, I'll add that to the TODO list too

[lukebemish]

PR publishing is currently failing due to the fat cli jar being too big or something...

[sciwhiz12]

Fixed. We increased the limit of PR publishing to 75 MB.

[Matyrobbrt]

Just a small comment. Other than that, (though not blocking), I think we might want to consider creating a generalised stub API given that there now would be two plugins that generate stubs.

That said, I did notice that some people requested that this feature be usable for mixin-based extensions too. I'm not sure how that would be best approached - it would probably involve additional enum extension files that are only for JST (so you only configure them in the buildscript and not the mod metadata), but it would run into issues if we enable checking for required interfaces - we might want to consider adding the possibility of a flag in the json file (that is "experimental") that skips said check? I'm also not sure exactly how mixin behaves in that case when the enum entry exists at runtime already.

[Matyrobbrt]

This proxy type should be configurable.

[Matyrobbrt]

Suggested change

	@CommandLine.Option(names = "--enum-extensions-required-interface", description = "The name (binary representation) of an interface to enforce that extendeable enums implement")
	@CommandLine.Option(names = "--enum-extensions-required-interface", description = "The name (binary representation) of an interface to enforce that extendable enums implement")

Keep in mind you'll have to update the readme too.

[lukebemish]

Practically speaking, how/if we want this to handle mixin injected entries is another issue. Personally, I don't see a major reason for it -- additions to non extensible enums are after all presumed to be rather unstable to begin with -- but my personal opinions aside it's certainly possible. However, doing it would take a bit extra work to play nice with recomp, ensuring consistent ordering behavior between recomp/binpatch/prod, etc.; I'd need to do some poking into how mixin currently handles all that and how it acts if you inject already existing entries. Similarly, some level of support would be needed in installertools to handle binpatch-only on CI. I'm also not sure how mixin handles abstract enums -- anonymous class name drift is also a potential worry in that case? It is likely not as simple, unfortunately, as just sticking an extra enum injection file pointed at the relevant enum if we want it to act "correctly", though definitely could be poked as a follow up. I think for now the validation can be kept as is and if that is implemented in the future an opt out can be added along with any other (if any) extra logic that needs.

Agreed about the unified stubs API, it might make stuff simpler. Not entirely sure there's a non breaking way to do that -- is it worth biting the bullet and just making that a breaking change? I'll poke it when I'm less ill and have the time.

[lukebemish]

For posterity: as discussed on discord, making mixin enums work with this is a lot of work and a whole other project in and of itself, and would involve mostly some changes to mixin to make it able to properly strip the post-recomp artifacts of the injected entry (field, initializer, presence in values array, etc.) and is most definitely outside the scope of this PR (and linked PRs).

[Matyrobbrt]

Agreed about the unified stubs API, it might make stuff simpler. Not entirely sure there's a non breaking way to do that -- is it worth biting the bullet and just making that a breaking change? I'll poke it when I'm less ill and have the time.

Breaking changes can be made quite liberally in JST, we control where it's used and we can update it rather easily.