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

Exploring inner modules for binary incompatibilities. Fix #127 #128

Closed

Conversation

dotta
Copy link
Contributor

@dotta dotta commented Oct 1, 2016

The former implementation of PackageInfo#accessibleClasses expects that for any
module Foo compiled by scalac, both Foo.class and Foo$.class classfiles
are produced. Because Foo$.class and Foo.class are essentially mirrors, and
users never access directly Foo$.class, MiMa only traverses the Foo.class. This
works well for top-level modules, but it breaks for inner modules, because no
mirror class is created for inner modules (only the module class is created).

The fix consist in treating inner modules differently, hence making sure inner
modules are part of the set returned by PackageInfo#accessibleClasses.

…abs#127

The former implementation of PackageInfo#accessibleClasses expects that for any
module `Foo` compiled by scalac, both `Foo.class` and `Foo$.class` classfiles
are produced. Because `Foo$.class` and `Foo.class` are essentially mirrors, and
users never access directly `Foo$.class`, MiMa only traverses the `Foo.class`. This
works well for top-level modules, but it breaks for inner modules, because no
mirror class is created for inner modules (only the module class is created).

The fix consist in treating inner modules differently, hence making sure inner
modules are part of the set returned by PackageInfo#accessibleClasses.
@dotta dotta force-pushed the issue/127-removing-inner-object branch from 3fa7bdd to 2d67b04 Compare October 3, 2016 05:59
@dotta
Copy link
Contributor Author

dotta commented Oct 4, 2016

@adriaanm @lrytz @retronym Could you guys have a quick look?

@retronym
Copy link
Contributor

retronym commented Oct 4, 2016

Because Foo$.class and Foo.class are essentially mirrors, and
users never access directly Foo$.class, MiMa only traverses the Foo.class

Could you expand on this with an example? It seems incorrect to me. scalac adds static forwarders to the class O for a subset of the members of O$.

For example:

⚡ printf "class Fooey { def foo = 42 }; class O extends Fooey; object O { def foo(x: Any) = x }" > /tmp/test.scala; scalac /tmp/test.scala; javap -classpath . O 'O$'
Compiled from "test.scala"
public class O extends Fooey {
  public O();
}
Compiled from "test.scala"
public final class O$ {
  public static final O$ MODULE$;
  public static {};
  public java.lang.Object foo(java.lang.Object);
}

O$.foo(Object) does not have a forwarders in O because O inherits a member with the same name.

}
// class, trait, and top-level module go through this condition
(prefix exists (_.decodedName == clazz.decodedName.substring(0, idx))) || // prefix before dollar is an accessible class detected previously
isInnerModule
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be more robust to check the InnerClasses attribute of the module class.

⚡ printf 'object N { class O }; class C { object O }' > /tmp/test.scala && scalac /tmp/test.scala && javap -classpath . -v 'C$O$' 'N$O' 2>&1 | grep --after=1 InnerClasses:
InnerClasses:
     public #17= #2 of #16; //O$=class C$O$ of class C
--
InnerClasses:
     public static #14= #2 of #13; //O=class N$O of class N

@dotta
Copy link
Contributor Author

dotta commented Oct 8, 2016

Hi Jason, thanks a lot for the feedback!

Because Foo$.class and Foo.class are essentially mirrors, and
users never access directly Foo$.class, MiMa only traverses the Foo.class

Could you expand on this with an example? It seems incorrect to me. scalac adds static forwarders to the class O for a subset of the members of O$.

You are absolutely right. And, in fact, my statement "MiMa only traverses the Foo.class" is not false: MiMa does traverse both Foo.class and Foo$.class exactly because of what you said. Hence, the fix in this PR isn't good. As you pointed out in your review, the classfile attribute InnerClasses should be used instead.

Unfortunately, I don't have more spare cycles to dedicate to this at the moment. I'm closing this PR.

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

Successfully merging this pull request may close these issues.

None yet

2 participants