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

Minor cleanup #649

Closed
wants to merge 1 commit into from
Closed

Minor cleanup #649

wants to merge 1 commit into from

Conversation

dubinsky
Copy link
Collaborator

@dubinsky dubinsky commented Feb 16, 2023

conflicted type annotations
removed some spurious ()s
removed some unused declarations
minor cleanup

removed some spurious `()`s
removed some unused declarations
minor cleanup
@dubinsky dubinsky marked this pull request as ready for review February 16, 2023 23:41
@SethTisue
Copy link
Member

The code cleanups LGTM, but are we sure the MiMa exclusions are safe...?

@dubinsky
Copy link
Collaborator Author

dubinsky commented Mar 9, 2023

are we sure the MiMa exclusions are safe...?

Exclusions are needed to add the last missing type annotations: it turns out that in the absence of them, different versions of the Scala compiler infer different types, so the same version of this library compiled with different versions of the Scala compiler results in binary incompatible JARs... This have been happening for a while - we just didn't know because no checks for this are run ;)

With this change, the APIs actually become compatible across the versions of the Scala compiler.

I think the exclusions are safe: they change some return types to more specific than what some versions of the Scala compiler infer; I do not know how to make absolutely sure that they are safe, but as a wise man once said:

we need to strike some kind of balance here between caution and paralysis, otherwise the library becomes impossible to improve at all

Is the fact that we are increasing the minor version on the next release relevant in this context?

Thank you!

@SethTisue
Copy link
Member

I wish I were comfortable with just shrugging at this, but sadly, this is a foundational library that is hugely common in big dependency trees. We already caused a lot of ecosystem disruption with the 1.x -> 2.x bump.

Is the fact that we are increasing the minor version on the next release relevant in this context?

Not really, since breaking bincompat would normally imply a major version bump, not just a minor one.

Are you certain that the bincompat breakage is only for Scala 3 users and not for Scala 2? I think we should be willing to take a bit more risk in a Scala 3 context, where the library is still relatively new and there isn't an army of legacy users to consider (merely a, I don't know, platoon of them? brigade?).

How about this: could we have one set of MiMa exclusions for Scala 2 and one for Scala 3, so that we can know for sure that any potential bincompat breakage is only on the Scala 3 side?

I think the exclusions are safe: they change some return types to more specific than what some versions of the Scala compiler infer; I do not know how to make absolutely sure that they are safe

Just to check, do you understand why MiMa considers the more specific types to break binary compatibility? Because I feel like we need to have a solid grasp of that in order to evaluate the risks here. In my experience, MiMa is hardly ever wrong.

@SethTisue
Copy link
Member

SethTisue commented Mar 13, 2023

This might interest @ckipp01, who was a key player in getting the 1->2 upgrade worked through the ecosystem.

also cc @lrytz

@dubinsky
Copy link
Collaborator Author

do you understand why MiMa considers the more specific types to break binary compatibility?

I do not - yet?
I am going to read up on this and document the consequences of every change; maybe then we'll see...
Thanks!

This was referenced Mar 26, 2023
@SethTisue
Copy link
Member

Do we close this one now that it's been split into #654 and #655?

@dubinsky
Copy link
Collaborator Author

Do we close this one now that it's been split into #654 and #655?

There is more to this whole than those two parts, but yes, let's close it: I'll package the rest of it separately.

@dubinsky dubinsky closed this Mar 27, 2023
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.

None yet

2 participants