Use public lookup

[liach]

The class specific lookup doesn't lend extra access to nms but exposes impl details of MinecraftReflection.

With the current lookup, users can just call lookup.findStaticGetter(MinecraftReflection.class, "VERSION", String.class) to steal the VERSION field, etc. In addition, the regular lookup doesn't have special advantage on accessing nms, as this MinecraftReflection class doesn't as well.

[zml2008]

I remember last time I tried to switch to a public lookup for one of these things it broke stuff -- have you tested this change in an active server env?

Really, this class was designed to be package-private but had that changed when the text-serializer-craftbukkit module got broken out from the larger bukkit platform.

[liach]

not on production server.

can you post a previous log? i've checked that all methods or fields it unreflects already are called to be accessible, so any lookup can unreflect them. other usages by this lookup are all targetting stuff this lookup has minimal trust to as far as i see. another concern may be that nms classes are from different classloaders and without a correct class instance from the right classloader (i think), lookup cannot find and call methods or do stuff.

[A248]

Strictly speaking, it is correct for adventure-platform to use its own lookup, as the lookup API is designed to reflect the access capabilities of the caller performing the access.

[liach]

the lookup API is designed to reflect the access capabilities of the caller performing the access.

You are understanding this exactly incorrectly: this means that you shouldn't expose a lookup obtained by MethodHandles.lookup() in one class to other classes, as this lookup exposes implementation details of this MinecraftReflection class inadvertently. Same for using own lookups: if you can control what members are looked up, it's totally safe; but it isn't quite easy to ensure that other users aren't using the methods in this class to steal the specific implementation details in this class. The other privateLookupIn in etc are to ensure access across modules defined by module reads and opens and nested/inner class relations. The MinecraftReflection API has no reason to expose any of these.

[A248]

The lookup is not exposed. There is no MinecraftReflection public API - this is very specifically documented as internal API. As a corollary, if adventure was using JPMS, MinecraftReflection would not be exported.

If you're on Java 8, unfortunately you cannot use JPMS. All the same, libraries frequently make use of .internal packages or similar. There are no guarantees for these packages and they are not considered part of the public API. They are not exposed.

[liach]

Just curious, what is JPMS?

The old lookup does have extra capabilities, but those capabilities are quite useless for the use cases, as the lookup has equivalent access to whatever any code in MinecraftReflection class does. Another minimal advantage is that the public lookup does not require extra calculation, compared to the lookup() call that involves some minor calculation.

After all, the main advantage of this change is to make people actually understand what this lookup actually needs. Like I am not going to use a copy on write array list to store items when an array list suffices all my needs. This usage of copy on write array list may make you think "oh, I need this concurrency feature" while you don't; same argument applies: you may think "oh, I need to access whatever any code in MinecraftReflection can" but it does not. This reduces confusion for maintenance.

[kashike]

Just curious, what is JPMS?

Java Platform Module System

[liach]

Ah, I never heard of that acronym! Even the full name appears to be the title of JEP only, unlike "indy" "condy" that has actually appeared in jdk's LambdaMetafactory. Wish you just called it "modules" 😄

[zml2008]

Irrelevant as of 6fa0890, the relevant class is now package-private.