Skip to content

Commit 98845ef

Browse files
som-snyttsom-snytt
committed
Clarify option sanity checks
1 parent 0a42ac7 commit 98845ef

File tree

2 files changed

+117
-86
lines changed

2 files changed

+117
-86
lines changed

‎compiler/src/dotty/tools/dotc/config/Settings.scala

Lines changed: 72 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -38,18 +38,12 @@ object Settings:
3838
this
3939
end SettingsState
4040

41-
case class ArgsSummary(
42-
sstate: SettingsState,
43-
arguments: List[String],
44-
errors: List[String],
45-
warnings: List[String]) {
41+
case class ArgsSummary(sstate: SettingsState, arguments: List[String], errors: List[String], warnings: List[String])
4642

47-
def fail(msg: String): Settings.ArgsSummary =
48-
ArgsSummary(sstate, arguments.tail, errors :+ msg, warnings)
49-
50-
def warn(msg: String): Settings.ArgsSummary =
51-
ArgsSummary(sstate, arguments.tail, errors, warnings :+ msg)
52-
}
43+
extension (summary: ArgsSummary)
44+
def fail(msg: String): ArgsSummary = summary.copy(errors = summary.errors :+ msg)
45+
def warn(msg: String): ArgsSummary = summary.copy(warnings = summary.warnings :+ msg)
46+
def skip(): ArgsSummary = summary.copy(arguments = if summary.arguments.isEmpty then Nil else summary.arguments.tail)
5347

