[fix] clean up previous diagnostics | #BAZEL-376 Done

[tanishiking]

According to the BSP specification build-server-protocol/build-server-protocol#484 BSP servers must publish build/publishDiagnostics with an empty array of of diagnostics to clean up previous diagnostics.
However, bazel-bsp didn't publish build/publishDiagnostics if build completed without errors or warnings, and as a result, BSP clients such as scalameta/metals couldn't clear previous diagnostics. due to missing notifications.

With this commit, bazel-bsp will publish build/publishDiagnostics with with an empty array of diagnostics if the build was successful.

https://youtrack.jetbrains.com/issue/BAZEL-376

[abrams27]

if it's not suuuuuper urgent, i'd postpone this PR / close it - at the moment we are working on a complete migration to BEP and prob we will address this issue as well

[tanishiking]

Sure, I'm fine with postponing this PR as it's not super urgent 👍 (just out of curiosity, what "complete migration to BEP" entails? It seems like most BSP notifications are already translated from BEP like abuild/publishDiagnostics).

However, I'd like to bring to your attention that there is a number of users awaiting the Bazel integration, and this feature is crucial for enabling that support. While we understand that the migration to BEP might address this issue, it would be greatly appreciated if the team could prioritize this feature in the upcoming efforts.

Anyway, thank you for your work / review on bazel-bsp :) This paved the way for Bazel integration from Metals.

[abrams27]

It seems like most BSP notifications are already translated from BEP like abuild/publishDiagnostics

that's actually the only notification xd

in general i believe that we will change the way how we process events, so prob we will change the way how this feature should be implemented;

on top of that i believe that we actually dont need a global state of it (BspCompileState), since each bazel call is self-contained and we can just return the empty array if a specific build was successful

[tanishiking]

Got it, thanks! Do you have a rough idea of how long it takes to refactor on BEP? If it takes a long time, we may want to implement a workaround on the BSP client (scalameta/metals) side to clear up former diagnostics. (e.g., clear up previous diagnostics on build/taskFinish with errors=0) 👍

on top of that i believe that we actually dont need a global state of it (BspCompileState), since each bazel call is self-contained and we can just return the empty array if a specific build was successful

I introduced the state (to manage which files had issues in previous builds) to reduce the number of notifications sent from the BSP server to the client.
We could publish build/publishDiagnostics for all TextDocuments (files) under a buildTarget (Bazel target) every time a build finishes successfully.

However, the BSP client (and possibly the server too) would experience a significant CPU load when processing a large number of build/publishDiagnostics with empty diagnostics. (See sbt/sbt#6847) and this CPU load could be particularly impactful for large projects that use Bazel for build.

I'm not sure if BEP has information on whether a target previously had problems, but if it doesn't, some form of build state might be needed to reduce the communication load between the BSP server and client.

[abrams27]

ohh right i completely forgot that there is no "wildcard" build/publishDiagnostics call and we need to call it per texDocument

I'm not sure if BEP has information on whether a target previously had problems

prob it doesnt : /

[tanishiking]

Maybe "wildcard" would be good to have in BSP 😄

[abrams27]

Do you have a rough idea of how long it takes to refactor on BEP?

we should deliver it next week, so i'll wait with the review here

[abrams27]

unfortunately the bep change (ae52b8f) didnt make it easier : (

some nitpicking from my side

[abrams27]

sorry for such long waiting time, last few weeks were veeery busy - looks nice just 2 questions / nitpicking

[abrams27]

lil thing and we can merge i think

[abrams27]

thanks a lot and sorry for the delay