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

[proposal] Literal Scala versions #16

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

satorg
Copy link
Contributor

@satorg satorg commented Oct 2, 2022

Disclaimer: everything is controversial here! Also for now it is supposed to fail binary compatibility checks against v0.1.1. Honestly, I am not sure if it is even feasible to maintain the binary compatibility in this case. But I am open to any thoughts or suggestions! Also since it is a pretty young project, perhaps breaking the bin-compat would not be that bad for now, if it helped to make it more powerful and user-friendly.

What?

The primary goal for this PR is to add support for compile-time literals (sver) for referencing scala versions, e.g.:

val lintUnsoundMatch = lintOption("unsound-match", version => version.isBetween(sver"2.11.0", sver"2.13.0"))

instead of

val lintUnsoundMatch = lintOption("unsound-match", version => version.isBetween(V2_11_0, V2_13_0))

Therefore it allows to get rid of pre-defined ScalaVersion constants like V2_11_0, etc.

To achieve that it makes the construction of ScalaVersion class safe, i.e. it becomes impossible to create a ScalaVersion instance that is not currently supported:

sver"a.b.c" // won't compile because it is not a version
sver"2.13.25" // won't compile because there's no such Scala version

Also it introduces runtime type-safe constructors:

ScalaVersion.from(2.12.0) // returns `Some(ScalaVersion(2, 12, 0))`
ScalaVersion.from(2.10.0) // returns `None` because 2.10.x are not supported anymore
ScalaVersion.fromString("2.11.0") // returns `Some(ScalaVersion(2, 11, 0))`

and their "unsafe" counterparts:

ScalaVersion.unsafeFrom(2.12.0) // returns `ScalaVersion(2, 12, 0)`
ScalaVersion.unsafeFrom(2.10.0) // throws `IllegalArgumentException`
ScalaVersion.fromString.unsafe("2.11.0") // returns `ScalaVersion(2, 11, 0)`

Moreover it introduces a collection of all Scala versions that the library is currently supposed to support:

ScalaVersion.knownVersions: SortedSet[ScalaVersion]

Since it is a SortedSet it allows range operations:

knownVersions.range(sver"2.12.0", sver"3.0.0") // all versions from 2.12.0 (inclusively) and until 3.0.0 (exclusively)

Which is an additional win, I think.

Why?

Unsafe (unvalidated) models are generally discouraged in Scala, especially in common-purpose libraries. However, simply adding safe from/fromString runtime-validated methods would make the library inconvenient for users: I would suppose that users would appreciate the ability to refer to some particular Scala version without extra hustle of checking results every time they need it. On the other hand, maintaining a huge bunch of all supported Scala version in a form of V2_11_0, V2_11_1, ..., V3_2_0, etc is kinda ugly and (a little bit) error-prone.

So now the idea of some macro-based solution emerges!

How?

The literally library seems to be pretty suitable for that: it allows to turn strings into literals of some specific type and the Scala versions are naturally represented as strings. So these two concepts match each other pretty well.

Unfortunately, it seems impossible to define a macro and use it within the same compilation module. Therefore the macros for the literals have to be extracted into a separate module scalac-options-macros. Furthermore, since ScalacVersion class itself is used by the macro, it has to be extracted out of the main module as well.

Hence there are two additional modules added: scala-options-macros and scala-options-core. The latter is proposed to host core classes like ScalaVersion (along with ScalaOption and maybe others in the future), whereas the primary scalac-options should keep the DSL definitions only (like constants from the ScalacOptions object).

Nevertheless, the new scala-options-macros and scala-options-core are made unpublishable. That means, they exist for the sake of compile-time separation only. Whereas the primary scalac-options module gets all the content from those two modules and hence looks like a "solid" single module, when published (see Macro-Projects).

What Else?

There's one more module introduced: scalac-options-testkit. For now it hosts Scalacheck's Gen and Arbitrary for ScalaVersion only which are also made used in some. Therefore the module's main config contains Sclalacheck instances, whereas the test config hosts all the unit tests. I think it may be pretty convenient anyway.

bldPatches(2, 13, 9)
bldPatches(3, 0, 2)
bldPatches(3, 1, 3)
bldPatches(3, 2, 0)
Copy link
Member

Choose a reason for hiding this comment

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

It could cause problems that this list must be maintained by hand.

I wonder if we can come up with some way that Scala Steward could recognise these versions and bump them, e.g. by using a String here instead of major,minor,patch?

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather not get into a position where we have to release a new version of this library (and all downstream tooling based on it) for every new Scala version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call out. I tried to avoid enumerating all known versions by hand, but for the sake of to be understood by scala-steward we could create a list of all known versions and put it into a separate file:

private [scalacoptions] KnownScalaVersions {
  protected val knownVersionStrings = Seq(
    "2.12.0",
    "2.12.1",
    ...
    "3.2.0"
  )
}

