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

Support Scala 3's Best Effort compilation #2049

Merged
merged 3 commits into from
Jun 27, 2024
Merged

Conversation

jchyb
Copy link
Contributor

@jchyb jchyb commented May 12, 2023

Related PRs

Explanation

This commit allows bloop to support the hopefully-upcoming Scala 3 best effort compilation for Metals, it being enabled with the other Metals options.

Best Effort is meant to be a set of Scala 3 compiler options that allow the errored, but typed programs, to be able to be serialized into a TASTy aligned format (Best Effort TASTy), and later to reuse those typed program trees in both the dependent projects, and in the presentation compiler.

Those best effort tasty files are always serialized to a seperate directory, which in this PR is managed by bloop. It is worth noting that the best effort compilation may fail, similarly to the regular compilation.

The best effort directory is kept outside of the regular build directory, in .bloop/project-id/scala-3/best-effort. There, two directories are managed, build and dep. Whenever metals compiles a project the build directory is populated with best effort artifacts. Then, if the best effort compilation is successful, they are copied to the 'dep' directory, which is used for dependencies, metals presentation compiler etc. This way if a compilation fails, the artifacts from the previous compilation can still be kept. This is not completely dissimilar to how the regular compilation works here, with the difference being the more straightforward implementation, without any counters that monitor how many tasks are being executed with the dep directory concurrently.

There may be some unnecessary redundancies in this draft PR,~~ where sometimes information about best effort directories of a project is unnecessarily passed around~~. Also, I probably need to add some tests.

