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.)

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

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"

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 =

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]
 }
 
 

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
 

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]
 }

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.
    *   <init>(b = foo, a = bar)

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

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