Accumulate exp warnings with logger (#2376)

* NIT: Refactor DirectivePreprocessor, contain common arguments in class' scope * Add accumulating logger warnings for experimental features * Make logging never repeat the same experimental warning
VirtusLab · Sep 19, 2023 · 16f321d · 16f321d
1 parent faa9e3c
commit 16f321d
Show file tree

Hide file tree

Showing 15 changed files with 255 additions and 129 deletions.
diff --git a/modules/build/src/main/scala/scala/build/CrossSources.scala b/modules/build/src/main/scala/scala/build/CrossSources.scala
@@ -235,6 +235,8 @@ object CrossSources {
           distinctSources
         }
 
+    logger.flushExperimentalWarnings
+
     val scopedRequirements       = preprocessedSources.flatMap(_.scopedRequirements)
     val scopedRequirementsByRoot = scopedRequirements.groupBy(_.path.root)
     def baseReqs(path: ScopePath): BuildRequirements = {

diff --git a/modules/build/src/main/scala/scala/build/PersistentDiagnosticLogger.scala b/modules/build/src/main/scala/scala/build/PersistentDiagnosticLogger.scala
@@ -1,12 +1,13 @@
 package scala.build
 
 import bloop.rifle.BloopRifleLogger
-import org.scalajs.logging.{Logger => ScalaJsLogger}
+import org.scalajs.logging.Logger as ScalaJsLogger
 
 import java.io.PrintStream
 
 import scala.build.errors.{BuildException, Diagnostic}
-import scala.scalanative.{build => sn}
+import scala.build.internals.FeatureType
+import scala.scalanative.build as sn
 
 class PersistentDiagnosticLogger(parent: Logger) extends Logger {
   private val diagBuilder = List.newBuilder[Diagnostic]
@@ -39,4 +40,9 @@ class PersistentDiagnosticLogger(parent: Logger) extends Logger {
   def compilerOutputStream: PrintStream = parent.compilerOutputStream
 
   def verbosity: Int = parent.verbosity
+
+  def experimentalWarning(featureName: String, featureType: FeatureType): Unit =
+    parent.experimentalWarning(featureName, featureType)
+
+  def flushExperimentalWarnings: Unit = parent.flushExperimentalWarnings
 }
diff --git a/modules/build/src/main/scala/scala/build/internal/util/WarningMessages.scala b/modules/build/src/main/scala/scala/build/internal/util/WarningMessages.scala
@@ -2,27 +2,46 @@ package scala.build.internal.util
 
 import scala.build.input.ScalaCliInvokeData
 import scala.build.internal.Constants
+import scala.build.internals.FeatureType
 import scala.build.preprocessing.directives.{DirectiveHandler, ScopedDirective}
 import scala.cli.commands.{SpecificationLevel, tags}
 import scala.cli.config.Key
 
 object WarningMessages {
   private val scalaCliGithubUrl = s"https://github.com/${Constants.ghOrg}/${Constants.ghName}"
-  private def experimentalFeatureUsed(featureName: String): String =
-    s"""$featureName is experimental.
-       |Please bear in mind that non-ideal user experience should be expected.
-       |If you encounter any bugs or have feedback to share, make sure to reach out to the maintenance team at $scalaCliGithubUrl""".stripMargin
-  def experimentalDirectiveUsed(name: String): String =
-    experimentalFeatureUsed(s"The `$name` directive")
 
-  def experimentalSubcommandUsed(name: String): String =
-    experimentalFeatureUsed(s"The `$name` sub-command")
+  private val experimentalNote =
+    s"""Please bear in mind that non-ideal user experience should be expected.
+       |If you encounter any bugs or have feedback to share, make sure to reach out to the maintenance team at $scalaCliGithubUrl""".stripMargin
+  def experimentalFeaturesUsed(namesAndFeatureTypes: Seq[(String, FeatureType)]): String = {
+    val message = namesAndFeatureTypes match {
+      case Seq((name, featureType)) => s"The `$name` $featureType is experimental"
+      case namesAndTypes =>
+        val nl                   = System.lineSeparator()
+        val distinctFeatureTypes = namesAndTypes.map(_._2).distinct
+        val (bulletPointList, featureNameToPrint) = if (distinctFeatureTypes.size == 1)
+          (
+            namesAndTypes.map((name, fType) => s" - `$name`")
+              .mkString(nl),
+            s"${distinctFeatureTypes.head}s" // plural form
+          )
+        else
+          (
+            namesAndTypes.map((name, fType) => s" - `$name` $fType")
+              .mkString(nl),
+            "features"
+          )
 
-  def experimentalOptionUsed(name: String): String =
-    experimentalFeatureUsed(s"The `$name` option")
+        s"""Some utilized $featureNameToPrint are marked as experimental:
+           |$bulletPointList""".stripMargin
+    }
+    s"""$message
+       |$experimentalNote""".stripMargin
+  }
 
-  def experimentalConfigKeyUsed(name: String): String =
-    experimentalFeatureUsed(s"The `$name` configuration key")
+  def experimentalSubcommandWarning(name: String): String =
+    s"""The `$name` sub-command is experimental.
+       |$experimentalNote""".stripMargin
 
   def rawValueNotWrittenToPublishFile(
     rawValue: String,

diff --git a/modules/build/src/main/scala/scala/build/preprocessing/DirectivesPreprocessor.scala b/modules/build/src/main/scala/scala/build/preprocessing/DirectivesPreprocessor.scala
@@ -15,7 +15,7 @@ import scala.build.errors.{
 }
 import scala.build.input.ScalaCliInvokeData
 import scala.build.internal.util.WarningMessages
-import scala.build.internal.util.WarningMessages.experimentalDirectiveUsed
+import scala.build.internals.FeatureType
 import scala.build.options.{
   BuildOptions,
   BuildRequirements,
@@ -27,56 +27,24 @@ import scala.build.preprocessing.directives.DirectivesPreprocessingUtils.*
 import scala.build.preprocessing.directives.PartiallyProcessedDirectives.*
 import scala.build.preprocessing.directives.*
 
-object DirectivesPreprocessor {
-  def preprocess(
-    content: String,
-    path: Either[String, os.Path],
-    cwd: ScopePath,
-    logger: Logger,
-    allowRestrictedFeatures: Boolean,
-    suppressWarningOptions: SuppressWarningOptions,
-    maybeRecoverOnError: BuildException => Option[BuildException]
-  )(using ScalaCliInvokeData): Either[BuildException, PreprocessedDirectives] = either {
-    val directives = value {
-      ExtractedDirectives.from(content.toCharArray, path, logger, maybeRecoverOnError)
-    }
-    value {
-      preprocess(
-        directives,
-        path,
-        cwd,
-        logger,
-        allowRestrictedFeatures,
-        suppressWarningOptions,
-        maybeRecoverOnError
-      )
-    }
-  }
+case class DirectivesPreprocessor(
+  path: Either[String, os.Path],
+  cwd: ScopePath,
+  logger: Logger,
+  allowRestrictedFeatures: Boolean,
+  suppressWarningOptions: SuppressWarningOptions,
+  maybeRecoverOnError: BuildException => Option[BuildException]
+)(
+  using ScalaCliInvokeData
+) {
+  def preprocess(content: String): Either[BuildException, PreprocessedDirectives] = for {
+    directives <- ExtractedDirectives.from(content.toCharArray, path, logger, maybeRecoverOnError)
+    res        <- preprocess(directives)
+  } yield res
 
-  def preprocess(
-    extractedDirectives: ExtractedDirectives,
-    path: Either[String, os.Path],
-    cwd: ScopePath,
-    logger: Logger,
-    allowRestrictedFeatures: Boolean,
-    suppressWarningOptions: SuppressWarningOptions,
-    maybeRecoverOnError: BuildException => Option[BuildException]
-  )(using ScalaCliInvokeData): Either[BuildException, PreprocessedDirectives] = either {
+  def preprocess(extractedDirectives: ExtractedDirectives)
+    : Either[BuildException, PreprocessedDirectives] = either {
     val ExtractedDirectives(directives, directivesPositions) = extractedDirectives
-    def preprocessWithDirectiveHandlers[T: ConfigMonoid](
-      remainingDirectives: Seq[StrictDirective],
-      directiveHandlers: Seq[DirectiveHandler[T]]
-    ): Either[BuildException, PartiallyProcessedDirectives[T]] =
-      applyDirectiveHandlers(
-        remainingDirectives,
-        directiveHandlers,
-        path,
-        cwd,
-        logger,
-        allowRestrictedFeatures,
-        suppressWarningOptions,
-        maybeRecoverOnError
-      )
 
     val (
       buildOptionsWithoutRequirements: PartiallyProcessedDirectives[BuildOptions],
@@ -88,16 +56,16 @@ object DirectivesPreprocessor {
     ) = value {
       for {
         regularUsingDirectives: PartiallyProcessedDirectives[BuildOptions] <-
-          preprocessWithDirectiveHandlers(directives, usingDirectiveHandlers)
+          applyDirectiveHandlers(directives, usingDirectiveHandlers)
         usingDirectivesWithRequirements: PartiallyProcessedDirectives[
           List[WithBuildRequirements[BuildOptions]]
         ] <-
-          preprocessWithDirectiveHandlers(
+          applyDirectiveHandlers(
             regularUsingDirectives.unused,
             usingDirectiveWithReqsHandlers
           )
         targetDirectives: PartiallyProcessedDirectives[BuildRequirements] <-
-          preprocessWithDirectiveHandlers(
+          applyDirectiveHandlers(
             usingDirectivesWithRequirements.unused,
             requireDirectiveHandlers
           )
@@ -142,14 +110,8 @@ object DirectivesPreprocessor {
 
   private def applyDirectiveHandlers[T: ConfigMonoid](
     directives: Seq[StrictDirective],
-    handlers: Seq[DirectiveHandler[T]],
-    path: Either[String, os.Path],
-    cwd: ScopePath,
-    logger: Logger,
-    allowRestrictedFeatures: Boolean,
-    suppressWarningOptions: SuppressWarningOptions,
-    maybeRecoverOnError: BuildException => Option[BuildException] = e => Some(e)
-  )(using ScalaCliInvokeData): Either[BuildException, PartiallyProcessedDirectives[T]] = {
+    handlers: Seq[DirectiveHandler[T]]
+  ): Either[BuildException, PartiallyProcessedDirectives[T]] = {
     val configMonoidInstance = implicitly[ConfigMonoid[T]]
     val shouldSuppressExperimentalFeatures =
       suppressWarningOptions.suppressExperimentalFeatureWarning.getOrElse(false)
@@ -161,11 +123,12 @@ object DirectivesPreprocessor {
       if !allowRestrictedFeatures && (handler.isRestricted || handler.isExperimental) then
         Left(DirectiveErrors(
           ::(WarningMessages.powerDirectiveUsedInSip(scopedDirective, handler), Nil),
+          // TODO: use positions from ExtractedDirectives to get the full directive underlined
           DirectiveUtil.positions(scopedDirective.directive.values, path)
         ))
       else
         if handler.isExperimental && !shouldSuppressExperimentalFeatures then
-          logger.message(experimentalDirectiveUsed(scopedDirective.directive.toString))
+          logger.experimentalWarning(scopedDirective.directive.toString, FeatureType.Directive)
         handler.handleValues(scopedDirective, logger)
 
     val handlersMap = handlers

diff --git a/modules/build/src/main/scala/scala/build/preprocessing/JavaPreprocessor.scala b/modules/build/src/main/scala/scala/build/preprocessing/JavaPreprocessor.scala
@@ -47,15 +47,15 @@ final case class JavaPreprocessor(
           val content: String = value(PreprocessingUtil.maybeRead(j.path))
           val scopePath       = ScopePath.fromPath(j.path)
           val preprocessedDirectives: PreprocessedDirectives = value {
-            DirectivesPreprocessor.preprocess(
-              content,
+            DirectivesPreprocessor(
               Right(j.path),
               scopePath,
               logger,
               allowRestrictedFeatures,
               suppressWarningOptions,
               maybeRecoverOnError
             )
+              .preprocess(content)
           }
           Seq(PreprocessedSource.OnDisk(
             path = j.path,
@@ -89,14 +89,15 @@ final case class JavaPreprocessor(
             else v.subPath
           val content = new String(v.content, StandardCharsets.UTF_8)
           val preprocessedDirectives: PreprocessedDirectives = value {
-            DirectivesPreprocessor.preprocess(
-              content,
+            DirectivesPreprocessor(
               Left(relPath.toString),
               v.scopePath,
               logger,
               allowRestrictedFeatures,
               suppressWarningOptions,
               maybeRecoverOnError
+            ).preprocess(
+              content
             )
           }
           val s = PreprocessedSource.InMemory(

diff --git a/modules/build/src/main/scala/scala/build/preprocessing/ScalaPreprocessor.scala b/modules/build/src/main/scala/scala/build/preprocessing/ScalaPreprocessor.scala
@@ -221,14 +221,15 @@ case object ScalaPreprocessor extends Preprocessor {
   )(using ScalaCliInvokeData): Either[BuildException, Option[ProcessingOutput]] = either {
     val (content0, isSheBang) = SheBang.ignoreSheBangLines(content)
     val preprocessedDirectives: PreprocessedDirectives =
-      value(DirectivesPreprocessor.preprocess(
-        extractedDirectives,
+      value(DirectivesPreprocessor(
         path,
         scopeRoot,
         logger,
         allowRestrictedFeatures,
         suppressWarningOptions,
         maybeRecoverOnError
+      ).preprocess(
+        extractedDirectives
       ))
 
     if (preprocessedDirectives.isEmpty) None

diff --git a/modules/build/src/test/scala/scala/build/tests/BuildProjectTests.scala b/modules/build/src/test/scala/scala/build/tests/BuildProjectTests.scala
@@ -3,19 +3,19 @@ package scala.build.tests
 import bloop.rifle.BloopRifleLogger
 import com.eed3si9n.expecty.Expecty.expect
 import coursier.cache.CacheLogger
-import org.scalajs.logging.{Logger => ScalaJsLogger, NullLogger}
+import org.scalajs.logging.{NullLogger, Logger as ScalaJsLogger}
 
 import java.io.PrintStream
-
-import scala.build.Ops._
+import scala.build.Ops.*
 import scala.build.errors.{BuildException, Diagnostic, Severity}
 import scala.build.input.Inputs
+import scala.build.internals.FeatureType
 import scala.build.options.{
   BuildOptions,
   InternalOptions,
   JavaOptions,
-  ScalacOpt,
   ScalaOptions,
+  ScalacOpt,
   Scope,
   ShadowingSeq
 }
@@ -61,6 +61,8 @@ class BuildProjectTests extends munit.FunSuite {
 
     override def verbosity = ???
 
+    override def experimentalWarning(featureName: String, featureType: FeatureType): Unit = ???
+    override def flushExperimentalWarnings: Unit                                          = ???
   }
 
   val bloopJavaPath = Position.Bloop("/home/empty/jvm/8/")

diff --git a/modules/build/src/test/scala/scala/build/tests/TestLogger.scala b/modules/build/src/test/scala/scala/build/tests/TestLogger.scala
@@ -3,12 +3,13 @@ package scala.build.tests
 import bloop.rifle.BloopRifleLogger
 import coursier.cache.CacheLogger
 import coursier.cache.loggers.{FallbackRefreshDisplay, RefreshLogger}
-import org.scalajs.logging.{Logger => ScalaJsLogger, NullLogger}
+import org.scalajs.logging.{NullLogger, Logger as ScalaJsLogger}
 
 import scala.build.errors.BuildException
 import scala.build.Logger
-import scala.scalanative.{build => sn}
+import scala.scalanative.build as sn
 import scala.build.errors.Diagnostic
+import scala.build.internals.FeatureType
 
 case class TestLogger(info: Boolean = true, debug: Boolean = false) extends Logger {
 
@@ -83,4 +84,9 @@ case class TestLogger(info: Boolean = true, debug: Boolean = false) extends Logg
     if (debug) 2
     else if (info) 0
     else -1
+
+  override def experimentalWarning(featureName: String, featureType: FeatureType): Unit =
+    System.err.println(s"Experimental $featureType `$featureName` used")
+
+  override def flushExperimentalWarnings: Unit = ()
 }
diff --git a/modules/cli/src/main/scala/scala/cli/commands/RestrictedCommandsParser.scala b/modules/cli/src/main/scala/scala/cli/commands/RestrictedCommandsParser.scala
@@ -9,6 +9,7 @@ import caseapp.core.{Arg, Error}
 import scala.build.Logger
 import scala.build.input.ScalaCliInvokeData
 import scala.build.internal.util.WarningMessages
+import scala.build.internals.FeatureType
 import scala.cli.ScalaCli
 import scala.cli.util.ArgHelpers.*
 
@@ -54,7 +55,7 @@ object RestrictedCommandsParser {
             ))
           case (r @ Right(Some(_, arg: Arg, _)), passedOption :: _)
               if arg.isExperimental && !shouldSuppressExperimentalWarnings =>
-            logger.message(WarningMessages.experimentalOptionUsed(passedOption))
+            logger.experimentalWarning(passedOption, FeatureType.Option)
             r
           case (other, _) =>
             other

diff --git a/modules/cli/src/main/scala/scala/cli/commands/ScalaCommand.scala b/modules/cli/src/main/scala/scala/cli/commands/ScalaCommand.scala
@@ -19,6 +19,7 @@ import scala.build.errors.BuildException
 import scala.build.input.{ScalaCliInvokeData, SubCommand}
 import scala.build.internal.util.WarningMessages
 import scala.build.internal.{Constants, Runner}
+import scala.build.internals.FeatureType
 import scala.build.options.{BuildOptions, ScalacOpt, Scope}
 import scala.build.{Artifacts, Directories, Logger, Positioned, ReplArtifacts}
 import scala.cli.commands.default.LegacyScalaOptions
@@ -288,12 +289,12 @@ abstract class ScalaCommand[T <: HasGlobalOptions](implicit myParser: Parser[T],
             message =
               s"""${hm.message}
                  |
-                 |${WarningMessages.experimentalSubcommandUsed(name)}""".stripMargin,
+                 |${WarningMessages.experimentalSubcommandWarning(name)}""".stripMargin,
             detailedMessage =
               if hm.detailedMessage.nonEmpty then
                 s"""${hm.detailedMessage}
                    |
-                   |${WarningMessages.experimentalSubcommandUsed(name)}""".stripMargin
+                   |${WarningMessages.experimentalSubcommandWarning(name)}""".stripMargin
               else hm.detailedMessage
           )
         )
@@ -354,13 +355,15 @@ abstract class ScalaCommand[T <: HasGlobalOptions](implicit myParser: Parser[T],
       ))
       sys.exit(1)
     else if isExperimental && !shouldSuppressExperimentalFeatureWarnings then
-      logger.message(WarningMessages.experimentalSubcommandUsed(name))
+      logger.experimentalWarning(name, FeatureType.Subcommand)
+
     maybePrintWarnings(options)
     maybePrintGroupHelp(options)
     buildOptions(options).foreach { bo =>
       maybePrintSimpleScalacOutput(options, bo)
       maybePrintToolsHelp(options, bo)
     }
+    logger.flushExperimentalWarnings
     runCommand(options, remainingArgs, options.global.logging.logger)
   }
 }