Update to Java16 and cleanup hundreds of warnings

[tadhunt]

Summary

• This is a…
  • [ ] Bug fix
  • [ ] Feature addition
  • [ ] Documentation
  • [X] Refactoring
  • [ ] Minor / simple change (like a typo)
  • [ ] Other
• Describe this change in 1-2 sentences:

Problem

Update to a more recent version of Java. Mojang/Spigot/Paper switched to Java16 as of 1.17, and to Java17 as of 1.18. Since MobArena has been updated to the Spigot 1.19 API, it makes sense to address any issues found as a result of the Java version upgrade.

I needed this in prep for for some work we're doing at MC Playdates for adding some new features that we'll be deploying on some ≥ 1.20 servers, so I figured I'd contribute it back in case it's useful for others, or for the project itself.

• GitHub issue (optional):

Solution

Mostly straightforward changes, but I'm still a Java newbie, so there is probably a better way to get rid of the the Math::sqrt (and friends) warnings than the way I did it, any help there would be appreciated.

Lots of the warnings were having to do with case statements that didn't handle all possible cases. That could be a style thing, because as far as I could tell, the old code was actually correct, but I changed them anyway to be more in line with what the java compiler seems to prefer.

Null warning analysis actually revealed a few potential bugs, so I addressed them as well.

The only remaining warnings with these changes are deprecation warnings. Mostly from the Spigot 1.19 API but also a few from Java itself. I'd also like to fix these warnings as well, but haven't had the chance yet.

Action

The warnings cleared up from potential use of null pointers are worth careful review, there may be cleaner ways to address them, and also with the math formula changes.

I've run through several playtest sessions, and it seems to work correctly, but more testing is alway appreciated :)

Build here: https://github.com/tadhunt/MobArena/actions/runs/6132387606

[Aaron2550]

But if MobArena uses the 1.19 API, and 1.19 requires Java 17, like you said, why not go to Java 17 directly? Why bother with Java 16? Did I miss something?

[tadhunt]

But if MobArena uses the 1.19 API, and 1.19 requires Java 17, like you said, why not go to Java 17 directly? Why bother with Java 16? Did I miss something?

Great question, maybe that's a better step forward. I know that even though MobArena is built against the Spigot 1.19 API, the desire is still to have it work on older versions, and I wasn't sure if going to Java17 meant that 1.17 would stop working?

[Aaron2550]

If it targets 1.19, it will not run on anything older anyway AFAIK

[garbagemule]

If it targets 1.19, it will not run on anything older anyway AFAIK

This is not necessarily true. Since MobArena uses the Bukkit API and nothing "behind" it (like the server implementation or NMS), as long as the API that MobArena interfaces with hasn't changed between a gien version and the target version, the code will compile and the plugin will run. In fact, even if the API changes, as long as it changes in a way that is ABI compatible with MobArena, it will still work. MobArena currently targets 1.19 but it runs on anything 1.13+ for this exact reason.

[Aaron2550]

Oh, okay! Ah ye i also didnt see api-version: 1.13 was unchanged

[tadhunt]

MobArena currently targets 1.19 but it runs on anything 1.13+ for this exact reason.

Ah got it. What's the right approach for maintaining backwards compatibility then? Does this mean that the released version of the plugin should still be compiled with Java8? I tested with Java8 and Java17, and in both cases it still seems to work correctly, and the warning fixes still reduce the warnings from hundreds down to just deprecation warnings.

[garbagemule]

Considering the vast majority (86 %) of servers running MobArena are on Minecraft 1.18+, I think we're at a point where we're okay to bump the minimum version to Java 17 and Spigot API 1.18 (including api-version: 1.18).

I'd love it if we could keep the commits somewhat cohesive and minimal in scope. For instance, I think it makes sense to update the GitHub Actions workflow to Java 17 in the same commit as bumping the configuration in the build file. I also think it makes sense to tackle any compile time errors as a result of bumping versions in the same commit. Since the project already has a bunch of compile time warnings, mending those should probably go in a separate cleanup commit, perhaps one where we fix all of the warnings if possible. Same goes for the potential bug fixes, which should also have their own commits. If we can fix stuff like the Scanner usage (?) and missing default branches in switch statements in one or more commits prior to the actual version bumps, I think that would be even better, because it would allow us to see exactly what has to change together with the version bumps vs. what was just holding the version bumps back, if that makes sense?

I'm also very much pondering moving from Maven to Gradle, but I'm guessing that will be much simpler after updating Java versions, since Gradle is a little finicky about those. That's just to say that whatever is necessary in the pom.xml to get the ball rolling is probably fine.

[Chew]

Just to chime in, we should not upgrade to Java 16 since it's EOL. Java 17 is the better choice here. Anyone running 16 should be running 17 anyway.

[tadhunt]

@garbagemule, Sounds good. I'll attempt to do things as recommended. I think it probably makes sense to do this stuff with new PRs, so I'll start on that by submitting a PR for the warnings cleanup with specific commits for each class of warning cleanup.

[tadhunt]

warnings: #773 null indirections: #774 scanner resource leak: #775 Spigot ≥ 1.18: #776 Github Actions deprecation warnings: #770

[tadhunt]

Closing this PR because everything here has been moved into the series of new PRs described above.