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

Binary compatibility issue in 1.15.0 #696

Closed
larsrh opened this issue Nov 2, 2020 · 18 comments · Fixed by #702
Closed

Binary compatibility issue in 1.15.0 #696

larsrh opened this issue Nov 2, 2020 · 18 comments · Fixed by #702
Milestone

Comments

@larsrh
Copy link
Contributor

larsrh commented Nov 2, 2020

Reported by @xuwei-k here: d4a3c25#r43785860

org.scalacheck.Gen.someOf

  • 1.15.0 <T:Ljava/lang/Object;>(Lorg/scalacheck/Gen<TT;>;Lorg/scalacheck/Gen<TT;>;Lscala/collection/immutable/Seq<Lorg/scalacheck/Gen<TT;>;>;)Lorg/scalacheck/Gen<Lscala/collection/Seq<TT;>;>;

  • 1.14.3 <T:Ljava/lang/Object;>(Lorg/scalacheck/Gen<TT;>;Lorg/scalacheck/Gen<TT;>;Lscala/collection/immutable/Seq<Lorg/scalacheck/Gen<TT;>;>;)Lorg/scalacheck/Gen<Lscala/collection/immutable/Seq<TT;>;>;

org.scalacheck.Gen.atLeastOne

  • 1.15.0 <T:Ljava/lang/Object;>(Lorg/scalacheck/Gen<TT;>;Lorg/scalacheck/Gen<TT;>;Lscala/collection/immutable/Seq<Lorg/scalacheck/Gen<TT;>;>;)Lorg/scalacheck/Gen<Lscala/collection/Seq<TT;>;>;

  • 1.14.3 <T:Ljava/lang/Object;>(Lorg/scalacheck/Gen<TT;>;Lorg/scalacheck/Gen<TT;>;Lscala/collection/immutable/Seq<Lorg/scalacheck/Gen<TT;>;>;)Lorg/scalacheck/Gen<Lscala/collection/immutable/Seq<TT;>;>;

@larsrh
Copy link
Contributor Author

larsrh commented Nov 2, 2020

These changes come from @non (#603), could you please comment?

@ashawley
Copy link
Contributor

ashawley commented Nov 2, 2020

Oscar raised his concern in a comment in #575, the original PR, and pointed out that type erasure means it's still binary compatible. So, this is only a source-compatibility breaking change?

@larsrh
Copy link
Contributor Author

larsrh commented Nov 2, 2020

@johnynek Can you comment on this?

@johnynek
Copy link
Contributor

johnynek commented Nov 2, 2020

  1. I assume this project is running mima, if not it should. was that run? we should trust tools far more than humans in general to make these judgements.
  2. to my eyes, it still appears the changed type is in the parameter of the result, and those are erased in JVM bytecode according to my understanding, so this appears binary (but not source) compatible to me. That said, maybe it is kind of bad kind of binary compatible because if you use the result it will result in a class cast exception... so maybe my argument is bad...

In 2.12 and earlier, I think immutable.Seq extends collection.Seq, but maybe that's not true in 2.13 so this differs a runtime exception...

If we are not running mima on all published versions, I think we need to update to do so, but even so, the version did bump to a new minor version, so maybe we just need to update the readme and say that there was a minor binary break in 2.13

@ashawley
Copy link
Contributor

ashawley commented Nov 3, 2020

Mima is run on all versions, and even 2.13 after #482, but, yes, it only deals with binary compatible changes, IIUC.

@SethTisue
Copy link
Member

SethTisue commented Nov 3, 2020

At the moment I'm only glancing at this and not digging, but as per lightbend-labs/mima#299 shouldn't MiMa have warned on a generic-signature change?

@ashawley
Copy link
Contributor

ashawley commented Nov 3, 2020

You're right, Seth. We disabled signature checking in 7bb0613. That's probably the issue.

@ashawley
Copy link
Contributor

ashawley commented Nov 3, 2020

Actually, that wasn't the issue. Signature checking was enabled by default in 0.4.0, but then changed to opt-in in 0.7.0, see lightbend-labs/mima#471

@SethTisue
Copy link
Member

SethTisue commented Nov 3, 2020

@ashawley ah, good for you for finding that, I failed to remember it. (on 299, I added a comment with a link to 471, to aid future seekers.)

but even if we set mimaReportSignatureProblems to true here, won't that filter (the one in 7bb0613) still filter the problem out?

@ashawley
Copy link
Contributor

ashawley commented Nov 3, 2020

That's right. Having both of these enabled didn't help. Turning them off, we do get the signature problems reported by MiMa.

@ashawley
Copy link
Contributor

ashawley commented Nov 3, 2020

We can probably remove the return type signatures added for Gen.someOf and Gen.atLeastOne in 126168f that would at least make things compatible again with what was in 1.14.3. I believe this is because the types for scala.collection.Seq in 2.13 and later are different from 2.13 and earlier. Inferred types by the compiler is the only way to get it right. Otherwise, we'd probalby need to source-fork Gen.scala.

@johnynek
Copy link
Contributor

johnynek commented Nov 3, 2020

You could just put those functions and type aliases in version dependent files and then call them from Gen.

@ashawley
Copy link
Contributor

ashawley commented Nov 3, 2020

I predict Erik had an issue with lining up the types with Gen.pick which was that way because it had an explicit type signature of Gen[collection.Seq] that snuck in back during the 2.13 migration work. Unfortunately, we're probably stuck with that signature of Gen.pick.

@ashawley ashawley added this to the 1.15.1 milestone Nov 3, 2020
@larsrh
Copy link
Contributor Author

larsrh commented Nov 4, 2020

Are you running a release for this, @ashawley?

@ashawley
Copy link
Contributor

ashawley commented Nov 4, 2020

No, I was not planning to do the 1.15.1 release. Would appreciate if you'd be willing to do it again. I'm not sure what others think, but it should be ready to go.

@larsrh
Copy link
Contributor Author

larsrh commented Nov 5, 2020

OK, I will take care of it.

@larsrh
Copy link
Contributor Author

larsrh commented Nov 5, 2020

I ran out of time today, but it's on my list for tomorrow.

@larsrh
Copy link
Contributor Author

larsrh commented Nov 6, 2020

Done.

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 a pull request may close this issue.

4 participants