Fix false positives for the codec registration checker (#166)

* readability improvement - comment and renaming * more readability improvements * PR fixes * commit for draft PR * fix better-files scope * fixed false positives for codec registration checker * added README notes * pr fixes 1 * pR fixes * PR fix * PR fixes * PR fixes * added comment
VirtusLab · Jun 21, 2022 · 884620f · 884620f
1 parent 5c13c96
commit 884620f
Show file tree

Hide file tree

Showing 8 changed files with 195 additions and 77 deletions.
diff --git a/build.sbt b/build.sbt
@@ -56,6 +56,29 @@ lazy val commonSettings = Seq(
       if (sys.env.getOrElse("CI", "false") == "true") "-Xfatal-warnings" else ""),
   libraryDependencies ++= commonDeps)
 
+// As usage of https://github.com/pathikrit/better-files has been added to the runtime logic of codec-registration-checker-compiler-plugin
+// and dump-persistence-schema-compiler-plugin - this dependency has to be provided within a fat jar when ASH gets published.
+// For reasons described in https://github.com/sbt/sbt/issues/2255 - without using fat-jar we would have java.lang.NoClassDefFoundErrors
+lazy val assemblySettings = Seq(
+  packageBin / publishArtifact := false, //we want to publish fat jar
+  Compile / packageBin / artifactPath := crossTarget.value / "packageBinPlaceholder.jar", //this ensures that normal jar doesn't override fat jar
+  assembly / assemblyMergeStrategy := {
+    case PathList(
+          "scala",
+          "annotation",
+          "nowarn.class" | "nowarn$.class"
+        ) => //scala-collection-compat duplicates no-warn.class, as it was added to scala 2.12 after its release
+      MergeStrategy.first
+    case x =>
+      (assembly / assemblyMergeStrategy).value.apply(x)
+  },
+  Compile / assembly / artifact := {
+    val art = (Compile / assembly / artifact).value
+    art.withClassifier(None)
+  },
+  assembly / assemblyJarName := s"${name.value}_${scalaBinaryVersion.value}-${version.value}.jar", //Warning: this is a default name for packageBin artefact. Without explicit rename of packageBin will result in race condition
+  addArtifact(Compile / assembly / artifact, assembly))
+
 publish / skip := true
 
 lazy val scalaVersionAxis = settingKey[Option[String]]("Project scala version")
@@ -127,6 +150,7 @@ lazy val serializabilityCheckerCompilerPlugin = (projectMatrix in file("serializ
   .jvmPlatform(scalaVersions = supportedScalaVersions)
 
 lazy val codecRegistrationCheckerCompilerPlugin = (projectMatrix in file("codec-registration-checker-compiler-plugin"))
+  .enablePlugins(AssemblyPlugin)
   .settings(name := "codec-registration-checker-compiler-plugin")
   .settings(commonSettings)
   .settings(
@@ -139,7 +163,8 @@ lazy val codecRegistrationCheckerCompilerPlugin = (projectMatrix in file("codec-
         }
         .getOrElse(Seq.empty)
     },
-    libraryDependencies ++= Seq(betterFiles % Test))
+    libraryDependencies += betterFiles)
+  .settings(assemblySettings: _*)
   .dependsOn(annotation, circeAkkaSerializer % Test)
   .jvmPlatform(scalaVersions = supportedScalaVersions)
 
@@ -189,23 +214,6 @@ lazy val dumpPersistenceSchemaCompilerPlugin = (projectMatrix in file("dump-pers
         }
         .getOrElse(Seq.empty)
     },
-    libraryDependencies ++= Seq(sprayJson, betterFiles, akkaActorTyped % Test, akkaPersistenceTyped % Test),
-    packageBin / publishArtifact := false, //we want to publish fat jar
-    Compile / packageBin / artifactPath := crossTarget.value / "packageBinPlaceholder.jar", //this ensures that normal jar doesn't override fat jar
-    assembly / assemblyMergeStrategy := {
-      case PathList(
-            "scala",
-            "annotation",
-            "nowarn.class" | "nowarn$.class"
-          ) => //scala-collection-compat duplicates no-warn.class, as it was added to scala 2.12 after its release
-        MergeStrategy.first
-      case x =>
-        (assembly / assemblyMergeStrategy).value.apply(x)
-    },
-    Compile / assembly / artifact := {
-      val art = (Compile / assembly / artifact).value
-      art.withClassifier(None)
-    },
-    assembly / assemblyJarName := s"${name.value}_${scalaBinaryVersion.value}-${version.value}.jar", //Warning: this is a default name for packageBin artefact. Without explicit rename of packageBin will result in race condition
-    addArtifact(Compile / assembly / artifact, assembly))
+    libraryDependencies ++= Seq(sprayJson, betterFiles, akkaActorTyped % Test, akkaPersistenceTyped % Test))
+  .settings(assemblySettings: _*)
   .jvmPlatform(scalaVersions = supportedScalaVersions)
