Fix Java 12 compatibility

[TheDGOfficial]

Description

Fixes this error:

[22:02:47 WARN]: java.lang.NoSuchFieldException: modifiers
[22:02:47 WARN]:        at java.base/java.lang.Class.getDeclaredField(Class.java:2569)
[22:02:47 WARN]:        at ch.njol.skript.Skript$4.run(Skript.java:1056)
[22:02:47 WARN]:        at java.base/java.lang.Thread.run(Thread.java:832)

The Field#modifiers field was removed on Java 12.

The eror happens on Java 12+ only and the stacktrace is only printed when Skript#testing returns true (i.e -ea/-enableassertions), but it should be fixed anyway.

By "fix", it just ignores the error. The behaviour is not same as on Java < 12. Can't find a better alternative rn, but probably there is.

Not tested on Java 11 but it would give a illegal access warning with or without this fix anyway. Perhaps unsetting static final fields should be removed altogether as most of them are constants? Don't know. The whole workaround may also be unnecessary, is this memory leak thing still exists on recent MC versions?

---

Target Minecraft Versions: any
Requirements: none
Related Issues: none

[TheDGOfficial]

Seems like it can be done with the use of sun.misc.Unsafe.
https://stackoverflow.com/questions/61141836/change-static-final-field-in-java-12

Still though, is this workaround still relevant on modern versions?

[TPGamesNL]

It's just some hacky code to provide some support for people who reload their server instead of restarting them, so IMO it can be removed entirely

[FranKusmiruk]

The modifiers field wasn't removed, Java 12 added the Field class and other classes from the java.lang.reflect package to the security filter (a more thorough explanation is given on this issue ticket). This makes the Field.modifiers inaccessible through the Class#getDeclaredField method.

You can still use VarHandles to read/modify the field as explained here.

[TheDGOfficial]

The modifiers field wasn't removed, Java 12 added the Field class and other classes from the java.lang.reflect package to the security filter (a more thorough explanation is given on this issue ticket). This makes the Field.modifiers inaccessible through the Class#getDeclaredField method.

You can still use VarHandles to read/modify the field as explained here.

By removed I meant that, but does not matter anyway. That's an implementation detail. VarHandle method is a hacky way too like Unsafe, but worse than Unsafe in my opinion. Sure, it is easier than Unsafe and avoids sun.misc import, but it gives an illegal access warning and all illegal access operations will be denied in Java 16.

Quick test case:

VarHandleModifiersTest.java

final class VarHandleModifiersTest {
    public static final void main(final String[] args) {
  	  try {
  		  final var lookup = java.lang.invoke.MethodHandles.privateLookupIn(java.lang.reflect.Field.class, java.lang.invoke.MethodHandles.lookup());
  		  final var modifiers = lookup.findVarHandle(java.lang.reflect.Field.class, "modifiers", int.class);
  	  } catch (final NoSuchFieldException | IllegalAccessException e) {
  		  e.printStackTrace();
  	  }
    }
}

Result on Java 15

$ java -showversion VarHandleModifiersTest.java
java version "15.0.2" 2021-01-19
Java(TM) SE Runtime Environment (build 15.0.2+7-27)
Java HotSpot(TM) 64-Bit Server VM (build 15.0.2+7-27, mixed mode, sharing)
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access using Lookup on VarHandleModifiersTest (file:/C:/Users/Mustafa/Desktop/script-test/VarHandleModifiersTest.java) to class java.lang.reflect.Field
WARNING: Please consider reporting this to the maintainers of VarHandleModifiersTest
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release

Result on Java 16 early access

$ "C:/Program Files/Java/jdk-16/bin/java.exe" -showversion VarHandleModifiersTest.java
openjdk version "16" 2021-03-16
OpenJDK Runtime Environment (build 16+36-2231)
OpenJDK 64-Bit Server VM (build 16+36-2231, mixed mode, sharing)
java.lang.IllegalAccessException: module java.base does not open java.lang.reflect to unnamed module @5cdd8682
        at java.base/java.lang.invoke.MethodHandles.privateLookupIn(MethodHandles.java:260)
        at VarHandleModifiersTest.main(VarHandleModifiersTest.java:4)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:78)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:567)
        at jdk.compiler/com.sun.tools.javac.launcher.Main.execute(Main.java:415)
        at jdk.compiler/com.sun.tools.javac.launcher.Main.run(Main.java:192)
        at jdk.compiler/com.sun.tools.javac.launcher.Main.main(Main.java:132)

Result on Java 17 early access is same, only changing version number.

My opinion is, we should either remove it completely, or use Unsafe (from JEP 396, targeting Java 16: "It is not a goal to remove, encapsulate, or modify any critical internal APIs of the JDK for which standard replacements do not yet exist. This means that sun.misc.Unsafe will remain available."). Or just ignore it in Java 12+ (PRs current behaviour)

From the behaviour said in the comment of the work around:

Comment of the work around

  	  // unset static fields to prevent memory leaks as Bukkit reloads the classes with a different classloader on reload
  	  // async to not slow down server reload, delayed to not slow down server shutdown

This patch in Paper probably fixes this behaviour:
https://github.com/PaperMC/Paper/blob/master/Spigot-API-Patches/0098-Close-Plugin-Class-Loaders-on-Disable.patch

[TPGamesNL]

By removed I meant that, but does not matter anyway. That's an implementation detail. VarHandle method is a hacky way too like Unsafe, but worse than Unsafe in my opinion. Sure, it is easier than Unsafe and avoids sun.misc import, but it gives an illegal access warning and all illegal access operations will be denied in Java 16.

Besides, VarHandles were added in Java 9, so Java 8 compatability wouldn't be possible.

[FranKusmiruk]

In that case, I think we're safe to just remove that piece of code, Skript doesn't support reloads either way. But ignoring it on future versions is fine as well so unless anyone has any issues with this PR, I am merging.