fix(#19266): better warning for impure synthetic lambda in stmt position

[i10416]

close #19266

Before this PR, checkStatementPurity filtered out impure expression too eagerly and closureDef ignores closure definition generated from eta-expansion on method with default arguments, which prevented impure lambda at statement position from showing a warning.

This PR modifies

• checkStatementPurity to check expression purity at later so that impure lambda will not slip through the check
• closureDef to match against a synthetic closure from eta-expansion on method with default arguments

Related to

• https://github.com/lampepfl/dotty/issues/2570
• https://github.com/lampepfl/dotty/pull/11769

[i10416]

Note for myself.

Default by-value arguments may have side-effect, so it is reasonable not to warn " A pure expression does nothing in statement position". Perhaps, we should warn NonUnitUnusedValue.

Method without default argument and method with default arguments for by-name parameters are pure ~and (I think) should show warnings instead of error~

It seems an expected behavior for compiler to raise error rather than warning.

https://github.com/lampepfl/dotty/pull/11769

import scala.collection.mutable.ArrayBuffer
import scala.util.Random

val arr = ArrayBuffer[Int]()

def fn0(x: Int)(y: String) = Some(s"$x$y")
def fn1(x: Int, y: Unit = arr.addOne(Random.nextInt()))(z:Double) = Some(s"$x,$y,$z")
def fn2(x: Int, y: => Unit = arr.addOne(Random.nextInt()))(z:Double) = Some(s"$x,$y,$z")

// This is pure and falls into the missing argument arm
// here https://github.com/lampepfl/dotty/blob/1716bcd9dbefbef88def848c09768a698b6b9ed9/tests/pos-with-compiler-cc/dotc/typer/Typer.scala#L4203
fn0(0)
// This is impure(perform side-effect to mutate `arr`). Therefore this passes through `checkStatementPurity`.
fn1(1)
// This is pure and falls into the missing argument arm 
// https://github.com/lampepfl/dotty/blob/1716bcd9dbefbef88def848c09768a698b6b9ed9/tests/pos-with-compiler-cc/dotc/typer/Typer.scala#L4203
fn2(2)

[i10416]

It seems expected behavior to raise error rather than warning :thinking:

Disallow lambdas in statement position or if the expected type is Unit. This was a loophole that opened up due to the changes to eta expansion in Scala 3. We did get a warning in these cases, but an error might be better.

https://github.com/lampepfl/dotty/pull/11769

[dwijnand]

@i10416 before I merge this, what's the status from your perspective? Are your commits here ready to be merged?

I think your arguments are very reasonable and your changes are right. But I wonder if it would be better to be bolder to make things simpler. I'm thinking we make closures not compile, whether they're pure or not. Using your example:

import scala.collection.mutable.ArrayBuffer
import scala.util.Random

val arr = ArrayBuffer[Int]()

def fn0(x: Int)(y: String) = Some(s"$x,$y")
def fn1(x: Int, y: Unit = arr.addOne(Random.nextInt()))(z:Double) = Some(s"$x,$y,$z")
def fn2(x: Int, y: => Unit = arr.addOne(Random.nextInt()))(z:Double) = Some(s"$x,$y,$z")

// Thus, it shouldn't compile.
fn0(0)
// This is impure(perform side-effect to mutate `arr`). Therefore this passes through `checkStatementPurity`.
fn1(1)
// This is pure and falls into the missing argument arm 
// https://github.com/lampepfl/dotty/blob/1716bcd9dbefbef88def848c09768a698b6b9ed9/tests/pos-with-compiler-cc/dotc/typer/Typer.scala#L4203
// Thus, it shouldn't compile.
fn2(2)

I think we should just also make fn1(1) not compile, even if it has a side-effecting default by-value argument. Any project that crucially relies on that effect can do one of many workarounds to preserving their program's behaviour. What do you think?

[i10416]

@dwijnand

Are your commits here ready to be merged?

Yes, it is ready for review.

I think we should just also make fn1(1) not compile, e.....

I agree with you. It feels consistent to prohibit lambda in statement position regardless of its purity. I guess few projects if any depend on this behavior and preserving similar behavior is not so hard.

I will add some changes and let's see whether any projects in the community build fail to compile.

[dwijnand]

@odersky do you agree to prohibiting more lambdas, extending #11769?

[dwijnand]

@odersky do you agree to prohibiting more lambdas, extending #11769?

@odersky asides from the extractor changes, which can be changed back, what do you think about making some more lambdas type errors, like #11769 did?

[som-snytt]

@dwijnand I was curious about the fate of test frameworks that rely on lambdas in statement pos, noted at the old PR. I did not try to vet that now.

[dwijnand]

Needs a rebase, for the new onnx submodule version.