Spongeapi: IDEs complain about CatalogTypes' initial null values

[CrystalMageX]

Currently all CatalogTypes' catalogs statically set everything to null, and have the implementation access check suppressing reflection on a static final field and add in the objects.

This is _very_ bad way of doing things because:

• [x] Our IDEs paint our code yellow screaming at us complaining: "Potential NullPointerException: must be null here!" This may distract the developer and delay the development, or if these warnings are turned off, lead to crashes and a waste of time debugging actual NPEs.
• [x] This will cause SecurityExceptions when porting Sponge to platforms that have tight security, and let users relax when downloading plugins from random websites.
• [x] It is a bad idea to use reflection to do things Java prevents you from doing on purpose. Java, a programming language used by a much larger community, including non-Minecraft users, must have a good reason for every aspect of its design. Even when _Forge mods had to force something to change_, they used _access transformers, a (only slightly) better way to do things_. But the target code was Mojang's, which they have no control over, and we are dealing with code that we are developing right here. This give us even less reason to use these reflection hacks.

Possible solutions are:

• ✘ Remove the static final modifiers. A malicious plugin (see the first answer's example here) could change them to whatever it likes, using the exact same way as how Sponge implementation do this currently. This is a bad solution however, because this makes it harder to change the values by mistake. See the next solution for the proper fix.
• ✔ Properly initialize the fields in the static constructor. A way to do this is getting the GameRegistry and asking it for each type. If the implementation doesn't have them ready, have passthrough proxy classes that have their target set using a package private way.

[kashike]

I've been meaning to bring this up - the most annoying thing for me, and others I'm sure, is that my IDE throws warnings at me (as you've mentioned). I'm :+1: if a solution comes up that always works.

[Deamon5550]

Having them initialize to something referencing the game registry is just asking for strange errors due to when the class is loaded, better to initialize them to some dummy object and replace them in the same way as we currently do.

[ST-DDT]

I also thought about this one.
Using Proxy Objects has the drawback of not been equal using ==. Or if you require it for all instances to be proxies then you have extra memory consumption and extra performance loss for wrapping/unwrapping them.

I thought about tricking the IDE compiler so it does not complain about the potential null.

public static final FooType BAR_VALUE = CatalogType.fake();

private static Object fake = new Object();

public <T> T fake() {
    return (T) fake;
}

And then this method is overwritten at runtime by Mixins to just return null.

[kashike]

I had the same thought, @ST-DDT, and I like it if it'll work. :+1:

[Aaron1011]

@CrystalMageX:

Our IDEs paint our code yellow screaming at us complaining: "Potential NullPointerException: must be null here!" This may distract the developer and delay the development, or if these warnings are turned off, lead to crashes and a waste of time debugging actual NPEs.

This isn't really much of an issue, especially because of our use of Optional. Any method or field which has a non-Optional type should _never_ return null. While having the fields initialized to null may be slightly confusing for new users, this is easily rectified by reading the docs.

This will cause SecurityExceptions when porting Sponge to platforms that have tight security, and let users relax when downloading plugins from random websites.

Given that the entire Sponge + Forge ecosystem not only transforms classes through the LaunchWrapper classloader, but dynamically builds and classloads new classes (event handlers), security exceptions from reflection are pretty much the least of our concerns.

It is a bad idea to use reflection to do things Java prevents you from doing on purpose. Java, a programming language used by a much larger community, including non-Minecraft users, must have a good reason for every aspect of its design. Even when Forge mods had to force something to change, they used access transformers, a (only slightly) better way to do things. But the target code was Mojang's, which they have no control over, and we are dealing with code that we are developing right here. This give us even less reason to use these reflection hacks.

See above.

[ST-DDT]

@Aaron1101 Regarding your first point.
This might be true for the Sponge API but others such as the Java API are not that well defined because they are too old.

[kashike]

@Aaron1011 It's annoying as hell having your IDE warn about every use of catalog type fields.

[gabizou]

From what I'm gathering from this discussion is that the REAL issue is the null checks in IDEs. Some ideas are being discussed, some as far as generating dummy implementations of the catalog types where by default they throw a form of UnsupportedOperationException unless the fields are replaced as we are currently replacing them.

@Aaron1011 I know it's very well possible to do such a thing, either to adapt the event generation to expand it to generate these catalog classes as necessary, and have simple static methods that simply take the catalog class itself.

[CrystalMageX]

@gabizou:

From what I'm gathering from this discussion is that the REAL issue is the null checks in IDEs.

For some, the IDEs being annoying might be the worst thing they use there CatalogTypes. For others, they want a better way to write their ported Sponge's initializers. Some might wonder why we need to use reflection to catalog something as simple as TextColors!

A solution that would satisfy both kinds of implementations that need to initialize stuff later in the loading process, and implementations that don't, and also fix the IDE warnings would be:

• Create a GameRegistry.staticInitializeType(Class<? extends CatalogType> clazz, String id) method, and call it from all catalogs.
• In implementations that require initializing the types at runtime, simply return null; and _keep using_ reflection, with absolutely _no other changes_ required, and with the _same results_ as before.
• In implementations that do not require initializing, and when implementations that do require doing so, initialize something more simple than a TextColor, things like BanTypes, they can initialize it cleanly, and _without_ reflection there.

@Aaron1011:

Given that the entire Sponge + Forge ecosystem

The Sponge + Forge ecosystem can _continue to do what they're doing_ with the above solution. GlowstonePlusPlus, a non-API project, does not require the use of setAccessible(true) reflection like SpongeAPI forces right now. That means that a security-conscious server owner can enforce a tight security policy without it breaking. SpongeAPI, a project that is an API, should never be the limiting factor of anything, including but not only, the security of peoples' Minecraft servers.

[gabizou]

For some, the IDEs being annoying might be the worst thing they use there CatalogTypes. For others, they want a better way to write their ported Sponge's initializers. Some might wonder why we need to use reflection to catalog something as simple as TextColors!

Why does it even matter to plugin developers whether we use reflection in the first place? It seriously doesn't matter whatsoever.

A solution that would satisfy both kinds of implementations that need to initialize stuff later in the loading process, and implementations that don't, and also fix the IDE warnings would be:

A majority of the initialization takes places prior to plugin classloading. In effect, this is _why_ there was the initial registry rewrite to the system we use now versus the mess that existed before. There's little reason to initialize a great many things later in the game lifecycle when 99.9% of all catalog instances are available during the static initialization of plugins (minus mod added blocks, but those aren't exposed through the API directly anyways).

Create a GameRegistry.staticInitializeType(Class clazz, String id) method, and call it from all catalogs.

Not gonna happen, a majority of the id's are either generated at runtime and/or self defined by the implementation. There's absolutely no reason to expose the string constants in the API whatsoever.

In implementations that require initializing the types at runtime, simply return null; and keep using reflection, with absolutely no other changes required, and with the same results as before.

I don't think you understand what setting a field once does. It sets the field and _that's it_. There's no extra reflection calls performed, there's no additional strain on the system, it's just plain old java.

In implementations that do not require initializing, and when implementations that do require doing so, initialize something more simple than a TextColor, things like BanTypes, they can initialize it cleanly, and without reflection there.

Again, of everything in the API, trying to avoid using reflection to _set catalog fields in the _implementation** is retarded. It's the _LEAST_ worrisome aspect of Java that you should be caring about at length, let alone complain that implementations have it difficult to change the fields. Hell, just pipe everything through a clone of the RegistryHelper and be done with it, maybe have to add some force removal of the final modifier on the field, and then re-assign it after setting it, but that's literally the most rudimentary magical aspect of practically our entire implementation.

The Sponge + Forge ecosystem can continue to do what they're doing with the above solution.

The solution I'm suggesting is the solution I'm going with, unless someone comes up with a better option in the future.

GlowstonePlusPlus, a non-API project, does not require the use of setAccessible(true) reflection like SpongeAPI forces right now. That means that a security-conscious server owner can enforce a tight security policy without it breaking. SpongeAPI, a project that is an API, should never be the limiting factor of anything, including but not only, the security of peoples' Minecraft servers.

You know there are ways to write security managers that forcefully check the callers to validate that the reflection setting is performed by the implementation and _no one else_. Complaining that allowing setting fields with reflection in this way is the least of your worries when it comes to security in Java, let alone in running a Minecraft server. Seriously, you're getting annoying trying to point out that requiring the use of reflection to set some fields on our own API through our own implementation (Which as I said already is entirely controllable) is a leak in security. If you want to complain about security in Java, go stand in line.

[CrystalMageX]

Ok, so my 2nd and 3rd points (both about security) are resolved. @gabizou has decided that reflection will be the only way to initialize the catalogs. Third-party implementations of SpongeAPI can only hope that that platform's security allows them to reflect their own packages, or else they will need to include a modified API with their custom static initializers, or the final modifier removed. This leaves us with the 1st point.

The 1st point is about IDEs complaining about NullPointerExceptions. It seems there is an even simpler way than @ST-DDT 's adding of a method. We can just extract the assignments to a static constructor, and they won't complain anymore. (tested with IntelliJ Community 2016.1, # IC-145.256).

Change this:

public class BanTypes {
    public static final BanType PROFILE = null;
    public static final BanType IP = null;

    private BanTypes() {
    }
}

to this:

public class BanTypes {
    public static final BanType PROFILE;
    public static final BanType IP;

    private BanTypes() {
    }

    static {
        PROFILE = null;
        IP = null;
    }
}