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

Jump to bottom

refactor: don't send task start/end for no-op compilations #2274

Merged
merged 1 commit into from
Feb 19, 2024
Merged

refactor: don't send task start/end for no-op compilations #2274

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
39 changes: 10 additions & 29 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 @@ -295,13 +295,7 @@ final class BspProjectReporter(
): Unit = {
val problemsInPreviousAnalysisPerFile = Reporter.groupProblemsByFile(previousSuccessfulProblems)

def mockNoOpCompileEventsAndEnd: CompilationEvent.EndCompilation = {
// When no-op, we keep reporting the start and the end of compilation for consistency
val startMsg = s"Start no-op compilation for ${project.name}"
logger.publishCompilationStart(
CompilationEvent.StartCompilation(project.name, project.bspUri, startMsg, taskId)
)

def recheckProblems = {
recentlyReportProblemsPerFile.foreach {
case (sourceFile, problemsPerFile) if reportAllPreviousProblems =>
reportAllProblems(sourceFile, problemsPerFile)
Expand All @@ -326,30 +320,17 @@ final class BspProjectReporter(
logger.noDiagnostic(CompilationEvent.NoDiagnostic(project.bspUri, sourceFile))
}
}

val liftedProblems = allProblems.toIterator.map(super.liftFatalWarning(_)).toList
CompilationEvent.EndCompilation(
project.name,
project.bspUri,
taskId,
liftedProblems,
code,
isNoOp = true,
isLastCycle = true,
clientClassesDir,
clientAnalysisOut
)
}

endEvent = Some(
if (cycleCount.get == 0) mockNoOpCompileEventsAndEnd
else {
// Great, let's report the pending end incremental cycle as the last one
val inputs =
CycleInputs(true, problemsInPreviousAnalysisPerFile, clientClassesDir, clientAnalysisOut)
processEndPreviousCycle(inputs, Some(code))
}
)
endEvent = if (cycleCount.get == 0) {
recheckProblems
None
} else {
// Great, let's report the pending end incremental cycle as the last one
val inputs =
CycleInputs(true, problemsInPreviousAnalysisPerFile, clientClassesDir, clientAnalysisOut)
Some(processEndPreviousCycle(inputs, Some(code)))
}

// Clear the state of files with problems at the end of compilation
clearedFilesForClient.clear()
Expand Down
41 changes: 17 additions & 24 deletions frontend/src/test/scala/bloop/DeduplicationSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,10 +111,10 @@ object DeduplicationSpec extends bloop.bsp.BspBaseSuite {

assertNoDiff(
firstCompiledState.lastDiagnostics(build.userProject),
"""#1: task start 2
"""#1: task start 1
| -> Msg: Compiling user (2 Scala sources)
| -> Data kind: compile-task
|#1: task finish 2
|#1: task finish 1
| -> errors 0, warnings 0
| -> Msg: Compiled 'user'
| -> Data kind: compile-report
Expand Down Expand Up @@ -153,14 +153,7 @@ object DeduplicationSpec extends bloop.bsp.BspBaseSuite {

assertNoDiff(
firstCompiledState.lastDiagnostics(build.userProject),
"""#2: task start 4
| -> Msg: Start no-op compilation for user
| -> Data kind: compile-task
|#2: task finish 4
| -> errors 0, warnings 0
| -> Msg: Compiled 'user'
| -> Data kind: compile-report
""".stripMargin
"" // expect None here since it's a no-op which turns into ""
)

// Same check as before because no-op should not show any more input
Expand Down Expand Up @@ -233,10 +226,10 @@ object DeduplicationSpec extends bloop.bsp.BspBaseSuite {

assertNoDiff(
firstCompiledState.lastDiagnostics(build.userProject),
"""#1: task start 2
"""#1: task start 1
| -> Msg: Compiling user (2 Scala sources)
| -> Data kind: compile-task
|#1: task finish 2
|#1: task finish 1
| -> errors 0, warnings 0
| -> Msg: Compiled 'user'
| -> Data kind: compile-report
Expand Down Expand Up @@ -326,10 +319,10 @@ object DeduplicationSpec extends bloop.bsp.BspBaseSuite {

assertNoDiff(
secondCompiledState.lastDiagnostics(build.userProject),
"""#2: task start 4
"""#2: task start 2
| -> Msg: Compiling user (1 Scala source)
| -> Data kind: compile-task
|#2: task finish 4
|#2: task finish 2
| -> errors 0, warnings 0
| -> Msg: Compiled 'user'
| -> Data kind: compile-report
Expand Down Expand Up @@ -412,10 +405,10 @@ object DeduplicationSpec extends bloop.bsp.BspBaseSuite {
// First compilation should not report warning
assertNoDiff(
firstCompiledState.lastDiagnostics(build.userProject),
"""#1: task start 2
"""#1: task start 1
| -> Msg: Compiling user (2 Scala sources)
| -> Data kind: compile-task
|#1: task finish 2
|#1: task finish 1
| -> errors 0, warnings 0
| -> Msg: Compiled 'user'
| -> Data kind: compile-report
Expand Down Expand Up @@ -563,13 +556,13 @@ object DeduplicationSpec extends bloop.bsp.BspBaseSuite {
// We reproduce the same streaming side effects during compilation
assertNoDiff(
firstCompiledState.lastDiagnostics(`B`),
"""#1: task start 2
"""#1: task start 1
| -> Msg: Compiling b (1 Scala source)
| -> Data kind: compile-task
|#1: b/src/B.scala
| -> List(Diagnostic(Range(Position(2,28),Position(2,28)),Some(Error),Some(_),Some(_),type mismatch; found : Int required: String,None,None,Some({"actions":[]})))
| -> reset = true
|#1: task finish 2
|#1: task finish 1
| -> errors 1, warnings 0
| -> Msg: Compiled 'b'
| -> Data kind: compile-report
Expand Down Expand Up @@ -644,13 +637,13 @@ object DeduplicationSpec extends bloop.bsp.BspBaseSuite {

assertNoDiff(
secondBspState.lastDiagnostics(`B`),
"""#2: task start 4
"""#2: task start 2
| -> Msg: Compiling b (1 Scala source)
| -> Data kind: compile-task
|#2: b/src/B.scala
| -> List(Diagnostic(Range(Position(2,28),Position(2,28)),Some(Error),Some(_),Some(_),type mismatch; found : Int required: String,None,None,Some({"actions":[]})))
| -> reset = true
|#2: task finish 4
|#2: task finish 2
| -> errors 1, warnings 0
| -> Msg: Compiled 'b'
| -> Data kind: compile-report
Expand Down Expand Up @@ -744,13 +737,13 @@ object DeduplicationSpec extends bloop.bsp.BspBaseSuite {

assertNoDiff(
thirdBspState.lastDiagnostics(`B`),
"""#3: task start 6
"""#3: task start 3
| -> Msg: Compiling b (1 Scala source)
| -> Data kind: compile-task
|#3: b/src/B.scala
| -> List()
| -> reset = true
|#3: task finish 6
|#3: task finish 3
| -> errors 0, warnings 0
| -> Msg: Compiled 'b'
| -> Data kind: compile-report
Expand Down Expand Up @@ -894,10 +887,10 @@ object DeduplicationSpec extends bloop.bsp.BspBaseSuite {
assertNoDiff(
firstCompiledState.lastDiagnostics(`B`),
s"""
|#1: task start 2
|#1: task start 1
| -> Msg: Compiling b (6 Scala sources)
| -> Data kind: compile-task
|#1: task finish 2
|#1: task finish 1
| -> errors 0, warnings 0
| -> Msg: Compiled 'b'
| -> Data kind: compile-report
Expand Down
79 changes: 23 additions & 56 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 @@ -83,13 +83,7 @@ class BspCompileSpec(
assertSameExternalClassesDirs(compiledState, secondCompiledState, projects)
assertNoDiff(
secondCompiledState.lastDiagnostics(`A`),
"""#2: task start 2
| -> Msg: Start no-op compilation for a
| -> Data kind: compile-task
|#2: task finish 2
| -> errors 0, warnings 0
| -> Msg: Compiled 'a'
| -> Data kind: compile-report""".stripMargin
"" // no-op
)
}
}
Expand Down Expand Up @@ -576,17 +570,10 @@ class BspCompileSpec(
assertSameExternalClassesDirs(thirdCompiledState, compiledState, projects)
assertNoDiff(
thirdCompiledState.lastDiagnostics(`A`),
"""#3: task start 3
| -> Msg: Start no-op compilation for a
| -> Data kind: compile-task
|#3: a/src/A.scala
"""#3: a/src/A.scala
| -> List()
| -> reset = true
|#3: task finish 3
| -> errors 0, warnings 0
| -> Msg: Compiled 'a'
| -> Data kind: compile-report
""".stripMargin
""".stripMargin // no-op so it only gives the compile-report
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, compile report is in the task finish, so we only get the compile response, no? But that should be fine as nothing really changes

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh, yea sorry that's my mistake. I was switching up the names of the compile response and compile report. You're correct, it's only the compile response that is now returned.

)

writeFile(`A`.srcFor("/A.scala"), Sources.`A3.scala`)
Expand All @@ -603,7 +590,7 @@ class BspCompileSpec(
assertSameExternalClassesDirs(fourthCompiledState, compiledState, projects)
assertNoDiff(
fourthCompiledState.lastDiagnostics(`A`),
"""#4: task start 4
"""#4: task start 3
Copy link
Contributor

Choose a reason for hiding this comment

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

Any idea why these changed if there was no noop compilation beforehand?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea exactly, since there was a no-op in there, there is no one less task which lowers the count.

| -> Msg: Compiling a (1 Scala source)
| -> Data kind: compile-task
|#4: a/src/A.scala
Expand All @@ -612,7 +599,7 @@ class BspCompileSpec(
|#4: a/src/A.scala
| -> List(Diagnostic(Range(Position(0,0),Position(0,26)),Some(Warning),Some(_),Some(_),Unused import,None,None,Some({"actions":[]})))
| -> reset = false
|#4: task finish 4
|#4: task finish 3
| -> errors 1, warnings 1
| -> Msg: Compiled 'a'
| -> Data kind: compile-report
Expand All @@ -626,13 +613,13 @@ class BspCompileSpec(
assertDifferentExternalClassesDirs(fifthCompiledState, compiledState, projects)
assertNoDiff(
fifthCompiledState.lastDiagnostics(`A`),
"""#5: task start 5
"""#5: task start 4
| -> Msg: Compiling a (1 Scala source)
| -> Data kind: compile-task
|#5: a/src/A.scala
| -> List(Diagnostic(Range(Position(0,0),Position(0,26)),Some(Warning),Some(_),Some(_),Unused import,None,None,Some({"actions":[]})))
| -> reset = true
|#5: task finish 5
|#5: task finish 4
| -> errors 0, warnings 1
| -> Msg: Compiled 'a'
| -> Data kind: compile-report
Expand All @@ -652,17 +639,17 @@ class BspCompileSpec(
assertSameExternalClassesDirs(sixthCompiledState, fifthCompiledState, projects)
assertNoDiff(
sixthCompiledState.lastDiagnostics(`A`),
"""#6: task start 6
"""#6: task start 5
| -> Msg: Compiling a (1 Scala source)
| -> Data kind: compile-task
|#6: a/src/A.scala
| -> List()
| -> reset = true
|#6: task finish 6
|#6: task finish 5
| -> errors 0, warnings 0
| -> Msg: Compiled 'a'
| -> Data kind: compile-report
|#6: task start 6
|#6: task start 5
| -> Msg: Compiling a (1 Scala source)
| -> Data kind: compile-task
|#6: a/src/A.scala
Expand All @@ -671,7 +658,7 @@ class BspCompileSpec(
|#6: a/src/A.scala
| -> List(Diagnostic(Range(Position(1,0),Position(3,1)),Some(Error),Some(_),Some(_),object creation impossible, since value y in trait Base of type Int is not defined,None,None,Some({"actions":[]})))
| -> reset = false
|#6: task finish 6
|#6: task finish 5
| -> errors 1, warnings 1
| -> Msg: Compiled 'a'
| -> Data kind: compile-report
Expand All @@ -686,13 +673,13 @@ class BspCompileSpec(

assertNoDiff(
seventhCompiledState.lastDiagnostics(`A`),
"""#7: task start 7
"""#7: task start 6
| -> Msg: Compiling a (1 Scala source)
| -> Data kind: compile-task
|#7: a/src/A.scala
| -> List(Diagnostic(Range(Position(0,0),Position(0,26)),Some(Warning),Some(_),Some(_),Unused import,None,None,Some({"actions":[]})))
| -> reset = true
|#7: task finish 7
|#7: task finish 6
| -> errors 0, warnings 0
| -> Msg: Compiled 'a'
| -> Data kind: compile-report
Expand Down Expand Up @@ -743,18 +730,12 @@ class BspCompileSpec(
assertSameExternalClassesDirs(compiledState.toTestState, secondCliCompiledState, `A`)

// BSP publishes warnings even if it's a no-op
// however it skips the task start/end
assertNoDiff(
compiledState.lastDiagnostics(`A`),
"""#1: task start 1
| -> Msg: Start no-op compilation for a
| -> Data kind: compile-task
|#1: a/src/main/scala/App.scala
"""#1: a/src/main/scala/App.scala
| -> List(Diagnostic(Range(Position(2,4),Position(2,4)),Some(Warning),Some(_),Some(_),a pure expression does nothing in statement position,None,None,Some({"actions":[]})))
| -> reset = true
|#1: task finish 1
| -> errors 0, warnings 0
| -> Msg: Compiled 'a'
| -> Data kind: compile-report""".stripMargin
| -> reset = true""".stripMargin
)
}
}
Expand Down Expand Up @@ -972,16 +953,9 @@ class BspCompileSpec(

assertNoDiff(
thirdCompiledState.lastDiagnostics(`A`),
"""#3: task start 3
| -> Msg: Start no-op compilation for a
| -> Data kind: compile-task
|#3: a/src/main/scala/Bar.scala
"""#3: a/src/main/scala/Bar.scala
| -> List()
| -> reset = true
|#3: task finish 3
| -> errors 0, warnings 0
| -> Msg: Compiled 'a'
| -> Data kind: compile-report""".stripMargin
| -> reset = true""".stripMargin
)
}
}
Expand Down Expand Up @@ -1083,14 +1057,7 @@ class BspCompileSpec(

assertNoDiff(
compiledState.lastDiagnostics(`B`),
"""|#2: task start 4
| -> Msg: Start no-op compilation for b
| -> Data kind: compile-task
|#2: task finish 4
| -> errors 0, warnings 0
| -> Msg: Compiled 'b'
| -> Data kind: compile-report
|""".stripMargin
"" // no-op
)

writeFile(`A`.srcFor("/Foo.scala"), Sources.`Foo3.scala`)
Expand All @@ -1100,7 +1067,7 @@ class BspCompileSpec(
assertValidCompilationState(thirdCompiledState, projects)
assertNoDiff(
thirdCompiledState.lastDiagnostics(`A`),
"""|#3: task start 5
"""|#3: task start 4
| -> Msg: Compiling a (1 Scala source)
| -> Data kind: compile-task
|#3: a/src/Foo.scala
Expand All @@ -1109,7 +1076,7 @@ class BspCompileSpec(
|#3: a/src/Foo.scala
| -> List(Diagnostic(Range(Position(1,0),Position(1,7)),Some(Error),Some(_),Some(_),Unused import,None,None,Some({"actions":[]})))
| -> reset = false
|#3: task finish 5
|#3: task finish 4
| -> errors 2, warnings 0
| -> Msg: Compiled 'a'
| -> Data kind: compile-report
Expand All @@ -1118,13 +1085,13 @@ class BspCompileSpec(

assertNoDiff(
compiledState.lastDiagnostics(`B`),
"""|#3: task start 6
"""|#3: task start 5
| -> Msg: Compiling b (1 Scala source)
| -> Data kind: compile-task
|#3: b/src/Buzz.scala
| -> List(Diagnostic(Range(Position(0,0),Position(0,7)),Some(Error),Some(_),Some(_),Unused import,None,None,Some({"actions":[]})))
| -> reset = true
|#3: task finish 6
|#3: task finish 5
| -> errors 1, warnings 0
| -> Msg: Compiled 'b'
| -> Data kind: compile-report
Expand Down
Loading
Loading