Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the -Wall option that enables all warnings #20545

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 37 additions & 26 deletions compiler/src/dotty/tools/dotc/config/ScalaSettings.scala
Original file line number Diff line number Diff line change
Expand Up @@ -158,43 +158,54 @@ private sealed trait WarningSettings:

val Whelp: Setting[Boolean] = BooleanSetting(WarningSetting, "W", "Print a synopsis of warning options.")
val XfatalWarnings: Setting[Boolean] = BooleanSetting(WarningSetting, "Werror", "Fail the compilation if there are any warnings.", aliases = List("-Xfatal-warnings"))
val WvalueDiscard: Setting[Boolean] = BooleanSetting(WarningSetting, "Wvalue-discard", "Warn when non-Unit expression results are unused.")
val WNonUnitStatement = BooleanSetting(WarningSetting, "Wnonunit-statement", "Warn when block statements are non-Unit expressions.")
val WenumCommentDiscard = BooleanSetting(WarningSetting, "Wenum-comment-discard", "Warn when a comment ambiguously assigned to multiple enum cases is discarded.")
val WimplausiblePatterns = BooleanSetting(WarningSetting, "Wimplausible-patterns", "Warn if comparison with a pattern value looks like it might always fail.")
val WunstableInlineAccessors = BooleanSetting(WarningSetting, "WunstableInlineAccessors", "Warn an inline methods has references to non-stable binary APIs.")
val Wall: Setting[Boolean] = BooleanSetting(WarningSetting, "Wall", "Enable all warnings.")
private val overrideWithWall: (Setting[Boolean], (Boolean, Any) => Boolean) =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel overrideWithWall and all the warning settings are over-engineered. I think a better solution is to create an object which reads all warning settings properly and stores in ctx.

Copy link
Contributor Author

@Linyxus Linyxus Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the suggestion! I'd appreciate some clarification: does the proposed solution mean that we create another object that takes Wall into consideration when reading all warning settings and store that object in the context (that object could be similar to WunusedHas)? Then at all use sites of these warning settings we read from that object instead?

(Wall, (a, b) => a || b.asInstanceOf[Boolean])
val WvalueDiscard: Setting[Boolean] = BooleanSetting(WarningSetting, "Wvalue-discard", "Warn when non-Unit expression results are unused.", overriddenBy = overrideWithWall)
val WNonUnitStatement = BooleanSetting(WarningSetting, "Wnonunit-statement", "Warn when block statements are non-Unit expressions.", overriddenBy = overrideWithWall)
val WenumCommentDiscard = BooleanSetting(WarningSetting, "Wenum-comment-discard", "Warn when a comment ambiguously assigned to multiple enum cases is discarded.", overriddenBy = overrideWithWall)
val WimplausiblePatterns = BooleanSetting(WarningSetting, "Wimplausible-patterns", "Warn if comparison with a pattern value looks like it might always fail.", overriddenBy = overrideWithWall)
val WunstableInlineAccessors = BooleanSetting(WarningSetting, "WunstableInlineAccessors", "Warn an inline methods has references to non-stable binary APIs.", overriddenBy = overrideWithWall)
val WunusedAll = ChoiceWithHelp("all", "")
private val overrideWunusedWithWall: (Setting[Boolean], (List[ChoiceWithHelp[String]], Any) => List[ChoiceWithHelp[String]]) =
def updateValue(fromUser: List[ChoiceWithHelp[String]], fromWall: Any): List[ChoiceWithHelp[String]] =
if fromUser.isEmpty && fromWall.asInstanceOf[Boolean] then
List(WunusedAll)
else fromUser
(Wall, updateValue)
val Wunused: Setting[List[ChoiceWithHelp[String]]] = MultiChoiceHelpSetting(
WarningSetting,
name = "Wunused",
helpArg = "warning",
descr = "Enable or disable specific `unused` warnings",
choices = List(
ChoiceWithHelp("nowarn", ""),
ChoiceWithHelp("all",""),
WunusedAll,
ChoiceWithHelp(
name = "imports",
description = "Warn if an import selector is not referenced.\n" +
"NOTE : overrided by -Wunused:strict-no-implicit-warn"),
ChoiceWithHelp("privates","Warn if a private member is unused"),
ChoiceWithHelp("locals","Warn if a local definition is unused"),
ChoiceWithHelp("explicits","Warn if an explicit parameter is unused"),
ChoiceWithHelp("implicits","Warn if an implicit parameter is unused"),
ChoiceWithHelp("params","Enable -Wunused:explicits,implicits"),
ChoiceWithHelp("linted","Enable -Wunused:imports,privates,locals,implicits"),
ChoiceWithHelp(
name = "strict-no-implicit-warn",
description = "Same as -Wunused:import, only for imports of explicit named members.\n" +
"NOTE : This overrides -Wunused:imports and NOT set by -Wunused:all"
),
// ChoiceWithHelp("patvars","Warn if a variable bound in a pattern is unused"),
ChoiceWithHelp(
name = "unsafe-warn-patvars",
description = "(UNSAFE) Warn if a variable bound in a pattern is unused.\n" +
"This warning can generate false positive, as warning cannot be\n" +
"suppressed yet."
)
ChoiceWithHelp("privates","Warn if a private member is unused"),
ChoiceWithHelp("locals","Warn if a local definition is unused"),
ChoiceWithHelp("explicits","Warn if an explicit parameter is unused"),
ChoiceWithHelp("implicits","Warn if an implicit parameter is unused"),
ChoiceWithHelp("params","Enable -Wunused:explicits,implicits"),
ChoiceWithHelp("linted","Enable -Wunused:imports,privates,locals,implicits"),
ChoiceWithHelp(
name = "strict-no-implicit-warn",
description = "Same as -Wunused:import, only for imports of explicit named members.\n" +
"NOTE : This overrides -Wunused:imports and NOT set by -Wunused:all"
),
// ChoiceWithHelp("patvars","Warn if a variable bound in a pattern is unused"),
ChoiceWithHelp(
name = "unsafe-warn-patvars",
description = "(UNSAFE) Warn if a variable bound in a pattern is unused.\n" +
"This warning can generate false positive, as warning cannot be\n" +
"suppressed yet."
)
),
default = Nil
default = Nil,
overriddenBy = overrideWunusedWithWall,
)
object WunusedHas:
def isChoiceSet(s: String)(using Context) = Wunused.value.pipe(us => us.contains(s))
Expand Down Expand Up @@ -296,7 +307,7 @@ private sealed trait WarningSettings:
def typeParameterShadow(using Context) =
allOr("type-parameter-shadow")

