Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Package access fixer fails for protected fields #92

Open
Juuxel opened this issue May 2, 2022 · 2 comments
Open

Package access fixer fails for protected fields #92

Juuxel opened this issue May 2, 2022 · 2 comments

Comments

@Juuxel
Copy link
Member

Juuxel commented May 2, 2022

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

  • The example code (example-code/src/main/java) contains two classes, example.Parent and example.Child
    • example.Parent has a protected field that is accessed in example.Child's constructor
  • Test case 1 (no remapping): run gradlew testWithoutRemapping to see "hello" when the jar is not remapper
  • Test case 2 (remapping): run gradlew testWithRemapping to see a JVM crash
    • When example.Child is remapped to example.child.Child and example.Parent stays intact, the protected access now becomes an error. (Accessing protected fields of another instance from a class not in the same package)
    • TR's package access fixer does not catch this since it's seemingly set to ignore protected access errors for subclasses even where it's not applicable.
    • This does not happen if you remove the protected modifier from the field and make it package-private. TR fixes that to be public.
  • The remapping code is in remapper/src/main/java/remapper/Main.java

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
Copy link
Collaborator

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
Copy link
Member Author

Juuxel commented May 4, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants