feature: Add support for Scala 3's Best Effort compilation #5219

jchyb · 2023-05-12T08:32:56Z

Related to feature request: scalameta/metals-feature-requests#327

Related PRs

dotty PR: Introduce Best Effort compilation options scala/scala3#17582
bloop PR: Support Scala 3's Best Effort compilation scalacenter/bloop#2049

The dotty one explains the reasoning behind the Best Effort compilation feature in detail, while the bloop one implements most of the logic of how projects are managed with the best effort options. All of them are needed for this feature to work here.

Explanation

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 as a TASTy aligned format (Best Effort TASTy), to be able to reuse those trees in both the dependent projects, and in the presentation compiler. This will allow better handling of projects that do not compile, and all of the dependent ones.

Basically, we are able to obtain a separate directory from bloop, that holds Best Effort TASTy, and can use it in PC. At the same time, SemanticDB generation is left as-is, those are created in the build file generated by bloop. Most of the best effort handling is done by bloop, including correctly setting the classpath with best effort directories. Let me quickly explain a few situations concerning project compilation that could happen before, and what can happen now with these PRs (especially with how the bloop one works):

Before:

project fails compilation (we get nothing)
project compiles („regular” artifacts for PC and goto def are generated) -> Project fails (previous artifacts can be reused)

As of this PR:

project fails compilation with an unsuccessful Best Effort compilation (we get nothing, like before)
project compiles (regular artifacts are generated) -> project fails compilation with an unsuccessful Best Effort (previous artifacts can be reused, like before)
project fails compilation with a successful Best Effort compilation (we get Best Effort artifacts) -> project compiles (we have only regular artifacts)
project compiles (we have only regular artifacts) -> project fails (we have both the regular artifacts (for running), and best effort artifacts, having both on the classpath did not cause any significant problems in my tests, but that can obviously be changed)
project fails compilation with a successful Best Effort compilation (we only have best effort artifacts) -> project fails compilation with an unsuccessful Best Effort compilation (we only have the previous best effort artifacts)

Some unnecessary options are replaced from the bloop ones for the presentation compiler
Previously, any project could be run from metals if semanticdb was able to be generated. This caused a somewhat weird UX for best effort. Now the build directory is explicitly checked for the existence of non META-INF files.
Previously, in metals doctor, compilation would be set as correct if a compilation did not produce any errors. However, with best effort compilation, the compiler may not find any additional errors, beyond the ones from its dependencies (meaning that a project that cannot actually be compiled, could show up as one that was compiling). A note about that was added in the doctor.

Integration tests were also added (probably too few for now).

metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scala

jchyb · 2024-07-02T12:05:15Z

I updated the bloop version (I did not realize before that bloop is published after every merge, that's so cool) and added some comments in a few places.
One thing that I should mention somewhere, is that while the best effort options in the compiler and bloop are able to work even with a file broken on a parser level, in metals I believe that scalameta is used for parsing and indexing, which is used in go-to-ref even before semanticdb is considered, so that does not work yet for those files (but presentation compiler does, so we do get completions etc.). I saw that there is a PR to use presentation compiler for go-to-ref in the future, so I did not try to fix anything here related to that scalameta parser usage

tgodzik · 2024-07-04T14:48:59Z

We can try on the next version 1.5.18-47-e7577874-SNAPSHOT

jchyb · 2024-07-04T16:11:40Z

I still did not manage to fix the MillServerCodeLensSuite test, but we might as well see how the rest are doing

tgodzik · 2024-07-04T16:32:54Z

Curiously, MillServerCodeLensSuite doesn't use Bloop so everything should work the same.

jchyb · 2024-07-04T16:36:04Z

Yes, those are broken by this PR, by the changes I made to checking if there are any artifacts in classpath before allowing to use code lens (working on this now). I believe that everything caused by the bloop changes was fixed

tgodzik · 2024-07-05T08:53:05Z

Looks like Mill issue might be unrelated. Somehow it started failing on main also 🤔

tgodzik

Thanks for working on this! I think we are almost finished. My plan for now is:

Release bloop with all the changes (it includes some other improvements)
Release metals
Merge this change so that we can have a period of testing.

tgodzik · 2024-07-08T15:11:42Z

metals/src/main/scala/scala/meta/internal/metals/Compilers.scala

-    if (isSuccessful) {
+    if (isSuccessful || isBestEffortCompilation) {
+      // // For best effort even if we get errors, we can generate new betasty files
+      // if (!isSuccessful && !isBestEffortCompilation) {


Should we remove the commented code?

tgodzik · 2024-07-08T15:21:50Z

tests/slow/src/test/scala/tests/best-effort/BestEffortCompilationSuite.scala

+  val scalaVersion = "3.5.0-RC1"
+
+  // implemented in bloop
+  test("best-effort-error-diagnostics") {


I wonder if we could reuse some of the tests in CompilersLspSuite for outline? We would need to make it into a base suite and extends here

Done, I did have to have them test different diagnostics for scala 3 and I also had to add saving after changes, before testing completions (but for scala-2s outline they should work identically). I could not make one of the test work (not sure why), so I moved it out of the base suite to an outline only one.

tgodzik · 2024-07-11T14:08:05Z

I already updated Bloop to 1.6.0, so you can rebase and we can merge after release

tgodzik · 2024-07-12T16:41:04Z

metals/src/main/scala/scala/meta/internal/metals/codelenses/RunTestCodeLens.scala

+   * but semanticDB and betasty (and thus the META_INF directory) may still be produced.
+   * We want to avoid creating run/test/debug code lens in those situations.
+   */
+  private def isNonMetaFileInClasspath(


Could we instead just check if the current build target is compiling? diagnostics.hasCompilationErrors(buildId) should be faster than looking up filesystem

That's true, but there is a case where a project may not produce any errors/diagnostics, but may be compiled based on betasty produced by a failing dependency. In that case we will still not get any artifacts there (outside of betasty/semanticdb), and trying to run will produce classpath errors.

Though now that I think about it, we could check diagnostics of every dependency of a project to guess if a project was compiled using betasty. I'll try doing that

After reviewing my changes just now, I realized that I do the same thing in doctor, not sure why I didn't think to do this here too until now 😅

tgodzik · 2024-07-12T16:45:09Z

tests/unit/src/test/scala/tests/OutlineLspSuite.scala

+  override val saveAfterChanges: Boolean = false
+  override val scala3Diagnostics = false
+
+  test("imports-for-non-compiling") {


Could we open an issue to look into it later?

Done (hopefully ok): #6593

tgodzik · 2024-07-12T16:46:04Z

tests/unit/src/test/scala/tests/best-effort/BestEffortCompilationSuite.scala

+    } yield ()
+  }
+
+  // Here we test wheater completion symbols persist after removing


Suggested change

// Here we test wheater completion symbols persist after removing

// Here we test whether completion symbols persist after removing

tgodzik · 2024-07-16T09:09:15Z

Looks like the tests are failing now, but this is expected since the test checks if the code lenses are kept when there are errors and maybe they shouldn't? At least not when the code is still not saved. I think we did it to avoid the code lenses popping in and out of existence thus moving the lines up and down.

Maybe it's enough if it doesn't do that onChange ? Since after saving with errors we would running some unknown version.

Let's change keep-after-error to do onChange for the error and if you could run scalafix via sbt scalafixAll that would be awesome.

This PR is meant to add support for Best Effort compilation in metals, with a few additional considerations: * Some unnecessary options are replaced from bloop and set for PC * Previously, any project could be run from metals if semanticdb was able to be generated. This caused a somewhat weird UX for best effort. Now the build directory is explicitly checked for the existence of non META-INF files. * Previously, in metals doctor, compilation would be set as correct if a compilation did not produce any errors. However, with best effort compilation, the compiler may not find any additional errors, beyond the ones from its dependencies (meaning that a project that cannot actually be compiled, could show up as on that was compiling). A note about that was added in the doctor.

Now we check diagnostics of all transitive dependencies for errors, instead of checking the existence of non meta-inf files. Also the keep-after-error test now does not check for remaining code lens after saving.

tgodzik

LGTM!

jchyb · 2024-07-19T09:16:45Z

Thank you so much for overseeing this!

kpodsiad · 2024-07-26T07:06:20Z

@jchyb @tgodzik I see PR for Bloop but I don't see sbt being mentioned anywhere. Does it mean that best effort compilation is only supported through Bloop?

tgodzik · 2024-07-26T08:23:45Z

Currently yes, it is a bit more complicated to set up than just adding a flag unfortunately

jchyb mentioned this pull request May 12, 2023

Support Scala 3's Best Effort compilation scalacenter/bloop#2049

Merged

tgodzik mentioned this pull request May 17, 2023

"Go to implementations" and "Go to references" stopped working #5176

Closed

jchyb force-pushed the best-effort branch from 0a4894f to 42b63e9 Compare July 19, 2023 12:26

jchyb force-pushed the best-effort branch 2 times, most recently from 36844fd to ac5ede7 Compare August 23, 2023 10:47

jchyb force-pushed the best-effort branch from ac5ede7 to 99ea04b Compare May 8, 2024 21:41

jchyb commented May 17, 2024

View reviewed changes

metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scala Show resolved Hide resolved

jchyb force-pushed the best-effort branch from d6a8525 to b3e06a2 Compare May 27, 2024 13:06

jchyb force-pushed the best-effort branch from b3e06a2 to 91506fd Compare July 2, 2024 11:50

jchyb force-pushed the best-effort branch from 91506fd to c7c36a4 Compare July 4, 2024 16:10

jchyb force-pushed the best-effort branch from c7c36a4 to 7608254 Compare July 4, 2024 17:30

jchyb force-pushed the best-effort branch from 7608254 to e8ad2fe Compare July 5, 2024 11:09

tgodzik requested changes Jul 8, 2024

View reviewed changes

jchyb force-pushed the best-effort branch from e8ad2fe to aaee76c Compare July 11, 2024 10:17

jchyb force-pushed the best-effort branch from aaee76c to afac10b Compare July 11, 2024 15:52

tgodzik requested changes Jul 12, 2024

View reviewed changes

jchyb mentioned this pull request Jul 15, 2024

imports-for-non-compiling outline test not working for Scala 3's best effort #6593

Closed

jchyb force-pushed the best-effort branch 2 times, most recently from 8f979b0 to 40dcd41 Compare July 15, 2024 11:06

jchyb requested a review from tgodzik July 15, 2024 11:11

tgodzik marked this pull request as ready for review July 15, 2024 15:33

jchyb force-pushed the best-effort branch from 40dcd41 to 356112b Compare July 18, 2024 11:22

jchyb added 3 commits July 18, 2024 13:23

Setup previous outline tests so they are alos run for best effort

8f01faa

Address review comments

20fd8f2

Now we check diagnostics of all transitive dependencies for errors, instead of checking the existence of non meta-inf files. Also the keep-after-error test now does not check for remaining code lens after saving.

jchyb force-pushed the best-effort branch from 356112b to 20fd8f2 Compare July 18, 2024 11:24

tgodzik approved these changes Jul 18, 2024

View reviewed changes

tgodzik merged commit ab9fd79 into scalameta:main Jul 18, 2024
23 of 24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: Add support for Scala 3's Best Effort compilation #5219

feature: Add support for Scala 3's Best Effort compilation #5219

jchyb commented May 12, 2023 •

edited

Loading

jchyb commented Jul 2, 2024

tgodzik commented Jul 4, 2024

jchyb commented Jul 4, 2024

tgodzik commented Jul 4, 2024

jchyb commented Jul 4, 2024 •

edited

Loading

tgodzik commented Jul 5, 2024

tgodzik left a comment

tgodzik Jul 8, 2024

tgodzik Jul 8, 2024

jchyb Jul 11, 2024

tgodzik commented Jul 11, 2024

tgodzik Jul 12, 2024

jchyb Jul 15, 2024

jchyb Jul 15, 2024

tgodzik Jul 12, 2024

jchyb Jul 15, 2024

tgodzik Jul 12, 2024

tgodzik commented Jul 16, 2024

tgodzik left a comment

jchyb commented Jul 19, 2024

kpodsiad commented Jul 26, 2024

tgodzik commented Jul 26, 2024

	// Here we test wheater completion symbols persist after removing
	// Here we test whether completion symbols persist after removing

feature: Add support for Scala 3's Best Effort compilation #5219

feature: Add support for Scala 3's Best Effort compilation #5219

Conversation

jchyb commented May 12, 2023 • edited Loading

Related PRs

Explanation

Before:

As of this PR:

Contents

jchyb commented Jul 2, 2024

tgodzik commented Jul 4, 2024

jchyb commented Jul 4, 2024

tgodzik commented Jul 4, 2024

jchyb commented Jul 4, 2024 • edited Loading

tgodzik commented Jul 5, 2024

tgodzik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgodzik commented Jul 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgodzik commented Jul 16, 2024

tgodzik left a comment

Choose a reason for hiding this comment

jchyb commented Jul 19, 2024

kpodsiad commented Jul 26, 2024

tgodzik commented Jul 26, 2024

jchyb commented May 12, 2023 •

edited

Loading

jchyb commented Jul 4, 2024 •

edited

Loading