From a13b6b336866c8d2340d90b53fe48cb4cfdc3734 Mon Sep 17 00:00:00 2001 From: Tomasz Godzik Date: Tue, 21 Nov 2023 19:52:02 +0100 Subject: [PATCH] improvement: Forward DAP errors properly to the client Previously, we would not show the errors coming from debug discovery and no errors would be forwarded to the client. Now, we do those two things: - we no longer hide the discovery errors, but in case no folder contained valid run configuration, we try to show the most relevant error - instead of throwing a random exception we use LSP build-in one, which is sent back to the client properly and will be displayed Closes scalameta/metals-vscode#1438 --- .../internal/metals/MetalsEnrichments.scala | 13 +++ .../internal/metals/MetalsLspService.scala | 3 +- .../internal/metals/WorkspaceLspService.scala | 60 +++++----- .../debug/BuildTargetClassesFinder.scala | 15 ++- .../internal/metals/debug/DebugProvider.scala | 107 ++++++++---------- .../scala/tests/DebugDiscoverySuite.scala | 32 +++--- 6 files changed, 115 insertions(+), 115 deletions(-) diff --git a/metals/src/main/scala/scala/meta/internal/metals/MetalsEnrichments.scala b/metals/src/main/scala/scala/meta/internal/metals/MetalsEnrichments.scala index 0129b27ed2e..b31969117b4 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/MetalsEnrichments.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/MetalsEnrichments.scala @@ -58,6 +58,8 @@ import com.google.gson.JsonObject import fansi.ErrorMode import io.undertow.server.HttpServerExchange import org.eclipse.lsp4j.TextDocumentIdentifier +import org.eclipse.lsp4j.jsonrpc.ResponseErrorException +import org.eclipse.lsp4j.jsonrpc.messages import org.eclipse.{lsp4j => l} /** @@ -203,6 +205,17 @@ object MetalsEnrichments def asJavaObject: CompletableFuture[Object] = future.asJava.asInstanceOf[CompletableFuture[Object]] + def liftToLspError(implicit ec: ExecutionContext): Future[A] = + future.recoverWith { case NonFatal(e) => + val newException = new ResponseErrorException( + new messages.ResponseError( + messages.ResponseErrorCode.InvalidRequest, + e.getMessage(), + null, + ) + ) + Future.failed(newException) + } def asJavaUnit(implicit ec: ExecutionContext): CompletableFuture[Unit] = future.ignoreValue.asJava diff --git a/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala b/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala index 26652e7088f..5d6a1359046 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala @@ -1904,11 +1904,10 @@ class MetalsLspService( def analyzeStackTrace(content: String): Option[ExecuteCommandParams] = stacktraceAnalyzer.analyzeCommand(content) - def debugDiscovery(params: DebugDiscoveryParams): CompletableFuture[Object] = + def debugDiscovery(params: DebugDiscoveryParams): Future[DebugSession] = debugProvider .debugDiscovery(params) .flatMap(debugProvider.asSession) - .asJavaObject def findBuildTargetByDisplayName(target: String): Option[b.BuildTarget] = buildTargets.findByDisplayName(target) diff --git a/metals/src/main/scala/scala/meta/internal/metals/WorkspaceLspService.scala b/metals/src/main/scala/scala/meta/internal/metals/WorkspaceLspService.scala index def71ee16f9..799e019bf62 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/WorkspaceLspService.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/WorkspaceLspService.scala @@ -23,8 +23,6 @@ import scala.meta.internal.metals.WindowStateDidChangeParams import scala.meta.internal.metals.clients.language.ConfiguredLanguageClient import scala.meta.internal.metals.clients.language.MetalsLanguageClient import scala.meta.internal.metals.config.StatusBarState -import scala.meta.internal.metals.debug.BuildTargetNotFoundException -import scala.meta.internal.metals.debug.BuildTargetUndefinedException import scala.meta.internal.metals.debug.DebugProvider import scala.meta.internal.metals.doctor.DoctorVisibilityDidChangeParams import scala.meta.internal.metals.doctor.HeadDoctor @@ -93,6 +91,7 @@ import org.eclipse.lsp4j.TextDocumentPositionParams import org.eclipse.lsp4j.TextEdit import org.eclipse.lsp4j.WorkspaceEdit import org.eclipse.lsp4j.WorkspaceSymbolParams +import org.eclipse.lsp4j.jsonrpc.ResponseErrorException import org.eclipse.lsp4j.jsonrpc.messages class WorkspaceLspService( @@ -639,10 +638,20 @@ class WorkspaceLspService( folderServices.foreach(_.pause()) } - private def displayNotFound(objectName: String, id: String) = - languageClient.showMessage( - Messages.errorMessageParams(s"$objectName not found: $id") - ) + private def failedRequest( + message: String + ): Future[Object] = { + Future + .failed( + new ResponseErrorException( + new messages.ResponseError( + messages.ResponseErrorCode.InvalidParams, + message, + null, + ) + ) + ) + } private def onFirstSatifying[T, R](mapTo: MetalsLspService => Future[T])( satisfies: T => Boolean, @@ -842,21 +851,16 @@ class WorkspaceLspService( ) match { case Some(service) => service.startDebugProvider(params).asJavaObject case None => - languageClient.showMessage( - Messages.errorMessageParams( - s"Could not find folder for build targets: ${targets.mkString(",")}" - ) - ) - Future.failed(DebugProvider.WorkspaceErrorsException).asJavaObject + failedRequest( + s"Could not find folder for build targets: ${targets.mkString(",")}" + ).asJavaObject } case ServerCommands.StartMainClass(params) if params.mainClass != null => DebugProvider .getResultFromSearches( folderServices.map(_.mainClassSearch(params)) - ) { - displayNotFound("Main class", params.mainClass) - Future.failed(new ju.NoSuchElementException(params.mainClass)) - } + ) + .liftToLspError .asJavaObject case ServerCommands.StartTestSuite(params) @@ -867,20 +871,15 @@ class WorkspaceLspService( _.isDefined, (service, someTarget) => service.startTestSuite(someTarget.get, params), - () => { - displayNotFound("Build target", params.target.toString()) - Future.failed(BuildTargetNotFoundException(params.target.getUri)) - }, + () => failedRequest(s"Could not find '${params.target}' build target"), ).asJavaObject case ServerCommands.ResolveAndStartTestSuite(params) if params.testClass != null => DebugProvider .getResultFromSearches( folderServices.map(_.testClassSearch(params)) - ) { - displayNotFound("Test class", params.testClass) - Future.failed(new ju.NoSuchElementException(params.testClass)) - } + ) + .liftToLspError .asJavaObject case ServerCommands.StartAttach(params) if params.hostName != null => onFirstSatifying(service => @@ -891,13 +890,16 @@ class WorkspaceLspService( _.isDefined, (service, someTarget) => service.createDebugSession(someTarget.get.getId()), - () => { - displayNotFound("Build target", params.buildTarget) - Future.failed(BuildTargetUndefinedException()) - }, + () => + failedRequest( + s"Could not find '${params.buildTarget}' build target" + ), ).asJavaObject case ServerCommands.DiscoverAndRun(params) => - getServiceFor(params.path).debugDiscovery(params) + getServiceFor(params.path) + .debugDiscovery(params) + .liftToLspError + .asJavaObject case ServerCommands.AnalyzeStacktrace(content) => Future { // Getting the service for focused document and first one otherwise diff --git a/metals/src/main/scala/scala/meta/internal/metals/debug/BuildTargetClassesFinder.scala b/metals/src/main/scala/scala/meta/internal/metals/debug/BuildTargetClassesFinder.scala index 1d13cd66872..11931f706f2 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/debug/BuildTargetClassesFinder.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/debug/BuildTargetClassesFinder.scala @@ -39,8 +39,9 @@ class BuildTargetClassesFinder( // We check whether there is a main in dependencies that is not reported via BSP case ClassNotFoundInBuildTargetException(className, target) => revertToDependencies(className, Some(target)) - case _: ClassNotFoundException => + case _: NoClassFoundException => revertToDependencies(className, buildTarget = None) + case _ => Nil } found match { case Nil => Failure(ex) @@ -109,7 +110,7 @@ class BuildTargetClassesFinder( } .reverse if (classes.nonEmpty) Success(classes) - else Failure(new ClassNotFoundException(className)) + else Failure(new NoClassFoundException(className)) } { targetName => buildTargets .findByDisplayName(targetName) @@ -148,15 +149,19 @@ class BuildTargetClassesFinder( case class BuildTargetNotFoundException(buildTargetName: String) extends Exception(s"Build target not found: $buildTargetName") -case class BuildTargetUndefinedException() - extends Exception("Debugger configuration is missing 'buildTarget' param.") - case class ClassNotFoundInBuildTargetException( className: String, buildTarget: b.BuildTarget, ) extends Exception( s"Class '$className' not found in build target '${buildTarget.getDisplayName()}'" ) + +case class NoClassFoundException( + className: String +) extends Exception( + s"Class '$className' not found in any build target" + ) + case class BuildTargetNotFoundForPathException(path: AbsolutePath) extends Exception( s"No build target could be found for the path: ${path.toString()}" diff --git a/metals/src/main/scala/scala/meta/internal/metals/debug/DebugProvider.scala b/metals/src/main/scala/scala/meta/internal/metals/debug/DebugProvider.scala index d97b8199217..2022b56bef6 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/debug/DebugProvider.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/debug/DebugProvider.scala @@ -688,40 +688,6 @@ class DebugProvider( languageClient.metalsStatus( Messages.DebugErrorsPresent(clientConfig.icons()) ) - case t: ClassNotFoundException => - languageClient.showMessage( - Messages.DebugClassNotFound.invalidClass(t.getMessage()) - ) - case ClassNotFoundInBuildTargetException(cls, buildTarget) => - languageClient.showMessage( - Messages.DebugClassNotFound - .invalidTargetClass(cls, buildTarget.getDisplayName()) - ) - case BuildTargetNotFoundException(target) => - languageClient.showMessage( - Messages.DebugClassNotFound - .invalidTarget(target) - ) - case e: BuildTargetNotFoundForPathException => - languageClient.showMessage( - Messages.errorMessageParams(e.getMessage()) - ) - case e: BuildTargetContainsNoMainException => - languageClient.showMessage( - Messages.errorMessageParams(e.getMessage()) - ) - case e: BuildTargetUndefinedException => - languageClient.showMessage( - Messages.errorMessageParams(e.getMessage()) - ) - case e: NoTestsFoundException => - languageClient.showMessage( - Messages.errorMessageParams(e.getMessage()) - ) - case e: RunType.UnknownRunTypeException => - languageClient.showMessage( - Messages.errorMessageParams(e.getMessage()) - ) case e @ SemanticDbNotFoundException => languageClient.metalsStatus( MetalsStatusParams( @@ -730,13 +696,7 @@ class DebugProvider( command = ClientCommands.RunDoctor.id, ) ) - case e @ DotEnvFileParser.InvalidEnvFileException(_) => - languageClient.showMessage(Messages.errorMessageParams(e.getMessage())) - - case e @ NoRunOptionException => - languageClient.showMessage( - Messages.errorMessageParams(e.getMessage()) - ) + case _ => } private def parseSessionName( @@ -829,7 +789,7 @@ class DebugProvider( _ <- buildTargetClasses.rebuildIndex(target) result <- Future(f()) } yield result - case Failure(_: ClassNotFoundException) => + case Failure(_: NoClassFoundException) => val allTargetIds = buildTargets.allBuildTargetIds for { _ <- compilations.compileTargets(allTargetIds) @@ -867,6 +827,26 @@ class DebugProvider( object DebugProvider { + private def exceptionOrder(t: Throwable): Int = t match { + /* Validation errors, should shown first as they are relevant for each build target. + */ + case DotEnvFileParser.InvalidEnvFileException(_) => 0 + case NoRunOptionException => 1 + case _: RunType.UnknownRunTypeException => 2 + /* Target found, but class not, means that we managed to find the proper workspace folder + */ + case _: ClassNotFoundInBuildTargetException => 3 + case _: BuildTargetContainsNoMainException => 4 + case _: NoTestsFoundException => 5 + /* These exception will show up and it's hard to pinpoint which folder we should be in + * since we failed to fin the exception anywhere. Probably just showing the error is enough. + */ + case _: BuildTargetNotFoundForPathException => 6 + case _: NoClassFoundException => 7 + case _: BuildTargetNotFoundException => 8 + case _ => 9 + } + /** * Given an occurence and a text document return the symbol of a main method * that could be defined using the Scala 3 @main annotation. @@ -958,16 +938,16 @@ object DebugProvider { protected def dapSessionParams(res: A): Future[DebugSessionParams] def createDapSession(args: A): Future[DebugSession] = dapSessionParams(args).flatMap(debugProvider.asSession(_)) - def searchResult: Future[(Option[A], ClassSearch[A])] = { + def searchResult: Future[(Try[A], ClassSearch[A])] = { Future { val resolved = search() searchPromise.trySuccess(resolved) } - searchPromise.future.map(e => (e.toOption, this)) + searchPromise.future.map(e => (e, this)) } - def retrySearchResult: Future[(Option[A], ClassSearch[A])] = + def retrySearchResult: Future[(Try[A], ClassSearch[A])] = searchPromise.future - .flatMap(debugProvider.retryAfterRebuild(_, search).map(_.toOption)) + .flatMap(debugProvider.retryAfterRebuild(_, search)) .map((_, this)) } @@ -1000,27 +980,32 @@ object DebugProvider { debugProvider.buildTestClassParams(res, params) } - def getResultFromSearches[A](searches: List[ClassSearch[A]])( - default: => Future[DebugSession] + def getResultFromSearches[A]( + searches: List[ClassSearch[A]] )(implicit ec: ExecutionContext): Future[DebugSession] = Future .sequence(searches.map(_.searchResult)) - .getFirstOrElse( - Future - .sequence(searches.map(_.retrySearchResult)) - .getFirstOrElse(default) - ) + .getFirstOrError + .recoverWith { case _ => + Future.sequence(searches.map(_.retrySearchResult)).getFirstOrError + } private implicit class FindFirstDebugSession[A]( - from: Future[List[(Option[A], ClassSearch[A])]] + from: Future[List[(Try[A], ClassSearch[A])]] ) { - def getFirstOrElse( - default: => Future[DebugSession] - )(implicit ec: ExecutionContext): Future[DebugSession] = - from.flatMap { - _.collectFirst { case (Some(dapSession), search) => - search.createDapSession(dapSession) - }.getOrElse(default) + def getFirstOrError(implicit ec: ExecutionContext): Future[DebugSession] = + from.flatMap { all => + def mostRelevantError() = all + .collect { case (Failure(error), _) => + error + } + .sortBy(DebugProvider.exceptionOrder) + .head + all + .collectFirst { case (Success(dapSession), search) => + search.createDapSession(dapSession) + } + .getOrElse(Future.failed(mostRelevantError())) } } } diff --git a/tests/unit/src/test/scala/tests/DebugDiscoverySuite.scala b/tests/unit/src/test/scala/tests/DebugDiscoverySuite.scala index a5d7391649c..22a25d697be 100644 --- a/tests/unit/src/test/scala/tests/DebugDiscoverySuite.scala +++ b/tests/unit/src/test/scala/tests/DebugDiscoverySuite.scala @@ -12,6 +12,8 @@ import scala.meta.internal.metals.debug.DotEnvFileParser.InvalidEnvFileException import scala.meta.internal.metals.debug.NoTestsFoundException import scala.meta.io.AbsolutePath +import org.eclipse.lsp4j.jsonrpc.ResponseErrorException + // note(@tgodzik) all test have `System.exit(0)` added to avoid occasional issue due to: // https://stackoverflow.com/questions/2225737/error-jdwp-unable-to-get-jni-1-2-environment class DebugDiscoverySuite @@ -148,10 +150,10 @@ class DebugDiscoverySuite "runOrTestFile", ).toJson ) - .recover { case e @ DebugProvider.NoRunOptionException => e } + .recover { case e: ResponseErrorException => e.getMessage } } yield assertNoDiff( result.toString(), - DebugProvider.NoRunOptionException.toString(), + DebugProvider.NoRunOptionException.getMessage(), ) } @@ -215,12 +217,10 @@ class DebugDiscoverySuite "run", ).toJson ) - .recover { case e: BuildTargetContainsNoMainException => - e - } + .recover { case e: ResponseErrorException => e.getMessage } } yield assertNoDiff( result.toString, - BuildTargetContainsNoMainException("a").toString(), + BuildTargetContainsNoMainException("a").getMessage(), ) } @@ -247,12 +247,10 @@ class DebugDiscoverySuite "run", ).toJson ) - .recover { case WorkspaceErrorsException => - WorkspaceErrorsException - } + .recover { case e: ResponseErrorException => e.getMessage } } yield assertNoDiff( result.toString, - WorkspaceErrorsException.toString(), + WorkspaceErrorsException.getMessage(), ) } @@ -324,10 +322,10 @@ class DebugDiscoverySuite envFile = fakePath, ).toJson ) - .recover { case e: InvalidEnvFileException => e } + .recover { case e: ResponseErrorException => e.getMessage } } yield assertNoDiff( result.toString, - InvalidEnvFileException(AbsolutePath(fakePath)).toString(), + InvalidEnvFileException(AbsolutePath(fakePath)).getMessage(), ) } @@ -426,10 +424,10 @@ class DebugDiscoverySuite "testTarget", ).toJson ) - .recover { case e: NoTestsFoundException => e } + .recover { case e: ResponseErrorException => e.getMessage } } yield assertNoDiff( result.toString(), - NoTestsFoundException("build target", "a").toString(), + NoTestsFoundException("build target", "a").getMessage(), ) } @@ -460,12 +458,10 @@ class DebugDiscoverySuite "testFile", ).toJson ) - .recover { case SemanticDbNotFoundException => - SemanticDbNotFoundException - } + .recover { case e: ResponseErrorException => e.getMessage } } yield assertNoDiff( result.toString, - SemanticDbNotFoundException.toString(), + SemanticDbNotFoundException.getMessage(), ) } }