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.

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

Fixes CodeAction (quickfix) support #1226

Merged
merged 3 commits into from
Jul 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 5 additions & 5 deletions build.sbt
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import Util._
import ZincBuildUtil._
import Dependencies._
import localzinc.Scripted, Scripted._
import com.typesafe.tools.mima.core._, ProblemFilters._
Expand Down Expand Up @@ -280,7 +280,7 @@ lazy val zincPersist = (projectMatrix in internalPath / "zinc-persist")
}),
Test / classLoaderLayeringStrategy := ClassLoaderLayeringStrategy.Flat,
mimaSettings,
mimaBinaryIssueFilters ++= Util.excludeInternalProblems,
mimaBinaryIssueFilters ++= ZincBuildUtil.excludeInternalProblems,
mimaBinaryIssueFilters ++= Seq(
exclude[IncompatibleMethTypeProblem]("xsbti.*"),
exclude[ReversedMissingMethodProblem]("xsbti.*"),
Expand Down Expand Up @@ -363,7 +363,7 @@ lazy val zincCore = (projectMatrix in internalPath / "zinc-core")
name := "zinc Core",
compileOrder := sbt.CompileOrder.Mixed,
mimaSettings,
mimaBinaryIssueFilters ++= Util.excludeInternalProblems,
mimaBinaryIssueFilters ++= ZincBuildUtil.excludeInternalProblems,
mimaBinaryIssueFilters ++= Seq(
exclude[IncompatibleMethTypeProblem]("xsbti.*"),
exclude[ReversedMissingMethodProblem]("xsbti.*"),
Expand Down Expand Up @@ -432,7 +432,7 @@ lazy val zincCompileCore = (projectMatrix in internalPath / "zinc-compile-core")
Compile / managedSourceDirectories += (Compile / generateContrabands / sourceManaged).value,
Compile / generateContrabands / sourceManaged := (internalPath / "zinc-compile-core" / "src" / "main" / "contraband-java").getAbsoluteFile,
mimaSettings,
mimaBinaryIssueFilters ++= Util.excludeInternalProblems,
mimaBinaryIssueFilters ++= ZincBuildUtil.excludeInternalProblems,
)
.defaultAxes(VirtualAxis.jvm, VirtualAxis.scalaPartialVersion(scala212))
.jvmPlatform(scalaVersions = List(scala212, scala213))
Expand Down Expand Up @@ -656,7 +656,7 @@ lazy val zincClassfile = (projectMatrix in internalPath / "zinc-classfile")
exclude[IncompatibleResultTypeProblem]("sbt.internal.inc.IndexBasedZipOps.*"),
exclude[ReversedMissingMethodProblem]("sbt.internal.inc.IndexBasedZipOps.*"),
),
mimaBinaryIssueFilters ++= Util.excludeInternalProblems,
mimaBinaryIssueFilters ++= ZincBuildUtil.excludeInternalProblems,
)
.defaultAxes(VirtualAxis.jvm, VirtualAxis.scalaPartialVersion(scala212))
.jvmPlatform(scalaVersions = scala212_213)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,8 @@ private final class CachedCompiler0(
underlyingReporter: DelegatingReporter,
compileProgress: CompileProgress
): Unit = {
// cast down to AnalysisCallback2
val callback2 = callback.asInstanceOf[xsbti.AnalysisCallback2]

if (command.shouldStopWithInfo) {
underlyingReporter.info(null, command.getInfoMessage(compiler), true)
Expand All @@ -163,7 +165,17 @@ private final class CachedCompiler0(
run.compileFiles(sources)
processUnreportedWarnings(run)
underlyingReporter.problems.foreach(p =>
callback.problem(p.category, p.position, p.message, p.severity, true)
callback2.problem2(
p.category,
p.position,
p.message,
p.severity,
true,
p.rendered,
p.diagnosticCode,
p.diagnosticRelatedInformation,
p.actions
)
)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -231,9 +231,14 @@ private final class DelegatingReporter(

private def getActions(pos: Position, pos1: xsbti.Position, msg: String): List[Action] =
if (pos.isRange) Nil
else if (msg.startsWith("procedure syntax is deprecated:")) {
val edit = workspaceEdit(List(textEdit(pos1, ": Unit = {")))
action("procedure syntax", None, edit) :: Nil
else if (msg.contains("procedure syntax is deprecated")) {
if (msg.contains("add `: Unit =`")) {
val edit = workspaceEdit(List(textEdit(pos1, ": Unit = ")))
action("procedure syntax (defn)", None, edit) :: Nil
} else if (msg.contains("add `: Unit`")) {
val edit = workspaceEdit(List(textEdit(pos1, ": Unit")))
action("procedure syntax (decl)", None, edit) :: Nil
} else Nil
} else Nil

import xsbti.Severity.{ Info, Warn, Error }
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/*
* Zinc - The incremental compiler for Scala.
* Copyright Scala Center, Lightbend, and Mark Harrah
*
* Licensed under Apache License 2.0
* SPDX-License-Identifier: Apache-2.0
*
* See the NOTICE file distributed with this work for
* additional information regarding copyright ownership.
*/

package xsbti;

import java.util.List;
import java.util.Optional;

/**
* Extension to AnalsysisCallback.
* This is a compatibility layer created for Scala 3.x compilers.
* Even though compiler-interface is NOT intended to be a public API,
* Scala 3 compiler publishes their own compiler bridge against Zinc de jour.
* By having this interface, they can test if `callback` supports AnalysisCallback2.
Comment on lines +21 to +22
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you envision this working? If the dotty bridge starts doing isInstanceOf[AnalysisCallback2] we'll get a class not found exception if we're running against an older sbt/zinc, so we'd need to catch the exception I guess?

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, I guess something like that. I figured it's better than just adding a new method.

Choose a reason for hiding this comment

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

also we have methodhandles

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can work with that, but it'd be great if we could coordinate on this sort of things before sbt releases in the future so we can take the time to come up with the best way forward, for example if we had gone with a problem that takes an xsbti.Problem we could have ensured we don't need another interface like this next time we add a field to xsbti.Problem.

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, I wanted to unblock the development on this, but I guess in the hindsight I should've waited for reviews.

*/
public interface AnalysisCallback2 extends AnalysisCallback {
/**
* Register a compilation problem.
*
* This error may have already been logged or not. Unreported problems may
* happen because the reporting option was not enabled via command-line.
*
* @param what The headline of the error.
* @param pos At a given source position.
* @param msg The in-depth description of the error.
* @param severity The severity of the error reported.
* @param reported Flag that confirms whether this error was reported or not.
* @param rendered If present, the string shown to the user when displaying this Problem.
* @param diagnosticCode The unique code attached to the diagnostic being reported.
* @param diagnosticRelatedInformation The possible releated information for the diagnostic being reported.
* @param actions Actions (aka quick fixes) that are able to either fix or address the issue that is causing this problem.
*/
void problem2(String what,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this something that the Scala 3 bridge also needs to call? I thought we had all the pieces in place already /cc @ckipp01
Also couldn't this interface take an xsbti.Problem so we don't need a new one every time there's a new field?

Copy link

Choose a reason for hiding this comment

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

Nope, I don't believe this is necessary for Scala 3. These changes were specific to the Scala 2 implementation. You can actually test the actions already with the nightly version of the compiler and the latest metals release and see that everything is working as expected in Scala 3.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

Huh, I guess it would seem so. I guess I don't fully understand how this is all working or when this run is called that you have linked since at least via BSP with sbt current the actions are forwarded as expected.

Copy link

@bishabosha bishabosha Jul 24, 2023

Choose a reason for hiding this comment

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

You can actually test the actions already with the nightly version of the compiler and the latest metals release and see that everything is working as expected in Scala 3.

I confirmed this

Copy link
Member Author

Choose a reason for hiding this comment

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

sbt uses xsbi.Problem in 2 different ways:

  1. via xsbi.Reporter which you get a live callback per compiler warnings
  2. via CompileAnalysis, ReadSourceInfos, SourceInfo, which contains list of xsbti.Problem

This bug fix pertains the second pathway. I think the initial implementation of LSP or BSP use to resend all errors and warnings for all files at the end of compile as a workaround to make sure compiler warnings do not disappear, then in 2022 a few folks realized we're bombarding Metals with unnecessary problems, and we got fixes:

I usually ask @adpi2 to figure out the BSP handling, so I only occasionally jump in so I am not sure if this double dipping is unavoidable, but as a principle, it seems like a good idea that the problems we collect in Analysis contains new problem informations (like CodeAction and DiagnosticRelatedInformation).

Copy link
Member

Choose a reason for hiding this comment

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

it seems like a good idea that the problems we collect in Analysis contains new problem informations (like CodeAction and DiagnosticRelatedInformation).

Agreed. In the BSP reporter, we still use the CompileAnalysis to report the warnings from the files that the incremental compiler skipped. It's good to have the code actions in there.

Position pos,
String msg,
Severity severity,
boolean reported,
Optional<String> rendered,
Optional<DiagnosticCode> diagnosticCode,
List<DiagnosticRelatedInformation> diagnosticRelatedInformation,
List<Action> actions);
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,13 @@ package inc

import java.io.File
import java.nio.file.{ Files, Path, Paths }
import java.util.{ EnumSet, UUID }
import java.util.concurrent.atomic.AtomicBoolean
import java.{ util => ju }
import ju.{ EnumSet, Optional, UUID }
import ju.concurrent.atomic.AtomicBoolean
import sbt.internal.inc.Analysis.{ LocalProduct, NonLocalProduct }
import sbt.internal.inc.JavaInterfaceUtil.EnrichOption
import sbt.util.{ InterfaceUtil, Level, Logger }
import sbt.util.InterfaceUtil.{ jo2o, t2 }
import sbt.util.InterfaceUtil.{ jl2l, jo2o, l2jl, t2 }

import scala.collection.JavaConverters._
import scala.collection.mutable
Expand Down Expand Up @@ -597,7 +598,7 @@ private final class AnalysisCallback(
progress: Option[CompileProgress],
incHandlerOpt: Option[Incremental.IncrementalCallback],
log: Logger
) extends xsbti.AnalysisCallback {
) extends xsbti.AnalysisCallback2 {
import Incremental.CompileCycleResult

// This must have a unique value per AnalysisCallback
Expand Down Expand Up @@ -681,14 +682,20 @@ private final class AnalysisCallback(
()
}

def problem(
override def problem2(
category: String,
pos: Position,
msg: String,
severity: Severity,
reported: Boolean
): Unit = {
for (path <- jo2o(pos.sourcePath())) {
reported: Boolean,
rendered: Optional[String],
diagnosticCode: Optional[xsbti.DiagnosticCode],
diagnosticRelatedInformation: ju.List[xsbti.DiagnosticRelatedInformation],
actions: ju.List[xsbti.Action],
): Unit =
for {
path <- jo2o(pos.sourcePath())
} {
val source = VirtualFileRef.of(path)
val map = if (reported) reporteds else unreporteds
map
Expand All @@ -698,13 +705,31 @@ private final class AnalysisCallback(
pos = pos,
msg = msg,
sev = severity,
rendered = None,
diagnosticCode = None,
diagnosticRelatedInformation = Nil,
actions = Nil,
rendered = jo2o(rendered),
diagnosticCode = jo2o(diagnosticCode),
diagnosticRelatedInformation = jl2l(diagnosticRelatedInformation),
actions = jl2l(actions),
))
}
}

override def problem(
category: String,
pos: Position,
msg: String,
severity: Severity,
reported: Boolean
): Unit =
problem2(
category = category,
pos = pos,
msg = msg,
severity = severity,
reported = reported,
rendered = Optional.empty(),
diagnosticCode = Optional.empty(),
diagnosticRelatedInformation = l2jl(Nil),
actions = l2jl(Nil),
)

def classDependency(onClassName: String, sourceClassName: String, context: DependencyContext) = {
if (onClassName != sourceClassName)
Expand Down
28 changes: 20 additions & 8 deletions internal/zinc-testing/src/main/scala/xsbti/TestCallback.scala
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,15 @@ package xsbti

import java.io.File
import java.nio.file.Path
import java.util
import java.util.Optional
import java.{ util => ju }
import ju.Optional

import xsbti.api.{ DependencyContext, ClassLike }

import scala.collection.mutable.ArrayBuffer

class TestCallback extends AnalysisCallback {
case class TestUsedName(name: String, scopes: util.EnumSet[UseScope])
class TestCallback extends AnalysisCallback2 {
case class TestUsedName(name: String, scopes: ju.EnumSet[UseScope])

val classDependencies = new ArrayBuffer[(String, String, DependencyContext)]
val binaryDependencies =
Expand Down Expand Up @@ -108,7 +108,7 @@ class TestCallback extends AnalysisCallback {
()
}

def usedName(className: String, name: String, scopes: util.EnumSet[UseScope]): Unit =
def usedName(className: String, name: String, scopes: ju.EnumSet[UseScope]): Unit =
usedNamesAndScopes(className) += TestUsedName(name, scopes)

override def api(source: File, api: ClassLike): Unit = ???
Expand All @@ -124,19 +124,31 @@ class TestCallback extends AnalysisCallback {

override def enabled(): Boolean = true

def problem(
override def problem(
category: String,
pos: xsbti.Position,
message: String,
severity: xsbti.Severity,
reported: Boolean
reported: Boolean,
): Unit = ()

override def problem2(
category: String,
pos: Position,
msg: String,
severity: Severity,
reported: Boolean,
rendered: Optional[String],
diagnosticCode: Optional[xsbti.DiagnosticCode],
diagnosticRelatedInformation: ju.List[xsbti.DiagnosticRelatedInformation],
actions: ju.List[xsbti.Action],
): Unit = ()

override def dependencyPhaseCompleted(): Unit = {}

override def apiPhaseCompleted(): Unit = {}

override def classesInOutputJar(): util.Set[String] = java.util.Collections.emptySet()
override def classesInOutputJar(): ju.Set[String] = ju.Collections.emptySet()

override def isPickleJava: Boolean = false

Expand Down
2 changes: 1 addition & 1 deletion project/Util.scala → project/ZincBuildUtil.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import sbt._
import Keys._
import xsbti.compile.CompileAnalysis

object Util {
object ZincBuildUtil {
lazy val apiDefinitions = TaskKey[Seq[File]]("api-definitions")
lazy val genTestResTask = TaskKey[Seq[File]]("gen-test-resources")

Expand Down