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

Rework the handling of inner classes. #49

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SpaceWalkerRS
Copy link

By convention inner class names are of the form com/example/Example$InnerName, which encodes both the outer class' name, as well as the inner class' inner name, but this is not required by the JVM spec. However, Stitch fully relies on classes' names following this convention, and if they do not, Stitch treats them as top level classes, even if the appropriate attributes are present. Similarly, if the inner and outer class attributes are not present, but the classes' names suggest the classes are nested, Stitch will treat them as such.

This PR fixes these issues by looking for the inner and outer class attributes to retrieve the enclosing class and inner name of a class, instead of relying on class names following the standard $ convention.

I ended up removing a significant part of JarReader, as it appeared to be dead code and it made it easier to add the new necessary parts. I'm just mentioning it here 'cause it's a fairly large change that isn't strictly related to the PR.

@@ -34,11 +33,14 @@ public class JarClassEntry extends AbstractJarEntry {
List<String> interfaces;
List<String> subclasses;
List<String> implementers;
String enclosingClass;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you should track the declaring and enclosing class both with the same variable. Consider:

class TopLevel {
  Comparator<Integer> comparator = new Comparator() {...};
  class Inner {}
}

your model cannot distinguish between the two classes here; Inner has a non-null "outer class" (declaring class), while the anonymous comparator implementation class has an EnclosingMethod with a class reference but no method name and type reference.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The distinction is that Inner will have a non-null "inner name", while the anonymous class will have its inner name set to null.

I do realize that I only look at the outer class attribute for the enclosing class' name, while for inner classes that attribute isn't present so I need to get it from the inner classes attribute.

Copy link
Contributor

@liach liach May 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No:

class TopLevel {
  static {
    class Local {}
  }
  class Inner {}
}

Should've given this example first and foremost

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, I assumed that would give <clinit> as the enclosing method, but that is not the case. You're right, I should track the enclosing/declaring class separately to properly be able to distinguish between inner and local classes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's fixed now


public JarRootEntry(File file) {
super(file.getName());

this.file = file;
this.classTree = new TreeMap<>(Comparator.naturalOrder());
this.allClasses = new ArrayList<>();
this.allClasses = new TreeMap<>(Comparator.naturalOrder());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
this.allClasses = new TreeMap<>(Comparator.naturalOrder());
this.allClasses = new TreeMap<>();

}

public boolean isInner() {
return hasEnclosingClass() && !hasEnclosingMethod() && innerName != null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will return true for this Local:

class TopLevel {
  static { class Local {} }
}

and since this method isn't used and stitch is not an api, I recommend removing this method.

@SpaceWalkerRS SpaceWalkerRS force-pushed the master branch 2 times, most recently from d18619e to b36f9a8 Compare May 8, 2023 16:32
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

Successfully merging this pull request may close these issues.

3 participants