Make BSP import more robust

[nafg]

(Previously posted at https://discord.com/channels/632150470000902164/940067748103487558/1207105622366228480)

With a project imported from Mill into IntelliJ via BSP, I regularly have scenarios like the following:

1. I update dependencies and sync (reimport the project)
2. It fails with something cryptic like this:
3. Now the entire project is red, so the IDE is very unheplful
4. I check mill-bsp.stderr and see something like

buildTargetScalacOptions caught exception: java.lang.Exception: Failure during task evaluation: app_common.test.compile Compilation failed
java.lang.Exception: Failure during task evaluation: app_common.test.compile Compilation failed
    at mill.eval.Evaluator.$anonfun$evalOrThrow$default$1$1(Evaluator.scala:42)
    at mill.eval.EvaluatorImpl$EvalOrThrow.apply(EvaluatorImpl.scala:67)

In short, unless there are no compile errors anywhere when I sync the build it breaks the whole IDE with no easy to find reason

This does not happen with SBT BSP import.

Suggestions:
A. If importing fails, it shouldn't make things worse in the IDE than before
B. A module failing to compile is an an exception, it should be a case the BSP code thinks about
C. Ideally it would be able to give IDE whatever information it can
D. Errors shouldn't be hidden somewhere inside a ginormous log file, they should be returned to the IDE

[nafg]

Another thing is that often after reimporting -Xsource:3 syntax like * imports are red until I do Repair IDE and get past Reopen Project (I confess, I'm not sure if I tried going straight to manually closing and reopening the project).

[lihaoyi]

I've hit this too. We should not need to compile everything to generate the IDE project

I vaguely remember fixing a bug like this before, but maybe I'm mistaken or it regressed

[lihaoyi]

In my case I suspect it's because I'm doing unusual things, e.g. compiling compiler plugins as part of the build, or depending on output .jar targets as part of unmanagedClasspath. Doing that, compiling stuff when generating the IDE project is unavoidable

@nafg is there any chance you are doing something similar?

[lefou]

Would be good to have a reproducer for that.

[lihaoyi]

Looking at Nagtoli's screenshots, it seens that the compile is failing when computing scalacOptions, which does hint at a compiler plugin being compiled and passed as a -Xplugin-classpath flag. That would be the same root cause as the case I hit

The other quality-of-life improvements we should still do though, or get the IntelliJ folks to do. We probably shouldn't crash the entire project's IDE setup just because one module failed to compile, and when things do crash we should show the error nicely rather than having to dig through a huge log file

[lihaoyi]

A simple repro would be a two module project with one module's unmanagedClasspath being set to the second module's compile() output. It's a bit contrived, but it should demonstrate the IDE import crashing if the second module fails to compile

[lefou]

I guess, it's in the nature of the BSP protocol, that we need to fail in cases we hit some failure along the way. The granularity is determined the by request, and the return type is all we can use to communicate result or failure. In BSP, besides compilation and some other requests, where compile result and failure is part of the result or notification, in scalacOptions there is no failure anticipated. Without having analyzed what these other tools do, I'd assume they either don't hit an error or are not using the protocol specified error channels. We simply don't have control over the granularity the client requests some data. Typically, clients send a single request for all build targets (= Mill modules).

What options do we have, if we hit an error in scalacOptions? Is just returning some wrong result (like an empty Seq) combined with some loosely coupled log error message sent to the client, ok? I think, the BSP spec says nothing about it.

It may be a general protocol design flaw. Currently, you can either send many small request, which might impose some overhead, or you send large requests, which take long and if they fail provide nothing. Instead, we should consider either more granular error results, e.g. per build target, or stream the results as they come, as we can do partially with compilation messages.

[nafg]

Somehow I don't have these issues with sbt or bleep Sent with Shortwave <https://www.shortwave.com?utm_medium=email&utm_content=signature&utm_source=bmFmdG9saWd1Z0BnbWFpbC5jb20=>

…

On Wed Feb 14, 2024, 02:49 PM GMT, Tobias Roeser ***@***.***> wrote: I guess, it's in the nature of the BSP protocol, that we need to fail in cases we hit some failure along the way. The granularity is determined the by request, and the return type is all we can use to communicate result or failure. In BSP, besides compilation and some other requests, where compile result and failure is part of the result or notification, in scalacOptions there is no failure anticipated. Without having analyzed what these other tools do, I'd assume they either don't hit an error or are not using the protocol specified error channels. We simply don't have control over the granularity the client requests some data. Typically, clients send a single request for all build targets (= Mill modules). What options do we have, if we hit an error in scalacOptions? Is just returning some wrong result (like an empty Seq) combined with some loosely coupled log error message sent to the client, ok? I think, the BSP spec says nothing about it. It may be a general protocol design flaw. Currently, you can either send many small request, which might impose some overhead, or you send large requests, which take long and if they fail provide nothing. Instead, we should consider either more granular error results, e.g. per build target, or stream the results as they come, as we can do partially with compilation messages. — Reply to this email directly, view it on GitHub <#3011 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAYAUC7FLXXGVBCK6Z3GI3YTTFG5AVCNFSM6AAAAABDHMG3XOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBTHE2TOMRYGM>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[lefou]

buildTargetScalacOptions caught exception: java.lang.Exception: Failure during task evaluation: app_common.test.compile Compilation failed
java.lang.Exception: Failure during task evaluation: app_common.test.compile Compilation failed
    at mill.eval.Evaluator.$anonfun$evalOrThrow$default$1$1(Evaluator.scala:42)
    at mill.eval.EvaluatorImpl$EvalOrThrow.apply(EvaluatorImpl.scala:67)

@nafg Can you run mill app_common.test.compile? Can you see, why it failed? If it is not failing, can you see in the logs why/how it failed when ran as part of BSP/install?

Can you trace back, why some scalaOptions depend on that target?

[nafg]

It was an ordinary compilation error Sent with Shortwave <https://www.shortwave.com?utm_medium=email&utm_content=signature&utm_source=bmFmdG9saWd1Z0BnbWFpbC5jb20=>

…

On Wed Feb 14, 2024, 05:36 PM GMT, Tobias Roeser ***@***.***> wrote: > buildTargetScalacOptions caught exception: java.lang.Exception: Failure during task evaluation: app_common.test.compile Compilation failed java.lang.Exception: Failure during task evaluation: app_common.test.compile Compilation failed at mill.eval.Evaluator.$anonfun$evalOrThrow$default$1$1(Evaluator.scala:42) at mill.eval.EvaluatorImpl$EvalOrThrow.apply(EvaluatorImpl.scala:67) @nafg <https://github.com/nafg> Can you run mill app_common.test.compile? Can you see, why it failed? If it is not failing, can you see in the logs why/how it failed when ran as part of BSP/install? — Reply to this email directly, view it on GitHub <#3011 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAYAUAZKIAB2IMISYHZTMLYTTYYFAVCNFSM6AAAAABDHMG3XOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBUGI4TMMRZGI>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[lefou]

Another thing is that often after reimporting -Xsource:3 syntax like * imports are red until I do Repair IDE and get past Reopen Project (I confess, I'm not sure if I tried going straight to manually closing and reopening the project).

@nafg This is unrelated to the initial issue. If you think this is an issue in Mill, please open a new issue.

[lihaoyi]

I think for scalacOptions in particular, falling back to an empty Seq() if evaluation fails is very reasonable. That would catch the most common case this appears - of compiling compiler plugins as part of the build - and would be no worse than the status quo of "nothing works, at all"

[nafg]

In my case I suspect it's because I'm doing unusual things, e.g. compiling compiler plugins as part of the build, or depending on output .jar targets as part of unmanagedClasspath. Doing that, compiling stuff when generating the IDE project is unavoidable

@nafg is there any chance you are doing something similar?

I don't think so. I do customize localClasspath somewhat conditionally but still:

❯ mill inspect app_common.localClasspath
[1/1] inspect 
app_common.localClasspath(build.sc:77)
    The *output* classfiles/resources from this module, used for execution,
    excluding upstream modules and third-party dependencies

Inputs:
    app_common.localCompileClasspath
    app_common.resources
    app_common.compile
    app_common.webapp
    app_common.generatedBuildInfo

❯ mill inspect app_common.generatedBuildInfo 
[1/1] inspect 
app_common.generatedBuildInfo(build.sc:38)

Inputs:

It's based on

  override def localClasspath =
    if (scalaJsAsAsset)
      T(super.localClasspath() ++ Seq(generatedBuildInfo(), app_js.getScalaJs()))
    else
      T(super.localClasspath() :+ generatedBuildInfo())

where

  def scalaJsAsAsset = insideCI

where

def insideCI = sys.env.get("CI").exists(Set("", "true", "1"))

Also, scalacOptions has a dependency on scalaVersion.

[nafg]

Another thing is that often after reimporting -Xsource:3 syntax like * imports are red until I do Repair IDE and get past Reopen Project (I confess, I'm not sure if I tried going straight to manually closing and reopening the project).

@nafg This is unrelated to the initial issue. If you think this is an issue in Mill, please open a new issue.

What do I know, but to me "errors in importing results in everything red" and "successful importing results in spurious red code" (when with sbt importing can fail and doesn't have either issue) seems like part of the same problem (which are unrelated to "why are compile errors causing import to fail" which is not the main issue.)

To me the main issue I'm reporting is that importing should result in a clean IDE state: like with SBT BSP import, failures should be displayed in the IDE not hidden in some log with the IDE broken (and certainly it shouldn't be broken without errors).

[lefou]

Opened discussion: build-server-protocol/build-server-protocol#655

[adpi2]

What options do we have, if we hit an error in scalacOptions? Is just returning some wrong result (like an empty Seq) combined with some loosely coupled log error message sent to the client, ok? I think, the BSP spec says nothing about it.

We had the same issue in sbt and I solved it (sbt/sbt#6609) by sending back a response with missing build targets. For instance if the clients want the scalacOptions of target a, b and c, and if there is a failure for b, sbt will send back a response with the scalacOptions of a and c. This is not implemented only for scalacOptions but for all multi-target requests.

I think I recall that Bloop has the same behavior and that's why I decided to implement it this way in sbt. On the client side Metals should handle such incomplete response quite well, for IntelliJ I am not sure.

[lefou]

@adpi2 Thank you for this response. This sounds like a nice generic approach. I think, I'm going to implement that in Mill as well. The only downside is, that the BSP client has no idea why the response was incomplete. We could push some informal log message to the client, though.

[adpi2]

The only downside is, that the BSP client has no idea why the response was incomplete.

Maybe it is time to implement build-server-protocol/build-server-protocol#204 then.

[lefou]

I've now gone with the approach proposed by @adpi2. If I encounter failure for some requested target identifiers, I just does not include them in the otherwise successful result. In parallel, I send a log message with ERROR level to the BSP client.

@nafg If you could test PR #3022, and report back, whether that matches your expectations, that would be great.

[nafg]

How do I test it?

[lefou]

How do I test it?

Once, our CI has some free cycles, I'll start a snapshot build. If you're eager, you can also compile the PR or the main branch with mill installLocalCache and use the version (mill show main.publishVersion) in your .mill-version.

[nafg]

Just tested it and it seems better already!