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

Enable generation of TASTy files readable for older compilers #14156

Merged
merged 22 commits into from
Jan 19, 2022

Conversation

prolativ
Copy link
Contributor

@prolativ prolativ commented Dec 21, 2021

This is the first step towards achieving forward binary compatibility between different Scala minor releases (implementation of the first part of this proposal)

  • Mark all parts of the the stdlib added after 3.0 with @since annotation specifying the minor release introducing the specific element of the API
  • Add -Yscala-release compiler flag to specify the minimal target Scala release (minor version) which should be able to use the produced TASTy files (per analogy to -release setting the target version of JDK). This both sets the version of the generated TASTy files to the specified value and checks that the compiled code contains no runtime dependencies to parts of the stdlib introduced in a release with a higher version than specified (assuming their definitions have been properly marked with @since) or coming from other TASTy files with a higher version. If no -Yscala-release is specified then the current (latest) release version is used. This takes into account the fact that unstable versions of the compiler might have their TASTy version set to a higher value than a later stable version so e.g. compilation with 3.2.0-RC1-SNAPSHOT and -Yscala-release 3.2 (or no -Yscala-release specified) will produce TASTy files with an experimental TASTy version while changing the compiler version to stable 3.2.0 or -Yscala-release to 3.1 or 3.0 will result in using a stable format. -Yscala-release should not change the semantics of the compiled code (and it might prevent some code from compiling if the same semantics are not achievable for an older compiler - e.g. if a resolved implicit instance was added in a newer release)
  • Modify Vulpix to enable constructing tests which require compiling or running some sources with different version of the compiler or different value of -Yscala-release. These versions are specified by adding appropriate suffixes to file names - similarly to _1, _2 suffixes for specifying order of compilation (which are still valid) one can add _r${release} and _c${compilerVersion} suffixes (e.g. _r3.0, _c3.1.0). Different kinds of suffixes can be mixed together on a single file name when needed

For future minor Scala versions (starting from 3.2.0) some additional changes might be necessary to keep this feature working:

  • Mark all new APIs added to stdlib with @since (when removing @experimental)
  • If the TASTy format actually changes (besides just extending stdlib) make sure that the produced TASTy doesn't contain anything that couldn't be read by the specified older version of the compiler. If some newer language features are unexpressible in older TASTy, the compilation should fail
  • Check usages of new language imports (if they might break TASTy like described above)
  • Add the new release in ScalaRelease.scala

Co-authored by @Kordyjan

@prolativ
Copy link
Contributor Author

One thing that makes me wonder now is whether we should rename -scala-release to -Yscala-release for now and then drop Y for 3.2.0

Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

