Support diagnostics in sbt

[tgodzik]

Is your feature request related to a problem? Please describe.
Thanks to scalameta/metals#1865 we now have most of the needed features for supporting sbt files.

One of the missing features is diagnostic support - Bloop doesn't currently compile sbt files, this will need to be somehow done via sbt, but is certainly non trivial

Describe the solution you'd like
A way to get full diagnostics about sbt files when saving.

Describe alternatives you've considered
Running sbt manually.

Search terms:
sbt diagnostics

[adpi2]

Is this feature only related to sbt server or also Bloop?

For sbt server this could be helpful when implemented: sbt/sbt#5953

[ckipp01]

Is this feature only related to sbt server or also Bloop?

This would be for both the sbt Bloop integration and the sbt server integration.

[tgodzik]

It's probably easier to do for sbt. I am not exactly sure how we would go about implementing it in Bloop 🤔 Maybe we could use diagnostics from the presentation compiler?

[retronym]

SBT support this is close to landing: sbt/sbt#6553

[ckipp01]

SBT support this is close to landing: sbt/sbt#6553

This is really cool @retronym! This will be huge for Metals users that want to use sbt as a BSP server with Metals. I'll continue to play around with this today!

Just to clarify though the main reason for this issue is to support diagnostics in your build files. I'm correct in saying that this still won't work with the added support in your pr correct? Since if a user makes a mistake in the build file, prompts a workspace/reload, the reload will still fail without any diagnostics being forwarded to the user about what is going wrong in the build file. That's the main issue we are trying to address here.

To illustrate.

Notice the missing l in libraryDependencies.

[Trace - 10:30:28 AM] Received response 'workspace/reload - (14)' in 2798ms
Result: null
Error: {
  "code": -32603,
  "message": "reload failed"
}

So in order for this feature request to be closed, we'd basically want a way to notify the user that ibraryDependencies isn't recognized, and have a diagnostic on that line.

[tgodzik]

Shouldn't this give as diagnostics when you make the typo? We could not allow running reload if there are any diagnostics present.

[ckipp01]

Shouldn't this give as diagnostics when you make the typo? We could not allow running reload if there are any diagnostics present.

Well that's sort of the thing, how do you know it's a typo and not an actual key? The only way would be able to save it / compile it. By doing so Metals will detect you changed your build definition and prompt the import/reload. The reload will then fail since that's not a valid key, and then you're sort of just stuck. It's the same thing with Bloop.

In an ideal world it'd be great to "recover" and basically go back to the last valid state, and then report the actual issue in the build definition.

[tgodzik]

By doing so Metals will detect you changed your build definition and prompt the import/reload.

We could delay the prompt until we get the diagnostics and only show it if they are empty?

[ckipp01]

By doing so Metals will detect you changed your build definition and prompt the import/reload.

We could delay the prompt until we get the diagnostics and only show it if they are empty?

