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

Restore binary compatibility with 1.14.0 #667

Merged
merged 1 commit into from
Jul 7, 2020

Conversation

smarter
Copy link
Contributor

@smarter smarter commented Jun 3, 2020

dbaeb80 broke binary compatibility by
making BooleanOperators private, this does not match the plan discussed
in #540:

The 1.15.x series will preserve binary-compatibility with 1.14.0 but
is expected to break source-compatibility (e.g. removing the implicit
keyword from some definitions)

This commit implements that: BooleanOperators is public again but is
also made non-implicit.

dbaeb80 broke binary compatibility by
making `BooleanOperators` private, this does not match the plan discussed
in typelevel#540:

> The 1.15.x series will preserve binary-compatibility with 1.14.0 but
  is expected to break source-compatibility (e.g. removing the implicit
  keyword from some definitions)

This commit implements that: BooleanOperators is public again but is
also made non-implicit.
@smarter

This comment has been minimized.

@ashawley ashawley mentioned this pull request Jun 4, 2020
@ashawley
Copy link
Contributor

ashawley commented Jun 4, 2020

I wrote that I had made the change in #498 (comment). I believe there was a reason for it rather than just removing the implicit. I'll need to try and remember why I did this. I admit that my comment was unfortunately very cryptic.

The BooleanOperators is now private[this], for backwards compatibility, in favor of importing the implicit propBoolean instead.

@ashawley ashawley added this to the 1.15.0 milestone Jun 4, 2020
@non
Copy link
Contributor

non commented Jun 30, 2020

@ashawley OK to merge this?

@non
Copy link
Contributor

non commented Jul 7, 2020

I'm just gonna go ahead and merge this. Thanks again @smarter for catching this!

@non non merged commit 80f2f48 into typelevel:master Jul 7, 2020
SethTisue added a commit to SethTisue/breeze that referenced this pull request Nov 7, 2020
one tiny source adjustment needed, to adapt to
typelevel/scalacheck#667
@ashawley
Copy link
Contributor

I remember the motivation now for changing BooleanOperators to private[this]: It was for usability around error-messages v. deprecation messages.

@ashawley
Copy link
Contributor

Further, changing the implicit to package-private actually is binary compatible. Mima is flagging it as a binary compatible change, but that is only for Java callers, see lightbend-labs/mima#375.

It's good that Mima does this for those occasional Scala libraries with downstream Java users. I'm pretty confident we don't have any Java callers, so this Mima check could be ignored.

@smarter
Copy link
Contributor Author

smarter commented Apr 10, 2021

Further, changing the implicit to package-private actually is binary compatible.

Maybe, but before this PR, private[this] was used which does get emitted as private and isn't binary compatible.

@ashawley
Copy link
Contributor

I believe that would emit the same byte code as the example in lightbend-labs/mima#375, but I could re-check.

@smarter
Copy link
Contributor Author

smarter commented Apr 10, 2021

It doesn't:

package bla

class A {
  private[this] def foo: Int = 1
  private[bla] def bar: Int = 1
}
% scalac A.scala
% javap -p bla/A.class
Compiled from "pp.scala"
public class bla.A {
  private int foo();
  public int bar();
  public bla.A();
}

@ashawley
Copy link
Contributor

So it does. I guess I made the incorrect change, then. I wrongly assumed private[this] was like package-private and therefore binary compatible. The change should have been private[scalacheck]. Thanks.

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