spurious "a pure expression does nothing" warning (awkward to suppress in 2.12, prone to occur in sbt builds)

[eed3si9n]

reproduction steps

Scala 2.13.3 or 2.12.12 with fatal warnings.

The following code emulates build.sbt, but I'm seeing it in sbt code really sbt/sbt#5743, and I'm afraid others will run into this if we upgrade to Scala 2.12.12.

object Test extends AnyRef with App {
  lazy val task1 = new AnyRef {
    val value: Int = {
      println("flip pancake")
      0
    }
  }

  lazy val task2 = new AnyRef {
    val value: Int = {
      println("flop pancake")
      0
    }
  }

  def use() =
    try {
      task1.value
      task2.value
      ()
    } catch {
      case _: Throwable => ()
    }

  use()
}

problem

predef-unit.scala:20: warning: a pure expression does nothing in statement position; multiline expressions might require enclosing parentheses
      task1.value
            ^
predef-unit.scala:21: warning: a pure expression does nothing in statement position; multiline expressions might require enclosing parentheses
      task2.value
            ^
error: No warnings can be incurred under -Werror.
2 warnings
1 error

expectations

task1.value may look pure-looking from the compiler's point of view, but it's actually a macro, and it's not.
The reproduction above exploits a bug that @som-snytt pointed out that it's not checking full path for laziness, but I'm not sure if fixing that alone would fix the situation with sbt.

Maybe it would be better to put this warning behind a flag as an opt-in or turn it into Scalafix rule instead?

notes

In the recent releases of Scala, "a pure expression does nothing in statement position" has become more enthusiastic/accurate in its reach. Maybe because it moved to RefCheck in scala/scala#8995?

This seems to be a common occurrence since there are multiple PRs related to this:

• Introduce Unit.void[A](x: A): Unit scala#7530
• Allow suppressing value-discard warning via type ascription to Unit scala#7563
• Predef.unit to workaround "a pure expression does nothing" scala#9164

People have suggested various creative solutions in reply to https://twitter.com/eed3si9n/status/1293272725415505921.

(compile.value, ())._2

locally { val _ = compile.value }

https://github.com/AVSystem/scala-commons/blob/87c92cfcf5cbbd183fe5d77a841352b4e58ece39/commons-core/src/main/scala/com/avsystem/commons/SharedExtensions.scala#L81

  class UniversalOps[A](private val a: A) extends AnyVal {
    /**
      * Explicit syntax to discard the value of a side-effecting expression.
      * Useful when `-Ywarn-value-discard` compiler option is enabled.
      */
    @silent
    def discard: Unit = ()
  }

[som-snytt]

Could someone let twitter know

scala 2.12.11> {
             | val _ = 42
             | val _ = 27
             | 3
             | }
<console>:14: error: _ is already defined as value _
               val _ = 27
                   ^

but

scala 2.13.3> {
            | val _ = 42
            | val _ = 27
            | 3
            | }
val res0: Int = 3

+1 for adding a rule to Abide Scalafix.

Probably the compiler should get out of the warnings business, except for migration warnings.

Also, instead of "error", for "that which has erred," it should just output "WRONG", with a caret at the position. It could expose a code for other tools. For some conditions, it might output, "Obviously WRONG".

[eed3si9n]

I'll repeat what I wrote here - scala/scala#6218 (comment)

Between (...: Unit) and val _ = ..., val _ = .... I think is less surprising to the person reading the code:

val _ = compile.value
val _ = copyResources.value

(...: Unit) has similar vibe to final val in a sense that all the tokens make sense, but the intent is opaque unless you're in the close circle of people who's been watching the development of 2.13 closely.

So if we're going to backport something to 2.12, maybe scala/scala#6218 is better, at least the less controversial part (import behavior change was contested in scala/scala3#6019) of the commit.

[SethTisue]

fwiw, I agree with Eugene that val _ = ... is the nicest, most idiomatic workaround form. @som-snytt how hard would it be to support that in 2.12.x?

[eed3si9n]

re: Pure Expression line of greeting cards, I used to send stickers with hand-written notes https://twitter.com/hochgi/status/961521854409256960

