Applied review suggestions

szymon-rd · szymon-rd · commit f146ca714555 · 2024-03-13T17:35:48.000+01:00
diff --git a/compiler/src/dotty/tools/backend/jvm/BackendUtils.scala b/compiler/src/dotty/tools/backend/jvm/BackendUtils.scala
@@ -20,7 +20,7 @@ class BackendUtils(val postProcessor: PostProcessor) {
   import bTypes.*
   import coreBTypes.jliLambdaMetaFactoryAltMetafactoryHandle
 
-  lazy val classfileVersion: Int = BackendUtils.classfileVersionMap(compilerSettings.target)
+  lazy val classfileVersion: Int = BackendUtils.classfileVersionMap(compilerSettings.target.toInt)
 
   lazy val extraProc: Int = {
     import GenBCodeOps.addFlagIf
@@ -169,21 +169,21 @@ class BackendUtils(val postProcessor: PostProcessor) {
 }
 
 object BackendUtils {
-  lazy val classfileVersionMap = Map(
-    "8" -> asm.Opcodes.V1_8,
-    "9" -> asm.Opcodes.V9,
-    "10" -> asm.Opcodes.V10,
-    "11" -> asm.Opcodes.V11,
-    "12" -> asm.Opcodes.V12,
-    "13" -> asm.Opcodes.V13,
-    "14" -> asm.Opcodes.V14,
-    "15" -> asm.Opcodes.V15,
-    "16" -> asm.Opcodes.V16,
-    "17" -> asm.Opcodes.V17,
-    "18" -> asm.Opcodes.V18,
-    "19" -> asm.Opcodes.V19,
-    "20" -> asm.Opcodes.V20,
-    "21" -> asm.Opcodes.V21,
-    "22" -> asm.Opcodes.V22,
+  lazy val classfileVersionMap: Map[Int, Int] = Map(
+    8 -> asm.Opcodes.V1_8,
+    9 -> asm.Opcodes.V9,
+    10 -> asm.Opcodes.V10,
+    11 -> asm.Opcodes.V11,
+    12 -> asm.Opcodes.V12,
+    13 -> asm.Opcodes.V13,
+    14 -> asm.Opcodes.V14,
+    15 -> asm.Opcodes.V15,
+    16 -> asm.Opcodes.V16,
+    17 -> asm.Opcodes.V17,
+    18 -> asm.Opcodes.V18,
+    19 -> asm.Opcodes.V19,
+    20 -> asm.Opcodes.V20,
+    21 -> asm.Opcodes.V21,
+    22 -> asm.Opcodes.V22,
   )
 }
diff --git a/compiler/src/dotty/tools/dotc/config/ScalaSettings.scala b/compiler/src/dotty/tools/dotc/config/ScalaSettings.scala
@@ -2,9 +2,8 @@ package dotty.tools.dotc
 package config
 
 import scala.language.unsafeNulls
-import dotty.tools.backend.jvm.BackendUtils.classfileVersionMap
 import dotty.tools.dotc.config.PathResolver.Defaults
-import dotty.tools.dotc.config.Settings.{Setting, SettingGroup}
+import dotty.tools.dotc.config.Settings.{Setting, SettingGroup, SettingCategory}
 import dotty.tools.dotc.config.SourceVersion
 import dotty.tools.dotc.core.Contexts.*
 import dotty.tools.dotc.rewrites.Rewrites
@@ -16,62 +15,32 @@ import scala.util.chaining.*
 
 import java.util.zip.Deflater
 
-object ScalaSettingCategories:
-  val RootSetting = ""
-  val WarningSetting = "W"
-  val ForkSetting = "Y"
-  val AdvancedSetting = "X"
-  val VerboseSetting = "V"
-
-object ScalaSettingsProperties:
-
-  private lazy val minTargetVersion = classfileVersionMap.keysIterator.map(_.toInt).min
-  private lazy val maxTargetVersion = classfileVersionMap.keysIterator.map(_.toInt).max
-
-  def supportedTargetVersions: List[String] =
-    (minTargetVersion to maxTargetVersion).toList.map(_.toString)
-
-  def supportedReleaseVersions: List[String] =
-    if scala.util.Properties.isJavaAtLeast("9") then
-      val jdkVersion = JDK9Reflectors.runtimeVersionMajor(JDK9Reflectors.runtimeVersion()).intValue()
-      val maxVersion = Math.min(jdkVersion, maxTargetVersion)
-      (minTargetVersion to maxVersion).toList.map(_.toString)
-    else List(minTargetVersion).map(_.toString)
-
-  def supportedScalaReleaseVersions: List[String] =
-    ScalaRelease.values.toList.map(_.show)
-
-  def supportedSourceVersions: List[String] =
-    SourceVersion.values.toList.map(_.toString)
-
-  def defaultClasspath: String = sys.env.getOrElse("CLASSPATH", ".")
-
-  def defaultPageWidth: Int = {
-    val defaultWidth = 80
-    val columnsVar = System.getenv("COLUMNS")
-    if columnsVar != null then columnsVar.toInt
-    else if Properties.isWin then
-      val ansiconVar = System.getenv("ANSICON") // eg. "142x32766 (142x26)"
-      if ansiconVar != null && ansiconVar.matches("[0-9]+x.*") then
-        ansiconVar.substring(0, ansiconVar.indexOf("x")).toInt
-      else defaultWidth
-    else defaultWidth
-  }
+enum ScalaSettingCategories(val prefixLetter: String) extends SettingCategory:
+  // Root settings, a category for setting that are used to configure the core compilation process
+  case RootSetting extends ScalaSettingCategories("")
+  // Warning settings, a category for settings that are used to enable and configure warnings
+  case WarningSetting extends ScalaSettingCategories("W")
+  // Fork / private settings, a category for settings that enable private or advanced features, mainly used for debugging the compiler
+  case ForkSetting extends ScalaSettingCategories("Y")
+  // Advanced settings, a category for settings that enable advanced, often unstable, features
+  case AdvancedSetting extends ScalaSettingCategories("X")
+  // Verbose settings, a category to configure the verbosity of the compiler
+  case VerboseSetting extends ScalaSettingCategories("V")
 
 object ScalaSettings extends ScalaSettings
 
 // Kept as seperate type to avoid breaking backward compatibility
 abstract class ScalaSettings extends SettingGroup, AllScalaSettings:
-  val settingsByCategory: Map[String, List[Setting[_]]] = 
+  val settingsByCategory: Map[SettingCategory, List[Setting[_]]] = 
     allSettings.groupBy(_.category)
       .view.mapValues(_.toList).toMap
       .withDefaultValue(Nil)
-  def categories: List[String] = settingsByCategory.keys.toList
-  val rootSettings: List[Setting[_]] = settingsByCategory(RootSetting)
-  val warningSettings: List[Setting[_]] = settingsByCategory(WarningSetting)
-  val forkSettings: List[Setting[_]] = settingsByCategory(ForkSetting)
-  val advancedSettings: List[Setting[_]] = settingsByCategory(AdvancedSetting)
-  val verboseSettings: List[Setting[_]] = settingsByCategory(VerboseSetting)
+  def categories: List[SettingCategory] = settingsByCategory.keys.toList.sortBy(_.prefixLetter)
+  val rootSettings: List[Setting[_]] = settingsByCategory(RootSetting).sortBy(_.name)
+  val warningSettings: List[Setting[_]] = settingsByCategory(WarningSetting).sortBy(_.name)
+  val forkSettings: List[Setting[_]] = settingsByCategory(ForkSetting).sortBy(_.name)
+  val advancedSettings: List[Setting[_]] = settingsByCategory(AdvancedSetting).sortBy(_.name)
+  val verboseSettings: List[Setting[_]] = settingsByCategory(VerboseSetting).sortBy(_.name)
   val settingsByAliases: Map[String, Setting[_]] = allSettings.flatMap(s => s.aliases.map(_ -> s)).toMap
 
   
diff --git a/compiler/src/dotty/tools/dotc/config/ScalaSettingsProperties.scala b/compiler/src/dotty/tools/dotc/config/ScalaSettingsProperties.scala
@@ -0,0 +1,41 @@
+package dotty.tools.dotc
+package config
+
+import dotty.tools.backend.jvm.BackendUtils.classfileVersionMap
+import dotty.tools.io.{AbstractFile, Directory, JDK9Reflectors, PlainDirectory, NoAbstractFile}
+import scala.language.unsafeNulls
+
+object ScalaSettingsProperties:
+
+  private lazy val minTargetVersion = classfileVersionMap.keysIterator.min
+  private lazy val maxTargetVersion = classfileVersionMap.keysIterator.max
+
+  def supportedTargetVersions: List[String] =
+    (minTargetVersion to maxTargetVersion).toList.map(_.toString)
+
+  def supportedReleaseVersions: List[String] =
+    if scala.util.Properties.isJavaAtLeast("9") then
+      val jdkVersion = JDK9Reflectors.runtimeVersionMajor(JDK9Reflectors.runtimeVersion()).intValue()
+      val maxVersion = Math.min(jdkVersion, maxTargetVersion)
+      (minTargetVersion to maxVersion).toList.map(_.toString)
+    else List(minTargetVersion).map(_.toString)
+
+  def supportedScalaReleaseVersions: List[String] =
+    ScalaRelease.values.toList.map(_.show)
+
+  def supportedSourceVersions: List[String] =
+    SourceVersion.values.toList.map(_.toString)
+
+  def defaultClasspath: String = sys.env.getOrElse("CLASSPATH", ".")
+
+  def defaultPageWidth: Int = {
+    val defaultWidth = 80
+    val columnsVar = System.getenv("COLUMNS")
+    if columnsVar != null then columnsVar.toInt
+    else if Properties.isWin then
+      val ansiconVar = System.getenv("ANSICON") // eg. "142x32766 (142x26)"
+      if ansiconVar != null && ansiconVar.matches("[0-9]+x.*") then
+        ansiconVar.substring(0, ansiconVar.indexOf("x")).toInt
+      else defaultWidth
+    else defaultWidth
+  }
diff --git a/compiler/src/dotty/tools/dotc/config/Settings.scala b/compiler/src/dotty/tools/dotc/config/Settings.scala
@@ -25,6 +25,9 @@ object Settings:
   val OptionTag: ClassTag[Option[?]]     = ClassTag(classOf[Option[?]])
   val OutputTag: ClassTag[AbstractFile]  = ClassTag(classOf[AbstractFile])
 
+  trait SettingCategory:
+    def prefixLetter: String
+
   class SettingsState(initialValues: Seq[Any], initialChanged: Set[Int] = Set.empty):
     private val values = ArrayBuffer(initialValues*)
     private val changed: mutable.Set[Int] = initialChanged.to(mutable.Set)
@@ -59,8 +62,14 @@ object Settings:
       ArgsSummary(sstate, arguments.tail, errors, warnings :+ msg)
   }
 
+  @unshared
+  val settingCharacters = "[a-zA-Z0-9_\\-]*".r
+  def validateSettingString(name: String): Unit = 
+    assert(settingCharacters.matches(name), s"Setting string $name contains invalid characters")
+
+
   case class Setting[T: ClassTag] private[Settings] (
-    category: String,
+    category: SettingCategory,
     name: String,
     description: String,
     default: T,
@@ -75,8 +84,10 @@ object Settings:
     // kept only for -Ykind-projector option compatibility
     legacyArgs: Boolean = false)(private[Settings] val idx: Int) {
   
-    
-    assert(name.startsWith(s"-$category"), s"Setting $name does not start with category -$category")
+    validateSettingString(prefix.getOrElse(name))
+    aliases.foreach(validateSettingString)
+    prefix.foreach(validateSettingString)
+    assert(name.startsWith(s"-${category.prefixLetter}"), s"Setting $name does not start with category -$category")
     assert(legacyArgs || !choices.exists(_.contains("")), s"Empty string is not supported as a choice for setting $name")
     // Without the following assertion, it would be easy to mistakenly try to pass a file to a setting that ignores invalid args.
     // Example: -opt Main.scala would be interpreted as -opt:Main.scala, and the source file would be ignored.
@@ -319,64 +330,58 @@ object Settings:
       setting
     }
 
-    @unshared
-    val settingCharacters = "[a-zA-Z0-9_\\-]*".r
-    def validateSetting(setting: String): String =
-      assert(settingCharacters.matches(setting), s"Setting $setting contains invalid characters")
-      setting 
-
-    def validateAndPrependName(name: String): String =
+    def prependName(name: String): String =
       assert(!name.startsWith("-"), s"Setting $name cannot start with -")
-      "-" + validateSetting(name)
+      "-" + name
 
-    def BooleanSetting(category: String, name: String, descr: String, initialValue: Boolean = false, aliases: List[String] = Nil): Setting[Boolean] =
-      publish(Setting(category, validateAndPrependName(name), descr, initialValue, aliases = aliases.map(validateSetting)))
+    def BooleanSetting(category: SettingCategory, name: String, descr: String, initialValue: Boolean = false, aliases: List[String] = Nil): Setting[Boolean] =
+      publish(Setting(category, prependName(name), descr, initialValue, aliases = aliases))
 
-    def StringSetting(category: String, name: String, helpArg: String, descr: String, default: String, aliases: List[String] = Nil): Setting[String] =
-      publish(Setting(category, validateAndPrependName(name), descr, default, helpArg, aliases = aliases.map(validateSetting)))
+    def StringSetting(category: SettingCategory, name: String, helpArg: String, descr: String, default: String, aliases: List[String] = Nil): Setting[String] =
+      publish(Setting(category, prependName(name), descr, default, helpArg, aliases = aliases))
 
-    def ChoiceSetting(category: String, name: String, helpArg: String, descr: String, choices: List[String], default: String, aliases: List[String] = Nil): Setting[String] =
-      publish(Setting(category, validateAndPrependName(name), descr, default, helpArg, Some(choices), aliases = aliases.map(validateSetting)))
+    def ChoiceSetting(category: SettingCategory, name: String, helpArg: String, descr: String, choices: List[String], default: String, aliases: List[String] = Nil): Setting[String] =
+      publish(Setting(category, prependName(name), descr, default, helpArg, Some(choices), aliases = aliases))
 
     // Allows only args after :, but supports empty string as a choice. Used for -Ykind-projector
-    def LegacyChoiceSetting(category: String, name: String, helpArg: String, descr: String, choices: List[String], default: String, aliases: List[String] = Nil): Setting[String] =
-      publish(Setting(category, validateAndPrependName(name), descr, default, helpArg, Some(choices), aliases = aliases.map(validateSetting), legacyArgs = true))
+    def LegacyChoiceSetting(category: SettingCategory, name: String, helpArg: String, descr: String, choices: List[String], default: String, aliases: List[String] = Nil): Setting[String] =
+      publish(Setting(category, prependName(name), descr, default, helpArg, Some(choices), aliases = aliases, legacyArgs = true))
 
-    def MultiChoiceSetting(category: String, name: String, helpArg: String, descr: String, choices: List[String], default: List[String], aliases: List[String] = Nil): Setting[List[String]] =
-      publish(Setting(category, validateAndPrependName(name), descr, default, helpArg, Some(choices), aliases = aliases.map(validateSetting)))
+    def MultiChoiceSetting(category: SettingCategory, name: String, helpArg: String, descr: String, choices: List[String], default: List[String], aliases: List[String] = Nil): Setting[List[String]] =
+      publish(Setting(category, prependName(name), descr, default, helpArg, Some(choices), aliases = aliases))
 
-    def MultiChoiceHelpSetting(category: String, name: String, helpArg: String, descr: String, choices: List[ChoiceWithHelp[String]], default: List[ChoiceWithHelp[String]], aliases: List[String] = Nil): Setting[List[ChoiceWithHelp[String]]] =
-      publish(Setting(category, validateAndPrependName(name), descr, default, helpArg, Some(choices), aliases = aliases.map(validateSetting)))
+    def MultiChoiceHelpSetting(category: SettingCategory, name: String, helpArg: String, descr: String, choices: List[ChoiceWithHelp[String]], default: List[ChoiceWithHelp[String]], aliases: List[String] = Nil): Setting[List[ChoiceWithHelp[String]]] =
+      publish(Setting(category, prependName(name), descr, default, helpArg, Some(choices), aliases = aliases))
 
-    def IntSetting(category: String, name: String, descr: String, default: Int, aliases: List[String] = Nil): Setting[Int] =
-      publish(Setting(category, validateAndPrependName(name), descr, default, aliases = aliases.map(validateSetting)))
+    def IntSetting(category: SettingCategory, name: String, descr: String, default: Int, aliases: List[String] = Nil): Setting[Int] =
+      publish(Setting(category, prependName(name), descr, default, aliases = aliases))
 
-    def IntChoiceSetting(category: String, name: String, descr: String, choices: Seq[Int], default: Int): Setting[Int] =
-      publish(Setting(category, validateAndPrependName(name), descr, default, choices = Some(choices)))
+    def IntChoiceSetting(category: SettingCategory, name: String, descr: String, choices: Seq[Int], default: Int): Setting[Int] =
+      publish(Setting(category, prependName(name), descr, default, choices = Some(choices)))
 
-    def MultiStringSetting(category: String, name: String, helpArg: String, descr: String, default: List[String] = Nil, aliases: List[String] = Nil): Setting[List[String]] =
-      publish(Setting(category, validateAndPrependName(name), descr, default, helpArg, aliases = aliases.map(validateSetting)))
+    def MultiStringSetting(category: SettingCategory, name: String, helpArg: String, descr: String, default: List[String] = Nil, aliases: List[String] = Nil): Setting[List[String]] =
+      publish(Setting(category, prependName(name), descr, default, helpArg, aliases = aliases))
 
-    def OutputSetting(category: String, name: String, helpArg: String, descr: String, default: AbstractFile): Setting[AbstractFile] =
-      publish(Setting(category, validateAndPrependName(name), descr, default, helpArg))
+    def OutputSetting(category: SettingCategory, name: String, helpArg: String, descr: String, default: AbstractFile): Setting[AbstractFile] =
+      publish(Setting(category, prependName(name), descr, default, helpArg))
 
-    def PathSetting(category: String, name: String, descr: String, default: String, aliases: List[String] = Nil): Setting[String] =
-      publish(Setting(category, validateAndPrependName(name), descr, default, aliases = aliases.map(validateSetting)))
+    def PathSetting(category: SettingCategory, name: String, descr: String, default: String, aliases: List[String] = Nil): Setting[String] =
+      publish(Setting(category, prependName(name), descr, default, aliases = aliases))
 
-    def PhasesSetting(category: String, name: String, descr: String, default: String = "", aliases: List[String] = Nil): Setting[List[String]] =
-      publish(Setting(category, validateAndPrependName(name), descr, if (default.isEmpty) Nil else List(default), aliases = aliases.map(validateSetting)))
+    def PhasesSetting(category: SettingCategory, name: String, descr: String, default: String = "", aliases: List[String] = Nil): Setting[List[String]] =
+      publish(Setting(category, prependName(name), descr, if (default.isEmpty) Nil else List(default), aliases = aliases))
 
-    def PrefixSetting(category: String, name: String, descr: String): Setting[List[String]] =
+    def PrefixSetting(category: SettingCategory, name: String, descr: String): Setting[List[String]] =
       val prefix = name.takeWhile(_ != '<')
-      publish(Setting(category, "-" + name, descr, Nil, prefix = Some(validateSetting(prefix))))
+      publish(Setting(category, "-" + name, descr, Nil, prefix = Some(prefix)))
 
-    def VersionSetting(category: String, name: String, descr: String, default: ScalaVersion = NoScalaVersion): Setting[ScalaVersion] =
-      publish(Setting(category, validateAndPrependName(name), descr, default))
+    def VersionSetting(category: SettingCategory, name: String, descr: String, default: ScalaVersion = NoScalaVersion): Setting[ScalaVersion] =
+      publish(Setting(category, prependName(name), descr, default))
 
-    def OptionSetting[T: ClassTag](category: String, name: String, descr: String, aliases: List[String] = Nil): Setting[Option[T]] =
-      publish(Setting(category, validateAndPrependName(name), descr, None, propertyClass = Some(summon[ClassTag[T]].runtimeClass), aliases = aliases.map(validateSetting)))
+    def OptionSetting[T: ClassTag](category: SettingCategory, name: String, descr: String, aliases: List[String] = Nil): Setting[Option[T]] =
+      publish(Setting(category, prependName(name), descr, None, propertyClass = Some(summon[ClassTag[T]].runtimeClass), aliases = aliases))
     
-    def DeprecatedSetting(category: String, name: String, descr: String, deprecationMsg: String): Setting[Boolean] =
-      publish(Setting(category, validateAndPrependName(name), descr, false, deprecationMsg = Some(deprecationMsg)))
+    def DeprecatedSetting(category: SettingCategory, name: String, descr: String, deprecationMsg: String): Setting[Boolean] =
+      publish(Setting(category, prependName(name), descr, false, deprecationMsg = Some(deprecationMsg)))
   }
 end Settings
diff --git a/compiler/test/dotty/tools/dotc/SettingsTests.scala b/compiler/test/dotty/tools/dotc/SettingsTests.scala
diff --git a/compiler/test/dotty/tools/dotc/config/ScalaSettingsTests.scala b/compiler/test/dotty/tools/dotc/config/ScalaSettingsTests.scala
