Value discard warning in one-limbed if

[som-snytt]

Compiler version

3.5

Minimized code

$ ~/projects/dotty/bin/scala -Wnonunit-statement -Wvalue-discard
Welcome to Scala 3.5.1-RC1-bin-SNAPSHOT-git-4636ce2 (21.0.2, Java OpenJDK 64-Bit Server VM).
Type in expressions for evaluation. Or try :help.

scala> import scala.util.chaining.given

scala> val it = Iterator.continually(42)
val it: Iterator[Int] = <iterator>

scala> it.hasNext.tap(if (_) it.next())
1 warning found
-- [E175] Potential Issue Warning: -------------------------------------------------------------------------------------
1 |it.hasNext.tap(if (_) it.next())
  |                      ^^^^^^^^^
  |                      discarded non-Unit value of type Int
val res0: Boolean = true

scala>

Output

It warns on one-legged or one-armed if.

Expectation

Scala 2 decides not to warn, because one-limbed if is intentionally a value-discarding Unit expression.

The revisted expectation is that it's easy to silence the warning, probably with a category:

-Wconf:cat=if-discard:s

It is a matter of taste or style or risk tolerance whether if (b) e is obviously discarding.

[Gedochao]

note: the same behaviour can be reproduced outside of REPL.

//> using options -Wnonunit-statement -Wvalue-discard
import scala.util.chaining.given
@main def main = {
  val it = Iterator.continually(42)
  it.hasNext.tap(if (_) it.next())
}

[nox213]

Scala 2 also still gives a warning in this case, right?

[som-snytt]

@nox213 why would you say that.

➜  ~ scala -Wnonunit-statement -Wvalue-discard
Welcome to Scala 2.13.14 (OpenJDK 64-Bit Server VM, Java 22.0.2).
Type in expressions for evaluation. Or try :help.

scala> import scala.util.chaining._
import scala.util.chaining._

scala> val it = Iterator.continually(42)
val it: Iterator[Int] = <iterator>

scala> it.hasNext.tap(if (_) it.next())
val res0: Boolean = true

scala>

[nox213]

Thanks for double checking, I was confused

[nox213]

class Boxed[A](a: A) {
  def isEmpty = false
  def foreach[U](f: A => U): Unit =
    if (!isEmpty) f(a) // warn, check is on
}

In Scala 2, it warns for the code above when compiled with the -Wvalue-discard flag. Is this a different case from the example you provided?

[som-snytt]

@nox213 yes, the expected type matters.

From the previous example, compare:

  def f(): Unit = it.hasNext.tap(if (_) it.next())
  def g() = it.hasNext.tap(if (_) it.next())

In Scala 3, if (b) expr is typed as Unit, i.e., discarding.

In Scala 2, if (b) expr is typed as if (b) expr else (), the LUB of expr and Unit.

[nox213]

Thanks for the explanation!

[raquo]

one-limbed if is intentionally a value-discarding Unit expression

I don't think this is a good reason to skip the warning in this case. The method def bar(): Unit is also "intentionally value-discarding", but if you try to return a non-Unit expr from it, you of course get the warning, as you should.

The one-limbed if behaves in the exact same way – discards the output of its content by design – so it should also produce the exact same warning.

FWIW, I have a use case in Laminar that relies on this warning for safety. The documentation was written pre-Scala-3.3.0 where Wvalue-discard was introduced, and explains the issue in Laminar terms, but here is the same issue minimized:

def foo(sideEffect: => Unit): Unit = sideEffect

def foo(f: Int => Unit): Unit = f(1)

val callback = (v: Int) => println(v.toString)

// valid usages
foo(callback(0))
foo(callback)

// user mistake – does not execute callback. Should warn.
foo(if (true) callback)

scastie

The last line currently throws a discard warning in Scala 3, and IMO it should stay that way. Both for consistency with def bar(): Unit = expr, and because an unwanted warning can be trivially and compactly silenced with : Unit (pending #21557), whereas if the warning stops triggering in this case for convenience, some otherwise-safe APIs become impossible to make safe – in a rare downgrade from Scala 2, no less.

[som-snytt]

Neither version of foo guarantees that the argument is evaluated.

def f[U](g: => U): U might help -Xlint:infer-any (if scala 3 had such a lint). Maybe it's worth linting infer-unit.

The feature request might be to ensure that function values are eventually applied.

[raquo]

Neither version of foo guarantees that the argument is evaluated.

foo itself is fine though? foo(exp) will always evaluate exp, whether directly when it's a Unit, or invoke it as a function when it's a function. The problem would happen upstream of foo, inside exp – if Scala were to discard a non-Unit value inside the if branch without a warning, when such a warning is explicitly requested in compiler options.

def f[U](g: => U): U might help -Xlint:infer-any (if scala 3 had such a lint). Maybe it's worth linting infer-unit.

... I don't understand how I would use this f, or how it would help avoid user mistakes like if (true) callback. But it seems that the user would somehow need to call f in addition to foo, which would defeat the purpose of the foo(sideEffect: => Unit) API, and wouldn't add any meaningful safety, since the user needs to remember to call f.

The feature request might be to ensure that function values are eventually applied.

You mean that Scala should detect that the callback in if (true) callback was never applied? Sadly, that won't be enough. I used functions only for the sake of minimization, in real life we also have non-function values such as observers:

val observer: Observer[Int] = ???

def foo(observer: Observer[Int]): Unit = observer.onNext(1)

// valid code
foo(observer)

// user mistake – does not execute observer. Should warn.
foo(if (true) observer)

Function-specific lints could be useful for other things, but not as a fix to this proposed bugfix.

---

My foo-s were just an example of me using value-discard warnings for their stated purpose.

tap(if (_) it.next()) and foo(if (true) observer) are the exact same problem. And both of them are only a problem because in Scala 3, single-armed if returns () – instead of value | (), as it does in Scala 2.

Scala 3 added this new place where non-Unit values are discarded, similar to def foo(): Unit = nonUnitExpr. There is no reason why this new value-discarding place should be exempt from -Wvalue-discard warnings that apply to other value-discarding places like function/method bodies.

[som-snytt]

The linked ticket is about nonunit-statement, but the opinion there is that a warning is desired.

I would demote this ticket to a "feature request" for a way to quiet the warning, perhaps -Wconf:cat=if-discard.