This seems to be lacking tests that check that the scala 3.0 compiler is actually able to use a library published by the new compiler with -scala-release 3.0. This kind of test could be done as sbt scripted tests, similar to how we already test scala 2 compatibility (https://github.com/lampepfl/dotty/tree/master/sbt-test/scala2-compat).

If the TASTy format actually changes (besides just extending stdlib)

Even though tasty didn't technically change between 3.0 and 3.1, the same code might now generate different tasty, whenever that happens we need tests that ensure the newer compiler output will still be interpreted correctly by the older compiler. Since we didn't keep track of situations where this happened while developing 3.1, we now need to go through every PR that was merged to check for these situations. The one I'm aware of is #12979

@smarter smarter removed their assignment Dec 21, 2021
@smarter
Copy link
Member

smarter commented Dec 21, 2021

whether we should rename -scala-release to -Yscala-release for now and then drop Y for 3.2.0

Possibly yes, but even if it's a -Y, that probably won't stop people from publishing libraries using it, so if we're saying that this is not ready for production usage yet then we need to carefully communicate this with library maintainers.

@smarter
Copy link
Member

smarter commented Dec 21, 2021

We should also have integration tests on whole projects I think, for example:

  1. Using the latest compiler, compile cats with -scala-release 3.0
  2. Using Scala 3.0.2, compile and run the tests of cats-effect with the output of step 1 on the classpath.

val tastyFilePath = classfile.path.stripSuffix(".class") + ".tasty"
val isTastyCompatible =
TastyFormat.isVersionCompatible(fileVersion = fileTastyVersion, compilerVersion = ctx.tastyVersion) ||
classRoot.symbol.showFullName.startsWith("scala.") // References to stdlib are considered safe because we check the values of @since annotations
Copy link
Member

@bishabosha bishabosha Dec 21, 2021

Choose a reason for hiding this comment

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

I'm not sure what this escape hatch helps with, shouldn't readFullHeader() already throw if the version is not compatible?

Edit: ok so we may be able to read the file, but this checks if it is safe to depend on with -scala-release version.

I am concerned about other libraries out there that use the scala package, such as Scalameta libraries

Copy link
Member

Choose a reason for hiding this comment

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

Even if only the scala stdlib used the scala package, this check still seems too broad: if the scala 3.2 compiler tries to read the stdlib of scala 3.3 and we actually changed tasty, it will crash when parsing the file.

Copy link
Member

Choose a reason for hiding this comment

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

I was suspicious of the @since treatment before, and I think I now understand better why.

Clearly, as noted by @smarter, it is not sufficient to guarantee that an older compiler can read the TASTy of the file containing the new definition. In fact, it does not even guarantee that it could read the other stuff present in that file, even if it could jump over the new definitions entirely. It could very well be that the rest of the code in the file produces new TASTy as the result of different language rules, or because TASTy got a new feature that simplifies encoding, etc. Since the library is not compiled with -scala-release, its TASTy will not be tailored to be readable by an older compiler.

Copy link
Member

Choose a reason for hiding this comment

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

Another, deeper problem with the @since treatment is that things like #14136 are fundamentally incompatible with it.

This wouldn't be an issue with the approach proposed by @julienrf at https://contributors.scala-lang.org/t/improving-scala-3-forward-compatibility/5298/4?u=sjrd (emit parts of the stdlib itself with -scala-release, and allow to depend on a newer stdlib than the compiler).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smarter maybe I missed something but in what situation would scala 3.2 compiler have to read TASTy of stdlib compiled with scala 3.3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you add a dependency to a library compiled for 3.3 but you're compiling with 3.2 you should get an error while reading TASTy files of classes defined inside this library anyway so this shouldn't really matter that much, right?

Copy link
Member

Choose a reason for hiding this comment

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

You might end up reading a tasty file from the standard library on the class path before reading one from the library you depend on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, so we could add an extra check, something like

val isTastyCompatible =
              TastyFormat.isVersionCompatible(fileTastyVersion, TastyVersion.compilerVersion) && (
                  TastyFormat.isVersionCompatible(fileTastyVersion, ctx.tastyVersion) ||
                  classRoot.symbol.showFullName.startsWith("scala.")
              )

or maybe handling these cases with different error messages. This should handle both the case when e.g.
a) we compile something with scala 3.3 compiler but with -scala-release 3.2 and we want the compilation to fail when the code has some dependency to a library targeting 3.3
b) we use scala 3.2 compiler to compile code which for whatever reason brings scala 3.3 stdlib to the classpath

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding a potential improvement of the check for whether something is indeed a part of stdlib: maybe we could be more specific in terms of the allowed package prefixes? I.e. check whether something starts with scala.annotation., scala.quoted. and so on? That might require slightly more work but still it shouldn't be that hard to maintain. Of course someone might still try to add something to these existing packages but that would be a very bad practice and I think we shouldn't focus on situations when someone writes such bad code on purpose

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I gave up the idea of more precise checks for prefixes. This seems not to be very flexible as some definitions are added directly to scala package so having to keep track of that is probably not worth the effort

package scala.annotation

/** An annotation that is used to mark symbols added to the stdlib after 3.0 release */
private[scala] class since(scalaRelease: String) extends scala.annotation.StaticAnnotation

Choose a reason for hiding this comment

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

I presume you have, but have you considered more type-safe alternatives like e.g.?

  • since(majorVersion: Int, minorVersion)
  • since(scalaRelease: "3.0" | "3.1")
  • since(scalaRelease: ScalaRelease)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The variant with two ints doesn't really give us much more safety. It would be still possible do use something invalid like since(1, -100).