5448
case class Setting[T: ClassTag] private[Settings] (
5549
name: String,
@@ -72,7 +66,7 @@ object Settings:
7266

7367
def isDefaultIn(state: SettingsState): Boolean = valueIn(state) == default
7468

75-
def isMultivalue: Boolean = implicitly[ClassTag[T]] == ListTag
69+
def isMultivalue: Boolean = summon[ClassTag[T]] == ListTag
7670

7771
def legalChoices: String =
7872
choices match {
@@ -85,91 +79,92 @@ object Settings:
8579
def tryToSet(state: ArgsSummary): ArgsSummary = {
8680
val ArgsSummary(sstate, arg :: args, errors, warnings) = state
8781
def update(value: Any, args: List[String]): ArgsSummary =
88-
var dangers = warnings
8982
val value1 =
90-
if changed && isMultivalue then
91-
val value0 = value.asInstanceOf[List[String]]
92-
val current = valueIn(sstate).asInstanceOf[List[String]]
93-
value0.filter(current.contains).foreach(s => dangers :+= s"Setting $name set to $s redundantly")
94-
current ++ value0
95-
else
96-
if changed then dangers :+= s"Flag $name set repeatedly"
97-
value
83+
if isMultivalue then valueIn(sstate).asInstanceOf[List[String]] ++ value.asInstanceOf[List[String]]
84+
else value
9885
changed = true
99-
ArgsSummary(updateIn(sstate, value1), args, errors, dangers)
100-
end update
86+
ArgsSummary(updateIn(sstate, value1), args, errors, warnings)
87+
88+
def fail(msg: String, args: List[String]) = ArgsSummary(sstate, args, errors :+ msg, warnings)
10189

102-
def fail(msg: String, args: List[String]) =
103-
ArgsSummary(sstate, args, errors :+ msg, warnings)
90+
def missingArg = fail(s"missing argument for option $name", args)
10491

105-
def missingArg =
106-
fail(s"missing argument for option $name", args)
92+
def setBoolean(argValue: String) =
93+
def checkAndSet(v: Boolean) =
94+
val dubious = changed && v != valueIn(sstate).asInstanceOf[Boolean]
95+
val updated = update(v, args)
96+
if dubious then updated.warn(s"Boolean flag $name flipped") else updated
97+
if argValue.equalsIgnoreCase("true") || argValue == "" then checkAndSet(true)
98+
else if argValue.equalsIgnoreCase("false") then checkAndSet(false)
99+
else fail(s"invalid boolean value $argValue", args)
107100

108101
def setString(argValue: String, args: List[String]) =
109102
choices match
110-
case Some(xs) if !xs.contains(argValue) =>
111-
fail(s"$argValue is not a valid choice for $name", args)
112-
case _ =>
113-
update(argValue, args)
103+
case Some(xs) if !xs.contains(argValue) => fail(s"$argValue is not a valid choice for $name", args)
104+
case _ if changed && argValue != valueIn(sstate).asInstanceOf[String] => update(argValue, args).warn(s"Option $name was updated")
105+
case _ => update(argValue, args)
114106

115107
def setInt(argValue: String, args: List[String]) =
116108
try
117109
val x = argValue.toInt
118110
choices match
119-
case Some(r: Range) if x < r.head || r.last < x =>
120-
fail(s"$argValue is out of legal range ${r.head}..${r.last} for $name", args)
121-
case Some(xs) if !xs.contains(x) =>
122-
fail(s"$argValue is not a valid choice for $name", args)
123-
case _ =>
124-
update(x, args)
125-
catch case _: NumberFormatException =>
126-
fail(s"$argValue is not an integer argument for $name", args)
127-
128-
def doSet(argRest: String) = ((implicitly[ClassTag[T]], args): @unchecked) match {
129-
case (BooleanTag, _) =>
130-
update(true, args)
131-
case (OptionTag, _) =>
132-
update(Some(propertyClass.get.getConstructor().newInstance()), args)
133-
case (ListTag, _) =>
134-
if (argRest.isEmpty) missingArg
135-
else
136-
val strings = argRest.split(",").toList
137-
choices match
138-
case Some(valid) => strings.filterNot(valid.contains) match
139-
case Nil => update(strings, args)
111+
case Some(r: Range) if x < r.head || r.last < x => fail(s"$argValue is out of legal range ${r.head}..${r.last} for $name", args)
112+
case Some(xs) if !xs.contains(x) => fail(s"$argValue is not a valid choice for $name", args)
113+
case _ => update(x, args)
114+
catch case _: NumberFormatException => fail(s"$argValue is not an integer argument for $name", args)
115+
116+
def doSet(argRest: String): ArgsSummary = (summon[ClassTag[T]]: @unchecked) match {
117+
case BooleanTag => setBoolean(argRest)
118+
case OptionTag => update(Some(propertyClass.get.getConstructor().newInstance()), args)
119+
case ListTag if argRest.isEmpty => missingArg
120+
case ListTag =>
121+
val split = argRest.split(",").toList
122+
def checkRepeated =
123+
var dangers: List[String] = Nil
124+
if changed then
125+
val current = valueIn(sstate).asInstanceOf[List[String]]
126+
split.filter(current.contains).foreach(s => dangers :+= s"Setting $name set to $s redundantly")
127+
dangers.foldLeft(update(split, args))((sum, w) => sum.warn(w))
128+
choices match
129+
case Some(valid) =>
130+
split.filterNot(valid.contains) match
131+
case Nil => checkRepeated
140132
case invalid => fail(s"invalid choice(s) for $name: ${invalid.mkString(",")}", args)
141-
case _ => update(strings, args)
142-
case (StringTag, _) if argRest.nonEmpty || choices.exists(_.contains("")) =>
143-
setString(argRest, args)
144-
case (StringTag, arg2 :: args2) =>
145-
if (arg2 startsWith "-") missingArg
146-
else setString(arg2, args2)
147-
case (OutputTag, arg :: args) =>
148-
val path = Directory(arg)
149-
val isJar = path.extension == "jar"
150-
if (!isJar && !path.isDirectory)
151-
fail(s"'$arg' does not exist or is not a directory or .jar file", args)
152-
else {
153-
val output = if (isJar) JarArchive.create(path) else new PlainDirectory(path)
154-
update(output, args)
155-
}
156-
case (IntTag, args) if argRest.nonEmpty =>
157-
setInt(argRest, args)
158-
case (IntTag, arg2 :: args2) =>
159-
setInt(arg2, args2)
160-
case (VersionTag, _) =>
133+
case _ => checkRepeated
134+
case StringTag if argRest.nonEmpty || choices.exists(_.contains("")) => setString(argRest, args)
135+
case StringTag =>
136+
args match
137+
case arg2 :: _ if arg2.startsWith("-") => missingArg
138+
case arg2 :: args2 => setString(arg2, args2)
139+
case _ => missingArg
140+
case OutputTag =>
141+
args match
142+
case arg :: rest =>
143+
val path = Directory(arg)
144+
val isJar = path.extension == "jar"
145+
if (!isJar && !path.isDirectory) fail(s"'$arg' does not exist or is not a directory or .jar file", rest)
146+
else
147+
val output = if (isJar) JarArchive.create(path) else new PlainDirectory(path)
148+
update(output, rest)
149+
case _ => missingArg
150+
case IntTag if argRest.nonEmpty => setInt(argRest, args)
151+
case IntTag =>
152+
args match
153+
case arg :: rest => setInt(arg, rest)
154+
case _ => missingArg
155+
case VersionTag =>
161156
ScalaVersion.parse(argRest) match {
162157
case Success(v) => update(v, args)
163-
case Failure(ex) => fail(ex.getMessage, args)
158+
case Failure(x) => fail(x.getMessage, args)
164159
}
165-
case (_, Nil) =>
166-
missingArg
160+
case _ => missingArg
167161
}
168162

169163
def matches(argName: String) = (name :: aliases).exists(_ == argName)
170164

165+
// begin
171166
if (prefix != "" && arg.startsWith(prefix))
172-
doSet(arg drop prefix.length)
167+
doSet(arg.drop(prefix.length))
173168
else if (prefix == "" && matches(arg.takeWhile(_ != ':')))
174169
doSet(arg.dropWhile(_ != ':').drop(1))
175170
else
@@ -236,7 +231,7 @@ object Settings:
236231
if state1 ne state then state1
237232
else loop(settings1)
238233
case Nil =>
239-
state.warn(s"bad option '$x' was ignored")
234+
state.warn(s"bad option '$x' was ignored").skip()
240235
processArguments(loop(allSettings.toList), processAll, skipped)
241236
case arg :: args =>
242237
if processAll then processArguments(stateWithArgs(args), processAll, skipped :+ arg)

‎compiler/test/dotty/tools/dotc/SettingsTests.scala

Lines changed: 45 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,9 @@ class SettingsTests {
2323

2424
@Test def jarOutput: Unit =
2525
val source = "tests/pos/Foo.scala"
26-
val out = Paths.get("out/jaredFoo.jar").normalize
27-
if (Files.exists(out)) Files.delete(out)
28-
val options = Array("-classpath", TestConfiguration.basicClasspath, "-d", out.toString, source)
26+
val out = Paths.get("out/jarredFoo.jar").normalize
27+
if Files.exists(out) then Files.delete(out)
28+
val options = Array("-Xmain-class", "Jarred", "-classpath", TestConfiguration.basicClasspath, "-d", out.toString, source)
2929
val reporter = Main.process(options)
3030
assertEquals(0, reporter.errorCount)
3131
assertTrue(Files.exists(out))
@@ -45,7 +45,7 @@ class SettingsTests {
4545
val bar = IntSetting("-bar", "Bar", 0)
4646

4747
val args = List("-foo", "b", "-bar", "1")
48-
val summary = Settings.processArguments(args, true)
48+
val summary = Settings.processArguments(args, processAll = true)
4949
assertTrue(summary.errors.isEmpty)
5050
withProcessedArgs(summary) {
5151
assertEquals("b", Settings.foo.value)
@@ -70,17 +70,17 @@ class SettingsTests {
7070

7171
@Test def `dont crash on many options`: Unit =
7272
object Settings extends SettingGroup:
73-
val option = BooleanSetting("-option", "Some option")
73+
val option = StringSetting("-option", "opt", "Some option", "zero")
7474

7575
val limit = 6000
76-
val args = List.fill(limit)("-option")
76+
val args = List.tabulate(limit)(i => if (i % 2 == 0) "-option" else i.toString)
7777
val summary = Settings.processArguments(args, processAll = true)
7878
assertTrue(summary.errors.isEmpty)
79-
assertEquals(limit-1, summary.warnings.size)
80-
assertTrue(summary.warnings.head.contains("repeatedly"))
79+
assertEquals(limit/2 - 1, summary.warnings.size) // should warn on all but first
80+
assertTrue(summary.warnings.head.contains("was updated"))
8181
assertEquals(0, summary.arguments.size)
8282
withProcessedArgs(summary) {
83-
assertTrue(Settings.option.value)
83+
assertEquals("5999", Settings.option.value.toString)
8484
}
8585

8686
@Test def `bad option warning consumes an arg`: Unit =
@@ -182,8 +182,44 @@ class SettingsTests {
182182
check(List("-foo", "100"))
183183
assertThrows[AssertionError](_.getMessage.contains("missing argument for option -foo"))(check(List("-foo")))
184184

185+
@Test def `option sanity checking`: Unit =
186+
object Settings extends SettingGroup:
187+
val option = BooleanSetting("-option", "Some option")
188+
val args = List("-option", "-option")
189+
val summary = Settings.processArguments(args, processAll = true)
190+
assertTrue("Multiple options is not an error", summary.errors.isEmpty)
191+
assertTrue("Multiple options is not a warning if consistent", summary.warnings.isEmpty)
192+
193+
@Test def `boolean option sanity checking`: Unit =
194+
object Settings extends SettingGroup:
195+
val option = BooleanSetting("-option", "Some option")
196+
val args = List("-option", "-option:false")
197+
val summary = Settings.processArguments(args, processAll = true)
198+
assertTrue("Multiple options is not an error", summary.errors.isEmpty)
199+
assertFalse("Multiple conflicting options is a warning", summary.warnings.isEmpty)
200+
assertTrue(summary.warnings.forall(_.contains("flipped")))
201+
202+
@Test def `string option may be consistent`: Unit =
203+
object Settings extends SettingGroup:
204+
val option = StringSetting("-option", "opt", "Some option", "none")
205+
val args = List("-option:something", "-option:something")
206+
val summary = Settings.processArguments(args, processAll = true)
207+
assertTrue("Multiple options is not an error", summary.errors.isEmpty)
208+
assertTrue("Multiple consistent options is not a warning", summary.warnings.isEmpty)
209+
210+
@Test def `string option must be consistent`: Unit =
211+
object Settings extends SettingGroup:
212+
val option = StringSetting("-option", "opt", "Some option", "none")
213+
val args = List("-option:something", "-option:nothing")
214+
val summary = Settings.processArguments(args, processAll = true)
215+
assertTrue("Multiple options is not an error", summary.errors.isEmpty)
216+
assertFalse("Multiple conflicting options is a warning", summary.warnings.isEmpty)
217+
assertTrue(summary.warnings.forall(_.contains("updated")))
218+
219+
// use the supplied summary for evaluating settings
185220
private def withProcessedArgs(summary: ArgsSummary)(f: SettingsState ?=> Unit) = f(using summary.sstate)
186221

222+
// evaluate a setting using only a SettingsState (instead of a full-blown Context)
187223
extension [T](setting: Setting[T])
188224
private def value(using ss: SettingsState): T = setting.valueIn(ss)
189225
}

0 commit comments

Comments
 (0)