Skip to content

Commit 2294533

Browse files
som-snyttsom-snytt
committed
Clarify option sanity checks
1 parent ce13350 commit 2294533

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
@@ -39,18 +39,12 @@ object Settings:
3939
this
4040
end SettingsState
4141

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

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

5549
case class Setting[T: ClassTag] private[Settings] (
5650
name: String,
@@ -73,7 +67,7 @@ object Settings:
7367

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

76-
def isMultivalue: Boolean = implicitly[ClassTag[T]] == ListTag
70+
def isMultivalue: Boolean = summon[ClassTag[T]] == ListTag
7771

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

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

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

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

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

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

166+
// begin
172167
if (prefix != "" && arg.startsWith(prefix))
173-
doSet(arg drop prefix.length)
168+
doSet(arg.drop(prefix.length))
174169
else if (prefix == "" && matches(arg.takeWhile(_ != ':')))
175170
doSet(arg.dropWhile(_ != ':').drop(1))
176171
else
@@ -237,7 +232,7 @@ object Settings:
237232
if state1 ne state then state1
238233
else loop(settings1)
239234
case Nil =>
240-
state.warn(s"bad option '$x' was ignored")
235+
state.warn(s"bad option '$x' was ignored").skip()
241236
processArguments(loop(allSettings.toList), processAll, skipped)
242237
case arg :: args =>
243238
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
@@ -25,9 +25,9 @@ class SettingsTests {
2525

2626
@Test def jarOutput: Unit =
2727
val source = "tests/pos/Foo.scala"
28-
val out = Paths.get("out/jaredFoo.jar").normalize
29-
if (Files.exists(out)) Files.delete(out)
30-
val options = Array("-classpath", TestConfiguration.basicClasspath, "-d", out.toString, source)
28+
val out = Paths.get("out/jarredFoo.jar").normalize
29+
if Files.exists(out) then Files.delete(out)
30+
val options = Array("-Xmain-class", "Jarred", "-classpath", TestConfiguration.basicClasspath, "-d", out.toString, source)
3131
val reporter = Main.process(options)
3232
assertEquals(0, reporter.errorCount)
3333
assertTrue(Files.exists(out))
@@ -47,7 +47,7 @@ class SettingsTests {
4747
val bar = IntSetting("-bar", "Bar", 0)
4848

4949
val args = List("-foo", "b", "-bar", "1")
50-
val summary = Settings.processArguments(args, true)
50+
val summary = Settings.processArguments(args, processAll = true)
5151
assertTrue(summary.errors.isEmpty)
5252
withProcessedArgs(summary) {
5353
assertEquals("b", Settings.foo.value)
@@ -72,17 +72,17 @@ class SettingsTests {
7272

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

7777
val limit = 6000
78-
val args = List.fill(limit)("-option")
78+
val args = List.tabulate(limit)(i => if (i % 2 == 0) "-option" else i.toString)
7979
val summary = Settings.processArguments(args, processAll = true)
8080
assertTrue(summary.errors.isEmpty)
81-
assertEquals(limit-1, summary.warnings.size)
82-
assertTrue(summary.warnings.head.contains("repeatedly"))
81+
assertEquals(limit/2 - 1, summary.warnings.size) // should warn on all but first
82+
assertTrue(summary.warnings.head.contains("was updated"))
8383
assertEquals(0, summary.arguments.size)
8484
withProcessedArgs(summary) {
85-
assertTrue(Settings.option.value)
85+
assertEquals("5999", Settings.option.value.toString)
8686
}
8787

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

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

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

0 commit comments

Comments
 (0)