Yea possibly, that might work. It doesn't look like diagnostics are being returned though in the situation. For example just testing it out, the compile goes, and returns with an error, but it looks like diagnostics aren't published for the build target. (Also not sure what's going on with the empty.max thing).

[Trace - 11:07:46 AM] Sending request 'buildTarget/compile - (20)'
Params: {
  "targets": [
    {
      "uri": "file:/Users/ckipp/Documents/scala-workspace/sanity/#sbt-build"
    }
  ]
}


[Trace - 11:07:46 AM] Received notification 'build/logMessage'
Params: {
  "type": 4,
  "message": "Processing buildTarget/compile"
}


[Trace - 11:07:46 AM] Received notification 'build/logMessage'
Params: {
  "type": 1,
  "message": "java.lang.UnsupportedOperationException: empty.max\n\tat scala.collection.TraversableOnce.max(TraversableOnce.scala:275)\n\tat scala.collection.TraversableOnce.max$(TraversableOnce.scala:273)\n\tat scala.collection.AbstractTraversable.max(Traversable.scala:108)\n\tat sbt.internal.server.BuildServerProtocol$.$anonfun$globalSettings$40(BuildServerProtocol.scala:164)\n\tat sbt.internal.server.BuildServerProtocol$.$anonfun$globalSettings$40$adapted(BuildServerProtocol.scala:163)\n\tat scala.Function1.$anonfun$compose$1(Function1.scala:49)\n\tat sbt.internal.util.$tilde$greater.$anonfun$$u2219$1(TypeFunctions.scala:62)\n\tat sbt.std.Transform$$anon$4.work(Transform.scala:68)\n\tat sbt.Execute.$anonfun$submit$2(Execute.scala:282)\n\tat sbt.internal.util.ErrorHandling$.wideConvert(ErrorHandling.scala:23)\n\tat sbt.Execute.work(Execute.scala:291)\n\tat sbt.Execute.$anonfun$submit$1(Execute.scala:282)\n\tat sbt.ConcurrentRestrictions$$anon$4.$anonfun$submitValid$1(ConcurrentRestrictions.scala:265)\n\tat sbt.CompletionService$$anon$2.call(CompletionService.scala:64)\n\tat java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)\n\tat java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)\n\tat java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)\n\tat java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)\n\tat java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)\n\tat java.base/java.lang.Thread.run(Thread.java:829)"
}


[Trace - 11:07:46 AM] Received notification 'build/logMessage'
Params: {
  "type": 1,
  "message": "(Global / bspBuildTargetCompile) java.lang.UnsupportedOperationException: empty.max"
}


[Trace - 11:07:46 AM] Received notification 'build/logMessage'
Params: {
  "type": 4,
  "message": "Processing buildTarget/compile"
}


[Trace - 11:07:46 AM] Received response 'buildTarget/compile - (20)' in 27ms
Result: null
Error: {
  "code": -33000,
  "message": ""
}

One other thing we'll need to change is to ensure now that the metals.sbt that we create is also done for sbt BSP so that we have the project/project/metals.sbt in order for navigation to work. Since right now that's only done for Bloop.

[ckipp01]

So when trying to start up sbt in this scenario you get:

[info] loading project definition from /Users/ckipp/Documents/scala-workspace/sanity/project
/Users/ckipp/Documents/scala-workspace/sanity/build.sbt:19: error: not found: value ibraryDependencies
    ibraryDependencies ++= Seq(

That's what we'd want right? I don't know if that is fully implemented on the sbt side to forward these as diagnostics via BSP. @adpi2 or @retronym do you know if these are forwarded as diagnostics in this scenario?

[retronym]

I have only implemented the server logic to export the classpath and automatic imports of the build. IntelliJ uses that to let it’s presentation compiler perform error highlighting and autocomplete I hadn’t considered the use case of just feeding the error/warnings back as IntelliJ doesn’t need that

[ckipp01]

BuildTargetCapabilities(canCompile = false, canTest = false, canRun = false),

Ah just seeing this part now. Is it possible for this to be set to compile?

I have only implemented the server logic to export the classpath and
automatic imports of the build. IntelliJ uses that to let it’s presentation
compiler perform error highlighting and autocomplete

Gotcha, that makes sense. For us on the Metals side, this would be super helpful if these were returned via BSP.

[dos65]

It seems that the only option for getting diagnostic would be to compile sbt-file on Metals side using Presentation Compiler.

I have some idea for the case when the build has errors and the project can't be imported.
For bloop, we can try to recover failed sbt bloopInstall run in the workspace directory by running the same command in $workspace/project. That would allow us to obtains meta-build connection and obtain sbt-metadata for editing *.sbt file.

[ckipp01]

Ok so one other thing I'm a bit confused about is how this actually works for navigation purposes.

Metals will warn you in the doctor that you need to do a build import no matter what in your -build build target since when we send in the buildTarget/scalacOptions request with the build being one of the build targets, there is no scalacOptions returned meaning that metals thinks there is no semanticdb compiler plugin included. Looking at the pr, this is just set to an empty vector on the sbt side:

 ScalacOptionsItem(
            build._1,
            Vector(), <-- this one
            classpath,
            new File(build._2.localBase, "project/target").toURI
          )

That's fine if there still is actually semanticdb files that we can locate and use, but I'm unsure of where these are as they seem to be empty in project/target/META_INF/semanticdb. Am I missing something?

[dos65]

@ckipp01 We don't actually need enabled semanticDb for sbt files. Everything is done using PC and InteractiveSemanticdbs.

[ckipp01]

@ckipp01 We don't actually need enabled semanticDb for sbt files. Everything is done using PC and InteractiveSemanticdbs.

Huh, I never realized this. I'll need to dig in more.

[retronym]

Looking at the pr, this is just set to an empty vector on the sbt side:

I've added a commit to pass the scalacOptions of the build into ScalacOptionsItem. But this may well be empty. The important part is the classpath.

[retronym]

One thing to be aware of is that the build itself used Scala 2.12.x (.14 at the moment, I think). So your need to use that version of the Scala presentation compiler, too.

[tgodzik]

We do use the presentation compiler version that is defined in the build target, so this should work all in all. Anyway, diagnostics will need to be handled separately, but that's to @retronym we will be able to provide completions, references etc. in sbt files 🎉

[adpi2]

@ckipp01 said

Just to clarify though the main reason for this issue is to support diagnostics in your build files.

I thought this was already working but it only partially does. Here is an example where it works:

// build.sbt
lazy val sanity = project
  .settings(Build.commonSettings)

// project/Build.scala
import sbt.Keys._
import sbt._

object Build {
  val commonSettings = Seq(
    ibraryDependencies ++= Seq(
      "com.lihaoyi" %% "pprint" % "0.6.6"
    )
  )
}

Using sbt as the build server. When I hit "Import changes", I receive the correct diagnostic:

In the BSP traces:

[Trace - 10:51:56 AM] Received notification 'build/publishDiagnostics'
Params: {
  "textDocument": {
    "uri": "file:///home/piquerez/scalameta/metals-feature-143/project/Build.scala"
  },
  "buildTarget": {
    "uri": "file:/home/piquerez/scalameta/metals-feature-143/project/#metals-feature-143-build/Compile"
  },
  "diagnostics": [
    {
      "range": {
        "start": {
          "line": 5,
          "character": 4
        },
        "end": {
          "line": 5,
          "character": 5
        }
      },
      "severity": 1,
      "source": "sbt",
      "message": "not found: value ibraryDependencies"
    }
  ],
  "reset": true
}

So the fact that it does not work in the build.sbt file is a bug rather than a unimplemented feature. @retronym's PR does not make anything better or worth on this. I am going to work on it.

@tgodzik said

We could delay the prompt until we get the diagnostics and only show it if they are empty?

That would be great! To do so with sbt as the build server we need @retronym's PR as a first step, and then we need the canCompile capability on the sbt build targets. It is not trivial but it should be feasible.

@dos65 said

It seems that the only option for getting diagnostic would be to compile sbt-file on Metals side using Presentation Compiler.

I think it would be better if sbt compiles its own files so that an sbt build target could be treated as any other build target.