From 94b938bb290a231694e5721368023bd6693bb2ed Mon Sep 17 00:00:00 2001 From: Som Snytt Date: Thu, 15 Sep 2016 23:16:40 -0700 Subject: SI-8040 Warn unused flags Introduce `-Ywarn-unused:x,y,z` and exploit `-Ywarn-unused:patvars`. Although the tree attachment for shielding patvars from warnings is not structural, sneaking the settings flag into the reflection internal TreeGen is awkward. Add test to ensure isolation of patvars warning from others. `-Ywarn-unused-import` is an alias for `-Ywarn-unused:imports`. `-Xlint:unused` is an alias for `-Ywarn-unused`, but not enabled yet. The help text advises to use `-Ywarn-unused`. The future can decide if `-Xlint:unused-imports` is warranted. --- .../reflect/reify/codegen/GenAnnotationInfos.scala | 1 - src/compiler/scala/tools/nsc/ast/TreeGen.scala | 2 + .../scala/tools/nsc/ast/parser/Parsers.scala | 3 +- .../scala/tools/nsc/settings/Warnings.scala | 40 ++++++++++++++--- .../scala/tools/nsc/typechecker/Analyzer.scala | 2 +- .../tools/nsc/typechecker/TypeDiagnostics.scala | 19 +++++--- .../scala/tools/reflect/FormatInterpolator.scala | 3 +- src/reflect/scala/reflect/internal/TreeGen.scala | 10 ++++- .../warn-unused-imports_2.scala | 2 +- test/files/neg/warn-unused-patvars.check | 12 +++++ test/files/neg/warn-unused-patvars.flags | 1 + test/files/neg/warn-unused-patvars.scala | 51 ++++++++++++++++++++++ 12 files changed, 126 insertions(+), 20 deletions(-) create mode 100644 test/files/neg/warn-unused-patvars.check create mode 100644 test/files/neg/warn-unused-patvars.flags create mode 100644 test/files/neg/warn-unused-patvars.scala diff --git a/src/compiler/scala/reflect/reify/codegen/GenAnnotationInfos.scala b/src/compiler/scala/reflect/reify/codegen/GenAnnotationInfos.scala index 2dded48251..089f07de06 100644 --- a/src/compiler/scala/reflect/reify/codegen/GenAnnotationInfos.scala +++ b/src/compiler/scala/reflect/reify/codegen/GenAnnotationInfos.scala @@ -10,7 +10,6 @@ trait GenAnnotationInfos { // however, when reifying free and tough types, we're forced to reify annotation infos as is // why is that bad? take a look inside def reifyAnnotationInfo(ann: AnnotationInfo): Tree = { - //val reifiedArgs = ann.args map // SI-8915 ann.args.foreach { arg => val saved1 = reifyTreeSymbols val saved2 = reifyTreeTypes diff --git a/src/compiler/scala/tools/nsc/ast/TreeGen.scala b/src/compiler/scala/tools/nsc/ast/TreeGen.scala index 762456c9c9..a2c37b59fb 100644 --- a/src/compiler/scala/tools/nsc/ast/TreeGen.scala +++ b/src/compiler/scala/tools/nsc/ast/TreeGen.scala @@ -371,4 +371,6 @@ abstract class TreeGen extends scala.reflect.internal.TreeGen with TreeDSL { Typed(New(anonClass.tpe), TypeTree(fun.tpe))) } } + + override def isPatVarWarnable = settings.warnUnusedPatVars } diff --git a/src/compiler/scala/tools/nsc/ast/parser/Parsers.scala b/src/compiler/scala/tools/nsc/ast/parser/Parsers.scala index ba1659d274..68bd663ab8 100644 --- a/src/compiler/scala/tools/nsc/ast/parser/Parsers.scala +++ b/src/compiler/scala/tools/nsc/ast/parser/Parsers.scala @@ -1950,7 +1950,7 @@ self => def pattern2(): Tree = { val p = pattern3() - if (in.token != AT) p + if (in.token != AT) p // var pattern upgraded later to x @ _ else p match { case Ident(nme.WILDCARD) => in.nextToken() @@ -1962,6 +1962,7 @@ self => val t = Bind(name, body) body match { case Ident(nme.WILDCARD) => t updateAttachment AtBoundIdentifierAttachment + case _ if !settings.warnUnusedPatVars => t updateAttachment AtBoundIdentifierAttachment case _ => t } } diff --git a/src/compiler/scala/tools/nsc/settings/Warnings.scala b/src/compiler/scala/tools/nsc/settings/Warnings.scala index 87534656f9..1a16480149 100644 --- a/src/compiler/scala/tools/nsc/settings/Warnings.scala +++ b/src/compiler/scala/tools/nsc/settings/Warnings.scala @@ -20,10 +20,35 @@ trait Warnings { val warnDeadCode = BooleanSetting("-Ywarn-dead-code", "Warn when dead code is identified.") val warnValueDiscard = BooleanSetting("-Ywarn-value-discard", "Warn when non-Unit expression results are unused.") val warnNumericWiden = BooleanSetting("-Ywarn-numeric-widen", "Warn when numerics are widened.") - // SI-7712, SI-7707 warnUnused not quite ready for prime-time - val warnUnused = BooleanSetting("-Ywarn-unused", "Warn when local and private vals, vars, defs, and types are unused.") - // currently considered too noisy for general use - val warnUnusedImport = BooleanSetting("-Ywarn-unused-import", "Warn when imports are unused.") + + object UnusedWarnings extends MultiChoiceEnumeration { + val Imports = Choice("imports", "Warn if an import selector is not referenced.") + val PatVars = Choice("patvars", "Warn if a variable bound in a pattern is unused.") + val Privates = Choice("privates", "Warn if a private member is unused.") + val Locals = Choice("locals", "Warn if a local definition is unused.") + val Params = Choice("params", "Warn if a value parameter is unused.") + val Implicits = Choice("implicits", "Warn if an implicit parameter is unused.") + } + + // The -Ywarn-unused warning group. + val warnUnused = MultiChoiceSetting( + name = "-Ywarn-unused", + helpArg = "warning", + descr = "Enable or disable specific `unused' warnings", + domain = UnusedWarnings, + default = Some(List("_")) + ) + + def warnUnusedImport = warnUnused contains UnusedWarnings.Imports + def warnUnusedPatVars = warnUnused contains UnusedWarnings.PatVars + def warnUnusedPrivates = warnUnused contains UnusedWarnings.Privates + def warnUnusedLocals = warnUnused contains UnusedWarnings.Locals + def warnUnusedParams = warnUnused contains UnusedWarnings.Params + def warnUnusedImplicits = warnUnused contains UnusedWarnings.Implicits + + BooleanSetting("-Ywarn-unused-import", "Warn when imports are unused.") withPostSetHook { s => + warnUnused.add(s"${if (s) "" else "-"}imports") + } // withDeprecationMessage s"Enable -Ywarn-unused:imports" val warnExtraImplicit = BooleanSetting("-Ywarn-extra-implicit", "Warn when more than one implicit parameter section is defined.") @@ -60,6 +85,8 @@ trait Warnings { val UnsoundMatch = LintWarning("unsound-match", "Pattern match may not be typesafe.") val StarsAlign = LintWarning("stars-align", "Pattern sequence wildcard must align with sequence component.") val Constant = LintWarning("constant", "Evaluation of a constant arithmetic expression results in an error.") + //val Unused = LintWarning("unused", "Warn when private and local definitions are unused.") + val Unused = LintWarning("unused", "Use -Ywarn-unused to warn when private and local definitions are unused.") def allLintWarnings = values.toSeq.asInstanceOf[Seq[LintWarning]] } @@ -82,6 +109,7 @@ trait Warnings { def warnUnsoundMatch = lint contains UnsoundMatch def warnStarsAlign = lint contains StarsAlign def warnConstant = lint contains Constant + def lintUnused = lint contains Unused // Lint warnings that are currently -Y, but deprecated in that usage @deprecated("Use warnAdaptedArgs", since="2.11.2") @@ -101,7 +129,9 @@ trait Warnings { helpArg = "warning", descr = "Enable or disable specific warnings", domain = LintWarnings, - default = Some(List("_"))) + default = Some(List("_"))) //.withPostSetHook (s => if (s contains Unused) warnUnused.add("_")) + + // restore -Xlint:unused hook when SI-8040 is complete allLintWarnings foreach { case w if w.yAliased => diff --git a/src/compiler/scala/tools/nsc/typechecker/Analyzer.scala b/src/compiler/scala/tools/nsc/typechecker/Analyzer.scala index 323fe1c171..b8ef439e03 100644 --- a/src/compiler/scala/tools/nsc/typechecker/Analyzer.scala +++ b/src/compiler/scala/tools/nsc/typechecker/Analyzer.scala @@ -104,7 +104,7 @@ trait Analyzer extends AnyRef for (workItem <- unit.toCheck) workItem() if (settings.warnUnusedImport) warnUnusedImports(unit) - if (settings.warnUnused) + if (settings.warnUnused.isSetByUser) typer checkUnused unit } finally { diff --git a/src/compiler/scala/tools/nsc/typechecker/TypeDiagnostics.scala b/src/compiler/scala/tools/nsc/typechecker/TypeDiagnostics.scala index f344364a75..b263a0fffd 100644 --- a/src/compiler/scala/tools/nsc/typechecker/TypeDiagnostics.scala +++ b/src/compiler/scala/tools/nsc/typechecker/TypeDiagnostics.scala @@ -505,7 +505,7 @@ trait TypeDiagnostics { // Only record type references which don't originate within the // definition of the class being referenced. if (t.tpe ne null) { - for (tp <- t.tpe ; if !treeTypes(tp) && !currentOwner.ownerChain.contains(tp.typeSymbol)) { + for (tp <- t.tpe if !treeTypes(tp) && !currentOwner.ownerChain.contains(tp.typeSymbol)) { tp match { case NoType | NoPrefix => case NullaryMethodType(_) => @@ -557,12 +557,17 @@ trait TypeDiagnostics { def unsetVars = localVars filter (v => !setVars(v) && !isUnusedTerm(v)) } - def apply(unit: CompilationUnit) = { + private def warningsEnabled: Boolean = { + val ss = settings + import ss._ + warnUnusedPatVars || warnUnusedPrivates || warnUnusedLocals || warnUnusedParams || warnUnusedImplicits + } + + def apply(unit: CompilationUnit): Unit = if (warningsEnabled) { val p = new UnusedPrivates p traverse unit.body - val unused = p.unusedTerms - unused foreach { defn: DefTree => - val sym = defn.symbol + for (defn: DefTree <- p.unusedTerms) { + val sym = defn.symbol val pos = ( if (defn.pos.isDefined) defn.pos else if (sym.pos.isDefined) sym.pos @@ -591,10 +596,10 @@ trait TypeDiagnostics { ) reporter.warning(pos, s"$why $what in ${sym.owner} is never used") } - p.unsetVars foreach { v => + for (v <- p.unsetVars) { reporter.warning(v.pos, s"local var ${v.name} in ${v.owner} is never set - it could be a val") } - p.unusedTypes foreach { t => + for (t <- p.unusedTypes) { val sym = t.symbol val why = if (sym.isPrivate) "private" else "local" reporter.warning(t.pos, s"$why ${sym.fullLocationString} is never used") diff --git a/src/compiler/scala/tools/reflect/FormatInterpolator.scala b/src/compiler/scala/tools/reflect/FormatInterpolator.scala index bbb2987679..857b733f59 100644 --- a/src/compiler/scala/tools/reflect/FormatInterpolator.scala +++ b/src/compiler/scala/tools/reflect/FormatInterpolator.scala @@ -19,7 +19,6 @@ abstract class FormatInterpolator { @inline private def truly(body: => Unit): Boolean = { body ; true } @inline private def falsely(body: => Unit): Boolean = { body ; false } - //private def fail(msg: String) = c.abort(c.enclosingPosition, msg) private def bail(msg: String) = global.abort(msg) def interpolate: Tree = c.macroApplication match { @@ -94,7 +93,7 @@ abstract class FormatInterpolator { case '\f' => "\\f" case '\r' => "\\r" case '\"' => "$" /* avoid lint warn */ + - "{'\"'} or a triple-quoted literal \"\"\"with embedded \" or \\u0022\"\"\"" // $" in future? + "{'\"'} or a triple-quoted literal \"\"\"with embedded \" or \\u0022\"\"\"" case '\'' => "'" case '\\' => """\\""" case x => "\\u%04x" format x diff --git a/src/reflect/scala/reflect/internal/TreeGen.scala b/src/reflect/scala/reflect/internal/TreeGen.scala index 373758d0a5..ade9ee84ac 100644 --- a/src/reflect/scala/reflect/internal/TreeGen.scala +++ b/src/reflect/scala/reflect/internal/TreeGen.scala @@ -899,8 +899,8 @@ abstract class TreeGen { case Ident(name) if treeInfo.isVarPattern(tree) && name != nme.WILDCARD => atPos(tree.pos) { val b = Bind(name, atPos(tree.pos.focus) (Ident(nme.WILDCARD))) - if (forFor) b updateAttachment AtBoundIdentifierAttachment - else b + if (!forFor && isPatVarWarnable) b + else b updateAttachment AtBoundIdentifierAttachment } case Typed(id @ Ident(name), tpt) if treeInfo.isVarPattern(id) && name != nme.WILDCARD => atPos(tree.pos.withPoint(id.pos.point)) { @@ -922,7 +922,13 @@ abstract class TreeGen { tree } } + + /** Can be overridden to depend on settings.warnUnusedPatvars. */ + def isPatVarWarnable: Boolean = true + + /** Not in for comprehensions, whether to warn unused pat vars depends on flag. */ object patvarTransformer extends PatvarTransformer(forFor = false) + /** Tag pat vars in for comprehensions. */ object patvarTransformerForFor extends PatvarTransformer(forFor = true) diff --git a/test/files/neg/warn-unused-imports/warn-unused-imports_2.scala b/test/files/neg/warn-unused-imports/warn-unused-imports_2.scala index ded1186209..56ad3393a1 100644 --- a/test/files/neg/warn-unused-imports/warn-unused-imports_2.scala +++ b/test/files/neg/warn-unused-imports/warn-unused-imports_2.scala @@ -96,7 +96,7 @@ trait Warn { trait Nested { { import p1._ // warn - trait Warn { // warn about unused local trait for good measure + trait Warn { // don't warn about unused local trait with -Ywarn-unused:imports import p2._ println(new A) println("abc".bippy) diff --git a/test/files/neg/warn-unused-patvars.check b/test/files/neg/warn-unused-patvars.check new file mode 100644 index 0000000000..002f7a07ca --- /dev/null +++ b/test/files/neg/warn-unused-patvars.check @@ -0,0 +1,12 @@ +warn-unused-patvars.scala:7: warning: private val x in trait Boundings is never used + private val x = 42 // warn, sanity check + ^ +warn-unused-patvars.scala:26: warning: local val x in method v is never used + val D(x) = d // warn, fixme + ^ +warn-unused-patvars.scala:30: warning: local val x in method w is never used + val D(x @ _) = d // warn, fixme (valdef pos is different) + ^ +error: No warnings can be incurred under -Xfatal-warnings. +three warnings found +one error found diff --git a/test/files/neg/warn-unused-patvars.flags b/test/files/neg/warn-unused-patvars.flags new file mode 100644 index 0000000000..d5bd86a658 --- /dev/null +++ b/test/files/neg/warn-unused-patvars.flags @@ -0,0 +1 @@ +-Ywarn-unused:-patvars,_ -Xfatal-warnings diff --git a/test/files/neg/warn-unused-patvars.scala b/test/files/neg/warn-unused-patvars.scala new file mode 100644 index 0000000000..6f4620c0c7 --- /dev/null +++ b/test/files/neg/warn-unused-patvars.scala @@ -0,0 +1,51 @@ + +case class C(a: Int, b: String, c: Option[String]) +case class D(a: Int) + +trait Boundings { + + private val x = 42 // warn, sanity check + + def c = C(42, "hello", Some("world")) + def d = D(42) + + def f() = { + val C(x, y, Some(z)) = c // no warn + 17 + } + def g() = { + val C(x @ _, y @ _, Some(z @ _)) = c // no warn + 17 + } + def h() = { + val C(x @ _, y @ _, z @ Some(_)) = c // no warn for z? + 17 + } + + def v() = { + val D(x) = d // warn, fixme + 17 + } + def w() = { + val D(x @ _) = d // warn, fixme (valdef pos is different) + 17 + } + +} + +trait Forever { + def f = { + val t = Option((17, 42)) + for { + ns <- t + (i, j) = ns // no warn + } yield (i + j) + } + def g = { + val t = Option((17, 42)) + for { + ns <- t + (i, j) = ns // no warn + } yield 42 + } +} -- cgit v1.2.3