and then use it from the ScalaVersion object:

object ScalaVersion extends KnownScalaVersions {
  val knownVersions = knownVersionStrings.map(fromString.unsafe)
}

or something like that.
Would it work, wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

Can we just leave it open ended? So unless a series is known to be EOL it will accept not-yet-released versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@armanbilge just to make sure I understand you correctly,

you would prefer to avoid having scala-steward involved into that completely, wouldn't you?
I.e. rather than having scala-steward involved, you're suggesting to make ScalaVersion more flexible and supporting versions like "2.13+", "2.13.9+", etc?

Copy link
Member

Choose a reason for hiding this comment

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

you would prefer to avoid having scala-steward involved into that completely, wouldn't you?

Right, it's not that I care about Scala Steward specifically, but rather that we would have to release a new version of this library (and thus a new version of all the plugins that use it) for every new Scala version.

you're suggesting to make ScalaVersion more flexible and supporting versions like "2.13+", "2.13.9+", etc?

I don't think that's necessary. But I do think it should accept 2.13.20 today even though that's not yet released.

scalacOptions.value
},
libraryDependencies ++= Seq(
"org.typelevel" %%% "literally" % literallyVersion
Copy link
Member

@DavidGregory084 DavidGregory084 Oct 6, 2022

Choose a reason for hiding this comment

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

Is there a way for us to check that literally doesn't end up on our users' runtime classpath when depending upon this library? Perhaps I'm being paranoid 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... I like your idea, but is it feasible in principle? I'll try to figure out that.

Copy link
Member

Choose a reason for hiding this comment

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

I don't necessarily mean an automated way to do this but it would be nice to create a project that depends on this library and check the runtimeClasspath in sbt 😃

import c.universe._

if (ScalaVersion.fromString(s).isEmpty)
Left(s"incorrect or unsupported Scala version")
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should use the same error message String as ScalaVersion.fromString.unsafe here?

val gen =
Arbitrary
.arbitrary[String]
.filterNot(Parser.genericVersionRe.pattern.matcher(_).matches())
Copy link
Member

Choose a reason for hiding this comment

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

Not urgent, but perhaps we should have a few example-based tests in here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean, tests for some pre-defined values like "0.1.2" , "3.4.5" and so on?

Copy link
Member

@DavidGregory084 DavidGregory084 left a comment

Choose a reason for hiding this comment

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

I like this proposal, although I have not felt the pain that motivates its introduction personally.

I'm a little worried about introducing a set of "known Scala versions", and it might annoy users if they can't declare that an option should be removed in an as-yet-unknown Scala version if they are unable to upgrade scalac-options for some reason, for example if they cannot upgrade the dependent plugins like sbt-typelevel or sbt-tpolecat due to an unrelated issue.

Do you think there is some way that we could mitigate that, for example by providing an escape hatch?

@armanbilge
Copy link
Member

I had a thought, not entirely sure if it's relevant here.

.isBetween(sver"2.11.0", sver"2.13.0"))

So far it seems like the plan for Scala 3 is to have LTS series e.g. Scala 3.3.x looks like to be an LTS, which will get backports of features from the active development series.

So it doesn't seem impossible we might frequently end up in a situation where a compiler flag is available in e.g. 3.3.10+ and 3.5.0+ but not in 3.4.x at all. Since it's not a clean in-between relationship, do we have a good way of expressing this?

I suppose it's not different than a flag that appears in the middle of 2.12 and 2.13 series.

@satorg satorg force-pushed the literal-scala-version branch 2 times, most recently from 59ed2ce to fb52978 Compare October 7, 2022 18:59
@DavidGregory084
Copy link
Member

So it doesn't seem impossible we might frequently end up in a situation where a compiler flag is available in e.g. 3.3.10+ and 3.5.0+ but not in 3.4.x at all. Since it's not a clean in-between relationship, do we have a good way of expressing this?

Yes actually the ScalacOption constructor just accepts an isSupported: ScalaVersion => Boolean function, which has been useful before (e.g. options that were removed in 2.13 that reappeared in 3.x, 2.12.x backports, that kind of thing)

@satorg
Copy link
Contributor Author

satorg commented Oct 10, 2022

Would we consider bringing something like Diet from cats-collection into scalac-option, btw?
Since it encodes sets of ranges of any type and allows their composition, it could be pretty helpful in accommodation of the idea of "not-in-between" versions?

@armanbilge
Copy link
Member

Sounds good to me. 👍

@satorg satorg force-pushed the literal-scala-version branch from fb52978 to 4ec3ca4 Compare October 13, 2022 18:48
@satorg satorg force-pushed the literal-scala-version branch from 4ec3ca4 to 40cbc47 Compare October 21, 2022 07:44
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