bugfix: Reload classloader when republishing rules (#7975)

Author: tgodzik

metals/src/main/scala/scala/meta/internal/metals/ScalafixProvider.scala

@@ -66,6 +66,7 @@ case class ScalafixProvider(
   def runRulesOrPrompt(
       file: AbsolutePath,
       rules: List[String],
+      isRepublished: Boolean = false,
   ): Future[List[l.TextEdit]] = {
     lazy val generatedRules =
       ScalafixLlmRuleProvider.generatedRules(workspace).keySet
@@ -85,14 +86,15 @@ case class ScalafixProvider(
         )
 
       }
-      runRules(file, rulesToRun, additionalDeps)
+      runRules(file, rulesToRun, additionalDeps, isRepublished = isRepublished)
     }
   }
 
   def runRuleFromDep(
       file: AbsolutePath,
       ruleName: String,
       ruleDep: Dependency,
+      isRepublished: Boolean = false,
   ): Future[List[l.TextEdit]] = {
     val scalaTarget = buildTargets.inverseSources(file)
     scalaTarget
@@ -106,6 +108,7 @@ case class ScalafixProvider(
           scalaTarget,
           List(ruleName),
           additionalDeps,
+          isRepublished = isRepublished,
         )
       }
       .getOrElse(Future.successful(Nil))