A union of string literals would require changes to the signature of since for every minor release, which is not nice, as since is technically a part of stdlib (even though it's private).
ScalaRelease can't be used here because it's a part of compiler's internals.
So I think we'll have to live with strings although I've added some additional checks to validate their correctness

Copy link
Member

Choose a reason for hiding this comment

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

could this be in the scala.annotation.internal package?

@smarter
Copy link
Member

smarter commented Dec 21, 2021

Since we didn't keep track of situations where this happened while developing 3.1, we now need to go through every PR that was merged to check for these situations. The one I'm aware of is #12979

I had a quick look through all commits between 3.0.2 and 3.1 and didn't find anything else suspicious, but it'd be good if someone else did a pass too.

@bishabosha
Copy link
Member

bishabosha commented Dec 22, 2021

Even though tasty didn't technically change between 3.0 and 3.1, the same code might now generate different tasty, whenever that happens we need tests that ensure the newer compiler output will still be interpreted correctly by the older compiler. Since we didn't keep track of situations where this happened while developing 3.1, we now need to go through every PR that was merged to check for these situations.

Another possible candidate - #11686 (addition of an ordinal method and deriving.Mirror.Sum parent to a companion object) - If we wanted we could even replicate the old behaviour by the companion to have the same API as 3.0 we can avoid caching the mirror in the companion for a generic sum type with a child that is also a generic sum and -scala-release is 3.0

@sjrd
Copy link
Member

sjrd commented Dec 22, 2021

The point should not be to replicate the old behavior. If users wanted the old behavior, they would use the old compiler.

We should in all cases compile code using the semantics of the new compiler. The only thing that the flag should change is how we encode those same semantics using older TASTy, and what valid code to reject because we can't.

@prolativ
Copy link
Contributor Author

#12949 seems to work correctly because the definitions from exports which were missing are added during the compilation of the class into which they are exported.
In case of #11686 it's not only about the TASTy that gets generated during compilation of sealed traits because summoned mirrors are synthesized on the fly at the place where they are used. So scala 3.0 compiler will still complain when compiling summon[Mirror.Of[Foo]] where Foo is the root of a sealed types hierarchy even if Foo was compiled with scala 3.1 and -scala-release 3.0. There's no crash at runtime so that's not worse than if Foo was compiled with scala 3.0. However if we add one intermediate level i.e. compile Foo with 3.1 and -scala-release 3.0 and capture the summoned instance in a val which gets compiled with the same compiler and release and then we refer to this value from code compiled with 3.0 then the compilation is successful and it also works at runtime. So I think this is what we should expect. I'll commit some tests for that.

import math.Ordered.orderingToOrdered
val latestRelease = ScalaRelease.latest
val specifiedRelease = scalaRelease
if ((specifiedRelease.majorVersion, specifiedRelease.minorVersion) < (latestRelease.majorVersion, latestRelease.majorVersion)) then
Copy link
Contributor

Choose a reason for hiding this comment

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

latestRelease.majorVersion used twice.

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 catch. Actually I spotted that later too but haven't pushed my changes yet.

@prolativ prolativ force-pushed the scala-target-flag branch 2 times, most recently from 6c52cbc to c35f47a Compare December 29, 2021 12:40
@prolativ
Copy link
Contributor Author

We should also have integration tests on whole projects I think, for example:

  1. Using the latest compiler, compile cats with -scala-release 3.0
  2. Using Scala 3.0.2, compile and run the tests of cats-effect with the output of step 1 on the classpath.

@smarter
If we want to test a project with scala 3.0.2 we would have to compile all of its transitive dependencies with -scala-release 3.0. And cats-effect has quite a lot of them. Do you insist on testing this particular project or should I choose something more lightweight not to slow down the community build too much?

@prolativ
Copy link
Contributor Author

prolativ commented Jan 3, 2022

Not sure how this would work with our infrastructure but if we decide to add a bunch of new projects to the community build as mentioned before (or rather old projects but compiled in a different way) would it make sense to create a new group of projects that would be tested in parallel to the existing groups A, B and C?

@smarter
Copy link
Member

smarter commented Jan 3, 2022

Do you insist on testing this particular project

No, that was just an example. Alternatively we could compile cats (or some other project) with -scala-release 3 but then compile and run the tests of that project with scala 3.0.2, so we don't need to deal with leaf projects with many dependencies. I don't have any specific idea of how this should be implemented and whether it should be implemented in the current community build or the jenkins-based one you've been working on.

@prolativ
Copy link
Contributor Author

prolativ commented Jan 3, 2022

Actually compiling one project with a specific scala release set and then running the tests of the same project using an older scala runtime would be more difficult to implement in our current community build (or at least I haven't found an easy way to use a different version of scala in Compile and Test in sbt). But I think I already have most of the building blocks needed to implement what you requested at first without introducing too many modifications to the existing community build so now it's mainly the question how to get real value from adding these tests without making the community build swell too much.
Regarding the new community build the plans for the nearest future are that it is going to co-exist with the current build rather than replace it. Also the scenario of checking how binary compatibility works forwards and backwards shouldn't be too difficult to implement in the new approach.
And besides these test do you have any other remarks to my PR? It should get merged before the branch cut-off for 3.1.2-RC1 and I wouldn't want it to be a blocker.

@smarter
Copy link
Member

smarter commented Jan 3, 2022

you have any other remarks to my PR?

I don't have time to review this in more details, sorry. Perhaps @bishabosha can take over.

@smarter
Copy link
Member

smarter commented Jan 3, 2022

now it's mainly the question how to get real value from adding these tests without making the community build swell too much.

I suggest only running them on nightly builds.

@bishabosha bishabosha self-requested a review January 3, 2022 15:52
@prolativ
Copy link
Contributor Author

@smarter I slightly extended our community build to include the requested tests although I didn't manage to fully test the intended behaviour because it turned out it would probably require some extra support from tooling.
The problem is that when a project A is compiled with scala 3.1.x and -Yscala-release 3.0 sbt doesn't handle this option in any special way and it automatically adds scala 3.1.x stdlib as a library dependency to the project, which is visible in its pom after publishing. Then if we try to build project B using scala 3.0.2 and B depends on A scala 3.1.x stdlib gets added to the classpath but its TASTy files can't be read by the older compiler.
To make the tests pass for now I build cats-effect and all of its dependencies with the latest compiler and -Yscala-release 3.0 but there's no project built with scala 3.0.2.
I would suggest merging these changes and then, after 3.1.2-RC1 gets released, we might start thinking how to provide proper support for -Yscala-release 3.0 in build tools and update the tests in our community build.

@bishabosha could you have a look at the PR?

@prolativ prolativ requested a review from smarter January 18, 2022 13:26
@@ -972,7 +972,7 @@ class ClassfileParser(
def isStdlibClass(cls: ClassDenotation): Boolean =
ctx.platform.classPath.findClassFile(cls.fullName.mangledString) match {
case Some(entry: ZipArchive#Entry) =>
entry.underlyingSource.map(_.name.startsWith("scala3-library_3-")).getOrElse(false)
entry.underlyingSource.map(_.name.startsWith("scala3-library_")).getOrElse(false)
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 still not a fan of this kind of check, as I said jars can have any names and it'd be weird if we suddenly started imposing undocumented restriction on jar names in some situations.

Copy link
Member

Choose a reason for hiding this comment

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

one situation where this could potentially fail: if someone uses sbt-assembly to package all their dependencies together and also uses staging, the compiler used at runtime will have only the sbt-assembly jar on its classpath which won't be called scala3-library

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if I understood you correctly. Let's consider a simple scenario.
Project A depends on project B.
Project B is packaged into sbt-assembly jar together with all of its dependencies (including stdlib)

Now we have a few possibilities of what version of the compiler A and B are compiled with:
a) Both A and B use the same minor version - OK
b) A uses 3.x1.y1 without -Yscala-release and B uses 3.x2.y2 if x1 > x2 - OK no matter if B uses -Yscala-release
c) A uses 3.x1.y1 and B uses 3.x2.y2 with -Yscala-release 3.x1 if x1 < x2 - here we encounter a problem because the fat jar will contain stdlib classes from 3.x2.y2 instead of 3.x1.y1 and 3.x1.y1 compiler won't be able to read them.
This isn't specifically a problem of this one check because the problem would still occur if x1 = 0 and x2 = 1. This seems all about the fact that build tools are not aware of -Yscala-release. But still we would need to release this feature as experimental first to let people play with it and then fix the build tools

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 have a specific scenario in mind, I just don't think it's wise to have a check like this.

Copy link
Member

Choose a reason for hiding this comment

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

I really think we should just warn people to not use package scala, it's already dangerous because of package private scala definitions.

Copy link
Member

Choose a reason for hiding this comment

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

(also the reason why we're special casing the standard library here should be documented in the code)

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 not opposed to having a filename-based check for now if it avoids accidentally using a 3.1 dependency from a -Yscala-release 3.0 project, but we should treat it as a temporary hack until we have a reliable way of detecting tasty files from the standard library (we could have a flag in the tasty file for that for example).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I thought about adding such kind of a flag to TASTy but for this proper solution we would have to wait until the next minor release. And even now in the worst case our checks would be too strict. So while checking TASTy version we make an exception if we're pretty sure some class comes from the stdlib while checking only the package prefix would allow some other libraries leak references to the newer stdlib API

Thanks to TASTy files, which are produced during compilation of Scala 3 sources, unlike Scala 2, Scala 3 is backward binary compatible between different minor versions (i.e. binary artifacts produced with Scala 3.x can be consumed by Scala 3.y programs as long as x <= y).
There are however already some ongoing attempts to make Scala 3 forward binary compatible which means that, with some restrictions, it might be possible to compile Scala code with a newer version of the compiler and then use the produced binaries as dependencies for a project using an older compiler.
In Scala 2 different minor versions of the compiler were free to change the way how they encode different language features in JVM bytecode so each bump of the compiler's minor version resulted in breaking binary compatibility and if a project had any Scala dependencies they all needed to be (cross-)compiled to the same minor Scala version that was used in that project itself.
While in Scala 3 the JVM encoding might still change between minor versions, an additional intermediate format of code representation called TASTy (from `Typed Abstract Syntax Tree`) was introduced.
Copy link
Member

Choose a reason for hiding this comment

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

This makes it sound like any minor version of Scala 3 might break binary compatibility, but we have no plans to do that so far and won't break it for many years hopefully. Moreover, if/when we do break bincompat, there's a good chance we'll call it Scala 4 to avoid confusion although this isn't settled (cf #10244 (comment))

@lrytz
Copy link
Member

lrytz commented Jan 19, 2022

When I use 3.2 and compile a library with -Yscala-target 3.1, what does the POM say? Will it depend on scala3-library_3:3.1.x or 3.2.x? Is this the reason for the exception in the ClassfileParser?

@prolativ prolativ requested a review from smarter January 19, 2022 09:48
@prolativ
Copy link
Contributor Author

@lrytz as already mentioned, -Yscala-release doesn't by itself change the dependencies declared in POM files. We would need to make build tools aware of this flag to handle this properly. Until that time users will have to use the workaround given in the docs

@lrytz
Copy link
Member

lrytz commented Jan 19, 2022

OK, thank you. If users of an older compiler need to manually enforce the Scala 3 library version, why is the exception in the ClassfileParser needed?

@prolativ
Copy link
Contributor Author

As explained in the code comment: special handling of stdlib in ClassfileParser is needed because by setting -Yscala-release 3.0 we make scala 3.1 compiler reject all TASTy files in a version newer than 28.0-0 (corresponding to scala 3.0). However When compiling code with scala 3.1 the stdlib from 3.1 is still on the classpath and this shouldn't cause the compiler to crash. And this is safe because during compilation of sources we check that all references to stdlib classes (and their methods etc.) will remain legal when the binaries of stdlib get replaced by an older version at runtime

@prolativ
Copy link
Contributor Author

Actually we first needed to find the workaround with dependencyOverrides while trying to test -Yscala-release in our community build

@lrytz
Copy link
Member

lrytz commented Jan 19, 2022

Got it.

  • Library Authors using 3.2 with -Yscala-release 3.1 will have the Scala 3.2 library on the classpath, the exception in the classfile parser allows that.
  • Users of 3.1 need to override the Scala 3 library version in the build tool to 3.1, otherwise they might end up with a 3.2 library on the classpath. An exception for this case would not be safe because the 3.2 Tasty might have entries that the 3.1 compiler doesn't understand.

Having -Yscala-release change the POM dependency in the build tools would be nice...

@prolativ prolativ merged commit 16b962e into scala:master Jan 19, 2022
@prolativ prolativ deleted the scala-target-flag branch January 19, 2022 15:03
@griggt
Copy link
Contributor

griggt commented Jan 19, 2022

It seems that the new Vulpix-based compat tests are failing on Windows, see test_windows_full workflow on merge:

https://github.com/lampepfl/dotty/runs/4869516643?check_suite_focus=true#step:4:900

Fatal compiler crash when compiling: tests\run\forwardCompat-strictEquals:
Cannot run program "C:\Windows\ServiceProfiles\NetworkService\.cache\dotty\test\compilers\scala3-3.0.2/bin/scalac": CreateProcess error=193, %1 is not a valid Win32 application
java.lang.ProcessBuilder.start(ProcessBuilder.java:1048)
java.lang.Runtime.exec(Runtime.java:621)
java.lang.Runtime.exec(Runtime.java:486)
dotty.tools.vulpix.ParallelTesting$Test.compileWithOtherCompiler(ParallelTesting.scala:539)

@prolativ
Copy link
Contributor Author

This should be fixed by #14301

@SethTisue
Copy link
Member

SethTisue commented Oct 20, 2022

This was removed by #15146, due to the considerations summarized in https://www.scala-lang.org/blog/2022/08/17/long-term-compatibility-plans.html

@Kordyjan Kordyjan added this to the 3.1.2 milestone Aug 2, 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.