CadixDev/Lorenz

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

CheaterCodes opened this issue · 0 comments

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.