FabricMC / tiny-remapper

Tiny JAR remapping tool.
GNU Lesser General Public License v3.0
115 stars 65 forks source link

Package access fixer fails for protected fields #92

Open Juuxel opened 2 years ago

Juuxel commented 2 years ago

See this reproduction repo: https://github.com/Juuxel/tr-package-access-reproduction

This issue also has practical effects when remapping Minecraft to Yarn in environments where all protected accesses are not transformed to be public (FML on Yarn).

sfPlayer1 commented 2 years ago

This is a major pain to handle because it breaks in the verifier, not in the fundamental access rules.

JVMS 4.10.1.8 demands " Otherwise, use of a member of an object of type Target requires that Target be assignable to the type of the current class." to access protected (non-static) members. Without that it is as if the member was package private.

If I read it correctly the type Target is the type in the stack frame at the instruction location, which isn't known without code analysis. It is not necessarily the same as T, C or D in 5.4.4.

A conservative estimation mode that treats Target = T could work in practice, but isn't ideal.

Juuxel commented 2 years ago

Yep, 5.4.4 also notes:

During verification of D, it was required that, even if T is a superclass of D, the target reference of a protected field access or method invocation must be an instance of D or a subclass of D (§4.10.1.8).

which seems to imply that Target and T are separate. I took a look at this issue when I originally reported it and it looks like it's not easy to solve with TR's current design since it needs to analyse the stack. See also JLS 6.6.2.1 which is the same rule in source code terms.