Bugfix (#48)

* improve log messages * remove leak * fixing regression introduced in #46 when multiple ignores would cancel each other out
scalacenter · Dec 23, 2022 · 9a13d7b · 9a13d7b
1 parent d6b9520
commit 9a13d7b
Show file tree

Hide file tree

Showing 3 changed files with 85 additions and 86 deletions.
diff --git a/src/main/scala/ch/epfl/scala/sbtmissinglink/MissingLinkPlugin.scala b/src/main/scala/ch/epfl/scala/sbtmissinglink/MissingLinkPlugin.scala
@@ -29,13 +29,22 @@ object MissingLinkPlugin extends AutoPlugin {
       override def apply(packageName: String): Boolean =
         packageName != name && (!ignoreSubpackages || !packageName.startsWith(s"$name."))
     }
+    private[sbtmissinglink] implicit object IgnoredPackages extends PackageFilters[IgnoredPackage] {
+      def apply(name: String)(filters: Seq[IgnoredPackage]): Boolean = filters.forall(_.apply(name))
+    }
 
     final case class TargetedPackage(name: String, targetSubpackages: Boolean = true)
         extends PackageFilter {
       override def apply(packageName: String): Boolean =
         packageName == name || (targetSubpackages && packageName.startsWith(s"$name."))
     }
 
+    private[sbtmissinglink] implicit object TargetedPackages
+        extends PackageFilters[TargetedPackage] {
+      def apply(name: String)(filters: Seq[TargetedPackage]): Boolean =
+        filters.exists(_.apply(name))
+    }
+
     val missinglinkCheck: TaskKey[Unit] =
       taskKey[Unit]("Run the missinglink checks")
 
@@ -84,34 +93,50 @@ object MissingLinkPlugin extends AutoPlugin {
         val log = streams.value.log
 
         val cp = fullClasspath.value
-        val classDir = (classDirectory in Compile).value
+        val classDir = (Compile / classDirectory).value
         val failOnConflicts = missinglinkFailOnConflicts.value
-        val ignoreSourcePackages = missinglinkIgnoreSourcePackages.value
-        val ignoreDestinationPackages = missinglinkIgnoreDestinationPackages.value
-        val targetSourcePackages = missinglinkTargetSourcePackages.value
-        val targetDestinationPackages = missinglinkTargetDestinationPackages.value
-
         assert(
-          ignoreSourcePackages.isEmpty || targetSourcePackages.isEmpty,
+          missinglinkIgnoreSourcePackages.value.isEmpty || missinglinkTargetSourcePackages.value.isEmpty,
           "ignoreSourcePackages and targetSourcePackages cannot be defined in the same project."
         )
 
         assert(
-          ignoreDestinationPackages.isEmpty || targetDestinationPackages.isEmpty,
+          missinglinkIgnoreDestinationPackages.value.isEmpty || missinglinkTargetDestinationPackages.value.isEmpty,
           "ignoreDestinationPackages and targetDestinationPackages cannot be defined in the same project."
         )
 
         val filter =
           missinglinkExcludedDependencies.value.foldLeft[ModuleFilter](_ => true)((k, v) => k - v)
 
         val conflicts = loadArtifactsAndCheckConflicts(cp, classDir, filter, log)
-        val filteredConflicts =
-          filterConflicts(
-            conflicts,
-            ignoreSourcePackages ++ targetSourcePackages,
-            ignoreDestinationPackages ++ targetDestinationPackages,
-            log
-          )
+
+        val conflictFilters = filterConflicts(
+          missinglinkIgnoreSourcePackages.value,
+          missinglinkIgnoreSourcePackages,
+          log,
+          "source",
+          _.fromClass,
+        ) andThen filterConflicts(
+          missinglinkTargetSourcePackages.value,
+          missinglinkTargetSourcePackages,
+          log,
+          "source",
+          _.fromClass,
+        ) andThen filterConflicts(
+          missinglinkIgnoreDestinationPackages.value,
+          missinglinkIgnoreDestinationPackages,
+          log,
+          "destination",
+          _.targetClass,
+        ) andThen filterConflicts(
+          missinglinkTargetDestinationPackages.value,
+          missinglinkTargetDestinationPackages,
+          log,
+          "destination",
+          _.targetClass,
+        )
+
+        val filteredConflicts = conflictFilters(conflicts)
 
         if (filteredConflicts.nonEmpty) {
           val initialTotal = conflicts.length
@@ -212,8 +237,11 @@ object MissingLinkPlugin extends AutoPlugin {
       .build()
   }
 
-  private def loadClass(f: File): DeclaredClass =
-    com.spotify.missinglink.ClassLoader.load(new FileInputStream(f))
+  private def loadClass(f: File): DeclaredClass = {
+    val is = new FileInputStream(f)
+    try com.spotify.missinglink.ClassLoader.load(is)
+    finally is.close()
+  }
 
   private def loadBootstrapArtifacts(bootstrapClasspath: String, log: Logger): List[Artifact] = {
     if (bootstrapClasspath == null) {
@@ -249,79 +277,43 @@ object MissingLinkPlugin extends AutoPlugin {
       ModuleArtifact(artifactLoader.load(f.data), f.get(moduleID.key))
     }
 
-    cp.filter(c => isValid(c.data)).map(fileToArtifact(_)).toList
+    cp.filter(c => isValid(c.data)).map(fileToArtifact).toList
   }
 
-  private def filterConflicts(
-    conflicts: Seq[Conflict],
-    sourceFilters: Seq[PackageFilter],
-    destinationFilters: Seq[PackageFilter],
-    log: Logger
-  ): Seq[Conflict] = {
-
-    def filter(
-      packageFilters: Seq[PackageFilter],
-      name: String,
-      setting: String,
-      field: Dependency => ClassTypeDescriptor
-    ): Seq[Conflict] => Seq[Conflict] = { input =>
-      if (packageFilters.nonEmpty) {
-        log.debug(s"Applying filters on $name packages: ${packageFilters.mkString(", ")}")
-
-        def isFiltered(conflict: Conflict): Boolean = {
-          val descriptor = field(conflict.dependency())
-          val className = descriptor.getClassName.replace('/', '.')
-          val conflictPackageName = className.substring(0, className.lastIndexOf('.'))
-
-          packageFilters.exists(_.apply(conflictPackageName))
-        }
+  private def filterConflicts[T <: PackageFilter](
+    packageFilters: Seq[T],
+    setting: SettingKey[_],
+    log: Logger,
+    name: String,
+    field: Dependency => ClassTypeDescriptor,
+  )(implicit pfs: PackageFilters[T]): Seq[Conflict] => Seq[Conflict] = { input =>
+    if (packageFilters.nonEmpty) {
+      log.debug(s"Applying filters on $name packages: ${packageFilters.mkString(", ")}")
+
+      def isFiltered(conflict: Conflict): Boolean = {
+        val descriptor = field(conflict.dependency())
+        val className = descriptor.getClassName.replace('/', '.')
+        val conflictPackageName = className.substring(0, className.lastIndexOf('.'))
+
+        pfs.apply(conflictPackageName)(packageFilters)
+      }
 
-        val filtered = input.filter(isFiltered)
-        val diff = input.length - filtered.length
+      val filtered = input.filter(isFiltered)
+      val diff = input.length - filtered.length
 
-        if (diff != 0) {
-          log.info(
-            s"""
-            |$diff conflicts found in ignored $name packages.
-            |Run plugin again without the '$setting' configuration to see all conflicts that were found.
+      if (diff != 0) {
+        log.info(
+          s"""
+            |$diff conflicts found in ignored ${name} packages.
+            |Run plugin again without the '${setting.key.label}' setting to see all conflicts that were found.
              """.stripMargin
-          )
-        }
-
-        filtered
-      } else {
-        input
+        )
       }
-    }
-
-    val filters = List(
-      filter(
-        sourceFilters.collect { case p: IgnoredPackage => p },
-        "source",
-        "ignoreSourcePackages",
-        _.fromClass
-      ),
-      filter(
-        sourceFilters.collect { case p: TargetedPackage => p },
-        "source",
-        "targetSourcePackages",
-        _.fromClass
-      ),
-      filter(
-        destinationFilters.collect { case p: IgnoredPackage => p },
-        "destination",
-        "ignoreSourcePackages",
-        _.targetClass
-      ),
-      filter(
-        destinationFilters.collect { case p: TargetedPackage => p },
-        "destination",
-        "targetDestinationPackages",
-        _.targetClass
-      )
-    )
 
-    Function.chain(filters).apply(conflicts)
+      filtered
+    } else {
+      input
+    }
   }
 
   private def outputConflicts(conflicts: Seq[Conflict], log: Logger): Unit = {
@@ -378,5 +370,10 @@ object MissingLinkPlugin extends AutoPlugin {
 
   private final case class ModuleArtifact(artifact: Artifact, module: Option[ModuleID])
 
-  private[sbtmissinglink] sealed trait PackageFilter extends (String => Boolean)
+  private[sbtmissinglink] sealed trait PackageFilter {
+    def apply(name: String): Boolean
+  }
+  private[sbtmissinglink] trait PackageFilters[T <: PackageFilter] {
+    def apply(name: String)(filters: Seq[T]): Boolean
+  }
 }
diff --git a/src/sbt-test/missinglink/ignore-destination-package/build.sbt b/src/sbt-test/missinglink/ignore-destination-package/build.sbt
@@ -14,5 +14,6 @@ lazy val `ignore-destination-package` = project
     // Speed up compilation a bit. Our .java files do not need to see the .scala files.
     compileOrder := CompileOrder.JavaThenScala,
 
-    missinglinkIgnoreDestinationPackages += IgnoredPackage("com.google.common")
+    missinglinkIgnoreDestinationPackages += IgnoredPackage("com.google.common"),
+    missinglinkIgnoreDestinationPackages += IgnoredPackage("whatever"),
   )
diff --git a/src/sbt-test/missinglink/ignore-source-package/build.sbt b/src/sbt-test/missinglink/ignore-source-package/build.sbt
@@ -14,5 +14,6 @@ lazy val `ignore-source-package` = project
     // Speed up compilation a bit. Our .java files do not need to see the .scala files.
     compileOrder := CompileOrder.JavaThenScala,
 
-    missinglinkIgnoreSourcePackages += IgnoredPackage("test")
+    missinglinkIgnoreSourcePackages += IgnoredPackage("test"),
+    missinglinkIgnoreSourcePackages += IgnoredPackage("whatever"),
   )