Merge pull request #10804 from prolativ/validate-settings-with-choices

smarter · web-flow · commit 12ff02fd169c · 2020-12-22T16:51:53.000+01:00
Fix #10756: Validate enumerative scalac settings
diff --git a/compiler/src/dotty/tools/dotc/config/Settings.scala b/compiler/src/dotty/tools/dotc/config/Settings.scala
@@ -59,7 +59,7 @@ object Settings {
     description: String,
     default: T,
     helpArg: String = "",
-    choices: Seq[T] = Nil,
+    choices: Option[Seq[T]] = None,
     prefix: String = "",
     aliases: List[String] = Nil,
     depends: List[(Setting[?], Any)] = Nil,
@@ -92,25 +92,10 @@ object Settings {
 
     def legalChoices: String =
       choices match {
-        case xs if xs.isEmpty => ""
-        case r: Range         => s"${r.head}..${r.last}"
-        case xs: List[?]      => xs.toString
-      }
-
-    def isLegal(arg: Any): Boolean =
-      choices match {
-        case xs if xs.isEmpty =>
-          arg match {
-            case _: T => true
-            case _ => false
-          }
-        case r: Range =>
-          arg match {
-            case x: Int => r.head <= x && x <= r.last
-            case _ => false
-          }
-        case xs: List[?] =>
-          xs.contains(arg)
+        case Some(xs) if xs.isEmpty => ""
+        case Some(r: Range)         => s"${r.head}..${r.last}"
+        case Some(xs)               => xs.mkString(", ")
+        case None                   => ""
       }
 
     def tryToSet(state: ArgsSummary): ArgsSummary = {
@@ -132,6 +117,12 @@ object Settings {
         ArgsSummary(sstate, args, errors :+ msg, warnings)
       def missingArg =
         fail(s"missing argument for option $name", args)
+      def setString(argValue: String, args: List[String]) =
+        choices match
+          case Some(xs) if !xs.contains(argValue) =>
+            fail(s"$argValue is not a valid choice for $name", args)
+          case _ =>
+            update(argValue, args)
       def doSet(argRest: String) = ((implicitly[ClassTag[T]], args): @unchecked) match {
         case (BooleanTag, _) =>
           update(true, args)
@@ -140,13 +131,11 @@ object Settings {
         case (ListTag, _) =>
           if (argRest.isEmpty) missingArg
           else update((argRest split ",").toList, args)
-        case (StringTag, _) if choices.nonEmpty && argRest.nonEmpty =>
-          if (!choices.contains(argRest))
-            fail(s"$arg is not a valid choice for $name", args)
-          else update(argRest, args)
+        case (StringTag, _) if argRest.nonEmpty =>
+          setString(argRest, args)
         case (StringTag, arg2 :: args2) =>
           if (arg2 startsWith "-") missingArg
-          else update(arg2, args2)
+          else setString(arg2, args2)
         case (OutputTag, arg :: args) =>
           val path = Directory(arg)
           val isJar = path.extension == "jar"
@@ -160,8 +149,10 @@ object Settings {
           try {
             val x = arg2.toInt
             choices match {
-              case r: Range if x < r.head || r.last < x =>
-                fail(s"$arg2 is out of legal range $legalChoices for $name", args2)
+              case Some(r: Range) if x < r.head || r.last < x =>
+                fail(s"$arg2 is out of legal range ${r.head}..${r.last} for $name", args2)
+              case Some(xs) if !xs.contains(x) =>
+                fail(s"$arg2 is not a valid choice for $name", args)
               case _ =>
                 update(x, args2)
             }
@@ -273,10 +264,13 @@ object Settings {
       publish(Setting(name, descr, default, helpArg, aliases = aliases))
 
     def ChoiceSetting(name: String, helpArg: String, descr: String, choices: List[String], default: String): Setting[String] =
-      publish(Setting(name, descr, default, helpArg, choices))
+      publish(Setting(name, descr, default, helpArg, Some(choices)))
+
+    def IntSetting(name: String, descr: String, default: Int): Setting[Int] =
+      publish(Setting(name, descr, default))
 
-    def IntSetting(name: String, descr: String, default: Int, range: Seq[Int] = Nil): Setting[Int] =
-      publish(Setting(name, descr, default, choices = range))
+    def IntChoiceSetting(name: String, descr: String, choices: Seq[Int], default: Int): Setting[Int] =
+      publish(Setting(name, descr, default, choices = Some(choices)))
 
     def MultiStringSetting(name: String, helpArg: String, descr: String): Setting[List[String]] =
       publish(Setting(name, descr, Nil, helpArg))
diff --git a/compiler/test/dotty/tools/dotc/SettingsTests.scala b/compiler/test/dotty/tools/dotc/SettingsTests.scala
@@ -4,6 +4,8 @@ package dotc
 import reporting.StoreReporter
 import vulpix.TestConfiguration
 
+import core.Contexts.{Context, ContextBase}
+import dotty.tools.dotc.config.Settings._
 import dotty.tools.vulpix.TestConfiguration.mkClasspath
 
 import java.nio.file._
@@ -37,4 +39,72 @@ class SettingsTests {
     val options  = Array("-encoding", "-d", outputDir.toString, source.toString)
     val reporter = Main.process(options, reporter = StoreReporter())
     assertEquals(1, reporter.errorCount)
+
+  @Test def acceptUnconstrained: Unit =
+    object Settings extends SettingGroup:
+      val foo = StringSetting("-foo", "foo", "Foo", "a")
+      val bar = IntSetting("-bar", "Bar", 0)
+
+    inContext {
+      val args = List("-foo", "b", "-bar", "1")
+      val summary = Settings.processArguments(args, true)
+      assertTrue(summary.errors.isEmpty)
+      assertEquals("b", Settings.foo.value)
+      assertEquals(1, Settings.bar.value)
+    }
+
+  @Test def validateChoices: Unit =
+    object Settings extends SettingGroup:
+      val foo = ChoiceSetting("-foo", "foo", "Foo", List("a", "b"), "a")
+      val bar = IntChoiceSetting("-bar", "Bar", List(0, 1, 2), 0)
+      val baz = IntChoiceSetting("-baz", "Baz", 0 to 10, 10)
+
+      val quux = ChoiceSetting("-quux", "quux", "Quux", List(), "")
+      val quuz = IntChoiceSetting("-quuz", "Quuz", List(), 0)
+
+    inContext {
+      val args = List("-foo", "b", "-bar", "1", "-baz", "5")
+      val summary = Settings.processArguments(args, true)
+      assertTrue(summary.errors.isEmpty)
+      assertEquals("b", Settings.foo.value)
+      assertEquals(1, Settings.bar.value)
+      assertEquals(5, Settings.baz.value)
+    }
+
+    inContext {
+      val args = List("-foo:b")
+      val summary = Settings.processArguments(args, true)
+      assertTrue(summary.errors.isEmpty)
+      assertEquals("b", Settings.foo.value)
+    }
+
+    inContext {
+      val args = List("-foo", "c", "-bar", "3", "-baz", "-1")
+      val summary = Settings.processArguments(args, true)
+      val expectedErrors = List(
+        "c is not a valid choice for -foo",
+        "3 is not a valid choice for -bar",
+        "-1 is out of legal range 0..10 for -baz"
+      )
+      assertEquals(expectedErrors, summary.errors)
+    }
+
+    inContext {
+      val args = List("-foo:c")
+      val summary = Settings.processArguments(args, true)
+      val expectedErrors = List("c is not a valid choice for -foo")
+      assertEquals(expectedErrors, summary.errors)
+    }
+
+    inContext {
+      val args = List("-quux", "a", "-quuz", "0")
+      val summary = Settings.processArguments(args, true)
+      val expectedErrors = List(
+        "a is not a valid choice for -quux",
+        "0 is not a valid choice for -quuz",
+      )
+      assertEquals(expectedErrors, summary.errors)
+    }
+
+  private def inContext(f: Context ?=> Unit) = f(using (new ContextBase).initialCtx.fresh)
 }