val WcheckInit: Setting[Boolean] = BooleanSetting(WarningSetting, "Wsafe-init", "Ensure safe initialization of objects.")
val WcheckInit: Setting[Boolean] = BooleanSetting(WarningSetting, "Wsafe-init", "Ensure safe initialization of objects.", overriddenBy = overrideWithWall)

/** -X "Extended" or "Advanced" settings */
private sealed trait XSettings:
Expand Down
21 changes: 15 additions & 6 deletions compiler/src/dotty/tools/dotc/config/Settings.scala
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,9 @@ object Settings:
// kept only for -Xkind-projector option compatibility
legacyArgs: Boolean = false,
// accept legacy choices (for example, valid in Scala 2 but no longer supported)
legacyChoices: Option[Seq[?]] = None)(private[Settings] val idx: Int):
legacyChoices: Option[Seq[?]] = None,
// override the value of this setting based on the value of another one
overriddenBy: (Setting[?], (T, Any) => T) | Null = null)(private[Settings] val idx: Int):

validateSettingString(prefix.getOrElse(name))
aliases.foreach(validateSettingString)
Expand All @@ -99,7 +101,14 @@ object Settings:

val allFullNames: List[String] = s"$name" :: s"-$name" :: aliases

def valueIn(state: SettingsState): T = state.value(idx).asInstanceOf[T]
def valueIn(state: SettingsState): T =
val thisVal = state.value(idx).asInstanceOf[T]
if overriddenBy == null then
thisVal
else
val (that, f) = overriddenBy.nn
val thatVal = that.valueIn(state)
f(thisVal, thatVal)

def updateIn(state: SettingsState, x: Any): SettingsState = x match
case _: T => state.update(idx, x)
Expand Down Expand Up @@ -378,8 +387,8 @@ object Settings:
assert(!name.startsWith("-"), s"Setting $name cannot start with -")
"-" + name

def BooleanSetting(category: SettingCategory, name: String, descr: String, initialValue: Boolean = false, aliases: List[String] = Nil, preferPrevious: Boolean = false, deprecation: Option[Deprecation] = None, ignoreInvalidArgs: Boolean = false): Setting[Boolean] =
publish(Setting(category, prependName(name), descr, initialValue, aliases = aliases, preferPrevious = preferPrevious, deprecation = deprecation, ignoreInvalidArgs = ignoreInvalidArgs))
def BooleanSetting(category: SettingCategory, name: String, descr: String, initialValue: Boolean = false, aliases: List[String] = Nil, preferPrevious: Boolean = false, deprecation: Option[Deprecation] = None, ignoreInvalidArgs: Boolean = false, overriddenBy: (Setting[?], (Boolean, Any) => Boolean) | Null = null): Setting[Boolean] =
publish(Setting(category, prependName(name), descr, initialValue, aliases = aliases, preferPrevious = preferPrevious, deprecation = deprecation, ignoreInvalidArgs = ignoreInvalidArgs, overriddenBy = overriddenBy))

