diff --git a/src/compiler/scala/tools/nsc/ast/parser/Parsers.scala b/src/compiler/scala/tools/nsc/ast/parser/Parsers.scala index 37f3f7e9916f..78ad5016d3fb 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 5c38ba7dcb60..b5d12f83683a 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 7c7094fc1053..b1ee9f6cd8c0 100644 --- a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala +++ b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala @@ -1723,19 +1723,83 @@ 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 = t match { + case Apply(f, _) => f.symbol.isJavaDefined && !isUniversalMember(f.symbol) + case _ => false + } + // The quirk of typechecking if is that the LUB often results in boring types. + // Parser adds suppressing attachment 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) // strict or + //case Block(_, Apply(label, Nil)) if label.symbol != null && nme.isLoopHeaderLabel(label.symbol.name) => false + case Block(_, res) => checkInterestingShapes(res) + case Match(_, cases) => cases.exists(k => checkInterestingShapes(k.body)) + 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 + && !treeInfo.hasExplicitUnit(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,8 @@ abstract class RefChecks extends Transform { // probably not, until we allow parameterised extractors tree - case Block(stats, expr) => + case blk @ Block(stats, expr) => + // diagnostic info val (count, result0, adapted) = expr match { case Block(expr :: Nil, Literal(Constant(()))) => (1, expr, true) @@ -1859,7 +1922,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 (!treeInfo.hasExplicitUnit(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 +1931,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 cd4b0b6580f8..f541113d1d09 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 } } @@ -1114,13 +1114,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.value) { - 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) && !treeInfo.hasExplicitUnit(tree)) + context.warning(tree.pos, "discarded non-Unit value", WarningCategory.WFlagValueDiscard) @inline def warnNumericWiden(tpSym: Symbol, ptSym: Symbol): Unit = if (!isPastTyper) { val isInharmonic = ( @@ -1245,7 +1241,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)) @@ -4729,7 +4725,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 926e2927b6a4..6045c42adec3 100644 --- a/src/reflect/scala/reflect/internal/StdAttachments.scala +++ b/src/reflect/scala/reflect/internal/StdAttachments.scala @@ -145,4 +145,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..ebd0772ee637 100644 --- a/src/reflect/scala/reflect/internal/TreeInfo.scala +++ b/src/reflect/scala/reflect/internal/TreeInfo.scala @@ -351,6 +351,40 @@ 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 != null && + (receiver.symbol.isGetter || receiver.symbol.isField) // 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 + } + fun.symbol != null && 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) @@ -746,6 +780,16 @@ abstract class TreeInfo { ((sym ne null) && sym.initialize.isTrait) } + def hasExplicitUnit(tree: Tree): Boolean = + explicitlyUnit(tree) || { + tree match { + case Apply(f, _) => hasExplicitUnit(f) + case TypeApply(f, _) => hasExplicitUnit(f) + case AppliedTypeTree(f, _) => hasExplicitUnit(f) + case _ => false + } + } + /** Applications in Scala can have one of the following shapes: * * 1) naked core: Ident(_) or Select(_, _) or basically anything else diff --git a/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala b/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala index a3a465498ac2..2f246872aaba 100644 --- a/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala +++ b/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala @@ -77,6 +77,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..6d9e0ef63e69 --- /dev/null +++ b/test/files/neg/nonunit-if.check @@ -0,0 +1,57 @@ +nonunit-if.scala:58: warning: discarded non-Unit value + if (!isEmpty) f(a) // warn, check is on + ^ +nonunit-if.scala:62: warning: discarded non-Unit value + f(a) // warn, check is on + ^ +nonunit-if.scala:13: warning: unused value + improved // warn + ^ +nonunit-if.scala:20: warning: unused value + new E().toString // warn + ^ +nonunit-if.scala:26: warning: unused value + Future(42) // warn + ^ +nonunit-if.scala:30: warning: unused value + copy() // warn + ^ +nonunit-if.scala:37: warning: unused value + 27 +: xs // warn + ^ +nonunit-if.scala:44: warning: a pure expression does nothing in statement position; multiline expressions may require enclosing parentheses + null // warn for purity + ^ +nonunit-if.scala:58: warning: unused value + if (!isEmpty) f(a) // warn, check is on + ^ +nonunit-if.scala:62: warning: unused value + f(a) // warn, check is on + ^ +nonunit-if.scala:73: warning: unused value + if (!fellback) action(z) // warn, check is on + ^ +nonunit-if.scala:81: warning: unused value + g // warn, check is on + ^ +nonunit-if.scala:79: warning: unused value + g // warn block statement + ^ +nonunit-if.scala:86: warning: unused value + g // warn + ^ +nonunit-if.scala:84: warning: unused value + g // warn + ^ +nonunit-if.scala:96: warning: unused value + if (b) { // warn, at least one branch looks interesting + ^ +nonunit-if.scala:116: warning: unused value + set += a // warn because cannot know whether the `set` was supposed to be consumed or assigned + ^ +nonunit-if.scala:146: warning: unused value + while (it.hasNext) it.next() // warn + ^ +error: No warnings can be incurred under -Werror. +18 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..17aee82cc15c --- /dev/null +++ b/test/files/neg/nonunit-if.scala @@ -0,0 +1,191 @@ +// scalac: -Werror -Wnonunit-statement -Wnonunit-if:true -Wvalue-discard +// debug: -Vprint:refchecks -Yprint-trees:format +import collection.ArrayOps +import collection.mutable.{ArrayBuilder, LinkedHashSet, ListBuffer} +import concurrent._ +import scala.reflect.ClassTag + +class C { + import ExecutionContext.Implicits._ + def c = { + def improved = Future(42) + def stale = Future(27) + improved // warn + stale + } +} +class D { + def d = { + class E + new E().toString // warn + new E().toString * 2 + } +} +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) + 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, check is on + def forall(f: A => Boolean): Unit = + if (!isEmpty) { + println(".") + f(a) // warn, check is on + } + 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, check is on + !fellback + } + def f(i: Int): Int = { + def g = 17 + if (i < 42) { + g // warn block statement + println("uh oh") + g // warn, check is on + } + 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] + private[this] val ys = ListBuffer.empty[Int] + private[this] var zs = ListBuffer.empty[Int] + def f(i: Int): Unit = { + bs.addOne(i) + xs.addOne(i) + ys.addOne(i) + zs.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/nonunit-statement.check b/test/files/neg/nonunit-statement.check new file mode 100644 index 000000000000..f98bcd3bc607 --- /dev/null +++ b/test/files/neg/nonunit-statement.check @@ -0,0 +1,39 @@ +nonunit-statement.scala:13: warning: unused value + improved // warn + ^ +nonunit-statement.scala:20: warning: unused value + new E().toString // warn + ^ +nonunit-statement.scala:26: warning: unused value + Future(42) // warn + ^ +nonunit-statement.scala:30: warning: unused value + copy() // warn + ^ +nonunit-statement.scala:37: warning: unused value + 27 +: xs // warn + ^ +nonunit-statement.scala:44: warning: a pure expression does nothing in statement position; multiline expressions may require enclosing parentheses + null // warn for purity + ^ +nonunit-statement.scala:79: warning: unused value + g // warn block statement + ^ +nonunit-statement.scala:86: warning: unused value + g // warn + ^ +nonunit-statement.scala:84: warning: unused value + g // warn + ^ +nonunit-statement.scala:96: warning: unused value + if (b) { // warn, at least one branch looks interesting + ^ +nonunit-statement.scala:116: warning: unused value + set += a // warn because cannot know whether the `set` was supposed to be consumed or assigned + ^ +nonunit-statement.scala:146: warning: unused value + while (it.hasNext) it.next() // warn + ^ +error: No warnings can be incurred under -Werror. +12 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..715a7cd3ebf5 --- /dev/null +++ b/test/files/neg/nonunit-statement.scala @@ -0,0 +1,191 @@ +// 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 + +class C { + import ExecutionContext.Implicits._ + def c = { + def improved = Future(42) + def stale = Future(27) + improved // warn + stale + } +} +class D { + def d = { + class E + new E().toString // warn + new E().toString * 2 + } +} +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) + 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 block statement + 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] + private[this] val ys = ListBuffer.empty[Int] + private[this] var zs = ListBuffer.empty[Int] + def f(i: Int): Unit = { + bs.addOne(i) + xs.addOne(i) + ys.addOne(i) + zs.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