Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bugfix: Don't republish old errors on successful compilation #2427

Merged
merged 1 commit into from
Sep 17, 2024
+81 −3
Merged

bugfix: Don't republish old errors on successful compilation #2427

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 22 additions & 3 deletions frontend/src/main/scala/bloop/reporter/BspProjectReporter.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,14 +301,31 @@ final class BspProjectReporter(
): Unit = {
val problemsInPreviousAnalysisPerFile = Reporter.groupProblemsByFile(previousSuccessfulProblems)

/**
* We need to report all problems if a BSP client just connected to the server and never compiled.
* `reportAllPreviousProblems` will be set to true in that case.
*
* However, if previously we had an error, which was reverted and got back to the state before the
* error, we will get a successfull noop compilation, which will not produce any problems.
*
* In this case we don't want to republish everything again, since diagnostics will contain the
* error which was reverted.
Comment on lines +311 to +312
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure to understand this. If the previous was a noop compilation, why would diagnostics contain any error? In other words, I think a noop compilation should clear the diagnostics that were stored anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also have a noop compilation that was an actual error. If nothing changed we would still have the same exact errors, so no need to recompile.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay but if it is a success, we should first clear problemsPerFiles so that it does not contain any error anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would make sense, but then when we have reportAllPreviousProblems set (this is set when first compilation for a target and client is run) then we would not report old warnings, since we would clean it. We can try to clean just the errors out, but we don't know if some warnings weren't generate along those errors.

So in essence, we don't know what exact problems were generated in the compilation before and those are the ones we should clear.

It's much easier to just special case this situation and just report everything in the previous successful compilation. And while I agree with you that something is wrong here, I can't figure out a more sensible solution, all alternative paths seem more complicated and carry their own drawbacks. This might be due to the fact that noop compilation is a bit of a trick, which is not really something that a lot of other tools using zinc do.

*
* Connected to https://github.com/VirtusLab/scala-cli/issues/2226 where CLI always connected anew.
*/
def shouldPublishAllProblems =
reportAllPreviousProblems && !wasPreviousSuccessful && code != bsp.StatusCode.Ok
adpi2 marked this conversation as resolved.
Show resolved Hide resolved
def mockNoOpCompileEventsAndEnd: Option[CompilationEvent.EndCompilation] = {
recentlyReportProblemsPerFile.foreach {
case (sourceFile, problemsPerFile) if reportAllPreviousProblems =>
case (sourceFile, problemsPerFile) if shouldPublishAllProblems =>
reportAllProblems(sourceFile, problemsPerFile)
case (sourceFile, problemsPerFile) =>
problemsInPreviousAnalysisPerFile.get(sourceFile) match {
case Some(problemsInPreviousAnalysis) =>
if (problemsInPreviousAnalysis.map(_.problem) == problemsPerFile.map(_.problem)) {
if (
problemsInPreviousAnalysis
.map(_.problem) == problemsPerFile.map(_.problem) && !reportAllPreviousProblems
) {
// If problems are the same, diagnostics in the editor are up-to-date, do nothing
()
} else {
Expand All @@ -322,8 +339,10 @@ final class BspProjectReporter(
}
}

case None =>
// there is nothing to clear if we need to rereport all problems
case None if !reportAllPreviousProblems =>
logger.noDiagnostic(CompilationEvent.NoDiagnostic(project.bspUri, sourceFile))
case _ =>
}
}
if (wasPreviousSuccessful) None
Expand Down
59 changes: 59 additions & 0 deletions frontend/src/test/scala/bloop/bsp/BspCompileSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -748,6 +748,65 @@ class BspCompileSpec(
}
}

test("no-op compile and not publish diagnostics from a previous failed CLI compilation") {
TestUtil.withinWorkspace { workspace =>
object Sources {
val `App.scala` =
"""/main/scala/App.scala
|object App {
| val a: String = ""
|}
""".stripMargin
}

val bspLogger = new RecordingLogger(ansiCodesSupported = false)
val `A` = TestProject(workspace, "a", List(Sources.`App.scala`))
val projects = List(`A`)

// compile successfully in a separate bsp connection and then break
loadBspState(workspace, projects, bspLogger) { state =>
val compiledState = state.compile(`A`)
assertExitStatus(compiledState, ExitStatus.Ok)
assertValidCompilationState(compiledState, projects)

writeFile(
`A`.srcFor("/main/scala/App.scala"),
"""|object App {
| val a: String = 1
|}
""".stripMargin
)
val secondCliCompiledState = compiledState.compile(`A`)
assertExitStatus(secondCliCompiledState, ExitStatus.CompilationError)
assertValidCompilationState(secondCliCompiledState, projects)
}

// connecting and compiling should not get any previous and no longer valid diagnostics
loadBspState(workspace, projects, bspLogger) { state =>
// Remove the error and get back to the previous state
writeFile(
`A`.srcFor("/main/scala/App.scala"),
Sources.`App.scala`
)
val compiledState = state.compile(`A`)
assertExitStatus(compiledState, ExitStatus.Ok)
assertValidCompilationState(compiledState, projects)

assertNoDiff(
compiledState.lastDiagnostics(`A`),
"""|#1: task start 1
| -> Msg: Start no-op compilation for a
| -> Data kind: compile-task
|#1: task finish 1
| -> errors 0, warnings 0
| -> Msg: Compiled 'a'
| -> Data kind: compile-report
|""".stripMargin
)
}
}
}

test("compile incrementally and publish warnings from a previous CLI compilation") {
TestUtil.withinWorkspace { workspace =>
object Sources {
Expand Down
Loading