[lrytz]

The problem here is that the warning is considering the path as pure even though it contains a lazy val. This should be fixed

class A {
  val b = { println("..."); 1 }
}
lazy val a = new A

def foo = { a; 1 }   // no warning, correct
def bar = { a.b; 1 } // warning, incorrect

[som-snytt]

Similar discussion where selection from object warns as pure scala/scala#9161 (comment)

The subsequent comment says the warning is "reasonable", presumably because nobody writes x.y for the side effect of initializing x.

Selection from of volatile #9053 (misread that while skimming)

[lrytz]

Thanks, @som-snytt

The subsequent comment says the warning is "reasonable", presumably because nobody writes x.y for the side effect of initializing x.

Right, I agree with that in fact.

[dwijnand]

Sorry, but let's not let the reproduction confuse the original issue (though fixing the lazy value thing might be useful to do anyways).

The real world equivalent of the reproduction

  def use() =
    try {
      task1.value
      task2.value
      ()
    } catch {
      case _: Throwable => ()
    }

is actually:

FullInstance.app[[T0[x]](T0[Int], T0[Int]), Unit](scala.Tuple2(task2, task1), (($p$macro$3: (Int, Int)) => {
  <synthetic> val $q$macro$2: Int = $p$macro$3._1;
  <synthetic> val $q$macro$1: Int = $p$macro$3._2;
  {
    ($q$macro$1: Int);
    ($q$macro$2: Int);
    ()
  }
}))(AList.tuple2[Int, Int])

Which has nothing to do with initialising/paths, it's just literally pointless statements, because the resulting value isn't needed, only the side-effect that happened to produce it.

Talking about this with Jason, it turns out Eugene had identified what had caused it from the get-go:

Maybe because it moved to RefCheck in scala/scala#8995

[lrytz]

Yeah, so here I stand by my position that this should be fixed on sbt's side. The easiest way out might be @nowarn, though that's not (yet?) in 2.12.

[SethTisue]

sbt 1.4.1 (released today) includes a workaround: sbt/sbt#5981

[eed3si9n]

Yea, I tried but it doesn't feel right. With sbt's macro, <task>.value can be part of any expression.
We pluck out the method invocation Apply(...) tree, do the calculation in another thread and drop in the result in a bunch of synthetic variables, sort of:

  <synthetic> val $q$macro$2: Int = $p$macro$3._1;
  <synthetic> val $q$macro$1: Int = $p$macro$3._2;

The safest thing to maintain the original build.sbt semantics would be to replace <task>.value with Ident($q$macro$2).

The workaround I got it to pass the tests handles only the Block(...) case, which hopefully is enough. Another workaround I thought of was to replace it with scala.Predef.identity($q$macro$2), adding extra method invocation just to appear busy.

[eed3si9n]

Yea, I tried but it doesn't feel right.

Trying to guess what code is pure after the macro expansion is done has proven to be very precarious. There's a regression discovered by @julienrf sbt/sbt#6126 that sbt 1.4.3 does not evaluate the body of a pure function, and @eatkins has identified the source to be the combination of sbt/sbt@b62ddaa, sbt/sbt@a44aee9 and sbt/sbt@3a1463b where I'm trying to strip out pure expressions.

[joan38]

We get hit by the same issue on mill:
https://github.com/joan38/mill-scalafix/blob/master/mill-scalafix/src/com/goyeau/mill/scalafix/StyleModule.scala#L11

[htmldoug]

Seems better in sbt 1.5.5, although still reproducible with conditional tasks:

foo := {
  if (!(Compile / publish / skip).value) {
    bar.value // a pure expression does nothing in statement position...
  }
}

[fabianhjr]

Also present in 2.13.6, as joan38 mentions it is present when using mill's T.command.

[som-snytt]

Somewhere, I noted that -Wmacros helps cope with unused warnings for macro expansions. The current "default" behavior is generous: it does not warn for unused defs introduced by a macro, but does consider new usages.

If one were inclined or motivated to tweak this warning, which apparently one is not, one could attempt such a mitigation. -Wnonunit-statement is the backstop for pure expr does nothing. IIRC it warns if pure expr does not.