Skip to content

Compile with Scala 3 #166

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

Merged
merged 1 commit into from
Feb 16, 2022
Merged

Compile with Scala 3 #166

merged 1 commit into from
Feb 16, 2022

Conversation

povder
Copy link
Contributor

@povder povder commented Dec 31, 2021

Compile with Scala 3. This is a prerequisite: scala/sbt-scala-module#150

Comment on lines +11 to +17
implicit def iterableDecorator[C](coll: C)(implicit it: IsIterable[C]): IterableDecorator[C, it.type] =
new IterableDecorator(coll)(it)

implicit def SeqDecorator[C](coll: C)(implicit seq: IsSeq[C]): SeqDecorator[C, seq.type] =
implicit def seqDecorator[C](coll: C)(implicit seq: IsSeq[C]): SeqDecorator[C, seq.type] =
new SeqDecorator(coll)(seq)

implicit def MapDecorator[C](coll: C)(implicit map: IsMap[C]): MapDecorator[C, map.type] =
implicit def mapDecorator[C](coll: C)(implicit map: IsMap[C]): MapDecorator[C, map.type] =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing these names is required for it to compile with Scala 3 (the name of implicit def can't clash with the existing class). I understand this breaks the compatibility so I'm leaving the decision here to the maintainers.

Options:

  1. Change the names, break the compatibility, don't bump the major version
  2. Change the names, break the compatibility, bump the major version
  3. Change the names only for scala 3 (create a dedicated file for scala 3)

Copy link
Collaborator

@julienrf julienrf Jan 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @povder, thank you for your contribution!

Option 1 is not practicable. We have to bump the major version number if we break the binary compatibility.

I am not sure about option 3: I believe it should be safe because the Scala 3 artifacts have no previous release, and users can not depend on both Scala 2 and Scala 3 artifacts in the same build. I wonder if this can create issues somehow, still… Also, I am a bit worried by the introduced complexity on our side.

Last thougth: it would be a good opportunity to move to 1.x.y versions instead of the current 0.x.y.

Thougths?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your input @julienrf. My preferred solution is to change the names for both Scala 2 and 3, keep the single file and bump version to 1.0.0.

Copy link
Member

@SethTisue SethTisue Jan 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't take it as given that this repo should ever go to 1.0.0. It seems plausible to me that it could stay in 0.x versions forever, with no bincompat guarantees at all.

It depends on whether you see the purpose of this library as making user-contributed code available for use in production, or just to make it available for people to try out so it can be further improved. And I'm not sure we've ever really decided that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so maybe another solution would be to go with a version 0.3.0, but I wonder what would be the problem of using 1.0.0 instead of 0.3.0? The only issue I see with 0.3.0 is that it won’t be possible to distinguish between a future release that breaks source compatibility and a future release that breaks binary compatibility (they would both have the number 0.4.0).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking more of the social aspect than the technical aspect. Version numbers send signals to human beings as well as to tooling, and staying on 0.x indefinitely helps communicate that this repo is just a pile of experimental code that may or may not have been seriously tested or vetted by anybody.

Copy link
Contributor Author

@povder povder Jan 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SethTisue I think people may still expect releases of this lib to be binary compatible on minor version bumps even if it hasn't ever been publicly communicated because versionPolicyIntention := Compatibility.BinaryCompatible is set.

Edit: after re-reading your comment I understand that your point was only about 1.0.0.not about not having a binary compatibility policy.

@povder povder mentioned this pull request Dec 31, 2021
@SethTisue
Copy link
Member

SethTisue commented Jan 6, 2022

This is a prerequisite: scala/sbt-scala-module#150

sbt-scala-module 3.0.1 is on its way to Maven Central

@povder
Copy link
Contributor Author

povder commented Jan 6, 2022

Okay, updated sbt-scala-module to 3.0.1. The build still fails because of MiMa violations and because sbt-dynver plugin is used I don't think I can bump the version to 0.3.0 as part of this PR. I believe bumping the version will make the MiMa violations go away.

@julienrf
Copy link
Collaborator

julienrf commented Jan 8, 2022

Thank you @povder!

The way to address the MiMa issues is to relax the compatibility intention by changing this:

versionPolicyIntention := Compatibility.BinaryCompatible,

to Compatibility.None.

@povder
Copy link
Contributor Author

povder commented Jan 8, 2022

For some reason I thought that 0.3.0 would be a new major version and so the binary compat promise would be kept but I understand now that it would need to be a 1.0.0.

Are you sure we want to drop the BinaryCompatible setting? It's up to the maintainers but my opinion is this library should continue using semantic versioning.

@julienrf
Copy link
Collaborator

julienrf commented Jan 8, 2022

releasing 0.3.0 would still follow semantic versioning: when the first version number is 0, then the second version number acts as a major number.

Unfortunately, I am not sure we can compile the existing code with Scala 3, so I guess we’d have to break the binary compatibility.

@povder
Copy link
Contributor Author

povder commented Jan 8, 2022

Ah, ok, so 0.3.0 would be the next major version.

The binary compatibility needs to be broken but if we release this as 0.3.0 then MiMa shouldn't complain and we don't need to change the version policy to Compatibility.None.

My current thinking is:

  • we don't need to change the bincompat setting if this is going to be released as 0.3.0
  • the build fails because MiMa complains that the bincompat is broken in 0.2.x series
  • I can't change the version to 0.3.0 to make the build pass because sbt-dynver is used and I think I'd need to make a tag in the repo or something to change the version
  • this PR's build check will never pass and the build will only succeed MiMa checks when merged to main and tagged

@julienrf
Copy link
Collaborator

julienrf commented Jan 9, 2022

  • we don't need to change the bincompat setting if this is going to be released as 0.3.0

No, we do have to change the compat setting because 0.3.0 would be a major release.

  • the build fails because MiMa complains that the bincompat is broken in 0.2.x series

No, it fails because we told it that we intend to stay BinaryCompatible.

  • I can't change the version to 0.3.0 to make the build pass because sbt-dynver is used and I think I'd need to make a tag in the repo or something to change the version

For now, we don’t need to change the version, it is okay to use the one computed by sbt-dynver. We just need to adjust the level of intended compatibility (to Compatibility.None).

@povder
Copy link
Contributor Author

povder commented Jan 9, 2022

Thanks for explaining @julienrf! I misunderstood how MiMa works.

Copy link
Member

@SethTisue SethTisue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, wdyt @julienrf ?

Copy link
Collaborator

@julienrf julienrf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, the changes look good to me, thank you @povder! However, I think we should either use Scala 3.0.2, or Scala 3.1.2-RC1 with the compiler option -Yscala-release:3.0.

@povder
Copy link
Contributor Author

povder commented Feb 6, 2022

Overall, the changes look good to me, thank you @povder! However, I think we should either use Scala 3.0.2, or Scala 3.1.2-RC1 with the compiler option -Yscala-release:3.0.

@julienrf I've changed Scala 3 version to 3.1.2-RC1. I had to add -Wconf:cat=deprecation:s compiler option for it to compile. When compiling with Scala 2 -Wconf:origin=scala.collection.IterableOps.toIterable:s is used but it looks like Scala 3.1 doesn't support origin filter so I had to silence the entire category.

@SethTisue
Copy link
Member

@lrytz is that true about -Wconf in Scala 3? is there an alternative?

@povder
Copy link
Contributor Author

povder commented Feb 6, 2022

@SethTisue since you're asking @lrytz about -Wconf I'll provide more info.

With -Wconf=cat=deprecation&origin=scala.collection.IterableOps.toIterable:s I'm getting this:

[error] Failed to parse `-Wconf` configuration: cat=deprecation&origin=scala.collection.IterableOps.toIterable:s
[error] unknown filter: origin

@julienrf
Copy link
Collaborator

julienrf commented Feb 7, 2022

Thank you @povder for your continuous effort! I recently discovered that Scala 3 support of forward compatibility is not mature enough (see the release notes). In particular, it requires special support from the tooling side, and that support is not ready yet. There is some work in progress at sbt/sbt#6814.
I am fine with merging this, but we should update to 3.2.0 final before the next release of scala-collection-contrib, and we should make sure the support on the tooling side is ready.

@lrytz
Copy link
Member

lrytz commented Feb 7, 2022

@lrytz is that true about -Wconf in Scala 3? is there an alternative?

yeah, the Wconf filters are not the same between 2 and 3. See scala/scala3#12857.

@SethTisue SethTisue merged commit d69d9b1 into scala:main Feb 16, 2022
@SethTisue
Copy link
Member

thank you @povder!

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.

4 participants