SingleImplPass doesn't handle annotations correctly

[justhecuke]

SingleImplPass searches for interfaces with only a single implementor class and then merges the interface into the implementor. This includes merging annotations, in particular the EnclosingClass, EnclosingMethod, InnerClass, and MemberClasses annotations.

However, those 4 names annotations have specific relations to each other with EnclosingClass and EnclosingMethod being explicitly mutually exclusive. A simple merge of the annotations leads to a break in the annotation's relationship and leaves dangling pointers to deleted interfaces.

This is particularly bad in the case of java.lang.Class.getCanonicalName() since the canonical name of an inner class is defined relate to its outer class which is found using the EnclosingClass annotation, which means that SingleImplPass can implicitly change the canonical name of single implementor classes.

Example:

package my.first.pkg;

// MemberClasses [C]
class A {
  // EnclosingClass A
  // InnerClass
  interface C {
    void method();
  }
}

package second.pkg.demo;

// no MemberClasses, EnclosingClass, or InnerClass
class B implements C {
  ...
}

In this situation, interface C should be merged into single implementor class B. However, after SingleImplPass merges C into B, B now inherits C's EnclosingClass and InnerClass annotations which changes B's canonical name from second.pkg.demo.B to my.first.pkg.A.B.

I am not sure what the proper fix should be, but at the very least when the relationship between the 4 annotations changed by SingleImplPass should be maintained by pointing the annotations away from deleted interfaces and to their single implementor class.

In my mind, it should be possible to drop the EnclosingClass, EnclosingMethod, and InnerClass annotations during the merge since it does not make sense to make the single implementor an inner class of the deleted interface's outer class.

[thezhangwei]

Very interesting. The reason we did not run into this is likely because we nuke those annotations with AnnoKillPass before running SingleImplPass.

If we do want to make sure getCanonicalName() works, that's a different story.

1. We can add EnclosingClass to SingleImplPass.anno_blocklist. But that will be way overly conservative.
2. Supply Proguard keep rule or annotations to keep the inner interface or keep its name. SingleImplPass does not remove what should be kept based on the Proguard rules. It should also respect the keepname rule. But we might have to make sure it does that.

The motivation for merging annotations was that if we don't do this, we would drop annotations inadvertently, I think.

[justhecuke]

I have two solutions in mind:

1. Since this is about changing the implementor class, we can add a check if we're merging an inner interface into an outer/stand-alone implementor to only do so if the implementor is renameable (not sure how redex determines this but calling can_rename is fairly simple).

2. When merging an interface into an implementor, do not merge EnclosingClass, modify the outer class's MemberClasses, and remove InnerClass if we removed EnclosingClass.

I'm working on (2) right now since it seems like it will give the most gains while also being safe in this particular case.

[thezhangwei]

SG. We can add a fix for #1.