@@ -131,12 +134,15 @@ case class ScalafixProvider(
       additionalDeps: Map[String, Dependency] = Map.empty,
       retried: Boolean = false,
       silent: Boolean = false,
+      isRepublished: Boolean = false,
   ): Future[List[l.TextEdit]] = {
     val fromDisk = file.toInput
     val inBuffers = file.toInputFromBuffers(buffers)
 
     additionalDeps.foreach { case (ruleName, dep) =>
-      scribe.debug(s"Running rule $ruleName with dep $dep")
+      scribe.debug(
+        s"Running rule $ruleName with dep $dep on $file, the rule is republished: $isRepublished"
+      )
     }
 
     compilations
@@ -150,6 +156,7 @@ case class ScalafixProvider(
             retried || isUnsaved(inBuffers.text, fromDisk.text),
             rules,
             additionalDeps = additionalDeps,
+            isRepublished = isRepublished,
           )
 
         scalafixEvaluation
@@ -416,6 +423,7 @@ case class ScalafixProvider(
       suggestConfigAmend: Boolean = true,
       shouldRetry: Boolean = true,
       additionalDeps: Map[String, Dependency] = Map.empty,
+      isRepublished: Boolean = false,
   ): Future[ScalafixEvaluation] = {
     val isScala3 = ScalaVersions.isScala3Version(scalaTarget.scalaVersion)
     val isSource3 = scalaTarget.scalac.getOptions().contains("-Xsource:3")
@@ -468,6 +476,7 @@ case class ScalafixProvider(
       urlClassLoaderWithExternalRule <- getRuleClassLoader(
         scalafixRulesKey,
         api.getClass.getClassLoader,
+        isRepublished,
       )
     } yield {
       val scalacOptions = {
@@ -715,7 +724,9 @@ case class ScalafixProvider(
   private def getRuleClassLoader(
       scalfixRulesKey: ScalafixRulesClasspathKey,
       scalafixClassLoader: ClassLoader,
+      isRepublished: Boolean,
   ): Future[URLClassLoader] = Future {
+    if (isRepublished) rulesClassloaderCache.remove(scalfixRulesKey)
     rulesClassloaderCache.getOrElseUpdate(
       scalfixRulesKey, {
         workDoneProgress.trackBlocking(
@@ -738,6 +749,7 @@ case class ScalafixProvider(
             Embedded.rulesClasspath(
               rulesDependencies.toList ++ organizeImportRule
             )
+          scribe.debug(s"Classpath: $paths")
           val classloader = Embedded.toClassLoader(
             Classpath(paths.map(AbsolutePath(_))),
             scalafixClassLoader,
@@ -773,6 +785,7 @@ case class ScalafixProvider(
       rules: List[String],
       additionalRules: (ScalaVersion) => Map[String, Dependency] = _ =>
         Map.empty,
+      isRepublished: Boolean = false,
   ): Future[List[l.TextEdit]] = {
     val result = for {
       buildId <- buildTargets.inverseSources(file)
@@ -783,6 +796,7 @@ case class ScalafixProvider(
         target,
         rules,
         additionalRules(target.scalaVersion),
+        isRepublished = isRepublished,
       )
     }
     result.getOrElse(Future.successful(Nil))

metals/src/main/scala/scala/meta/internal/metals/mcp/ScalafixLlmRuleProvider.scala

@@ -227,7 +227,11 @@ class ScalafixLlmRuleProvider(
         case Left(error) => Future.successful(Left(error))
         case Right(publishedRuleName) =>
           compilations.compileTargets(allTargets.map(_.id)).flatMap { _ =>
-            runScalafixRuleForAllTargets(publishedRuleName, files)
+            runScalafixRuleForAllTargets(
+              publishedRuleName,
+              files,
+              isRepublished = true,
+            )
               .map { scalafixRunResult =>
                 if (scalafixRunResult.changeWasApplied) {
                   Right(scalafixRunResult)
@@ -251,6 +255,7 @@ class ScalafixLlmRuleProvider(
   def runScalafixRuleForAllTargets(
       ruleName: String,
       runOnSources: List[AbsolutePath] = Nil,
+      isRepublished: Boolean = false,
   ): Future[ScalafixRunResult] = {
     val allTargets =
       buildTargets.allBuildTargetIds.map(buildTargets.scalaTarget).collect {
@@ -272,6 +277,7 @@ class ScalafixLlmRuleProvider(
         ruleName,
         sources,
         dependency,
+        isRepublished,
       )
     }
     Future
@@ -311,14 +317,24 @@ class ScalafixLlmRuleProvider(
       ruleName: String,
       sources: List[AbsolutePath],
       publishedRule: Option[Dependency],
+      isRepublished: Boolean = false,
   ): Future[ScalafixRunResult] = {
     val all =
       sources.filter(file => file.filename.isScala).map { file =>
         val ruleRun = publishedRule match {
           case Some(dependency) =>
-            scalafixProvider.runRuleFromDep(file, ruleName, dependency)
+            scalafixProvider.runRuleFromDep(
+              file,
+              ruleName,
+              dependency,
+              isRepublished,
+            )
           case None =>
-            scalafixProvider.runRulesOrPrompt(file, List(ruleName))
+            scalafixProvider.runRulesOrPrompt(
+              file,
+              List(ruleName),
+              isRepublished,
+            )
         }
         ruleRun
           .map { edits =>

tests/unit/src/test/scala/tests/mcp/McpServerLspSuite.scala

@@ -1,5 +1,7 @@
 package tests.mcp
 
+import scala.concurrent.Future
+
 import scala.meta.internal.metals.mcp.CursorEditor
 import scala.meta.internal.metals.mcp.McpConfig
 import scala.meta.internal.metals.mcp.McpMessages
@@ -113,6 +115,8 @@ class McpServerLspSuite extends BaseLspSuite("mcp-server") with McpTestUtils {
       input: String,
       expectedMessage: String,
       expectedRules: String,
+      ruleName: String,
+      reverseRule: Option[String => String] = None,
   )(implicit loc: Location): Unit = {
     test(testName) {
       cleanWorkspace()
@@ -173,13 +177,34 @@ class McpServerLspSuite extends BaseLspSuite("mcp-server") with McpTestUtils {
             .get,
           initial,
         )
-        _ <- client.runScalafixRule("ReplaceJohnWithJohnatan")
+        _ <- client.runScalafixRule(ruleName)
         _ = assertNoDiff(
           server.buffers
             .get(workspace.resolve("a/src/main/scala/com/example/Hello.scala"))
             .get,
           expectedResult,
         )
+        _ <- reverseRule match {
+          case Some(function) =>
+            for {
+              _ <- client.generateScalafixRule(
+                function(input),
+                "Replaces John with Johnatan",
+              )
+              _ = assertNoDiff(
+                server.buffers
+                  .get(
+                    workspace.resolve(
+                      "a/src/main/scala/com/example/Hello.scala"
+                    )
+                  )
+                  .get,
+                initial,
+              )
+            } yield ()
+          case None => Future.successful(())
+        }
+
         _ <- client.shutdown()
       } yield ()
     }
@@ -214,6 +239,13 @@ class McpServerLspSuite extends BaseLspSuite("mcp-server") with McpTestUtils {
        |- RemoveUnused: Removes unused imports and terms that reported by the compiler under -Wunused
        |- ReplaceJohnWithJohnatan: Replaces John with Johnatan
        |""".stripMargin,
+    "ReplaceJohnWithJohnatan",
+    Some(input =>
+      input.replace(
+        "val newValue = value.replace(\"John\", \"\\\"Johnatan\\\"\")",
+        "val newValue = value.replace(\"Johnatan\", \"\\\"John\\\"\")",
+      )
+    ),
   )
 
   checkScalafixRule(
@@ -245,6 +277,7 @@ class McpServerLspSuite extends BaseLspSuite("mcp-server") with McpTestUtils {
        |- RemoveUnused: Removes unused imports and terms that reported by the compiler under -Wunused
        |- Replacer: Replaces John with Johnatan
        |""".stripMargin,
+    "Replacer",
   )
 
   test("list-scalafix-rules") {