From f1df8f36f8ecd01054f8ee8e4045fc4520198a50 Mon Sep 17 00:00:00 2001 From: Som Snytt Date: Mon, 24 May 2021 17:26:58 -0700 Subject: [PATCH] processArgs is tailrec Long lists of compiler args are not unusual, so avoid running out of stack. --- .../dotty/tools/dotc/config/CliCommand.scala | 2 + .../dotty/tools/dotc/config/Settings.scala | 50 ++++------ .../test/dotty/tools/dotc/SettingsTests.scala | 94 +++++++++++++++---- compiler/test/dotty/tools/utils.scala | 16 ++++ 4 files changed, 113 insertions(+), 49 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/config/CliCommand.scala b/compiler/src/dotty/tools/dotc/config/CliCommand.scala index e361519bb54d..6e9da7fefb62 100644 --- 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("@")} ${formatDescription("A text file containing compiler arguments (options and source files).")}\n") + end availableOptionsMsg protected def shortUsage: String = s"Usage: $cmdName " diff --git a/compiler/src/dotty/tools/dotc/config/Settings.scala b/compiler/src/dotty/tools/dotc/config/Settings.scala index d68d5c433064..fb175474f1c3 100644 --- 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 index 5303fb070c7a..8369f6f77caa 100644 --- a/compiler/test/dotty/tools/dotc/SettingsTests.scala +++ b/compiler/test/dotty/tools/dotc/SettingsTests.scala @@ -21,7 +21,7 @@ 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) @@ -29,7 +29,6 @@ class SettingsTests { 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 index b7023a577234..6d5677f0e8f8 100644 --- 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