def StringSetting(category: SettingCategory, name: String, helpArg: String, descr: String, default: String, aliases: List[String] = Nil, deprecation: Option[Deprecation] = None): Setting[String] =
publish(Setting(category, prependName(name), descr, default, helpArg, aliases = aliases, deprecation = deprecation))
Expand All @@ -390,8 +399,8 @@ object Settings:
def MultiChoiceSetting(category: SettingCategory, name: String, helpArg: String, descr: String, choices: List[String], default: List[String] = Nil, legacyChoices: List[String] = Nil, aliases: List[String] = Nil, deprecation: Option[Deprecation] = None): Setting[List[String]] =
publish(Setting(category, prependName(name), descr, default, helpArg, Some(choices), legacyChoices = Some(legacyChoices), aliases = aliases, deprecation = deprecation))

def MultiChoiceHelpSetting(category: SettingCategory, name: String, helpArg: String, descr: String, choices: List[ChoiceWithHelp[String]], default: List[ChoiceWithHelp[String]], legacyChoices: List[String] = Nil, aliases: List[String] = Nil, deprecation: Option[Deprecation] = None): Setting[List[ChoiceWithHelp[String]]] =
publish(Setting(category, prependName(name), descr, default, helpArg, Some(choices), legacyChoices = Some(legacyChoices), aliases = aliases, deprecation = deprecation))
def MultiChoiceHelpSetting(category: SettingCategory, name: String, helpArg: String, descr: String, choices: List[ChoiceWithHelp[String]], default: List[ChoiceWithHelp[String]], legacyChoices: List[String] = Nil, aliases: List[String] = Nil, deprecation: Option[Deprecation] = None, overriddenBy: (Setting[?], (List[ChoiceWithHelp[String]], Any) => List[ChoiceWithHelp[String]]) | Null = null): Setting[List[ChoiceWithHelp[String]]] =
publish(Setting(category, prependName(name), descr, default, helpArg, Some(choices), legacyChoices = Some(legacyChoices), aliases = aliases, deprecation = deprecation, overriddenBy = overriddenBy))

def IntSetting(category: SettingCategory, name: String, descr: String, default: Int, aliases: List[String] = Nil, deprecation: Option[Deprecation] = None): Setting[Int] =
publish(Setting(category, prependName(name), descr, default, aliases = aliases, deprecation = deprecation))
Expand Down
12 changes: 12 additions & 0 deletions tests/warn/i18559a.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
-- [E198] Unused Symbol Warning: tests/warn/i18559a.scala:4:28 ---------------------------------------------------------
4 | import collection.mutable.Set // warn
| ^^^
| unused import
-- [E198] Unused Symbol Warning: tests/warn/i18559a.scala:8:8 ----------------------------------------------------------
8 | val x = 1 // warn
| ^
| unused local definition
-- [E198] Unused Symbol Warning: tests/warn/i18559a.scala:11:26 --------------------------------------------------------
11 | import SomeGivenImports.given // warn
| ^^^^^
| unused import
15 changes: 15 additions & 0 deletions tests/warn/i18559a.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
//> using options -Wall
// This test checks that -Wall turns on -Wunused:all if -Wunused is not set
object FooImportUnused:
import collection.mutable.Set // warn

object FooUnusedLocal:
def test(): Unit =
val x = 1 // warn

object FooGivenUnused:
import SomeGivenImports.given // warn

object SomeGivenImports:
given Int = 0
given String = "foo"
12 changes: 12 additions & 0 deletions tests/warn/i18559b.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
-- Warning: tests/warn/i18559b.scala:8:6 -------------------------------------------------------------------------------
8 | val localFile: String = s"${url.##}.tmp" // warn
| ^
| Access non-initialized value localFile. Calling trace:
| ├── class RemoteFile(url: String) extends AbstractFile: [ i18559b.scala:7 ]
| │ ^
| ├── abstract class AbstractFile: [ i18559b.scala:3 ]
| │ ^
| ├── val extension: String = name.substring(4) [ i18559b.scala:5 ]
| │ ^^^^
| └── def name: String = localFile [ i18559b.scala:9 ]
| ^^^^^^^^^
9 changes: 9 additions & 0 deletions tests/warn/i18559b.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
//> using options -Wall
// This test checks that -Wall turns on -Wsafe-init
abstract class AbstractFile:
def name: String
val extension: String = name.substring(4)

class RemoteFile(url: String) extends AbstractFile:
val localFile: String = s"${url.##}.tmp" // warn
def name: String = localFile
4 changes: 4 additions & 0 deletions tests/warn/i18559c.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
-- [E198] Unused Symbol Warning: tests/warn/i18559c.scala:8:8 ----------------------------------------------------------
8 | val x = 1 // warn
| ^
| unused local definition
15 changes: 15 additions & 0 deletions tests/warn/i18559c.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
//> using options -Wall -Wunused:locals
// This test checks that -Wall leaves -Wunused:... untouched if it is already set
object FooImportUnused:
import collection.mutable.Set // not warn

object FooUnusedLocal:
def test(): Unit =
val x = 1 // warn

object FooGivenUnused:
import SomeGivenImports.given // not warn

object SomeGivenImports:
given Int = 0
given String = "foo"
Loading