Merge pull request #302 from adpi2/fix-metals-4532

Fix metals#4532: Handle empty jar files

Author: adpi2

build.sbt

@@ -89,6 +89,7 @@ lazy val tests = projectMatrix
     publish := {},
     // Test / javaOptions += "-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=1044",
     Test / fork := true,
+    Test / baseDirectory := (ThisBuild / baseDirectory).value / "tests",
     // do not use sbt logger, otherwise the output of a test only appears at the end of the suite
     Test / testOptions += Tests.Argument(TestFrameworks.MUnit, "+l"),
     Test / testOptions := (Test / testOptions)

core/src/main/scala/ch/epfl/scala/debugadapter/ClassSystem.scala

@@ -8,37 +8,38 @@ import java.nio.file.Path
 import java.nio.file.Files
 import java.net.URI
 import scala.collection.mutable
-import scala.util.control.NonFatal
 import java.util.Collections
+import scala.util.Try
 
 sealed trait ClassSystem {
-  def within[T](f: (FileSystem, Path) => T): Option[T]
+  def name: String
+  def within[T](f: (FileSystem, Path) => T): Try[T]
   def readBytes(path: String): Array[Byte] =
     within { (_, root) =>
       Files.readAllBytes(root.resolve(path))
     }.get
 }
 
 final case class ClassJar(absolutePath: Path) extends ClassSystem {
-  def within[T](f: (FileSystem, Path) => T): Option[T] =
+  def name: String = absolutePath.toString
+  def within[T](f: (FileSystem, Path) => T): Try[T] =
     IO.withinJarFile(absolutePath)(fs => f(fs, fs.getPath("/")))
 }
 
 final case class ClassDirectory(absolutePath: Path) extends ClassSystem {
-  def within[T](f: (FileSystem, Path) => T): Option[T] =
-    Some(f(FileSystems.getDefault, absolutePath))
+  def name: String = absolutePath.toString
+  def within[T](f: (FileSystem, Path) => T): Try[T] = Try {
+    f(FileSystems.getDefault, absolutePath)
+  }
 }
 
 final case class JavaRuntimeSystem(classLoader: ClassLoader, javaHome: Path) extends ClassSystem {
+  def name: String = javaHome.toString
   def fileSystem: FileSystem =
     JavaRuntimeSystem.getFileSystem(classLoader, javaHome)
 
-  def within[T](f: (FileSystem, Path) => T): Option[T] = {
-    try {
-      Some(f(fileSystem, fileSystem.getPath("/modules")))
-    } catch {
-      case NonFatal(_) => None
-    }
+  def within[T](f: (FileSystem, Path) => T): Try[T] = Try {
+    f(fileSystem, fileSystem.getPath("/modules"))
   }
 }
 

core/src/main/scala/ch/epfl/scala/debugadapter/SourceEntry.scala

@@ -2,7 +2,15 @@ package ch.epfl.scala.debugadapter
 
 import java.nio.file.Path
 
-sealed trait SourceEntry
-final case class SourceDirectory(directory: Path) extends SourceEntry
-final case class SourceJar(jar: Path) extends SourceEntry
-final case class StandaloneSourceFile(absolutePath: Path, relativePath: String) extends SourceEntry
+sealed trait SourceEntry {
+  def name: String
+}
+final case class SourceDirectory(directory: Path) extends SourceEntry {
+  def name: String = directory.toString
+}
+final case class SourceJar(jar: Path) extends SourceEntry {
+  def name: String = jar.toString
+}
+final case class StandaloneSourceFile(absolutePath: Path, relativePath: String) extends SourceEntry {
+  def name: String = relativePath.toString
+}

core/src/main/scala/ch/epfl/scala/debugadapter/internal/ClassEntryLookUp.scala

@@ -12,6 +12,8 @@ import ClassEntryLookUp.readSourceContent
 import scala.util.matching.Regex
 import ch.epfl.scala.debugadapter.internal.scalasig.ScalaSig
 import ch.epfl.scala.debugadapter.internal.scalasig.Decompiler
+import ch.epfl.scala.debugadapter.internal.ScalaExtension.*
+import scala.util.Try
 
 private case class SourceLine(uri: URI, lineNumber: Int)
 
@@ -60,7 +62,9 @@ private class ClassEntryLookUp(
       sourceUriToClassFiles(sourceUri)
         .groupBy(_.classSystem)
         .foreach { case (classSystem, classFiles) =>
-          classSystem.within((_, root) => loadLineNumbers(root, classFiles, sourceUri))
+          classSystem
+            .within((_, root) => loadLineNumbers(root, classFiles, sourceUri))
+            .warnFailure(logger, s"Cannot load line numbers in ${classSystem.name}")
         }
     }
 
@@ -117,7 +121,7 @@ private class ClassEntryLookUp(
   }
 
   def getSourceContent(sourceUri: URI): Option[String] =
-    sourceUriToSourceFile.get(sourceUri).flatMap(readSourceContent)
+    sourceUriToSourceFile.get(sourceUri).flatMap(readSourceContent(_, logger))
 
   def getSourceFile(fqcn: String): Option[URI] =
     classNameToSourceFile.get(fqcn).map(_.uri)
@@ -161,7 +165,7 @@ private object ClassEntryLookUp {
     new ClassEntryLookUp(entry, Map.empty, Map.empty, Map.empty, Map.empty, Seq.empty, Seq.empty, logger)
 
   private[internal] def apply(entry: ClassEntry, logger: Logger): ClassEntryLookUp = {
-    val sourceFiles = entry.sourceEntries.flatMap(SourceEntryLookUp.getAllSourceFiles)
+    val sourceFiles = entry.sourceEntries.flatMap(SourceEntryLookUp.getAllSourceFiles(_, logger))
     ClassEntryLookUp(entry, sourceFiles, logger)
   }
 
@@ -175,6 +179,7 @@ private object ClassEntryLookUp {
       val classFiles = entry.classSystems.flatMap { classSystem =>
         classSystem
           .within(readAllClassFiles(classSystem))
+          .warnFailure(logger, s"Cannot list the class files in ${classSystem.name}")
           .getOrElse(Vector.empty)
       }
 
@@ -235,7 +240,7 @@ private object ClassEntryLookUp {
                     // so we try to find the right package declaration in each file
                     // it would be very unfortunate that 2 sources file with the same name
                     // declare the same package.
-                    manySourceFiles.filter(s => findPackage(s, classFile.fullPackage)) match {
+                    manySourceFiles.filter(s => findPackage(s, classFile.fullPackage, logger)) match {
                       case sourceFile :: Nil =>
                         recordSourceFile(sourceFile)
                       case _ =>
@@ -325,36 +330,35 @@ private object ClassEntryLookUp {
 
   private def findPackage(
       sourceFile: SourceFile,
-      fullPackage: String
+      fullPackage: String,
+      logger: Logger
   ): Boolean = {
     // for "a.b.c" it returns Seq("a.b.c", "b.c", "c")
     // so that we can match on "package a.b.c" or "package b.c" or "package c"
     val nestedPackages = fullPackage.split('.').foldLeft(Seq.empty[String]) { (nestedParts, newPart) =>
       nestedParts.map(outer => s"$outer.$newPart") :+ newPart
     }
-    val sourceContent = readSourceContent(sourceFile)
+    val sourceContent = readSourceContent(sourceFile, logger)
     nestedPackages.exists { `package` =>
       val quotedPackage = Regex.quote(`package`)
       val matcher = s"package\\s+(object\\s+)?$quotedPackage(\\{|:|;|\\s+)".r
       sourceContent.exists(matcher.findFirstIn(_).isDefined)
     }
   }
 
-  private def readSourceContent(sourceFile: SourceFile): Option[String] = {
+  private def readSourceContent(sourceFile: SourceFile, logger: Logger): Option[String] = {
     withinSourceEntry(sourceFile.entry) { root =>
       val sourcePath = root.resolve(sourceFile.relativePath)
       new String(Files.readAllBytes(sourcePath))
     }
+      .warnFailure(logger, s"Cannot read content of ${sourceFile.uri}")
   }
 
-  private def withinSourceEntry[T](
-      sourceEntry: SourceEntry
-  )(f: Path => T): Option[T] = {
+  private def withinSourceEntry[T](sourceEntry: SourceEntry)(f: Path => T): Try[T] = {
     sourceEntry match {
       case SourceJar(jar) => IO.withinJarFile(jar)(fs => f(fs.getPath("/")))
-      case SourceDirectory(dir) => Some(f(dir))
-      case StandaloneSourceFile(absolutePath, _) =>
-        Some(f(absolutePath.getParent))
+      case SourceDirectory(dir) => Try(f(dir))
+      case StandaloneSourceFile(absolutePath, _) => Try(f(absolutePath.getParent))
     }
   }
 }

core/src/main/scala/ch/epfl/scala/debugadapter/internal/IO.scala

@@ -5,16 +5,19 @@ import java.nio.file.FileSystem
 import java.nio.file.FileSystems
 import java.nio.file.Path
 import java.util
+import scala.util.Try
 import scala.util.control.NonFatal
+import scala.util.Failure
+import scala.util.Success
 
 private[debugadapter] object IO {
-  def withinJarFile[T](absolutePath: Path)(f: FileSystem => T): Option[T] = {
+  def withinJarFile[T](absolutePath: Path)(f: FileSystem => T): Try[T] = try {
     val uri = URI.create(s"jar:${absolutePath.toUri}")
-    val fileSystem =
-      FileSystems.newFileSystem(uri, new util.HashMap[String, Any])
-    try Some(f(fileSystem))
-    catch {
-      case NonFatal(e) => None
-    } finally fileSystem.close()
+    val fileSystem = FileSystems.newFileSystem(uri, new util.HashMap[String, Any])
+    try Success(f(fileSystem))
+    finally fileSystem.close()
+  } catch {
+    case NonFatal(exception) => Failure(exception)
+    case zipError: util.zip.ZipError => Failure(zipError)
   }
 }

core/src/main/scala/ch/epfl/scala/debugadapter/internal/SourceEntryLookUp.scala

@@ -10,6 +10,8 @@ import java.nio.file.Path
 import java.nio.file.Files
 import scala.jdk.CollectionConverters.*
 import java.net.URI
+import ch.epfl.scala.debugadapter.Logger
+import ch.epfl.scala.debugadapter.internal.ScalaExtension.*
 
 private case class SourceFile(
     entry: SourceEntry,
@@ -21,12 +23,15 @@ private case class SourceFile(
 }
 
 private object SourceEntryLookUp {
-  def getAllSourceFiles(entry: SourceEntry): Seq[SourceFile] = {
+  def getAllSourceFiles(entry: SourceEntry, logger: Logger): Seq[SourceFile] = {
     entry match {
       case SourceJar(jar) =>
-        IO.withinJarFile(jar) { fileSystem =>
-          getAllSourceFiles(entry, fileSystem, fileSystem.getPath("/")).toVector
-        }.getOrElse(Vector.empty)
+        IO
+          .withinJarFile(jar) { fileSystem =>
+            getAllSourceFiles(entry, fileSystem, fileSystem.getPath("/")).toVector
+          }
+          .warnFailure(logger, s"Cannot list the source files in ${entry.name}")
+          .getOrElse(Vector.empty)
       case SourceDirectory(directory) =>
         getAllSourceFiles(entry, FileSystems.getDefault, directory).toSeq
       case StandaloneSourceFile(absolutePath, relativePath) =>

core/src/main/scala/ch/epfl/scala/debugadapter/internal/SourceLookUpProvider.scala

@@ -82,7 +82,7 @@ private[debugadapter] object SourceLookUpProvider {
     val sourceFilesByEntry = parrallelEntries
       .flatMap(_.sourceEntries)
       .distinct
-      .map(entry => entry -> SourceEntryLookUp.getAllSourceFiles(entry))
+      .map(entry => entry -> SourceEntryLookUp.getAllSourceFiles(entry, logger))
       .toMap
     val allLookUps = parrallelEntries
       .map(entry => ClassEntryLookUp(entry, entry.sourceEntries.flatMap(sourceFilesByEntry.apply), logger))

tests/input/class-entry/empty-sources.jar

@@ -8,6 +8,9 @@ import ch.epfl.scala.debugadapter.ClassEntry
 import ch.epfl.scala.debugadapter.testfmk.NoopLogger
 import munit.FunSuite
 import scala.concurrent.duration.*
+import ch.epfl.scala.debugadapter.Library
+import java.nio.file.Paths
+import ch.epfl.scala.debugadapter.SourceJar
 
 /**
  * This is a test class that also
@@ -17,6 +20,14 @@ class ClassEntryLookUpStats extends FunSuite {
   override def munitTimeout: Duration = 120.seconds
   private val jvmCache = JvmCache().withDefaultIndex
 
+  test("empty source jar") {
+    val entry = getClassEntry("empty")
+    printAndCheck("empty", entry)(
+      classCount => assertEquals(classCount, 0),
+      orphanCount => assertEquals(orphanCount, 0)
+    )
+  }
+
   test("adopt:1.8.0-292") {
     printAndCheck("adopt:1.8.0-292")(
       classCount => assert(classCount > 0),
@@ -208,4 +219,12 @@ class ClassEntryLookUpStats extends FunSuite {
     classCountAssertion(classCount)
     orphanAssertion(orphanClassCount)
   }
+
+  private def getClassEntry(name: String): ClassEntry = {
+    val directory = Paths.get("input/class-entry").toAbsolutePath
+    val classJar = directory.resolve(s"$name.jar")
+    val sourceJar = directory.resolve(s"$name-sources.jar")
+    val sourceEntry = SourceJar(sourceJar)
+    Library(name, "", classJar, Seq(sourceEntry))
+  }
 }