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

AbstractClassMappingImpl.complete(...) infers wrong method name #55

Open
CheaterCodes opened this issue Nov 7, 2021 · 0 comments
Open

Comments

@CheaterCodes
Copy link

CheaterCodes commented Nov 7, 2021

Context

I have three classes:

abstract class A {
  private void foo() {
    [...]
  }
}

interface B {
  void foo();
}

class C extends A implements B {
  public void foo() {
    [...]
  }
}

I want to remap these classes using the following mapping set:

A.foo()V -> A.methodA()V
B.foo()V -> B.methodB()V

Problem

Expected result:
C.foo()V is overriding B.foo() and should therefore be remapped to C.methodB()V

Actual result:
C.foo()V gets remapped to C.methodA()V even though A.foo()V is private and can't be overriden.

Note that the erronous result is also ordering dependent, so it might not occur in all situations.

Origin

The reason for this happening are the changes in commit 2a7c0b6.
The changes were made in response to issue CadixDev/Mercury#14.

The argument made in the code comments doesn't make sense to me:

Check if there are any methods here that override the return type of a parent method.

While the issue in Mercury is correct in saying that

Java allows overriding methods with different return types, if the type is a subclass of the OG return type.

, the same is not applicable to the JVM, as the spec says:

An instance method mC can override another instance method mA iff all of the following are true:

  • mC has the same name and descriptor as mA.
    [...]

Suggested Solution

The minimum fix would be to add accessibility checks to the erronous code, preventing it from inheriting a name from a private method. However, keep in mind that the JVM requires strictly identical method descriptors, so this solution could still cause issues (A method String foo() can not override the method Object foo() in the JVM, and inferring the names that way is still erronous).

My preferred fix would be to revert the referenced commit in Lorenz and instead fix the issue in a tool specifically made for source remapping if possible (e.g. Mercury).

If separating asm remapping from source remapping is too difficult, maybe there could be a flag of some sort to change that behaviour.

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

1 participant