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

Introducing overloads in value classes no longer creates a binary incompatibilty with client code. #7896

Merged
merged 1 commit into from
Mar 21, 2019

Conversation

retronym
Copy link
Member

@retronym retronym commented Mar 19, 2019

Prior to this change, the following linkage error could occur.

class C(val self: String) extends AnyVal { def foo = 1 }
object Test extends App { new C("").foo }
% scalac C.scala

% javap -cp /tmp 'C$'
Compiled from "62"
public final class C$ {
  public static C$ MODULE$;
  public static {};
  public final int foo$extension(java.lang.String);
...
class C(val self: String) extends AnyVal { def foo = 1; def foo(a: Any) = 2 }
% scalac C.scala

% javap -cp /tmp 'C$'
Compiled from "62"
public final class C$ {
  public static C$ MODULE$;
  public static {};
  public final int foo$extension0(java.lang.String);
  public final int foo$extension1(java.lang.String, java.lang.Object);
..
}

% && scala Test
java.lang.NoSuchMethodError: C$.foo$extension(Ljava/lang/String;)I
	at Test$.delayedEndpoint$Test$1(63:1)
	at Test$delayedInit$body.apply(63:1)

After this change, we no longer use the $0, $1, ... suffixes for overloaded methods, so we don't have the problem anymore.

@scala-jenkins scala-jenkins added this to the 2.13.1 milestone Mar 19, 2019
@retronym
Copy link
Member Author

Here's a followup after re-starring: https://github.com/retronym/scala/tree/topic/ext-method-overload-restarr

@retronym retronym modified the milestones: 2.13.1, 2.13.0-RC1 Mar 19, 2019
@retronym retronym requested a review from lrytz March 20, 2019 03:28
@lrytz
Copy link
Member

lrytz commented Mar 20, 2019

I wonder why it wasn't done this way initially. 5118fd0d08

@retronym
Copy link
Member Author

The SIP doesn't address overloading. Perhaps during Martin's WIP (before the commit to scala/scala) it was just based on "dead reckoning" to the corresponding name, and was then changed to a linear scan of the family of methods to "gain a level of insensitivity of how overloaded types are ordered between phases and picklings.".

Once you're doing that scan, you might as well just use overloading, which importantly extends this sensitivity to separate compilation

This avoids binary incompatibilities when adding/removing/reordering
overloaded methods in value classes.
@adriaanm
Copy link
Contributor

let's restarr before RC1 to get the full fix in

@adriaanm adriaanm merged commit 8729d71 into scala:2.13.x Mar 21, 2019
@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Apr 1, 2019
@dwijnand
Copy link
Member

dwijnand commented Apr 5, 2019

This avoids binary incompatibilities when adding/removing/reordering
overloaded methods in value classes.

I don't get the title or this description (and there are no test examples). Could someone give an example of what this change allows now? Thank you.

@retronym retronym changed the title Extension methods of overloads are themselves overloaded Introducing overloads in value classes no longer creates a binary incompatibilty with client code. Apr 8, 2019
@retronym
Copy link
Member Author

retronym commented Apr 8, 2019

@dwijnand Thanks for the feedback, I've updated the title and description.

@dwijnand
Copy link
Member

dwijnand commented Apr 8, 2019

@retronym
Copy link
Member Author

retronym commented Apr 9, 2019

I wonder why it wasn't done this way initially

@lrytz I asked @odersky last night. He told me the motivation for the suffixes was because Scala's overload resolution only uses the first parameter section, and in all cases that is the $this parameter.

$ scalac -Xprint:extmethods <(echo 'class C(val a: Any) extends AnyVal { def foo(a: Int) = 1; def foo(a: String) = 2 } ') | grep foo
    def foo(a: Int): Int = C.foo$extension0(C.this)(a);
    def foo(a: String): Int = C.foo$extension1(C.this)(a);
    final def foo$extension0($this: C)(a: Int): Int = 1;
    final def foo$extension1($this: C)(a: String): Int = 2;

This doesn't really matter in our implementation, because we create the C$.foo$extension selection with an explicit Symbol, rather than just using a Name and relying on Scala's overload resolution.

smarter added a commit to dotty-staging/dotty that referenced this pull request Aug 8, 2019
See scala/scala#7896 and follow-up
scala/scala#7922, we can do both steps at once
since we always run with a bootstrapped dotty-library on the classpath, even
if the compiler itself is not bootstrapped.
smarter added a commit to dotty-staging/dotty that referenced this pull request Aug 8, 2019
See scala/scala#7896 and follow-up
scala/scala#7922, we can do both steps at once
since we always run with a bootstrapped dotty-library on the classpath, even
if the compiler itself is not bootstrapped.
smarter added a commit to smarter/dotty that referenced this pull request Aug 14, 2019
See scala/scala#7896 and follow-up
scala/scala#7922, we can do both steps at once
since we always run with a bootstrapped dotty-library on the classpath, even
if the compiler itself is not bootstrapped.
smarter added a commit to dotty-staging/dotty that referenced this pull request Aug 16, 2019
See scala/scala#7896 and follow-up
scala/scala#7922, we can do both steps at once
since we always run with a bootstrapped dotty-library on the classpath, even
if the compiler itself is not bootstrapped.
bishabosha pushed a commit to dotty-staging/dotty that referenced this pull request Aug 19, 2019
See scala/scala#7896 and follow-up
scala/scala#7922, we can do both steps at once
since we always run with a bootstrapped dotty-library on the classpath, even
if the compiler itself is not bootstrapped.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes worth highlighting in next release notes
Projects
None yet
6 participants