diff --git a/...iler-plugin/src/main/scala/org/virtuslab/ash/CodecRegistrationCheckerCompilerPlugin.scala b/...iler-plugin/src/main/scala/org/virtuslab/ash/CodecRegistrationCheckerCompilerPlugin.scala
@@ -12,6 +12,8 @@ import scala.tools.nsc.plugins.Plugin
 import scala.tools.nsc.plugins.PluginComponent
 
 import org.virtuslab.ash.CodecRegistrationCheckerCompilerPlugin.directClassDescendantsCacheFileName
+import org.virtuslab.ash.CodecRegistrationCheckerCompilerPlugin.disableFlag
+import org.virtuslab.ash.CodecRegistrationCheckerCompilerPlugin.sourceCodeDirectoryFlag
 
 class CodecRegistrationCheckerCompilerPlugin(override val global: Global) extends Plugin {
   override val name: String = "codec-registration-checker-plugin"
@@ -24,9 +26,18 @@ class CodecRegistrationCheckerCompilerPlugin(override val global: Global) extend
   override val components: List[PluginComponent] = List(classSweep, serializerCheck)
 
   override def init(options: List[String], error: String => Unit): Boolean = {
-    if (options.contains("--disable"))
+    if (options.contains(disableFlag))
       return false
 
+    options.find(flag => flag.contains(sourceCodeDirectoryFlag)) match {
+      case Some(directoryFlag) =>
+        pluginOptions.sourceCodeDirectoryToCheck = directoryFlag.replace(s"$sourceCodeDirectoryFlag=", "")
+      case None =>
+        error(
+          s"Required $sourceCodeDirectoryFlag option has not been set. Please, specify the $sourceCodeDirectoryFlag and retry compilation")
+        return false
+    }
+
     options.filterNot(_.startsWith("-")).headOption match {
       case Some(path) =>
         try {
@@ -63,10 +74,12 @@ class CodecRegistrationCheckerCompilerPlugin(override val global: Global) extend
         false
     }
   }
-  override val optionsHelp: Option[String] = Some("""
+  override val optionsHelp: Option[String] = Some(s"""
       |. - directory where cache file will be saved, required
-      |--disable - disables the plugin
+      |$disableFlag - disables the plugin
+      |$sourceCodeDirectoryFlag - path of the source code directory, which has to be checked with this plugin
       |""".stripMargin)
+
 }
 
 object CodecRegistrationCheckerCompilerPlugin {
@@ -76,6 +89,9 @@ object CodecRegistrationCheckerCompilerPlugin {
   val serializabilityTraitType = "org.virtuslab.ash.annotation.SerializabilityTrait"
   val serializerType = "org.virtuslab.ash.annotation.Serializer"
 
+  val disableFlag = "--disable"
+  val sourceCodeDirectoryFlag = "--source-code-directory"
+
   def parseCacheFile(buffer: ByteBuffer): Seq[ParentChildFQCNPair] = {
     StandardCharsets.UTF_8.decode(buffer).toString.split("\n").toSeq.filterNot(_.isBlank).map(_.split(",")).map {
       case Array(a, b) => ParentChildFQCNPair(a, b)

diff --git a/...er-compiler-plugin/src/main/scala/org/virtuslab/ash/CodecRegistrationCheckerOptions.scala b/...er-compiler-plugin/src/main/scala/org/virtuslab/ash/CodecRegistrationCheckerOptions.scala
@@ -21,9 +21,15 @@ import java.io.File
  *   (when `sbt compile` was incremental). And if we can't detect it - this would lead to runtime errors (see README
  *   for more details). `oldParentChildFQCNPairs` gets declared on the plugin's init
  *   by invoking the `CodecRegistrationCheckerCompilerPlugin.parseCacheFile` method.
+ *
+ * @param sourceCodeDirectoryToCheck - path of the source code directory that hold files with traits and classes
+ *   checked by this plugin (i.e. types that are saved into `directClassDescendantsCacheFile`). This parameter is
+ *   needed to fix possible false-positives in certain situations.
+ *   Check https://github.com/VirtusLab/akka-serialization-helper/issues/141 for details about such false-positives.
  */
 case class CodecRegistrationCheckerOptions(
     var directClassDescendantsCacheFile: File = null,
-    var oldParentChildFQCNPairs: Seq[ParentChildFQCNPair] = null)
+    var oldParentChildFQCNPairs: Seq[ParentChildFQCNPair] = null,
+    var sourceCodeDirectoryToCheck: String = null)
 
 case class ParentChildFQCNPair(parentFQCN: String, childFQCN: String)
diff --git a/...iler-plugin/src/main/scala/org/virtuslab/ash/SerializerCheckCompilerPluginComponent.scala b/...iler-plugin/src/main/scala/org/virtuslab/ash/SerializerCheckCompilerPluginComponent.scala
@@ -13,6 +13,8 @@ import scala.tools.nsc.Global
 import scala.tools.nsc.Phase
 import scala.tools.nsc.plugins.PluginComponent
 
+import better.files._
+
 import org.virtuslab.ash.CodecRegistrationCheckerCompilerPlugin.classSweepPhaseName
 import org.virtuslab.ash.CodecRegistrationCheckerCompilerPlugin.serializabilityTraitType
 import org.virtuslab.ash.CodecRegistrationCheckerCompilerPlugin.serializerCheckPhaseName
@@ -23,9 +25,12 @@ class SerializerCheckCompilerPluginComponent(
     options: CodecRegistrationCheckerOptions,
     override val global: Global)
     extends PluginComponent {
+
   import global._
+
   override val phaseName: String = serializerCheckPhaseName
   override val runsAfter: List[String] = List(classSweepPhaseName)
+
   override def description: String =
     s"Checks marked serializer for references to classes found in $serializerCheckPhaseName"
 
@@ -40,34 +45,7 @@ class SerializerCheckCompilerPluginComponent(
     new StdPhase(prev) {
       override def apply(unit: global.CompilationUnit): Unit = {
         if (typesNotDumped) {
-          val raf = new RandomAccessFile(options.directClassDescendantsCacheFile, "rw")
-          try {
-            val channel = raf.getChannel
-            val lock = channel.lock()
-            try {
-              val buffer = ByteBuffer.allocate(channel.size().toInt)
-              channel.read(buffer)
-
-              val parentChildFQCNPairsFromCacheFile =
-                CodecRegistrationCheckerCompilerPlugin.parseCacheFile(buffer.rewind()).toSet
-              val outParentChildFQCNPairs =
-                ((parentChildFQCNPairsFromCacheFile -- classSweepFQCNPairsToUpdate) |
-                classSweepFoundFQCNPairs).toList
-
-              val outData: String =
-                outParentChildFQCNPairs.map(pair => pair.parentFQCN + "," + pair.childFQCN).sorted.mkString("\n")
-              channel.truncate(0)
-              channel.write(ByteBuffer.wrap(outData.getBytes(StandardCharsets.UTF_8)))
-
-              typeNamesToCheck ++= outParentChildFQCNPairs.groupBy(_.parentFQCN)
-              typesNotDumped = false
-            } finally {
-              lock.close()
-            }
-
-          } finally {
-            raf.close()
-          }
+          dumpTypesIntoCacheFile()
         }
 
         unit.body
@@ -141,16 +119,22 @@ class SerializerCheckCompilerPluginComponent(
         }
 
         val fullyQualifiedClassNamesFromFoundTypes = collectTypeArgs(foundTypes.toSet).map(_.typeSymbol.fullName)
-
-        val missingFullyQualifiedClassNames =
+        val possibleMissingFullyQualifiedClassNames =
           typeNamesToCheck(fqcn).map(_.childFQCN).filterNot(fullyQualifiedClassNamesFromFoundTypes)
-        if (missingFullyQualifiedClassNames.nonEmpty) {
-          reporter.error(
-            serializerImplDef.pos,
-            s"""No codecs for ${missingFullyQualifiedClassNames
-              .mkString(", ")} are registered in class annotated with @$serializerType.
-               |This will lead to a missing codec for Akka serialization in the runtime.
-               |Current filtering regex: $filterRegex""".stripMargin)
+
+        if (possibleMissingFullyQualifiedClassNames.nonEmpty) {
+          val actuallyMissingFullyQualifiedClassNames = collectMissingClassNames(
+            possibleMissingFullyQualifiedClassNames)
+          if (actuallyMissingFullyQualifiedClassNames.nonEmpty) {
+            reporter.error(
+              serializerImplDef.pos,
+              s"""No codecs for ${actuallyMissingFullyQualifiedClassNames
+                .mkString(", ")} are registered in class annotated with @$serializerType.
+                 |This will lead to a missing codec for Akka serialization in the runtime.
+                 |Current filtering regex: $filterRegex""".stripMargin)
+          } else {
+            removeOutdatedTypesFromCacheFile(possibleMissingFullyQualifiedClassNames)
+          }
         }
       }
 
@@ -176,6 +160,67 @@ class SerializerCheckCompilerPluginComponent(
             None
         }
       }
+
+      private def collectMissingClassNames(fullyQualifiedClassNames: List[String]): List[String] = {
+        val sourceCodeDir = File(options.sourceCodeDirectoryToCheck)
+        val sourceCodeFilesAsStrings =
+          (for (file <- sourceCodeDir.collectChildren(_.name.endsWith(".scala"))) yield file.contentAsString).toList
+
+        def typeIsDefinedInScalaFiles(fqcn: String): Boolean = {
+          val indexOfLastDotInFQCN = fqcn.lastIndexOf('.')
+          val packageName = fqcn.substring(0, indexOfLastDotInFQCN)
+          val typeName = fqcn.substring(indexOfLastDotInFQCN + 1)
+          sourceCodeFilesAsStrings.exists(fileAsString => {
+            fileAsString.startsWith(s"package $packageName") &&
+            (fileAsString.contains(s"class $typeName") || fileAsString.contains(s"trait $typeName") || fileAsString
+              .contains(s"object $typeName"))
+          })
+        }
+
+        fullyQualifiedClassNames.filter(typeIsDefinedInScalaFiles)
+      }
+
+      private def dumpTypesIntoCacheFile(): Unit = {
+        val raf = new RandomAccessFile(options.directClassDescendantsCacheFile, "rw")
+        try {
+          val channel = raf.getChannel
+          val lock = channel.lock()
+          try {
+            val buffer = ByteBuffer.allocate(channel.size().toInt)
+            channel.read(buffer)
+            val parentChildFQCNPairsFromCacheFile =
+              CodecRegistrationCheckerCompilerPlugin.parseCacheFile(buffer.rewind()).toSet
+            val outParentChildFQCNPairs =
+              ((parentChildFQCNPairsFromCacheFile -- classSweepFQCNPairsToUpdate) |
+              classSweepFoundFQCNPairs).toList
+            val outData: String =
+              outParentChildFQCNPairs.map(pair => pair.parentFQCN + "," + pair.childFQCN).sorted.mkString("\n")
+            channel.truncate(0)
+            channel.write(ByteBuffer.wrap(outData.getBytes(StandardCharsets.UTF_8)))
+
+            typeNamesToCheck ++= outParentChildFQCNPairs.groupBy(_.parentFQCN)
+            typesNotDumped = false
+          } finally {
+            lock.close()
+          }
+        } finally {
+          raf.close()
+        }
+      }
+
+      private def removeOutdatedTypesFromCacheFile(typeNamesToRemove: List[String]): Unit = {
+        val cacheFile = options.directClassDescendantsCacheFile.toScala
+        val contentWithoutOutdatedTypes =
+          cacheFile.contentAsString
+            .split("\n")
+            .toList
+            .filterNot(line => typeNamesToRemove.exists(typeName => line.contains(typeName)))
+            .mkString("\n")
+            .stripMargin
+        cacheFile.clear()
+        cacheFile.write(contentWithoutOutdatedTypes)
+      }
+
     }
   }
 }