Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Minor cleanup #649

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

Closed
dubinsky wants to merge 1 commit into scala:main from dubinsky:minor-cleanup
Closed

Minor cleanup #649

dubinsky wants to merge 1 commit into scala:main from dubinsky:minor-cleanup

Conversation

Copy link
Contributor

@dubinsky dubinsky commented Feb 16, 2023
edited
Loading

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
Copy link
Member

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

Copy link
Contributor 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!

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.

Copy link
Member

SethTisue commented Mar 13, 2023
edited
Loading

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

also cc @lrytz

Copy link
Contributor 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
Copy link
Member

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

Copy link
Contributor 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.

SethTisue reacted with thumbs up emoji

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /