Nothing warns about unconsumed values in for comprehension

[keynmol]

Compiler version

3.3.1+ (tested on nightly as well)

Minimized code

This shows several places where it works and the one where it doesn't

//> using scala 3.nightly
//> using option -Wnonunit-statement 

def method: List[Int] = List(25)

@main def repro = 
  method // WARNING here ✅
  
  val _ = for 
    x <- method 
    _ = method // Nothing here ❌
  yield x

  method.flatMap: x =>
    method.map: y=>
      method // WARNING here ✅
      List()

Output

// TODO add output here

Expectation

I expected a warning because it's a non-unit statement (?).

[keynmol]

As a note, fixing this would be a huge improvement in purely functional codebases (CE/ZIO) - this failure mode is responsible for quietly missed log statements, assertions that aren't actioned, concurrency bugs, etc.

[SethTisue]

Scala 2.13 doesn't warn either:

% cat S.scala      
//> using scala 2.13.12
//> using option -Wnonunit-statement

object Repro extends App {
  def method: List[Int] = List(25)
  def foo(): List[Int] =
    for {
      x <- method
      _ = method // Nothing here ❌
    } yield x
}
% scala-cli compile S.scala
Compiling project (Scala 2.13.12, JVM (17))
Compiled project (Scala 2.13.12, JVM (17))

@som-snytt probably knows if there's a Scala 2 ticket on that

[szymon-rd]

I will look at that after I finish the current couple of tasks on linting.

[aherlihy]

Closing this issue as not a bug, because the purpose of this syntax (_ = method) is to explicitly discard this warning. It would be inconsistent to add a warning here, but not in other contexts where _ = ... is used. We could consider adding a different, new warning to catch this particular case, or contribute to the discussion on improvements on for comprehension syntax, but either case wouldn't make use of -Wnounit-statement.

[som-snytt]

Trees at typer for scala 2 & 3:

➜  snips scalac -Wnonunit-statement -Xlint -Vprint -d /tmp/sandbox for-unused.scala
[[syntax trees at end of                     typer]] // for-unused.scala
package <empty> {
  class C extends scala.AnyRef {
    def <init>(): C = {
      C.super.<init>();
      ()
    };
    def i: Int = 42;
    def f(xs: List[Int]): List[Int] = xs.map[(Int, Int)](((x: Int) => {
  val y: Int = C.this.i;
  scala.Tuple2.apply[Int, Int](x, y)
})).map[Int](((x$1: (Int, Int)) => (x$1: (Int, Int) @unchecked) match {
      case (_1: Int, _2: Int): (Int, Int)((x @ _), (y @ _)) => x
    }))
  }
}

and

➜  snips scalac -Wnonunit-statement -Wunused:all -Vprint:typer -d /tmp/sandbox for-unused.scala
[[syntax trees at end of                     typer]] // for-unused.scala
package <empty> {
  class C() extends Object() {
    def i: Int = 42
    def f(xs: List[Int]): List[Int] =
      xs.map[(Int, Int)](
        {
          def $anonfun(x: Int): (Int, Int) =
            {
              val y: Int = this.i
              Tuple2.apply[Int, Int](x, y)
            }
          closure($anonfun)
        }
      ).map[Int](
        {
          def $anonfun(x$1: (Int, Int)): Int =
            x$1:(x$1 : (Int, Int)) @unchecked match
              {
                case Tuple2.unapply[Int, Int](x @ _, y @ _) => x:Int
              }
          closure($anonfun)
        }
      )
  }
}

That is for

class C {
  def i = 42
  def f(xs: List[Int]) =
    for (x <- xs; y = i) yield x
}

where the y is consumed by the tupling for "midstream assignment".

To notice it is unused, you must fast-forward to the map where the pattern var is unused; but it may be marked synthetic? I no longer remember why it is not marked unused.

The scala 2 umbrella ticket (for improved diagnostics of for comprehensions) is scala/bug#10287.

When the "improved for comprehension" PR is merged, we'll need "improved diagnostics for improved for comprehensions".

[som-snytt]

I see there was a question mark in the expectation section, so I broadened the title to survive legal challenge.

[som-snytt]

Scala 2 manages to warn, but Scala 3 does not yet.

def g = for (i <- List(1)) yield answer    // warn map.(i => 42)

Scala 2 could win on "points", as they say in boxing (the sport, not the value class conversion).

[aherlihy]

The specific issue I was referring to arises with the use of _ = ... since after desugaring it's indistinguishable from the cases where you want to indicate to the compiler explicitly not to warn. Without that, are you proposing a warning for both:

def method1: List[Int] = List(25)
def method2: List[Int] = List(2)
def method3: List[Int] = List(10)
@main def repro =
  for
    x <- method1 // warn that x is unused
    z = method2  // warn that z is unused
  yield method3

cases? the x and z are used in the desugared code (see below) in order to pass them along to subsequent calls to map, so would need to special case their treatment depending on if those variables, in an earlier phase, had been within a for comprehension.

Desugared:

@main def repro: List[List[Int]] =
      method1.map[(Int, List[Int])](
        {
          def $anonfun(x: Int): (Int, List[Int]) =
            {
              val z: List[Int] = method2
              Tuple2.apply[Int, List[Int]](x, z)
            }
          closure($anonfun)
        }
      ).map[List[Int]](
        {
          def $anonfun(x$1: (Int, List[Int])): List[Int] =
            x$1:(x$1 : (Int, List[Int])) @unchecked match
              {
                case Tuple2.unapply[Int, List[Int]](x @ _, z @ _) => method3
              }
          closure($anonfun)
        }
      )
  }

[keynmol]

From a user's perspective, as both x and z are no longer valid after the scope of for and within that scope there are no visible usages, I'd expect an unused warning, disabled via _.

The specific expansion of for comp is an implementation detail and IMO doesn't justify lack of warning – from a philosophical point of view, warning on unused named values and variables is designed to help with local reasoning - "if this thing has no name, that means it cannot be used in any later part of the code, so I can just not think about it anymore"

[mbovel]

From what we discussed with the team and what I understood, there are 2 distinct issues.

Warning about unused values

As described in @som-snytt's comment:

for (x <- List(1); y = x) do println(x) // should warn about unused y

This is definitely a shortcoming of -Wunused and something we want to fix soon.

There is an existing issue for this: #18289.

Warning about "unconsumed" values

As described in this issue's description, it seems that it would be useful for some uses-cases to have a warning when "assigning" a non-unit value to _ in a for-comprehension:

def foo(): Int = 42
for x <- List(1); _ = foo() do println(x) // should warn about `foo()` returning non-unit?

However, as @aherlihy noted, we don't and probably don't want to warn when assigning to val _ =:

val _ = foo() // no warning

So why should it be the case in a for-comprehension? Why should the right hand-side of an assignment to _ be of unit type? It seems that these warnings would be useful mostly for functional programming libraries, so maybe one could tailor something specifically for these use-cases? And it's not clear if that should be linked to -Wnonunit-statement or not.

[Gedochao]

The warning about unused values will be tracked under #18289
What we'll track in this issue is just the unconsumed values.
This will not be worked on by the compiler maintainence team, but external contributions are welcome.

[som-snytt]

The specific expansion of for comp is an implementation detail

It is a specification detail.

After spending some time with the elaboration Scala 2 for comps, or belaboration would be a better word, I am sympathetic to keynmol's request, and I'll try to motivate it (at a later date).

It would be nice if we had valdef syntax still, to mean "I intend to discard a value, not just to omit naming it."

for (x <- xs; val _ = f(x))

The difference is that midstream assignment always gets a name (per spec).

val x$1 @ _ = f(x)

That is because the value is always packed in the tupling step, even though when untupled it is underscore (ignored).

Note that -Wnonunit-statement absolves singleton types as well as unit. You'd expect adding to a list buffer not to warn.

[lbialy]

In the spirit of the original issue, I think the problem @keynmol had in mind when writing about _ = method in for-comp being a huge problem in pure functional codebases is that it probably could be a warning (enabled with a compiler flag) when you assign-discard a value of the same type as the enclosing for-comprehension. Here's a specific example:

def log(msg: String): IO[Unit] = ???

for 
  res1 <- doStuff() // IO[Result]
  _ = log(s"Did stuff and got ${res1.output}") // silently discarded IO[Unit]
  res2 <- doOtherStuff() // IO[AnotherResult]
yield (res1, res2) // IO[(Result, AnotherResult)]

Maybe this would be more helpful?