Merge pull request #12589 from som-snytt/issue/12588

smarter · web-flow · commit 74ea1af6e751 · 2021-05-26T16:23:41.000+02:00
processArgs is final for some tailrec
diff --git a/compiler/src/dotty/tools/dotc/config/CliCommand.scala b/compiler/src/dotty/tools/dotc/config/CliCommand.scala
@@ -63,6 +63,7 @@ trait CliCommand:
     }
 
     sg.processArguments(expandedArguments, processAll = true, settingsState = ss)
+  end distill
 
   /** Creates a help message for a subset of options based on cond */
   protected def availableOptionsMsg(cond: Setting[?] => Boolean)(using settings: ConcreteSettings)(using SettingsState): String =
@@ -108,6 +109,7 @@ trait CliCommand:
           ""
       s"${formatName(s.name)} ${formatDescription(s.description)}${formatSetting("Default", defaultValue)}${formatSetting("Choices", s.legalChoices)}"
     ss.map(helpStr).mkString("", "\n", s"\n${formatName("@<file>")} ${formatDescription("A text file containing compiler arguments (options and source files).")}\n")
+  end availableOptionsMsg
 
   protected def shortUsage: String = s"Usage: $cmdName <options> <source files>"
 
diff --git a/compiler/src/dotty/tools/dotc/config/Settings.scala b/compiler/src/dotty/tools/dotc/config/Settings.scala
@@ -21,25 +21,22 @@ object Settings {
   val OptionTag: ClassTag[Option[?]]     = ClassTag(classOf[Option[?]])
   val OutputTag: ClassTag[AbstractFile]  = ClassTag(classOf[AbstractFile])
 
-  class SettingsState(initialValues: Seq[Any]) {
+  class SettingsState(initialValues: Seq[Any]):
     private val values = ArrayBuffer(initialValues: _*)
     private var _wasRead: Boolean = false
 
     override def toString: String = s"SettingsState(values: ${values.toList})"
 
-    def value(idx: Int): Any = {
+    def value(idx: Int): Any =
       _wasRead = true
       values(idx)
-    }
 
     def update(idx: Int, x: Any): SettingsState =
-      if (_wasRead)
-        new SettingsState(values.toSeq).update(idx, x)
-      else {
+      if (_wasRead) then SettingsState(values.toSeq).update(idx, x)
+      else
         values(idx) = x
         this
-      }
-  }
+  end SettingsState
 
   case class ArgsSummary(
     sstate: SettingsState,
@@ -67,18 +64,11 @@ object Settings {
 
     private var changed: Boolean = false
 
-    def valueIn(state: SettingsState): T =
-      state.value(idx).asInstanceOf[T]
+    def valueIn(state: SettingsState): T = state.value(idx).asInstanceOf[T]
 
-    def updateIn(state: SettingsState, x: Any): SettingsState = x match {
+    def updateIn(state: SettingsState, x: Any): SettingsState = x match
       case _: T => state.update(idx, x)
-      case _ =>
-        // would like to do:
-        // throw new ClassCastException(s"illegal argument, found: $x of type ${x.getClass}, required: ${implicitly[ClassTag[T]]}")
-        // but this runs afoul of primitive types. Concretely: if T is Boolean, then x is a boxed Boolean and the test will fail.
-        // Maybe this is a bug in Scala 2.10?
-        state.update(idx, x.asInstanceOf[T])
-    }
+      case _ => throw IllegalArgumentException(s"found: $x of type ${x.getClass.getName}, required: ${implicitly[ClassTag[T]]}")
 
     def isDefaultIn(state: SettingsState): Boolean = valueIn(state) == default
 
@@ -94,7 +84,7 @@ object Settings {
 
     def tryToSet(state: ArgsSummary): ArgsSummary = {
       val ArgsSummary(sstate, arg :: args, errors, warnings) = state
-      def update(value: Any, args: List[String]) =
+      def update(value: Any, args: List[String]): ArgsSummary =
         var dangers = warnings
         val value1 =
           if changed && isMultivalue then
@@ -107,6 +97,7 @@ object Settings {
             value
         changed = true
         ArgsSummary(updateIn(sstate, value1), args, errors, dangers)
+      end update
       def fail(msg: String, args: List[String]) =
         ArgsSummary(sstate, args, errors :+ msg, warnings)
       def missingArg =
@@ -209,7 +200,7 @@ object Settings {
     /** Iterates over the arguments applying them to settings where applicable.
      *  Then verifies setting dependencies are met.
      *
-     *  This temporarily takes a boolean indicating whether to keep
+     *  This takes a boolean indicating whether to keep
      *  processing if an argument is seen which is not a command line option.
      *  This is an expedience for the moment so that you can say
      *
@@ -221,28 +212,27 @@ object Settings {
      *
      *  to get their arguments.
      */
-    def processArguments(state: ArgsSummary, processAll: Boolean, skipped: List[String]): ArgsSummary = {
+    @tailrec
+    final def processArguments(state: ArgsSummary, processAll: Boolean, skipped: List[String]): ArgsSummary =
       def stateWithArgs(args: List[String]) = ArgsSummary(state.sstate, args, state.errors, state.warnings)
-      state.arguments match {
+      state.arguments match
         case Nil =>
           checkDependencies(stateWithArgs(skipped))
         case "--" :: args =>
           checkDependencies(stateWithArgs(skipped ++ args))
         case x :: _ if x startsWith "-" =>
-          @tailrec def loop(settings: List[Setting[?]]): ArgsSummary = settings match {
+          @tailrec def loop(settings: List[Setting[?]]): ArgsSummary = settings match
             case setting :: settings1 =>
               val state1 = setting.tryToSet(state)
-              if (state1 ne state) processArguments(state1, processAll, skipped)
+              if state1 ne state then state1
               else loop(settings1)
             case Nil =>
-              processArguments(state.warn(s"bad option '$x' was ignored"), processAll, skipped)
-          }
-          loop(allSettings.toList)
+              state.warn(s"bad option '$x' was ignored")
+          processArguments(loop(allSettings.toList), processAll, skipped)
         case arg :: args =>
-          if (processAll) processArguments(stateWithArgs(args), processAll, skipped :+ arg)
+          if processAll then processArguments(stateWithArgs(args), processAll, skipped :+ arg)
           else state
-      }
-    }
+    end processArguments
 
     def processArguments(arguments: List[String], processAll: Boolean, settingsState: SettingsState = defaultState): ArgsSummary =
       processArguments(ArgsSummary(settingsState, arguments, Nil, Nil), processAll, Nil)
diff --git a/compiler/test/dotty/tools/dotc/SettingsTests.scala b/compiler/test/dotty/tools/dotc/SettingsTests.scala
@@ -21,15 +21,14 @@ class SettingsTests {
     assertEquals(1, reporter.errorCount)
     assertEquals("'not_here' does not exist or is not a directory or .jar file", reporter.allErrors.head.message)
 
-  @Test def jarOutput: Unit = {
+  @Test def jarOutput: Unit =
     val source = "tests/pos/Foo.scala"
     val out = Paths.get("out/jaredFoo.jar").normalize
     if (Files.exists(out)) Files.delete(out)
     val options = Array("-classpath", TestConfiguration.basicClasspath, "-d", out.toString, source)
     val reporter = Main.process(options)
     assertEquals(0, reporter.errorCount)
     assertTrue(Files.exists(out))
-  }
 
   @Test def `t8124 Don't crash on missing argument`: Unit =
     val source    = Paths.get("tests/pos/Foo.scala").normalize
@@ -45,15 +44,70 @@ class SettingsTests {
       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)
-      given SettingsState = summary.sstate
+    val args = List("-foo", "b", "-bar", "1")
+    val summary = Settings.processArguments(args, true)
+    assertTrue(summary.errors.isEmpty)
+    withProcessedArgs(summary) {
       assertEquals("b", Settings.foo.value)
       assertEquals(1, Settings.bar.value)
     }
 
+  @Test def `workaround dont crash on many files`: Unit =
+    object Settings extends SettingGroup
+
+    val args = "--" :: List.fill(6000)("file.scala")
+    val summary = Settings.processArguments(args, processAll = true)
+    assertTrue(summary.errors.isEmpty)
+    assertEquals(6000, summary.arguments.size)
+
+  @Test def `dont crash on many files`: Unit =
+    object Settings extends SettingGroup
+
+    val args = List.fill(6000)("file.scala")
+    val summary = Settings.processArguments(args, processAll = true)
+    assertTrue(summary.errors.isEmpty)
+    assertEquals(6000, summary.arguments.size)
+
+  @Test def `dont crash on many options`: Unit =
+    object Settings extends SettingGroup:
+      val option = BooleanSetting("-option", "Some option")
+
+    val limit = 6000
+    val args = List.fill(limit)("-option")
+    val summary = Settings.processArguments(args, processAll = true)
+    assertTrue(summary.errors.isEmpty)
+    assertEquals(limit-1, summary.warnings.size)
+    assertTrue(summary.warnings.head.contains("repeatedly"))
+    assertEquals(0, summary.arguments.size)
+    withProcessedArgs(summary) {
+      assertTrue(Settings.option.value)
+    }
+
+  @Test def `bad option warning consumes an arg`: Unit =
+    object Settings extends SettingGroup:
+      val option = BooleanSetting("-option", "Some option")
+
+    val args = List("-adoption", "dogs", "cats")
+    val summary = Settings.processArguments(args, processAll = true)
+    assertTrue(summary.errors.isEmpty)
+    assertFalse(summary.warnings.isEmpty)
+    assertEquals(2, summary.arguments.size)
+
+  @Test def `bad option settings throws`: Unit =
+    object Settings extends SettingGroup:
+      val option = BooleanSetting("-option", "Some option")
+
+    def checkMessage(s: String): (Throwable => Boolean) = t =>
+      if t.getMessage == s then true
+      else
+        println(s"Expected: $s, Actual: ${t.getMessage}")
+        false
+
+    val default = Settings.defaultState
+    assertThrows[IllegalArgumentException](checkMessage("found: not an option of type java.lang.String, required: Boolean")) {
+      Settings.option.updateIn(default, "not an option")
+    }
+
   @Test def validateChoices: Unit =
     object Settings extends SettingGroup:
       val foo = ChoiceSetting("-foo", "foo", "Foo", List("a", "b"), "a")
@@ -63,25 +117,27 @@ class SettingsTests {
       val quux = ChoiceSetting("-quux", "quux", "Quux", List(), "")
       val quuz = IntChoiceSetting("-quuz", "Quuz", List(), 0)
 
-    inContext {
+    locally {
       val args = List("-foo", "b", "-bar", "1", "-baz", "5")
       val summary = Settings.processArguments(args, true)
       assertTrue(summary.errors.isEmpty)
-      given SettingsState = summary.sstate
-      assertEquals("b", Settings.foo.value)
-      assertEquals(1, Settings.bar.value)
-      assertEquals(5, Settings.baz.value)
+      withProcessedArgs(summary) {
+        assertEquals("b", Settings.foo.value)
+        assertEquals(1, Settings.bar.value)
+        assertEquals(5, Settings.baz.value)
+      }
     }
 
-    inContext {
+    locally {
       val args = List("-foo:b")
       val summary = Settings.processArguments(args, true)
       assertTrue(summary.errors.isEmpty)
-      given SettingsState = summary.sstate
-      assertEquals("b", Settings.foo.value)
+      withProcessedArgs(summary) {
+        assertEquals("b", Settings.foo.value)
+      }
     }
 
-    inContext {
+    locally {
       val args = List("-foo", "c", "-bar", "3", "-baz", "-1")
       val summary = Settings.processArguments(args, true)
       val expectedErrors = List(
@@ -92,14 +148,14 @@ class SettingsTests {
       assertEquals(expectedErrors, summary.errors)
     }
 
-    inContext {
+    locally {
       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 {
+    locally {
       val args = List("-quux", "a", "-quuz", "0")
       val summary = Settings.processArguments(args, true)
       val expectedErrors = List(
@@ -109,7 +165,7 @@ class SettingsTests {
       assertEquals(expectedErrors, summary.errors)
     }
 
-  private def inContext(f: Context ?=> Unit) = f(using (new ContextBase).initialCtx.fresh)
+  private def withProcessedArgs(summary: ArgsSummary)(f: SettingsState ?=> Unit) = f(using summary.sstate)
 
   extension [T](setting: Setting[T])
     private def value(using ss: SettingsState): T = setting.valueIn(ss)
diff --git a/compiler/test/dotty/tools/utils.scala b/compiler/test/dotty/tools/utils.scala
@@ -4,7 +4,10 @@ import java.io.File
 import java.nio.charset.StandardCharsets.UTF_8
 
 import scala.io.Source
+import scala.reflect.ClassTag
 import scala.util.Using.resource
+import scala.util.chaining.given
+import scala.util.control.{ControlThrowable, NonFatal}
 
 def scripts(path: String): Array[File] = {
   val dir = new File(getClass.getResource(path).getPath)
@@ -17,3 +20,16 @@ private def withFile[T](file: File)(action: Source => T): T =
 
 def readLines(f: File): List[String] = withFile(f)(_.getLines.toList)
 def readFile(f: File): String = withFile(f)(_.mkString)
+
+private object Unthrown extends ControlThrowable
+
+def assertThrows[T <: Throwable: ClassTag](p: T => Boolean)(body: => Any): Unit =
+  try
+    body
+    throw Unthrown
+  catch
+    case Unthrown        => throw AssertionError("Expression did not throw!")
+    case e: T if p(e)    => ()
+    case failed: T       => throw AssertionError(s"Exception failed check: $failed").tap(_.addSuppressed(failed))
+    case NonFatal(other) => throw AssertionError(s"Wrong exception: expected ${implicitly[ClassTag[T]]} but was ${other.getClass.getName}").tap(_.addSuppressed(other))
+end assertThrows
