From f33a40e18b3f5203055983e465af6e100587e309 Mon Sep 17 00:00:00 2001 From: Som Snytt Date: Mon, 21 Feb 2022 20:53:11 -0800 Subject: [PATCH] Add -Wnonunit-statement and -Wnonunit-if Warn about any interesting value in statement position, for some definition of interest. Generalizes the current warning for "pure" expressions. By default, warns about unibranch if, which lack an else in user code. Specify -Wnonunit-if:false to turn off warning about unibranch if. (That also turns off -Wvalue-discard for those cases.) --- .../scala/tools/nsc/ast/parser/Parsers.scala | 18 +- .../scala/tools/nsc/settings/Warnings.scala | 4 + .../tools/nsc/typechecker/RefChecks.scala | 90 +++++++- .../nsc/typechecker/StdAttachments.scala | 4 - .../scala/tools/nsc/typechecker/Typers.scala | 16 +- .../reflect/internal/StdAttachments.scala | 4 + .../scala/reflect/internal/TreeInfo.scala | 33 +++ .../reflect/runtime/JavaUniverseForce.scala | 1 + test/files/neg/nonunit-if.check | 60 ++++++ test/files/neg/nonunit-if.scala | 172 ++++++++++++++++ test/files/neg/nonunit-statement.check | 42 ++++ test/files/neg/nonunit-statement.scala | 194 ++++++++++++++++++ test/files/neg/value-discard.check | 6 +- test/files/neg/value-discard.scala | 14 +- .../files/neg/virtpatmat_unreach_select.scala | 2 +- 15 files changed, 623 insertions(+), 37 deletions(-) create mode 100644 test/files/neg/nonunit-if.check create mode 100644 test/files/neg/nonunit-if.scala create mode 100644 test/files/neg/nonunit-statement.check create mode 100644 test/files/neg/nonunit-statement.scala diff --git a/src/compiler/scala/tools/nsc/ast/parser/Parsers.scala b/src/compiler/scala/tools/nsc/ast/parser/Parsers.scala index 8357a3330b1b..c740cf6198f0 100644 --- a/src/compiler/scala/tools/nsc/ast/parser/Parsers.scala +++ b/src/compiler/scala/tools/nsc/ast/parser/Parsers.scala @@ -1617,8 +1617,22 @@ self => val cond = condExpr() newLinesOpt() val thenp = expr() - val elsep = if (in.token == ELSE) { in.nextToken(); expr() } - else literalUnit + val elsep = + if (in.token == ELSE) { + in.nextToken() + expr() + } + else { + // user asked to silence warnings on unibranch if; also suppresses value discard + if (settings.warnNonUnitIf.isSetByUser && !settings.warnNonUnitIf.value) { + thenp match { + case Block(_, res) => res.updateAttachment(TypedExpectingUnitAttachment) + case _ => () + } + thenp.updateAttachment(TypedExpectingUnitAttachment) + } + literalUnit + } If(cond, thenp, elsep) } parseIf diff --git a/src/compiler/scala/tools/nsc/settings/Warnings.scala b/src/compiler/scala/tools/nsc/settings/Warnings.scala index a48fafe02746..67ad9f9014dd 100644 --- a/src/compiler/scala/tools/nsc/settings/Warnings.scala +++ b/src/compiler/scala/tools/nsc/settings/Warnings.scala @@ -113,6 +113,10 @@ trait Warnings { ) ) withAbbreviation "-Ywarn-macros" val warnDeadCode = BooleanSetting("-Wdead-code", "Warn when dead code is identified.") withAbbreviation "-Ywarn-dead-code" + val warnNonUnitIf = BooleanSetting("-Wnonunit-if", "Warn when if statements are non-Unit expressions, enabled by -Wnonunit-statement.") + import scala.language.existentials + val warnNonUnitStatement = BooleanSetting("-Wnonunit-statement", "Warn when block statements are non-Unit expressions.") + .enablingIfNotSetByUser(warnNonUnitIf :: Nil) val warnValueDiscard = BooleanSetting("-Wvalue-discard", "Warn when non-Unit expression results are unused.") withAbbreviation "-Ywarn-value-discard" val warnNumericWiden = BooleanSetting("-Wnumeric-widen", "Warn when numerics are widened.") withAbbreviation "-Ywarn-numeric-widen" val warnOctalLiteral = BooleanSetting("-Woctal-literal", "Warn on obsolete octal syntax.") withAbbreviation "-Ywarn-octal-literal" diff --git a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala index 1ba0bc494ef0..034d04119ad6 100644 --- a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala +++ b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala @@ -21,6 +21,8 @@ import scala.tools.nsc.settings.NoScalaVersion import symtab.Flags._ import transform.Transform +import PartialFunction.cond + /** Post-attribution checking and transformation. * * This phase checks the following postconditions: @@ -1723,19 +1725,81 @@ abstract class RefChecks extends Transform { } // Verify classes extending AnyVal meet the requirements - private def checkAnyValSubclass(clazz: Symbol) = { + private def checkAnyValSubclass(clazz: Symbol) = if (clazz.isDerivedValueClass) { if (clazz.isTrait) reporter.error(clazz.pos, "Only classes (not traits) are allowed to extend AnyVal") else if (clazz.hasAbstractFlag) reporter.error(clazz.pos, "`abstract` modifier cannot be used with value classes") } - } private def checkUnexpandedMacro(t: Tree) = if (!t.isDef && t.hasSymbolField && t.symbol.isTermMacro) reporter.error(t.pos, "macro has not been expanded") + // if expression in statement position (of template or block) + // looks like a useful value that should not be ignored, warn and return true + // User specifies that an expression is boring by ascribing `e: Unit`. + // The subtree `e` will bear an attachment, but may be wrapped in adaptations. + private def checkInterestingResultInStatement(t: Tree): Boolean = { + def isUninterestingSymbol(sym: Symbol): Boolean = + sym != null && ( + sym.isConstructor || + sym.hasPackageFlag || + sym.isPackageObjectOrClass || + sym == BoxedUnitClass || + sym == AnyClass || + sym == AnyRefClass || + sym == AnyValClass + ) + def isUninterestingType(tpe: Type): Boolean = + tpe != null && ( + isUnitType(tpe) || + tpe.typeSymbol.isBottomClass || + tpe =:= UnitTpe || + tpe =:= BoxedUnitTpe || + isTrivialTopType(tpe) + ) + // java lacks this.type idiom to distinguish side-effecting method, so ignore result of invoking java method. + def isJavaApplication(t: Tree): Boolean = cond(t) { + case Apply(f, _) => f.symbol.isJavaDefined && !isUniversalMember(f.symbol) + } + // The quirk of typechecking if is that the LUB often results in boring types. + // Parser adds suppressing attatchment on `if (b) expr` when user has `-Wnonunit-if:false`. + def checkInterestingShapes(t: Tree): Boolean = + t match { + case If(_, thenpart, elsepart) => checkInterestingShapes(thenpart) || checkInterestingShapes(elsepart) + //case Block(_, Apply(label, Nil)) if label.symbol != null && nme.isLoopHeaderLabel(label.symbol.name) => false + case Block(_, res) => checksForInterestingResult(res) + case _ => checksForInterestingResult(t) + } + // tests for various flavors of blandness in expressions. + def checksForInterestingResult(t: Tree): Boolean = ( + !t.isDef && !treeInfo.isPureDef(t) // ignore defs + && !isUninterestingSymbol(t.symbol) // ctors, package, Unit, Any + && !isUninterestingType(t.tpe) // bottom types, Unit, Any + && !treeInfo.isThisTypeResult(t) // buf += x + && !treeInfo.isSuperConstrCall(t) // just a thing + && !explicitlyUnit(t) // suppressed by explicit expr: Unit + && !isJavaApplication(t) // Java methods are inherently side-effecting + ) + // begin checkInterestingResultInStatement + settings.warnNonUnitStatement.value && checkInterestingShapes(t) && { + val msg = "unused value" + val where = t match { + case Block(_, res) => res + case If(_, thenpart, Literal(Constant(()))) => + thenpart match { + case Block(_, res) => res + case _ => thenpart + } + case _ => t + } + refchecksWarning(where.pos, msg, WarningCategory.OtherPureStatement) + true + } + } // end checkInterestingResultInStatement + override def transform(tree: Tree): Tree = { val savedLocalTyper = localTyper val savedCurrentApplication = currentApplication @@ -1775,14 +1839,12 @@ abstract class RefChecks extends Transform { case Template(parents, self, body) => localTyper = localTyper.atOwner(tree, currentOwner) - for (stat <- body) { - if (treeInfo.isPureExprForWarningPurposes(stat)) { + for (stat <- body) + if (!checkInterestingResultInStatement(stat) && treeInfo.isPureExprForWarningPurposes(stat)) { val msg = "a pure expression does nothing in statement position" val clause = if (body.lengthCompare(1) > 0) "; multiline expressions may require enclosing parentheses" else "" refchecksWarning(stat.pos, s"$msg$clause", WarningCategory.OtherPureStatement) } - } - validateBaseTypes(currentOwner) checkOverloadedRestrictions(currentOwner, currentOwner) // scala/bug#7870 default getters for constructors live in the companion module @@ -1849,7 +1911,15 @@ abstract class RefChecks extends Transform { // probably not, until we allow parameterised extractors tree - case Block(stats, expr) => + case blk @ Block(stats, expr) => + // if block is value discard, copy any TypedExpectingUnitAttachment to outermost Apply in case there were implicit args + blk match { + case Block((inner @ treeInfo.Applied(_, _, _)) :: Nil, Literal(Constant(()))) => + if (inner.exists(explicitlyUnit)) + inner.updateAttachment(TypedExpectingUnitAttachment) + case _ => + } + // diagnostic info val (count, result0, adapted) = expr match { case Block(expr :: Nil, Literal(Constant(()))) => (1, expr, true) @@ -1859,7 +1929,7 @@ abstract class RefChecks extends Transform { val isMultiline = stats.lengthCompare(1 - count) > 0 def checkPure(t: Tree, supple: Boolean): Unit = - if (!analyzer.explicitlyUnit(t) && treeInfo.isPureExprForWarningPurposes(t)) { + if (!explicitlyUnit(t) && treeInfo.isPureExprForWarningPurposes(t)) { val msg = "a pure expression does nothing in statement position" val parens = if (isMultiline) "multiline expressions might require enclosing parentheses" else "" val discard = if (adapted) "; a value can be silently discarded when Unit is expected" else "" @@ -1868,8 +1938,8 @@ abstract class RefChecks extends Transform { else if (!parens.isEmpty) s"$msg; $parens" else msg refchecksWarning(t.pos, text, WarningCategory.OtherPureStatement) } - // check block for unintended expr placement - stats.foreach(checkPure(_, supple = false)) + // check block for unintended "expression in statement position" + stats.foreach { t => if (!checkInterestingResultInStatement(t)) checkPure(t, supple = false) } if (result0.nonEmpty) checkPure(result0, supple = true) def checkImplicitlyAdaptedBlockResult(t: Tree): Unit = diff --git a/src/compiler/scala/tools/nsc/typechecker/StdAttachments.scala b/src/compiler/scala/tools/nsc/typechecker/StdAttachments.scala index 45b701a33c3f..88f96e34c474 100644 --- a/src/compiler/scala/tools/nsc/typechecker/StdAttachments.scala +++ b/src/compiler/scala/tools/nsc/typechecker/StdAttachments.scala @@ -196,10 +196,6 @@ trait StdAttachments { * track of other adapted trees. */ case class OriginalTreeAttachment(original: Tree) - - /** Marks a Typed tree with Unit tpt. */ - case object TypedExpectingUnitAttachment - def explicitlyUnit(tree: Tree): Boolean = tree.hasAttachment[TypedExpectingUnitAttachment.type] } diff --git a/src/compiler/scala/tools/nsc/typechecker/Typers.scala b/src/compiler/scala/tools/nsc/typechecker/Typers.scala index 97d78749730c..7eb7d2b11b08 100644 --- a/src/compiler/scala/tools/nsc/typechecker/Typers.scala +++ b/src/compiler/scala/tools/nsc/typechecker/Typers.scala @@ -750,7 +750,7 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper if (immediate) { action() } else { - unit.toCheck += (() => action()) + unit.toCheck += (() => action(): Unit) true } } @@ -1111,13 +1111,9 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper @inline def tpdPos(transformed: Tree) = typedPos(tree.pos, mode, pt)(transformed) @inline def tpd(transformed: Tree) = typed(transformed, mode, pt) - @inline def warnValueDiscard(): Unit = if (!isPastTyper && settings.warnValueDiscard) { - def isThisTypeResult = (tree, tree.tpe) match { - case (Apply(Select(receiver, _), _), SingleType(_, sym)) => sym == receiver.symbol - case _ => false - } - if (!isThisTypeResult && !explicitlyUnit(tree)) context.warning(tree.pos, "discarded non-Unit value", WarningCategory.WFlagValueDiscard) - } + @inline def warnValueDiscard(): Unit = + if (!isPastTyper && settings.warnValueDiscard.value && !treeInfo.isThisTypeResult(tree) && !explicitlyUnit(tree)) + context.warning(tree.pos, "discarded non-Unit value", WarningCategory.WFlagValueDiscard) @inline def warnNumericWiden(tpSym: Symbol, ptSym: Symbol): Unit = if (!isPastTyper) { val isInharmonic = ( @@ -1242,7 +1238,7 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper case ct @ FoldableConstantType(value) if mode.inNone(TYPEmode | FUNmode) && (ct <:< pt) && canAdaptConstantTypeToLiteral => // (0) adaptConstant(value) case OverloadedType(_, _) if !mode.inFunMode => // (1) - inferExprAlternative(tree, pt) + inferExprAlternative(tree, pt): Unit adaptAfterOverloadResolution(tree, mode, pt, original) case NullaryMethodType(restpe) => // (2) if (hasUndets && settings.lintUniversalMethods && (isCastSymbol(tree.symbol) || isTypeTestSymbol(tree.symbol)) && context.undetparams.exists(_.owner == tree.symbol)) @@ -4726,7 +4722,7 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper def typedIf(tree: If): If = { val cond1 = checkDead(context, typedByValueExpr(tree.cond, BooleanTpe)) - // One-legged ifs don't need a lot of analysis + // Unibranch if normally has unit value else, but synthetic code may emit empty else. if (tree.elsep.isEmpty) return treeCopy.If(tree, cond1, typed(tree.thenp, UnitTpe), tree.elsep) setType UnitTpe diff --git a/src/reflect/scala/reflect/internal/StdAttachments.scala b/src/reflect/scala/reflect/internal/StdAttachments.scala index 4cb68fa65322..ef85d1eea69f 100644 --- a/src/reflect/scala/reflect/internal/StdAttachments.scala +++ b/src/reflect/scala/reflect/internal/StdAttachments.scala @@ -138,4 +138,8 @@ trait StdAttachments { // Use of _root_ is in correct leading position of selection case object RootSelection extends PlainAttachment + + /** Marks a Typed tree with Unit tpt. */ + case object TypedExpectingUnitAttachment + def explicitlyUnit(tree: Tree): Boolean = tree.hasAttachment[TypedExpectingUnitAttachment.type] } diff --git a/src/reflect/scala/reflect/internal/TreeInfo.scala b/src/reflect/scala/reflect/internal/TreeInfo.scala index 3ba0f488e27a..660b935ed26c 100644 --- a/src/reflect/scala/reflect/internal/TreeInfo.scala +++ b/src/reflect/scala/reflect/internal/TreeInfo.scala @@ -351,6 +351,39 @@ abstract class TreeInfo { case _ => false } + /** Is tree an application with result `this.type`? + * Accept `b.addOne(x)` and also `xs(i) += x` + * where the op is an assignment operator. + */ + def isThisTypeResult(tree: Tree): Boolean = tree match { + case Applied(fun @ Select(receiver, op), _, _) => + tree.tpe match { + case ThisType(sym) => sym == receiver.symbol + case SingleType(_, sym) => sym == receiver.symbol + case _ => + def check(sym: Symbol): Boolean = + (sym == receiver.symbol) || { + receiver match { + case Apply(_, _) => Precedence(op.decoded).level == 0 // xs(i) += x + case _ => receiver.symbol.isGetter // xs.addOne(x) for var xs + } + } + @tailrec def loop(mt: Type): Boolean = mt match { + case MethodType(_, restpe) => + restpe match { + case ThisType(sym) => check(sym) + case SingleType(_, sym) => check(sym) + case _ => loop(restpe) + } + case PolyType(_, restpe) => loop(restpe) + case _ => false + } + loop(fun.symbol.info) + } + case _ => + tree.tpe.isInstanceOf[ThisType] + } + /** * Named arguments can transform a constructor call into a block, e.g. * (b = foo, a = bar) diff --git a/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala b/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala index 61108d372dde..b8399ec2874e 100644 --- a/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala +++ b/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala @@ -74,6 +74,7 @@ trait JavaUniverseForce { self: runtime.JavaUniverse => this.ChangeOwnerAttachment this.InterpolatedString this.RootSelection + this.TypedExpectingUnitAttachment this.noPrint this.typeDebug // inaccessible: this.posAssigner diff --git a/test/files/neg/nonunit-if.check b/test/files/neg/nonunit-if.check new file mode 100644 index 000000000000..25bf5bba42c9 --- /dev/null +++ b/test/files/neg/nonunit-if.check @@ -0,0 +1,60 @@ +nonunit-if.scala:53: warning: discarded non-Unit value + Future(42): Unit // nowarn { F(42)(ctx) }: Unit where annot is on F(42), TODO spurious -Wvalue-discard + ^ +nonunit-if.scala:60: warning: discarded non-Unit value + if (!isEmpty) f(a) // warn + ^ +nonunit-if.scala:64: warning: discarded non-Unit value + f(a) // warn + ^ +nonunit-if.scala:13: warning: unused value + improved // warn + ^ +nonunit-if.scala:21: warning: unused value + new E().toString // warn + ^ +nonunit-if.scala:28: warning: unused value + Future(42) // warn + ^ +nonunit-if.scala:32: warning: unused value + copy() // warn + ^ +nonunit-if.scala:39: warning: unused value + 27 +: xs // warn + ^ +nonunit-if.scala:46: warning: a pure expression does nothing in statement position; multiline expressions may require enclosing parentheses + null // warn for purity + ^ +nonunit-if.scala:60: warning: unused value + if (!isEmpty) f(a) // warn + ^ +nonunit-if.scala:64: warning: unused value + f(a) // warn + ^ +nonunit-if.scala:75: warning: unused value + if (!fellback) action(z) // warn, not value discard + ^ +nonunit-if.scala:83: warning: unused value + g // warn + ^ +nonunit-if.scala:81: warning: unused value + g // warn + ^ +nonunit-if.scala:88: warning: unused value + g // warn + ^ +nonunit-if.scala:86: warning: unused value + g // warn + ^ +nonunit-if.scala:98: warning: unused value + if (b) { // warn, at least one branch looks interesting + ^ +nonunit-if.scala:118: warning: unused value + set += a // warn because cannot know whether the `set` was supposed to be consumed or assigned + ^ +nonunit-if.scala:149: warning: unused value + while (it.hasNext) it.next() // warn + ^ +error: No warnings can be incurred under -Werror. +19 warnings +1 error diff --git a/test/files/neg/nonunit-if.scala b/test/files/neg/nonunit-if.scala new file mode 100644 index 000000000000..2793fdac0bd5 --- /dev/null +++ b/test/files/neg/nonunit-if.scala @@ -0,0 +1,172 @@ + +// scalac: -Werror -Wnonunit-statement -Wvalue-discard + +import collection.mutable.{LinkedHashSet, ListBuffer} +import concurrent._ + +// typo, misplaced expression during refactor +class C { + import ExecutionContext.Implicits._ + def c = { + def improved = Future(42) + def stale = Future(27) + improved // warn + stale + } +} +// edited expression not deleted +class D { + def d = { + class E + new E().toString // warn + new E().toString * 2 + } +} +// unused future in template +class F { + import ExecutionContext.Implicits._ + Future(42) // warn +} +// unused template expression uses synthetic method of class +case class K(s: String) { + copy() // warn +} +// mutations returning this are ok +class Mutate { + val b = ListBuffer.empty[Int] + b += 42 // nowarn, returns this.type + val xs = List(42) + 27 +: xs // warn + + def f(x: Int): this.type = this + def g(): Unit = f(42) // nowarn +} +// some uninteresting expressions may warn for other reasons +class WhoCares { + null // warn for purity + ??? // nowarn for impurity +} +// explicit Unit ascription to opt out of warning, even for funky applies +class Absolution { + def f(i: Int): Int = i+1 + import ExecutionContext.Implicits._ + Future(42): Unit // nowarn { F(42)(ctx) }: Unit where annot is on F(42), TODO spurious -Wvalue-discard + f(42): Unit // nowarn +} +// warn uni-branched unless user disables it with -Wnonunit-if:false +class Boxed[A](a: A) { + def isEmpty = false + def foreach[U](f: A => U): Unit = + if (!isEmpty) f(a) // warn + def forall(f: A => Boolean): Unit = + if (!isEmpty) { + println(".") + f(a) // warn + } + def take(p: A => Boolean): Option[A] = { + while (isEmpty || !p(a)) () + Some(a).filter(p) + } +} +class Unibranch[A, B] { + def runWith[U](action: B => U): A => Boolean = { x => + val z = null.asInstanceOf[B] + val fellback = false + if (!fellback) action(z) // warn, not value discard + !fellback + } + def f(i: Int): Int = { + def g = 17 + if (i < 42) { + g // warn + println("uh oh") + g // warn + } + while (i < 42) { + g // warn + println("uh oh") + g // warn + } + 42 + } +} +class Dibranch { + def i: Int = ??? + def j: Int = ??? + def f(b: Boolean): Int = { + // if-expr might have an uninteresting LUB + if (b) { // warn, at least one branch looks interesting + println("true") + i + } + else { + println("false") + j + } + 42 + } +} +class Next[A] { + val all = ListBuffer.empty[A] + def f(it: Iterator[A], g: A => A): Unit = + while (it.hasNext) + all += g(it.next()) // nowarn +} +class Setting[A] { + def set = LinkedHashSet.empty[A] + def f(a: A): Unit = { + set += a // warn because cannot know whether the `set` was supposed to be consumed or assigned + println(set) + } +} + +// neither StringBuilder warns, because either append is Java method or returns this.type +// while loop looks like if branch with block1(block2, jump to label), where block2 typed as non-unit +class Strung { + def iterator = Iterator.empty[String] + def addString(b: StringBuilder, start: String, sep: String, end: String): StringBuilder = { + val jsb = b.underlying + if (start.length != 0) jsb.append(start) + val it = iterator + if (it.hasNext) { + jsb.append(it.next()) + while (it.hasNext) { + jsb.append(sep) + jsb.append(it.next()) + } + } + if (end.length != 0) jsb.append(end) + b + } + def f(b: java.lang.StringBuilder, it: Iterator[String]): String = { + while (it.hasNext) { + b.append("\n") + b.append(it.next()) + } + b.toString + } + def g(b: java.lang.StringBuilder, it: Iterator[String]): String = { + while (it.hasNext) it.next() // warn + b.toString + } +} + +/* testing 1,2,3 +class Huh[A, B](a: A) { + def isEmpty = false + def runWith[U](action: B => U): A => Boolean = { x => + val z = null.asInstanceOf[B] + val fellback = false + if (!fellback) {action(z)} // warn + !fellback + } + def foreach[U](f: A => U): Unit = + if (!isEmpty) f(a) // warn + def y = 42 + val x = { + isEmpty // warn + if (isEmpty) y // warn + 42 + } +} +*/ diff --git a/test/files/neg/nonunit-statement.check b/test/files/neg/nonunit-statement.check new file mode 100644 index 000000000000..8ea037ee67b1 --- /dev/null +++ b/test/files/neg/nonunit-statement.check @@ -0,0 +1,42 @@ +nonunit-statement.scala:54: warning: discarded non-Unit value + Future(42): Unit // nowarn { F(42)(ctx) }: Unit where annot is on F(42), TODO spurious -Wvalue-discard + ^ +nonunit-statement.scala:14: warning: unused value + improved // warn + ^ +nonunit-statement.scala:22: warning: unused value + new E().toString // warn + ^ +nonunit-statement.scala:29: warning: unused value + Future(42) // warn + ^ +nonunit-statement.scala:33: warning: unused value + copy() // warn + ^ +nonunit-statement.scala:40: warning: unused value + 27 +: xs // warn + ^ +nonunit-statement.scala:47: warning: a pure expression does nothing in statement position; multiline expressions may require enclosing parentheses + null // warn for purity + ^ +nonunit-statement.scala:82: warning: unused value + g // warn + ^ +nonunit-statement.scala:89: warning: unused value + g // warn + ^ +nonunit-statement.scala:87: warning: unused value + g // warn + ^ +nonunit-statement.scala:99: warning: unused value + if (b) { // warn, at least one branch looks interesting + ^ +nonunit-statement.scala:119: warning: unused value + set += a // warn because cannot know whether the `set` was supposed to be consumed or assigned + ^ +nonunit-statement.scala:150: warning: unused value + while (it.hasNext) it.next() // warn + ^ +error: No warnings can be incurred under -Werror. +13 warnings +1 error diff --git a/test/files/neg/nonunit-statement.scala b/test/files/neg/nonunit-statement.scala new file mode 100644 index 000000000000..9fa91069305f --- /dev/null +++ b/test/files/neg/nonunit-statement.scala @@ -0,0 +1,194 @@ +// scalac: -Werror -Wnonunit-statement -Wnonunit-if:false -Wvalue-discard +// debug: -Vprint:refchecks -Yprint-trees:format +import collection.ArrayOps +import collection.mutable.{ArrayBuilder, LinkedHashSet, ListBuffer} +import concurrent._ +import scala.reflect.ClassTag + +// typo, misplaced expression during refactor +class C { + import ExecutionContext.Implicits._ + def c = { + def improved = Future(42) + def stale = Future(27) + improved // warn + stale + } +} +// edited expression not deleted +class D { + def d = { + class E + new E().toString // warn + new E().toString * 2 + } +} +// unused future in template +class F { + import ExecutionContext.Implicits._ + Future(42) // warn +} +// unused template expression uses synthetic method of class +case class K(s: String) { + copy() // warn +} +// mutations returning this are ok +class Mutate { + val b = ListBuffer.empty[Int] + b += 42 // nowarn, returns this.type + val xs = List(42) + 27 +: xs // warn + + def f(x: Int): this.type = this + def g(): Unit = f(42) // nowarn +} +// some uninteresting expressions may warn for other reasons +class WhoCares { + null // warn for purity + ??? // nowarn for impurity +} +// explicit Unit ascription to opt out of warning, even for funky applies +class Absolution { + def f(i: Int): Int = i+1 + import ExecutionContext.Implicits._ + Future(42): Unit // nowarn { F(42)(ctx) }: Unit where annot is on F(42), TODO spurious -Wvalue-discard + f(42): Unit // nowarn +} +// warn uni-branched unless user disables it with -Wnonunit-if:false +class Boxed[A](a: A) { + def isEmpty = false + def foreach[U](f: A => U): Unit = + if (!isEmpty) f(a) // nowarn, check is off + def forall(f: A => Boolean): Unit = + if (!isEmpty) { + println(".") + f(a) // nowarn, check is off + } + def take(p: A => Boolean): Option[A] = { + while (isEmpty || !p(a)) () + Some(a).filter(p) + } +} +class Unibranch[A, B] { + def runWith[U](action: B => U): A => Boolean = { x => + val z = null.asInstanceOf[B] + val fellback = false + if (!fellback) action(z) // nowarn, check is off + !fellback + } + def f(i: Int): Int = { + def g = 17 + if (i < 42) { + g // warn + println("uh oh") + g // nowarn, check is off + } + while (i < 42) { + g // warn + println("uh oh") + g // warn + } + 42 + } +} +class Dibranch { + def i: Int = ??? + def j: Int = ??? + def f(b: Boolean): Int = { + // if-expr might have an uninteresting LUB + if (b) { // warn, at least one branch looks interesting + println("true") + i + } + else { + println("false") + j + } + 42 + } +} +class Next[A] { + val all = ListBuffer.empty[A] + def f(it: Iterator[A], g: A => A): Unit = + while (it.hasNext) + all += g(it.next()) // nowarn +} +class Setting[A] { + def set = LinkedHashSet.empty[A] + def f(a: A): Unit = { + set += a // warn because cannot know whether the `set` was supposed to be consumed or assigned + println(set) + } +} + +// neither StringBuilder warns, because either append is Java method or returns this.type +// while loop looks like if branch with block1(block2, jump to label), where block2 typed as non-unit +class Strung { + def iterator = Iterator.empty[String] + def addString(b: StringBuilder, start: String, sep: String, end: String): StringBuilder = { + val jsb = b.underlying + if (start.length != 0) jsb.append(start) + val it = iterator + if (it.hasNext) { + jsb.append(it.next()) + while (it.hasNext) { + jsb.append(sep) + jsb.append(it.next()) + } + } + if (end.length != 0) jsb.append(end) + b + } + def f(b: java.lang.StringBuilder, it: Iterator[String]): String = { + while (it.hasNext) { + b.append("\n") + b.append(it.next()) + } + b.toString + } + def g(b: java.lang.StringBuilder, it: Iterator[String]): String = { + while (it.hasNext) it.next() // warn + b.toString + } +} + +class J { + import java.util.Collections + def xs: java.util.List[Int] = ??? + def f(): Int = { + Collections.checkedList[Int](xs, classOf[Int]) + 42 + } +} + +class Variant { + var bs = ListBuffer.empty[Int] + val xs = ListBuffer.empty[Int] + def f(i: Int): Unit = { + bs.addOne(i) + xs.addOne(i) + println("done") + } +} + +final class ArrayOops[A](private val xs: Array[A]) extends AnyVal { + def other: ArrayOps[A] = ??? + def transpose[B](implicit asArray: A => Array[B]): Array[Array[B]] = { + val aClass = xs.getClass.getComponentType + val bb = new ArrayBuilder.ofRef[Array[B]]()(ClassTag[Array[B]](aClass)) + if (xs.length == 0) bb.result() + else { + def mkRowBuilder() = ArrayBuilder.make[B](ClassTag[B](aClass.getComponentType)) + val bs = new ArrayOps(asArray(xs(0))).map((x: B) => mkRowBuilder()) + for (xs <- other) { + var i = 0 + for (x <- new ArrayOps(asArray(xs))) { + bs(i) += x + i += 1 + } + } + for (b <- new ArrayOps(bs)) bb += b.result() + bb.result() + } + } +} diff --git a/test/files/neg/value-discard.check b/test/files/neg/value-discard.check index 1133fbe94a84..04814148c199 100644 --- a/test/files/neg/value-discard.check +++ b/test/files/neg/value-discard.check @@ -1,11 +1,11 @@ value-discard.scala:6: warning: discarded non-Unit value - mutable.Set[String]().remove("") // expected to warn + mutable.Set[String]().remove("") // warn because suspicious receiver ^ value-discard.scala:13: warning: discarded non-Unit value - def subtract(): Unit = mutable.Set.empty[String].subtractOne("") // warn + def subtract(): Unit = mutable.Set.empty[String].subtractOne("") // warn because suspicious receiver ^ value-discard.scala:17: warning: a pure expression does nothing in statement position; multiline expressions might require enclosing parentheses - "" // warn + "" // warn pure expr ^ error: No warnings can be incurred under -Werror. 3 warnings diff --git a/test/files/neg/value-discard.scala b/test/files/neg/value-discard.scala index 2306bc917da4..eed8a29a4d12 100644 --- a/test/files/neg/value-discard.scala +++ b/test/files/neg/value-discard.scala @@ -1,21 +1,21 @@ -// scalac: -Ywarn-value-discard -Xfatal-warnings +// scalac: -Wvalue-discard -Werror final class UnusedTest { import scala.collection.mutable def remove(): Unit = { - mutable.Set[String]().remove("") // expected to warn + mutable.Set[String]().remove("") // warn because suspicious receiver } def removeAscribed(): Unit = { - mutable.Set[String]().remove(""): Unit // no warn + mutable.Set[String]().remove(""): Unit // nowarn } - def subtract(): Unit = mutable.Set.empty[String].subtractOne("") // warn + def subtract(): Unit = mutable.Set.empty[String].subtractOne("") // warn because suspicious receiver def warnings(): Unit = { val s: mutable.Set[String] = mutable.Set.empty[String] - "" // warn - "": Unit // no warn - s.subtractOne("") // no warn + "" // warn pure expr + "": Unit // nowarn + s.subtractOne("") // nowarn } } diff --git a/test/files/neg/virtpatmat_unreach_select.scala b/test/files/neg/virtpatmat_unreach_select.scala index b3ca30e2eab0..db6caf85b1a3 100644 --- a/test/files/neg/virtpatmat_unreach_select.scala +++ b/test/files/neg/virtpatmat_unreach_select.scala @@ -1,4 +1,4 @@ -// scalac: -Xfatal-warnings +// scalac: -Werror // class Test { object severity extends Enumeration