frontend/src/main/scala/bloop/cli/Commands.scala Outdated Show resolved Hide resolved
@@ -70,6 +71,7 @@ case class CompileOutPaths(
analysisOut: AbsolutePath,
genericClassesDir: AbsolutePath,
externalClassesDir: AbsolutePath,
bestEffortDirs: Option[BestEffortDirs],
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we instead put it into META-INF directory similar to semanticdb? Or would that be picked up as normal classfiles?

Alternatively, if META-INF is not an option I would just put into a totally separate directory and ideally not handle it specifically in Bloop. The reason for that is that later we would need to reimplement this in sbt, mill, bazel, which will be a lot of problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There should not be any conflict between the best effort and regular tasty, so that is not an issue. The problem is that I needed to add a special handling of these "best effort tasty" files into bloop, regardless of where they will be put (but for it to work I needed 2 separate directories for those files). I'll try to explain:

There is a subset of compilation errors (minority), for which the compiler will not be able to produce those betasty files for now (and it does not sound easily fixable), instead just crashing (probably not ok, will need to have a separate error for best effort compilation fail). In those cases, metals would work as before. That creates an issue, that when a successful best effort compilation is followed by an unsuccessful BE compilation we are left with nothing (unlike following successful comp with a failing one, where we still have info from the successful one), as we delete the previous be-tasty files before recompiling to avoid stale signatures, etc. So I came up with having a separate directories for compilation (always starts empty, is filled with new betasty files), and dependencies (where we copy from the previous directory after confirming the best-effort compilation was successful, and actually use it in metals presentation compiler and downstream projects). Bloop actually already does something similar for regular compilation, but in a much more complex manner, with semaphore-like counters, etc. Now, we don't do that with the semanticdb files - I do not remember why this never became an issue there, will need to reinvestigate, maybe I am missing something.

I will admit I knew I would need to reimplement this for sbt, but did not know there were even more build servers to support (I incorrectly assumed that when using other build tools only bloop build server was available in metals). I guess we could keep the bloop behavior here, and just use a single directory (like META-INF) in other build servers (without the nice fail with successful best effort to fail with unsuccessful best effort fallback)

Copy link
Contributor

Choose a reason for hiding this comment

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

Now, we don't do that with the semanticdb files - I do not remember why this never became an issue there, will need to reinvestigate, maybe I am missing something.

What we actually do currently is that when files are invalidated, we back them up and move them back if the compilation failed:

/* Restore all files from backuped last successful compilation to make sure

We also do that for supported products:
val supportedCompileProducts = List(".sjsir", ".nir", ".tasty")

I think semanticdb files are not invalidated (that might be bug though 🤔 )

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 looked into the semanticdb thing very quickly while working on this now and it turns out there indeed seems to be a bug, but just adding .semanticdb will not fix it. The thing is the above file types are all tied to classfiles, which is a fact used during the invalidation. Semanticdb is tied to a source file, which means that it's harder to notice the invalid signatures, but they can be generated by simultaneous renamings of the file and changes to it.

@jchyb
Copy link
Contributor Author

jchyb commented Jul 6, 2023

Minor update - while trying to remove the custom file handling for betasty files, I discovered the my current implementation here is quite a bit broken. I did not take into account that files compiled with best-effort dependencies which do not introduce their own errors are treated by zinc as successful compilations, so for those I would obtain .semanticdb files (but would mistakenly clean newly produced .betasty). For compilations with errors the exact opposite would happen (semanticdb would be invalidated and cleaned, while betasty would be kept). I'll probably need to add a way to differentiate between successful non-best-effort compilations and best-effort compilations without shown errors

@jchyb
Copy link
Contributor Author

jchyb commented Jul 19, 2023

I fixed the issue mentioned by me above. I also removed the custom path handling for best effort compilation as asked. Now instead of specifying custom directory, we get best-effort-tasty files in META-INF/best-effort, similarly to semanticdb.

Now, I could not just reuse the aforementioned behavior in BloopClassFileManager.scala for handling outdated files, since it predominantly relies on recognizing which classifies were generated/deleted on making decisions based on that (not a luxury we have). Still, I could very easily remove the weird custom two-directory structure and custom handlings that came with it. I also could remove changes to the bsp and bloop-config projects which is a big plus, as previously we would have to use them to pass those directories around.

The PR is still very much WIP, the things I need to add:

  • most important - I believe it now does full recompilation of every upstream "failed" best-effort project, so I am going to need to find what causes this
  • add (any) tests
  • proper handling of failed best-effort compilations (now we rely on seeing if the compiler crashes, this of course will be changed into a separate error with non crash stack-trace, like is done for failing macros)
  • cleanup of the handleBestEffortSuccess, I kind of hacked it together quickly for now and it has some redundant elements (sorry)

@jchyb jchyb force-pushed the best-effort branch 2 times, most recently from 9f2386d to 8e006f6 Compare May 29, 2024 08:30
@jchyb jchyb marked this pull request as ready for review May 29, 2024 08:30
This commit allows bloop to support best effort compilation for Metals,
with it being enabled with the other Metals options.

Best Effort is meant to be a set of Scala 3 compiler options that allow
the errored, but typed programs, to be able to be serialized into a
TASTy aligned format (Best Effort TASTy), and later to reuse those
typed program trees in both the dependent projects, and in the
presentation compiler.

Those best effort tasty files are serialized to a
META-INF/best-effort in classesDir. Best effort compilation
may fail, similarly to the regular compilation. In that case we
stop compiling the downstream dependencies.

Currently best-effort compilation is unable t assist in producing the
zinc analysis files, which lead to the projects being recompiled every
time. This is solved with a custom hashing solution.
@jchyb
Copy link
Contributor Author

jchyb commented Jun 24, 2024

@tgodzik Here is what I actually changed since the last review:

  • I made it so that betasty coming from successful compilation is not copied into a clientDir (classes-Metals…).
  • I discovered that before, it was easy to produce an outdated betasty file that would not be cleaned up by bloop, by alternating between successful compilations and unsuccessful ones. I added a test for that too.

About copying last successful compilation artifacts into a clientDir to reuse in incremental compilations. I got a little confused before and it looks like there are two bloop-internal-classes directories used in a compilation: readOnlyClassesDir and newClassesDir. In best effort compilation, readOnlyClassesDir from what I understand will always represent the last successful compilation (with .class and .tasty), and newClassesDir will include newly compiled artifacts (.betasty when compiling erroring file). So perhaps we do not need to copy successful artifacts into a clientDir as I mentioned before (since it is still saved and reused on the bloop side).

@jchyb jchyb requested a review from tgodzik June 24, 2024 09:44
@tgodzik
Copy link
Contributor

tgodzik commented Jun 24, 2024

So perhaps we do not need to copy successful artifacts into a clientDir as I mentioned before (since it is still saved and reused on the bloop side).

Ach yes, pretty sure incremental compilation has nothing to do with the client dir. From what I remember there are 3 different directories:

  • readonly for current compilation later to be copied over
  • internal bloop directory
  • client directories for each client (pretty sure we can get rid of it.)

For incremental compilation only the second is important for sure.

@@ -41,7 +41,7 @@ final class BloopClassFileManager(
private[this] val generatedFiles = new mutable.HashSet[File]

// Supported compile products by the class file manager
private[this] val supportedCompileProducts = List(".sjsir", ".nir", ".tasty")
val supportedCompileProducts = List(".sjsir", ".nir", ".tasty", ".betasty")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we just put it into an object? We wouldn't need to pass manager in the params

t,
elapsed,
_,
bestEffortProducts @ Some(BestEffortProducts(compileProducts, previousHash))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bestEffortProducts @ Some(BestEffortProducts(compileProducts, previousHash))
bestEffortProducts @ Some(BestEffortProducts(previousCompilationResults, previousHash))

minor, but this seems misleading down since you delete newClassesDir and then copy from compileProducts.newClassesDir

And those are two different directories, right? You delete the current directory and copy over old classes dir?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, newClassesDir Is a newly created directory for new scalac compilation artifacts (which we will not have in this case so we delete it). The we copy to the previous (complete) results to clientDir (useful when we e.g restart metals and it's assigned a new empty directory)

backend/src/main/scala/bloop/Compiler.scala Show resolved Hide resolved
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM

@tgodzik tgodzik merged commit 79f9e6e into scalacenter:main Jun 27, 2024
17 checks passed
@tgodzik
Copy link
Contributor

tgodzik commented Jun 27, 2024

Btw. you test out 1.5.18-38-79f9e6e0-SNAPSHOT version on the Metals PR

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.

2 participants