Support Scala 3's Best Effort compilation #2049
Conversation
| @@ -153,6 +153,10 @@ object Commands { | |||
| incremental: Boolean = true, | |||
| @HelpMessage("Pipeline the compilation of modules in your build. By default, false.") | |||
| pipeline: Boolean = false, | |||
| @HelpMessage( | |||
| "Activates the best effort mode, used for specific compiler versions. By default, false." | |||
There was a problem hiding this comment.
I don't think we need it in the CLI
| @@ -70,6 +71,7 @@ case class CompileOutPaths( | |||
| analysisOut: AbsolutePath, | |||
| genericClassesDir: AbsolutePath, | |||
| externalClassesDir: AbsolutePath, | |||
| bestEffortDirs: Option[BestEffortDirs], | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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:
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 🤔 )
There was a problem hiding this comment.
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.
|
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 |
| * Handles successful Best Effort compilation. | ||
| * Does not persist incrementalCompilation analysis. | ||
| */ | ||
| def handleBestEffortSuccess( |
There was a problem hiding this comment.
So this is needed so that we don't persist analysis? What happens if we do? Could explain a bit more in the comment?
There was a problem hiding this comment.
Oh sorry, this was a forget-me-not I ironically forgot about. I'm still going to have to run through this method, clean it up and make things less redundant - but basically now everything should at the very least work. About the comment, for now I reused a some methods from the way successful compilation is handled, however I had to remove handling of analysis.bin files from those for two reasons:
- to work it needs xsbti.compile.MiniSetup obtained from "successful" zinc compilation which we do not always have
- I guess we could reimplement it but I saw no purpose to it for those "best-effort" cases - it does not seem to affect anything there
|
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 The PR is still very much WIP, the things I need to add:
|
jchyb
force-pushed
the
best-effort
branch
3 times, most recently
from
50a3090
to
ac0b1c0
Compare
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. Additionally, leftover best-effort compilation directories are now properly cleaned if replaced.
jchyb
force-pushed
the
best-effort
branch
6 times, most recently
from
ce5b9c3
to
d691ee1
Compare
…lient Also fix issue with disappearing errors
|
|
||
| if (noopBestEffortResult.isDefined) { | ||
| logger.debug("Skipping redundant best-effort compilation") | ||
| return Task { noopBestEffortResult.get } |
There was a problem hiding this comment.
This is probably a bit ugly as nonLocal returns are not used anywhere else in bloop, but any other solution would destroy the diff in Compiler.scala. Maybe I should squash the commits into one and then add a change from this to an else/pattern match, so that the review can be done on the basis of the first commit, but the actual merged code will be cleaner?
There was a problem hiding this comment.
Might be good to squash as it's already a lot of commits with contradicting changes
| case a: ReporterAction.ProcessEndCompilation => | ||
| a.code match { | ||
| case BspStatusCode.Cancelled | BspStatusCode.Error => | ||
| reporter.processEndCompilation(previousSuccessfulProblems, a.code, None, None) | ||
| reporter.processEndCompilation(previousProblems, a.code, None, None) | ||
| reporter.reportEndCompilation() |
There was a problem hiding this comment.
Changes here (and similar below) are not strictly related to best effort compilation, but I found out that clicking go-to-reference from metals on a dependency project pointing to a dependent project, I would get the "Deduplicating compilation of (...)" log and I would lose all of the diagnostics from metals. This (and the same below) is the culprit, and it seems to me that returning previousProblems (not necessarily from last successful run) is more correct here
There was a problem hiding this comment.
och, good catch! Any chance to get it as a separate fix maybe?
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,buildanddep. Whenever metals compiles a project thebuilddirectory 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 thedepdirectory 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.